Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cattr from CurrentAttributes are lost when retrying jobs #5090

Closed
baptistejub opened this issue Dec 9, 2021 · 3 comments
Closed

cattr from CurrentAttributes are lost when retrying jobs #5090

baptistejub opened this issue Dec 9, 2021 · 3 comments

Comments

@baptistejub
Copy link

Ruby version: 2.7.5p203
Rails version: 6.1.4.1
Sidekiq / Pro / Enterprise version(s): sidekiq 6.3.1

When a job having some current attributes fails and is put into the retry queue, we can see that, when retrying it, the current attributes (cattr) are missing from the retried job payload.
I was expected to see the whole payload and thus the current attributes when retrying a job. Not sure if it's the desired behaviour.

I think it's happening here:
https://github.com/mperham/sidekiq/blob/main/lib/sidekiq/middleware/current_attributes.rb#L23
when the server is pushing back the job from the retry queue to the processing queue, nothing sets the attributes of the Rails CurrentAttributes class (which is then empty) and so it overwrites the original cattr with an empty hash.

Not sure if we should skip setting the cattr key here when already present or ensure that a retried job sets the CurrentAttributes before pushing back to the queue.

Simple example :
initializer

require 'sidekiq/web'
require 'sidekiq/middleware/current_attributes'

Sidekiq::CurrentAttributes.persist(JobContext)

lib/my_job.rb

class MyJob
  include Sidekiq::Job

  sidekiq_options queue: "critical", retry: 1

  def perform
    puts "Locale attribute: #{JobContext.locale}"
    raise 'oops'
  end
end
2021-12-09T10:18:08.859Z pid=47468 tid=6h38 class=MyJob jid=f2227185b134bd4d063ba672 INFO: start
Locale attribute: en
2021-12-09T10:18:08.909Z pid=47468 tid=6h38 class=MyJob jid=f2227185b134bd4d063ba672 elapsed=0.05 INFO: fail
2021-12-09T10:18:08.909Z pid=47468 tid=6h38 WARN: {"context":"Job raised exception","job":{"retry":1,"queue":"critical","class":"MyJob","args":[],"jid":"f2227185b134bd4d063ba672","created_at":1639045088.83644,"cattr":{"locale":"en"},"enqueued_at":1639045088.853143},"jobstr":"{\"retry\":1,\"queue\":\"critical\",\"class\":\"MyJob\",\"args\":[],\"jid\":\"f2227185b134bd4d063ba672\",\"created_at\":1639045088.83644,\"cattr\":{\"locale\":\"en\"},\"enqueued_at\":1639045088.853143}"}
2021-12-09T10:18:08.909Z pid=47468 tid=6h38 WARN: RuntimeError: oops
2021-12-09T10:18:08.909Z pid=47468 tid=6h38 WARN: /my_project/lib/my_job.rb:8:in `perform'
2021-12-09T10:18:26.990Z pid=47468 tid=6h8o class=MyJob jid=f2227185b134bd4d063ba672 INFO: start
Locale attribute:
2021-12-09T10:18:27.034Z pid=47468 tid=6h8o class=MyJob jid=f2227185b134bd4d063ba672 INFO: Adding dead MyJob job f2227185b134bd4d063ba672
2021-12-09T10:18:27.038Z pid=47468 tid=6h8o class=MyJob jid=f2227185b134bd4d063ba672 elapsed=0.048 INFO: fail
2021-12-09T10:18:27.038Z pid=47468 tid=6h8o WARN: {"context":"Job raised exception","job":{"retry":1,"queue":"critical","class":"MyJob","args":[],"jid":"f2227185b134bd4d063ba672","created_at":1639045088.83644,"cattr":{},"enqueued_at":1639045106.987346,"error_message":"oops","error_class":"RuntimeError","failed_at":1639045088.904974,"retry_count":0},"jobstr":"{\"retry\":1,\"queue\":\"critical\",\"class\":\"MyJob\",\"args\":[],\"jid\":\"f2227185b134bd4d063ba672\",\"created_at\":1639045088.83644,\"cattr\":{},\"enqueued_at\":1639045106.987346,\"error_message\":\"oops\",\"error_class\":\"RuntimeError\",\"failed_at\":1639045088.904974,\"retry_count\":0}"}
2021-12-09T10:18:27.038Z pid=47468 tid=6h8o WARN: RuntimeError: oops
2021-12-09T10:18:27.038Z pid=47468 tid=6h8o WARN: /my_project/lib/my_job.rb:8:in `perform'
@mperham
Copy link
Collaborator

mperham commented Dec 9, 2021

Good detective work. That line should probably use ||=.

@mperham mperham closed this as completed in c92a521 Dec 9, 2021
@mperham
Copy link
Collaborator

mperham commented Dec 9, 2021

You can try using main if you want to test the fix.

@baptistejub
Copy link
Author

Looks all good now, thanks for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants