From bb33191164d172a99b61087605b77d70629a10b8 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Sun, 12 Nov 2023 12:00:05 -0500 Subject: [PATCH] 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 --- CHANGELOG.md | 6 +++ lib/job-iteration.rb | 12 ++++- lib/job-iteration/iteration.rb | 38 ++++++---------- test/unit/iteration_test.rb | 80 +++++++++++++++++++--------------- 4 files changed, 74 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4322bf02..cfbc6c95 100644 --- 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 index ba524cc0..77e48ec2 100644 --- 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 index 11edf7bd..b58eb242 100644 --- 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 index 642b1f8c..27d60e1c 100644 --- 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