Use min max job runtime between class & global

The existing approach set `JobIteration.max_job_runtime` as the default value of the `job_iteration_max_job_runtime` `class_attribute`. However, this approach would close around the value at the time the `Iteration` module was included in the host `class`. This means that if a job class is defined before the host application sets `max_job_runtime`, the setting will not be honoured. For example, if using the `maintenance_tasks` gem, which defines a `TaskJob`, the default would be read when the gem is loaded, prior to initializers running, which is where the host application typically sets the global `max_job_runtime`. This commit changes the implementation in the following ways: - We no longer restrict the ability to increase the maximum in child classes. There were ways to get around the restriction, and there may be legitimate scenarios in which an application needs to allow an job to exceed its parent class' maximum job runtime, so we can simplify the implementation by removing the constraint. - We now take the minimum between the class attribute and the global maximum, which works around the default race, while still ensuring the application can enforce a maximum across all jobs, for stability. Co-authored-by: Sander Verdonschot <sander.verdonschot@shopify.com>
Shopify · Nov 15, 2023 · 5faeb1d · 5faeb1d
1 parent 742ee07
commit 5faeb1d
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 62 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,7 +1,13 @@
 ### Main (unreleased)
 
+### Changes
+
+- [437](https://github.com/Shopify/job-iteration/pull/437) - Use minimum between per-class `job_iteration_max_job_runtime` and `JobIteration.max_job_runtime`, instead of enforcing only setting decreasing values.
+  Because it is possible to change the global or parent values after setting the value on a class, it is not possible to truly enforce the decreasing value constraint. Instead, we now use the minimum between the global value and per-class value. This is considered a non-breaking change, as it should not break any **existing** code, it only removes the constraint on new classes.
+
 ### Bug fixes
 
+- [437](https://github.com/Shopify/job-iteration/pull/437) - Defer reading `JobIteration.max_job_runtime` until runtime, instead of closing around the value at the time of job definition.
 - [431](https://github.com/Shopify/job-iteration/pull/431) - Use `#id_value` instead of `send(:id)`
 when generating position for cursor based on `:id` column (Rails 7.1 and above, where composite
 primary models are now supported). This ensures we grab the value of the id column, rather than a

diff --git a/lib/job-iteration.rb b/lib/job-iteration.rb
@@ -29,14 +29,22 @@ def logger
   # This setting will make it to always interrupt a job after it's been iterating for 5 minutes.
   # Defaults to nil which means that jobs will not be interrupted except on termination signal.
   #
-  # This setting can be further reduced (but not increased) by using the inheritable per-class
-  # job_iteration_max_job_runtime setting.
+  # This setting can be overriden by using the inheritable per-class job_iteration_max_job_runtime setting. At runtime,
+  # the lower of the two will be used.
   # @example
   #
   #   class MyJob < ActiveJob::Base
   #     include JobIteration::Iteration
   #     self.job_iteration_max_job_runtime = 1.minute
   #     # ...
+  #
+  # Note that if a sub-class overrides its parent's setting, only the global and sub-class setting will be considered,
+  # not the parent's.
+  # @example
+  #
+  #   class ChildJob < MyJob
+  #     self.job_iteration_max_job_runtime = 3.minutes # MyJob's 1.minute will be discarded.
+  #     # ...
   attr_accessor :max_job_runtime
 
   # Configures a delay duration to wait before resuming an interrupted job.

diff --git a/lib/job-iteration/iteration.rb b/lib/job-iteration/iteration.rb
@@ -47,30 +47,9 @@ def inspected_cursor
 
       class_attribute(
         :job_iteration_max_job_runtime,
-        instance_writer: false,
+        instance_accessor: false,
         instance_predicate: false,
-        default: JobIteration.max_job_runtime,
       )
-
-      singleton_class.prepend(PrependedClassMethods)
-    end
-
-    module PrependedClassMethods
-      def job_iteration_max_job_runtime=(new)
-        existing = job_iteration_max_job_runtime
-
-        if existing && (!new || new > existing)
-          existing_label = existing.inspect
-          new_label = new ? new.inspect : "#{new.inspect} (no limit)"
-          raise(
-            ArgumentError,
-            "job_iteration_max_job_runtime may only decrease; " \
-              "#{self} tried to increase it from #{existing_label} to #{new_label}",
-          )
-        end
-
-        super
-      end
     end
 
     module ClassMethods
@@ -291,13 +270,22 @@ def instrumentation_tags
     end
 
     def job_should_exit?
-      if job_iteration_max_job_runtime && start_time && (Time.now.utc - start_time) > job_iteration_max_job_runtime
-        return true
-      end
+      max_job_runtime = job_iteration_max_job_runtime
+      return true if max_job_runtime && start_time && (Time.now.utc - start_time) > max_job_runtime
 
       JobIteration.interruption_adapter.call || (defined?(super) && super)
     end
 
+    def job_iteration_max_job_runtime
+      global_max = JobIteration.max_job_runtime
+      class_max = self.class.job_iteration_max_job_runtime
+
+      return global_max unless class_max
+      return class_max unless global_max
+
+      [global_max, class_max].min
+    end
+
     def handle_completed(completed)
       case completed
       when nil # someone aborted the job but wants to call the on_complete callback

diff --git a/test/unit/iteration_test.rb b/test/unit/iteration_test.rb
@@ -311,6 +311,17 @@ def test_global_max_job_runtime
     end
   end
 
+  def test_global_max_job_runtime_with_updated_value
+    freeze_time
+    with_global_max_job_runtime(10.minutes) do
+      klass = build_slow_job_class(iterations: 3, iteration_duration: 30.seconds)
+      with_global_max_job_runtime(1.minute) do
+        klass.perform_now
+        assert_partially_completed_job(cursor_position: 2)
+      end
+    end
+  end
+
   def test_per_class_max_job_runtime_with_default_global
     freeze_time
     parent = build_slow_job_class(iterations: 3, iteration_duration: 30.seconds)
@@ -358,57 +369,56 @@ def test_per_class_max_job_runtime_with_global_set
     end
   end
 
-  def test_max_job_runtime_cannot_unset_global
+  def test_per_class_max_job_runtime_with_global_set_lower
+    freeze_time
     with_global_max_job_runtime(30.seconds) do
-      klass = Class.new(ActiveJob::Base) do
-        include JobIteration::Iteration
+      parent = build_slow_job_class(iterations: 3, iteration_duration: 30.seconds)
+      child = Class.new(parent) do
+        self.job_iteration_max_job_runtime = 1.minute
       end
 
-      error = assert_raises(ArgumentError) do
-        klass.job_iteration_max_job_runtime = nil
-      end
+      parent.perform_now
+      assert_partially_completed_job(cursor_position: 1)
+      ActiveJob::Base.queue_adapter.enqueued_jobs = []
 
-      assert_equal(
-        "job_iteration_max_job_runtime may only decrease; " \
-          "#{klass} tried to increase it from 30 seconds to nil (no limit)",
-        error.message,
-      )
+      child.perform_now
+      assert_partially_completed_job(cursor_position: 1)
     end
   end
 
-  def test_max_job_runtime_cannot_be_higher_than_global
-    with_global_max_job_runtime(30.seconds) do
-      klass = Class.new(ActiveJob::Base) do
-        include JobIteration::Iteration
+  def test_unset_per_class_max_job_runtime_and_global_set
+    freeze_time
+    with_global_max_job_runtime(1.minute) do
+      parent = build_slow_job_class(iterations: 3, iteration_duration: 30.seconds)
+      parent.job_iteration_max_job_runtime = 30.seconds
+      child = Class.new(parent) do
+        self.job_iteration_max_job_runtime = nil
       end
 
-      error = assert_raises(ArgumentError) do
-        klass.job_iteration_max_job_runtime = 1.minute
-      end
+      parent.perform_now
+      assert_partially_completed_job(cursor_position: 1)
+      ActiveJob::Base.queue_adapter.enqueued_jobs = []
 
-      assert_equal(
-        "job_iteration_max_job_runtime may only decrease; #{klass} tried to increase it from 30 seconds to 1 minute",
-        error.message,
-      )
+      child.perform_now
+      assert_partially_completed_job(cursor_position: 2)
     end
   end
 
-  def test_max_job_runtime_cannot_be_higher_than_parent
-    with_global_max_job_runtime(1.minute) do
-      parent = Class.new(ActiveJob::Base) do
-        include JobIteration::Iteration
-        self.job_iteration_max_job_runtime = 30.seconds
+  def test_unset_per_class_max_job_runtime_and_unset_global_and_set_parent
+    freeze_time
+    with_global_max_job_runtime(nil) do
+      parent = build_slow_job_class(iterations: 3, iteration_duration: 30.seconds)
+      parent.job_iteration_max_job_runtime = 30.seconds
+      child = Class.new(parent) do
+        self.job_iteration_max_job_runtime = nil
       end
-      child = Class.new(parent)
 
-      error = assert_raises(ArgumentError) do
-        child.job_iteration_max_job_runtime = 45.seconds
-      end
+      parent.perform_now
+      assert_partially_completed_job(cursor_position: 1)
+      ActiveJob::Base.queue_adapter.enqueued_jobs = []
 
-      assert_equal(
-        "job_iteration_max_job_runtime may only decrease; #{child} tried to increase it from 30 seconds to 45 seconds",
-        error.message,
-      )
+      child.perform_now
+      assert_empty(ActiveJob::Base.queue_adapter.enqueued_jobs)
     end
   end