Ensure that maintenance tasks have a default max runtime

[simbasdad]

Prior to Shopify/job-iteration#240, the maximum runtime of an iteration was determined by reading JobIteration.max_job_runtime while the job was running. After this change, the value of that variable is cached with the job when the class is loaded.

The maintenance_tasks engine sets JobIteration.max_job_runtime to 5 minutes in an after_initialize block, unless it is already configured. Prior to the aforementioned PR, this was fine because the value was read at runtime. However, in staging and production environments where eager loading is used, MaintenanceTasks::TaskJob is loaded before the after_initialize block is run. If an application did not explicitly configure JobIteration.max_job_runtime, MaintenanceTasks::TaskJob will use the default value of nil provided by the job-iteration gem. This means maintenance tasks will never be pre-empted.

This change explicitly sets the job_iteration_max_job_runtime class attribute on MaintenanceTasks::TaskJob. If JobIteration.max_job_runtime has been configured, it uses that, otherwise, it defaults to 5 minutes.

I did not remove the after_initialize block, as applications that are not using eager loading may depend on the existing behaviour.

[adrianna-chang-shopify]

Hey @simbasdad ! Thanks for catching this -- I'm sure it must've been a bit tricky to nail down why tasks weren't pausing to that change in job-iteration 😩

What do you think about setting the value inside the after_initialize block instead? I believe something like this would work:

# lib/maintenance_tasks/engine.rb
config.after_initialize do
  JobIteration.max_job_runtime ||= 5.minutes
  MaintenanceTasks::TaskJob.job_iteration_max_job_runtime ||= JobIteration.max_job_runtime
end

This way, we keep our "default" logic together, and we we can rely directly on the value of JobIteration.max_job_runtime instead of having that magic "5 minutes" default encoded in multiple places.

[simbasdad]

What do you think about setting the value inside the after_initialize block instead?

I think that should work, and agree that keeping the "5 minutes" in one place is a good idea.

Not sure if I'm going to find the time to do this today, and I'm away next week. If you think this can wait, I'm happy to do it when I'm back. If you want to get out a fix sooner rather than later, I'm totally fine if you want to take over the PR.

[adrianna-chang-shopify]

@simbasdad I can tweak and merge this PR later today, or early next week at the latest! Thanks for bringing this up, though ❤️

[sambostock]

This will still result in us reading the config at the time the concern is included, which may be before the host application sets the value. I think it would make sense to make it dynamic by using a method instead.

-    included do
+    included do |base|

Suggested change

	self.job_iteration_max_job_runtime = JobIteration.max_job_runtime || 5.minutes
	class << base
	def job_iteration_max_job_runtime = JobIteration.max_job_runtime || 5.minutes
	end

although maybe that could be defined in class_methods instead... 🤔

---

From there, I think it would make sense to deprecate, and eventually remove

# lib/maintenance_tasks/engine.rb
config.after_initialize do
  JobIteration.max_job_runtime ||= 5.minutes

because really this gem shouldn't be setting a global config affecting other jobs (acknowledging the per-job option didn't exist at the time that was added).

We could also make that 5.minutes configurable as MaintenanceTasks.max_job_runtime, if we wanted, and just have 5.minutes as the default, but that's not a big deal. 🤷

[adrianna-chang-shopify]

Yeah, let's wait for Shopify/job-iteration#436 to ship, fixing in job-iteration is more correct anyways. I'd still prefer to see the default configured in our engine though (could be in an initializer block instead of after_initialize though, as discussed).

because really this gem shouldn't be setting a global config affecting other jobs

Completely agree. Initially I was wondering if we'd even need to deprecate it, but I guess you could end up with a change in behaviour: if you're using JobIteration.max_job_runtime to control the value, and we switch to using the class-level config, then our default could get picked up instead of the configured value in the app.

Deprecating might be tricky though, would we warn if the global writer is used and tell people to switch to using the class-level config on MaintenanceTasks::TaskJob?

[gmcgibbon]

It seems this has been fixed upstream in the job-iteration gem. I assume this can be closed, so I'll do that.