Skip to content

fix: Use current attributes instead of activejob_traceable #3794

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

Merged
merged 1 commit into from
Jun 6, 2025

Conversation

rsempe
Copy link
Collaborator

@rsempe rsempe commented Jun 6, 2025

Unfortunately, we've seen that our current use of activejob_traceable is not thread safe with Sidekiq 😢

With the addition of activity logs, we've seen that CurrentContext can use wrong information (membership_id and source) because we're not resetting values at the start of each job. Two jobs can sequentially run on the same thread.

The goal of this PR is to not use activejob_traceable anymore (and remove a dependency 🎉 ), but introduce ActiveSupport::CurrentAttributes in Sidekiq jobs.

This middleware allows to:

  • persist values from CurrentContext into the Sidekiq job
  • restore those values before the job starts running
  • reset values after the job finishes
module Sidekiq
  module CurrentAttributes
    class Middleware
      def call(worker, job, queue)
        # Deserialize CurrentContext attributes into thread
        ::CurrentContext.set(job["current_attributes"]) do
          yield
        end
      ensure
        ::CurrentContext.reset_all
      end
    end
  end
end

@rsempe rsempe force-pushed the thread-safe-sidekiq branch from bb8cdca to f410f93 Compare June 6, 2025 13:15
Copy link
Contributor

@julienbourdeau julienbourdeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beautiful!

@rsempe rsempe merged commit 892580f into main Jun 6, 2025
14 checks passed
@rsempe rsempe deleted the thread-safe-sidekiq branch June 6, 2025 14:03
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

Successfully merging this pull request may close these issues.

5 participants