Merge pull request #516 from Shopify/avoid-iteration-for-stopping-rac…

…e-condition

Avoid iteration for stopping race condition

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
 

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

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
 

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!(