Merge pull request #345 from Shopify/fix-reenqueue-before-shutdown

Postpone reenqueuing the iteration job until after callbacks are done

app/jobs/maintenance_tasks/task_job.rb

@@ -97,8 +97,27 @@ def on_shutdown
       @ticker.persist
     end
 
+    # We are reopening a private part of Job Iteration's API here, so we should
+    # ensure the method is still defined upstream. This way, in the case where
+    # the method changes upstream, we catch it at load time instead of at
+    # runtime while calling `super`.
+    unless private_method_defined?(:reenqueue_iteration_job)
+      error_message = <<~HEREDOC
+        JobIteration::Iteration#reenqueue_iteration_job is expected to be
+        defined. Upgrading the maintenance_tasks gem should solve this problem.
+      HEREDOC
+      raise error_message
+    end
+    def reenqueue_iteration_job(should_ignore: true)
+      super() unless should_ignore
+      @reenqueue_iteration_job = true
+    end
+
     def after_perform
       @run.save!
+      if defined?(@reenqueue_iteration_job) && @reenqueue_iteration_job
+        reenqueue_iteration_job(should_ignore: false)
+      end
     end
 
     def on_error(error)

lib/maintenance_tasks.rb

@@ -9,6 +9,10 @@
 require 'pagy'
 require 'pagy/extras/bulma'
 
+# Force the TaskJob class to load so we can verify upstream compatibility with
+# the JobIteration gem
+require_relative '../app/jobs/maintenance_tasks/task_job'
+
 # The engine's namespace module. It provides isolation between the host
 # application's code and the engine-specific code. Top-level engine constants
 # and variables are defined under this module.

test/jobs/maintenance_tasks/task_job_test.rb

@@ -164,6 +164,24 @@ class TaskJobTest < ActiveJob::TestCase
       assert_equal 'Task Maintenance::DeletedTask not found.', run.error_message
     end
 
+    test '.perform_now delays reenqueuing the job after interruption until all callbacks are finished' do
+      JobIteration.stubs(interruption_adapter: -> { true })
+
+      AnotherTaskJob = Class.new(TaskJob) do
+        after_perform { self.class.times_interrupted = times_interrupted }
+
+        class << self
+          attr_accessor :times_interrupted
+        end
+      end
+      AnotherTaskJob.perform_now(@run)
+
+      # The job should not yet have been reenqueued, so times_interrupted should
+      # be 0
+      assert_equal 0, AnotherTaskJob.times_interrupted
+      assert_enqueued_jobs 1
+    end
+
     test '.perform_now does not enqueue another job if Run errors' do
       run = Run.create!(task_name: 'Maintenance::ErrorTask')