From c69bb5d5be8138bbadc3a2fb188a048d470671ff Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 8 Dec 2020 00:05:01 +0000 Subject: [PATCH] DEV: Always enqueue sidekiq jobs after database transaction commit (#11293) When jobs are enqueued inside a transaction, it's possible that they will be executed before the necessary data is available in the database. This commit ensures all jobs are enqueued in an ActiveRecord after_commit hook. One potential downside here is if the job fails to enqueue, the transaction will no longer be aborted. However, the chance of that happening is reasonably low, and the impact is significantly lower than the current issue where jobs are scheduled before their data is ready. --- app/jobs/base.rb | 2 +- spec/jobs/jobs_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/app/jobs/base.rb b/app/jobs/base.rb index b69302861a..fdcd1dd55e 100644 --- a/app/jobs/base.rb +++ b/app/jobs/base.rb @@ -311,7 +311,7 @@ module Jobs hash['queue'] = queue end - klass.client_push(hash) + DB.after_commit { klass.client_push(hash) } else # Otherwise execute the job right away opts.delete(:delay_for) diff --git a/spec/jobs/jobs_spec.rb b/spec/jobs/jobs_spec.rb index 0f3f8d48ca..5ee5ff09a7 100644 --- a/spec/jobs/jobs_spec.rb +++ b/spec/jobs/jobs_spec.rb @@ -29,6 +29,28 @@ describe Jobs do end end + it "enqueues the job after the current transaction has committed" do + jobs = Jobs::ProcessPost.jobs + expect(jobs.length).to eq(0) + + Jobs.enqueue(:process_post, post_id: 1) + expect(jobs.length).to eq(1) + + ActiveRecord::Base.transaction do + Jobs.enqueue(:process_post, post_id: 1) + expect(jobs.length).to eq(1) + end + expect(jobs.length).to eq(2) + + # Failed transation + ActiveRecord::Base.transaction do + Jobs.enqueue(:process_post, post_id: 1) + raise ActiveRecord::Rollback + end + + expect(jobs.length).to eq(2) # No change + end + it "does not pass current_site_id when 'all_sites' is present" do Sidekiq::Testing.fake! do jobs = Jobs::ProcessPost.jobs