Skip to content

Commit

Permalink
Lookup global max_job_runtime at runtime
Browse files Browse the repository at this point in the history
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 removes the static default passed the `class_attribute`, and
instead overrides the reader it provides with an implementation that
delegates to `JobIteration.max_job_runtime`, ensuring we use the current
global value at runtime.

It should be noted that this does mean it's possible for the restriction
on _increasing_ the `max_job_runtime` to fail to hold:

```ruby
class MyJob < ApplicationJob
  include JobIteration::Iteration
  self.job_iteration_max_job_runtime = 1.hour
  # ...
end
JobIteration.max_job_runtime = 1.minute
```

but this is unlikely to occur in practice outside of jobs classes
provided by gems (as the host app's classes would be defined after
setting the global max in an initializer), but gems probably should be
making decisions about max job runtime anyway.
  • Loading branch information
sambostock committed Nov 12, 2023
1 parent 742ee07 commit e6c3763
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

### Bug fixes

- [436](https://github.com/Shopify/job-iteration/pull/436) - 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
Expand Down
12 changes: 9 additions & 3 deletions lib/job-iteration/iteration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def inspected_cursor
end
end

included do |_base|
included do |base|
define_callbacks :start
define_callbacks :shutdown
define_callbacks :complete
Expand All @@ -49,10 +49,16 @@ def inspected_cursor
:job_iteration_max_job_runtime,
instance_writer: false,
instance_predicate: false,
default: JobIteration.max_job_runtime,
)

singleton_class.prepend(PrependedClassMethods)
class << base
prepend(PrependedClassMethods)

def job_iteration_max_job_runtime
# Lookup default every time, instead of closing around its value when the class is defined.
JobIteration.max_job_runtime
end
end
end

module PrependedClassMethods
Expand Down
11 changes: 11 additions & 0 deletions test/unit/iteration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit e6c3763

Please sign in to comment.