diff --git a/app/models/maintenance_tasks/run.rb b/app/models/maintenance_tasks/run.rb index d6348999..abcc74ca 100644 --- a/app/models/maintenance_tasks/run.rb +++ b/app/models/maintenance_tasks/run.rb @@ -177,11 +177,14 @@ 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 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 93c4374b..07dc6a80 100644 --- a/test/models/maintenance_tasks/run_test.rb +++ b/test/models/maintenance_tasks/run_test.rb @@ -235,18 +235,30 @@ 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 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!(