Lookup global max_job_runtime at runtime

[sambostock]

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 PR 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.

See #240 (comment) for context.

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

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.

---

• Related to Ensure that maintenance tasks have a default max runtime maintenance_tasks#918

[simbasdad]

If I'm reading this code a month from now, I'm not sure it would be clear to me why we're doing this.

What is the advantage of this method vs doing 'job_iteration_max_job_runtime || JobIteration.max_job_runtimeinjob_should_exit?`

You could even do [job_iteration_max_job_runtime, JobIteration.max_job_runtime].compact.min to enforce that a job can't override the global default.

[adrianna-chang-shopify]

+1, I feel like this simplifies the logic, versus having to worry about whether the job-level config is shadowing the global one properly. I don't see that we explicitly mentioned anywhere that job_iteration_max_job_runtime defaults to the value of JobIteration.max_job_runtime, so I don't think changing the way this works would be a breaking change.

[Mangara]

I think I'm missing something about this. Wouldn't it always return the default, instead of the override?

Why define this method here, rather than with the other methods added by this class, such as job_should_exit??

[sambostock]

What's doing on here is that we're using class_attribute, and the way it implements "inheritable attributes" is that when you invoke the setter method, it redefines the method on the receiver.

So stepping through it, this is more or less what happens:

class ParentJob < ActiveJob::Base
  include JobIteration::Iteration
end

The include causes the included block to run which first invokes

class ParentJob < ActiveJob::Base
  class_attribute(
    :job_iteration_max_job_runtime,
    instance_writer: false,
    instance_predicate: false,
  )
end

which basically does

class ParentJob < ActiveJob::Base
  class << self
    def job_iteration_max_job_runtime
      nil # or whatever default is passed in
    end
    
    def job_iteration_max_job_runtime=(value)
      # ...redefine job_iteration_max_job_runtime to return value...
    end
  end
end

Because there's no way to pass in a dynamic default (we want it to delegate to JobIteration.max_job_runtime, not use the value at the time the method was defined; that's the bug), the only way to do that is to define the method ourselves, which is what this new part of included does

class ParentJob < ActiveJob::Base
  class << self
    def job_iteration_max_job_runtime
      JobIteration.max_job_runtime
    end
  end
end

From there, if a consumer invokes the setter method,

class ChildJob < ParentJob
  self.job_iteration_max_job_runtime = 1.minute
end

it will redefine the method, "shadowing" the parent one

class ChildJob < ParentJob
  class << self
    def job_iteration_max_job_runtime
      1.minute
    end
  end
end

The semantics of class_attribute match what we want, it just doesn't allow for a dynamic default, hence why we have to do this.

---

We can't just put the method in ClassMethods, because the method defined by class_attributes would shadow it, but it does occur to me we could put it in PrependedClassMethods, which should shadow the class_attributes implementation, while still allowing consumers to override it.

[Mangara]

I think part of my confusion comes from class_attribute adding both class and instance accessor methods. This implementation only overrides one of them, but not the other, right?