Skip to content

iteration: don't allow double-retrying a job#27

Merged
kirs merged 2 commits into
masterfrom
double-reenqueue
Dec 5, 2018
Merged

iteration: don't allow double-retrying a job#27
kirs merged 2 commits into
masterfrom
double-reenqueue

Conversation

@sirupsen

@sirupsen sirupsen commented Dec 4, 2018

Copy link
Copy Markdown
Contributor

Addresses #26

If a job ends up calling retry_job in its on_shutdown hook either directly or indirectly (through a retry extension that re-enqueues on execution error) and should_job_exit? is true (e.g. exceeding Iteration.max_job_runtime)—then the job will get double-enqueued. This can cause exponential job growth.

Example. Imagine that Iteration.max_job_runtime = 5.minutes and a job will take 2 hours to complete. This means there's a total of (2 * 60) / 5 = 24 interruptions. On each interruption, every job will spawn two jobs. This will lead to a total of 2^24 = 16 million jobs in just a span of about 2 hours.

Discussion. super unless @retried seems fairly heavy-handed, and I worry about how it'll interface with downstream dependencies. Likely, bumping this in shopify/shopify's test suite will reveal problems with this approach. That said, I can't think of a legit use-case for calling retry_job twice in the same context. This seems like a sensible thing to guard against and we've missed unless @retried repeatedly. That said, I'd be surprised if @kirs hasn't considered porting this into the monkey-patch since he's suffered the most pain with these conditionals for double-reenqueue. An alternative would be to try to teach retry logic about iteration, but that almost feels more ugly.

@sirupsen sirupsen requested review from GustavoCaso and kirs December 4, 2018 21:52
@kirs

kirs commented Dec 5, 2018

Copy link
Copy Markdown
Contributor

That said, I'd be surprised if @kirs hasn't considered porting this into the monkey-patch since he's suffered the most pain with these conditionals for double-reenqueue.

Huh https://github.com/Shopify/shopify/pull/168602 🙃

@kirs

kirs commented Dec 5, 2018

Copy link
Copy Markdown
Contributor

Thanks Simon!

@kirs

kirs commented Dec 5, 2018

Copy link
Copy Markdown
Contributor

CI passed on Shopify Core pointed to this branch.

@kirs kirs merged commit e22ada0 into master Dec 5, 2018
@kirs kirs deleted the double-reenqueue branch December 5, 2018 11:07
@kirs kirs temporarily deployed to rubygems December 5, 2018 11:14 Inactive
@GustavoCaso

Copy link
Copy Markdown
Contributor

Thanks, Simon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants