From 3c425d7282a1b9bd88848f9f672e70e23c76be1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20Barri=C3=A9?= Date: Tue, 2 Nov 2021 16:55:03 +0100 Subject: [PATCH 1/2] Avoid query in regular case --- app/models/maintenance_tasks/run.rb | 1 + test/models/maintenance_tasks/run_test.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/maintenance_tasks/run.rb b/app/models/maintenance_tasks/run.rb index d6348999..409a6e5e 100644 --- a/app/models/maintenance_tasks/run.rb +++ b/app/models/maintenance_tasks/run.rb @@ -177,6 +177,7 @@ def time_to_completion # # If the run is stopping already, it will not transition to running. def running + return if stopping? updated = self.class.where(id: id).where.not(status: STOPPING_STATUSES) .update_all(status: :running, updated_at: Time.now) > 0 if updated diff --git a/test/models/maintenance_tasks/run_test.rb b/test/models/maintenance_tasks/run_test.rb index 93c4374b..3b627484 100644 --- a/test/models/maintenance_tasks/run_test.rb +++ b/test/models/maintenance_tasks/run_test.rb @@ -235,13 +235,13 @@ class RunTest < ActiveSupport::TestCase end end - test "#running doesn't set a stopping run to running" do + test "#running doesn't set a stopping run to running without a query" do [:cancelling, :pausing].each do |status| run = Run.create!( task_name: "Maintenance::UpdatePostsTask", status: status, ) - run.running + assert_equal 0, count_uncached_queries { run.running } refute_predicate run, :running? end From a71a3dbc57eb06a5cc2e7f754899c6ccf7bf5220 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20Barri=C3=A9?= Date: Tue, 2 Nov 2021 16:57:31 +0100 Subject: [PATCH 2/2] Prevent even one iteration in the race condition case --- app/models/maintenance_tasks/run.rb | 2 ++ test/dummy/app/jobs/custom_task_job.rb | 6 ++++++ test/jobs/maintenance_tasks/task_job_test.rb | 17 ++++++++++++++++- test/models/maintenance_tasks/run_test.rb | 12 ++++++++++++ 4 files changed, 36 insertions(+), 1 deletion(-) diff --git a/app/models/maintenance_tasks/run.rb b/app/models/maintenance_tasks/run.rb index 409a6e5e..abcc74ca 100644 --- a/app/models/maintenance_tasks/run.rb +++ b/app/models/maintenance_tasks/run.rb @@ -183,6 +183,8 @@ def running if updated self.status = :running clear_attribute_changes([:status]) + else + reload_status end end diff --git a/test/dummy/app/jobs/custom_task_job.rb b/test/dummy/app/jobs/custom_task_job.rb index 551c6682..24ecb5c7 100644 --- a/test/dummy/app/jobs/custom_task_job.rb +++ b/test/dummy/app/jobs/custom_task_job.rb @@ -6,4 +6,10 @@ class CustomTaskJob < MaintenanceTasks::TaskJob raise "Error enqueuing" if run.task_name == "Maintenance::EnqueueErrorTask" throw :abort if run.task_name == "Maintenance::CancelledEnqueueTask" end + + class_attribute :race_condition_hook, instance_accessor: false + + before_perform(prepend: true) do + CustomTaskJob.race_condition_hook&.call + end end diff --git a/test/jobs/maintenance_tasks/task_job_test.rb b/test/jobs/maintenance_tasks/task_job_test.rb index 393a61e4..30ccc282 100644 --- a/test/jobs/maintenance_tasks/task_job_test.rb +++ b/test/jobs/maintenance_tasks/task_job_test.rb @@ -53,7 +53,7 @@ class TaskJobTest < ActiveJob::TestCase assert_no_enqueued_jobs end - test ".perform_now skips iterations when Run is cancelled" do + test ".perform_now avoids iterating when Run is cancelled" do @run.cancelling! Maintenance::TestTask.any_instance.expects(:process).never @@ -64,6 +64,21 @@ class TaskJobTest < ActiveJob::TestCase assert_no_enqueued_jobs end + test ".perform_now avoids iterating and cancels Run when a race occurs between starting and cancelling" do + CustomTaskJob.race_condition_hook = -> do + Run.find(@run.id).update(status: :cancelling) + end + + Maintenance::TestTask.any_instance.expects(:process).never + + CustomTaskJob.perform_now(@run) + + assert_predicate(@run.reload, :cancelled?) + assert_no_enqueued_jobs + ensure + CustomTaskJob.race_condition_hook = nil + end + test ".perform_now updates tick_count" do Maintenance::TestTask.any_instance.expects(:process).twice diff --git a/test/models/maintenance_tasks/run_test.rb b/test/models/maintenance_tasks/run_test.rb index 3b627484..07dc6a80 100644 --- a/test/models/maintenance_tasks/run_test.rb +++ b/test/models/maintenance_tasks/run_test.rb @@ -247,6 +247,18 @@ class RunTest < ActiveSupport::TestCase end end + test "#running doesn't set a stopping run to running and reloads the status" do + [:cancelling, :pausing].each do |status| + run = Run.create!( + task_name: "Maintenance::UpdatePostsTask", + ) + Run.find(run.id).update(status: status) # race condition + run.running + + assert_equal status.to_s, run.status + end + end + test "#cancel transitions the Run to cancelling if not paused" do [:enqueued, :running, :pausing, :interrupted].each do |status| run = Run.create!(