Merge pull request #293 from Shopify/task-deleted-during-run

Handle error handling for deleted tasks
Shopify · Jan 11, 2021 · 614d1e4 · 614d1e4
2 parents ee0db3a + dbe3234
commit 614d1e4
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 5 deletions.
diff --git a/app/jobs/maintenance_tasks/task_job.rb b/app/jobs/maintenance_tasks/task_job.rb
@@ -90,7 +90,7 @@ def after_perform
     end
 
     def on_error(error)
-      @ticker.persist
+      @ticker.persist if defined?(@ticker)
       @run.persist_error(error)
       MaintenanceTasks.error_handler.call(error)
     end

diff --git a/app/validators/maintenance_tasks/run_status_validator.rb b/app/validators/maintenance_tasks/run_status_validator.rb
@@ -11,7 +11,8 @@ class RunStatusValidator < ActiveModel::Validator
       # enqueued -> pausing occurs when the task is paused before starting.
       # enqueued -> cancelling occurs when the task is cancelled
       #   before starting.
-      # enqueued -> errored occurs when the task job fails to be enqueued.
+      # enqueued -> errored occurs when the task job fails to be enqueued, or
+      #   if the Task is deleted before is starts running.
       'enqueued' => ['running', 'pausing', 'cancelling', 'errored'],
       # pausing -> paused occurs when the task actually halts performing and
       #   occupies a status of paused.
@@ -56,7 +57,9 @@ class RunStatusValidator < ActiveModel::Validator
       #   it is interrupted.
       # interrupted -> cancelling occurs when the task is cancelled by the user
       #   while it is interrupted.
-      'interrupted' => ['running', 'pausing', 'cancelling'],
+      # interrupted -> errored occurs when the task is deleted while it is
+      #   interrupted.
+      'interrupted' => ['running', 'pausing', 'cancelling', 'errored'],
     }
 
     # Validate whether a transition from one Run status

diff --git a/test/jobs/maintenance_tasks/task_job_test.rb b/test/jobs/maintenance_tasks/task_job_test.rb
@@ -142,6 +142,28 @@ class TaskJobTest < ActiveJob::TestCase
       TaskJob.perform_now(run)
     end
 
+    test '.perform_now handles when the Task cannot be found' do
+      run = Run.new(task_name: 'Maintenance::DeletedTask')
+      run.save(validate: false)
+
+      TaskJob.perform_now(run)
+
+      assert_equal 'MaintenanceTasks::Task::NotFoundError', run.error_class
+      assert_equal 'Task Maintenance::DeletedTask not found.', run.error_message
+    end
+
+    test '.perform_now handles when the Task cannot be found when resuming after interruption' do
+      run = Run.new(task_name: 'Maintenance::DeletedTask')
+      run.save(validate: false)
+      run.running! # the Task existed when the run started
+      run.interrupted! # but not after interruption
+
+      TaskJob.perform_now(run)
+
+      assert_equal 'MaintenanceTasks::Task::NotFoundError', run.error_class
+      assert_equal 'Task Maintenance::DeletedTask not found.', run.error_message
+    end
+
     test '.perform_now does not enqueue another job if Run errors' do
       run = Run.create!(task_name: 'Maintenance::ErrorTask')
 

diff --git a/test/validators/maintenance_tasks/run_status_validator_test.rb b/test/validators/maintenance_tasks/run_status_validator_test.rb
@@ -179,7 +179,7 @@ class RunStatusValidatorTest < ActiveSupport::TestCase
       assert_no_invalid_transitions([:running], :interrupted)
     end
 
-    test 'run can go from enqueued, running, pausing or cancelling to errored' do
+    test 'run can go from enqueued, running, pausing, interrupted or cancelling to errored' do
       enqueued_run = Run.create!(task_name: 'Maintenance::UpdatePostsTask')
       enqueued_run.status = :errored
 
@@ -201,6 +201,14 @@ class RunStatusValidatorTest < ActiveSupport::TestCase
 
       assert pausing_run.valid?
 
+      interrupted_run = Run.create!(
+        task_name: 'Maintenance::UpdatePostsTask',
+        status: :interrupted
+      )
+      interrupted_run.status = :errored
+
+      assert interrupted_run.valid?
+
       cancelling_run = Run.create!(
         task_name: 'Maintenance::UpdatePostsTask',
         status: :cancelling
@@ -210,7 +218,7 @@ class RunStatusValidatorTest < ActiveSupport::TestCase
       assert cancelling_run.valid?
 
       assert_no_invalid_transitions(
-        [:enqueued, :running, :pausing, :cancelling],
+        [:enqueued, :running, :pausing, :interrupted, :cancelling],
         :errored
       )
     end