Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ActiveJob 5.2 retry logic for old jobs #36057

Merged
merged 1 commit into from Apr 23, 2019

Conversation

Projects
None yet
5 participants
@jhawthorn
Copy link
Member

commented Apr 22, 2019

Rails 6 introduces retries per-exception, instead of a global count of retries. Because ActiveJob 5.2 doesn't serialize the execution count per-exception, when ActiveJob 6.0 picks up an "old" job it can't know the exception count in the new format.

This can also be an issue if AJ 6.0 serializes a new job with exception_executions which is later picked up by AJ 5.2, which would clear exception_executions (since it has no knowledge of it).

Previously we handled this by resetting exception_executions, if it wasn't defined on a job, which could result in the worst case retrying the job 2x the times we should.

This commit changes how we handle loading a legacy job: instead of resetting exception_executions, we instead will always use the global executions count.

This way, jobs which only have one retry_on (and didn't have a behaviour change in AJ 6) are backwards-and-forwards-compatible with counts respected exactly.

Jobs with multiple retry_on will revert to the AJ5.2 behaviour if they were ever run under AJ5.2.

Fixes #35986

cc @rosa @cpruitt @kaspth @tenderlove @eileencodes 馃檱

@rails-bot rails-bot bot added the activejob label Apr 22, 2019

@eileencodes eileencodes added this to the 6.0.0 milestone Apr 22, 2019

@kaspth
Copy link
Member

left a comment

Is it possible to write a test job where we mutate the arguments after serialization, maybe?

Ideally we'd have a 5.2 job that we process on 6.0 and the other way around.

Perhaps something like the tests here is possible? https://github.com/rails/rails/pull/26017/files

Show resolved Hide resolved activejob/lib/active_job/exceptions.rb Outdated
Show resolved Hide resolved activejob/lib/active_job/exceptions.rb Outdated
@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Review @rosa

Use ActiveJob 5.2 retry logic for old jobs
Rails 6 introduces retries per-exception, instead of a global count of
retries. Because ActiveJob 5.2 doesn't serialize the execution count
per-exception, when ActiveJob 6.0 picks up an "old" job it can't know
the exception count in the new format.

This can also be an issue if AJ 6.0 serializes a new job with
exception_executions which is later picked up by AJ 5.2, which would
clear exception_executions (since it has no knowledge of it).

Previously we handled this by resetting exception_executions, if it
wasn't defined on a job, which could result in the worst case retrying
the job 2x the times we should.

This commit changes how we handle loading a legacy job: instead of
resetting exception_executions, we instead will always use the global
executions count.

This way, jobs which only have one retry_on (and didn't have a behaviour
change in AJ 6) are backwards-and-forwards-compatible with counts
respected exactly.

Jobs with multiple retry_on will revert to the AJ5.2 behaviour if they
were ever run under AJ5.2.

@jhawthorn jhawthorn force-pushed the jhawthorn:activejob_retry_logic branch from da04bf6 to 5d9359b Apr 22, 2019

@jhawthorn

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

Rebased and added tests.

Ideally we'd have a 5.2 job that we process on 6.0 and the other way around.

I couldn't quite do this, but I tested against two states we'd expect to see jobs which had come from AJ 5.2

@kaspth

kaspth approved these changes Apr 22, 2019

Copy link
Member

left a comment

Dig the extra test coverage!

I guess the main way to conceptualize this is that if Active Job 5.2 touches a job at any point exception_executions will be wiped on re-enqueue. So a job with max 5 retries enqueued on 6.0 and attempted 4 times on 6.0, if it then got picked up by 5.2 it would only see executions: 0. The job would then execute up to 2x of the max in the worst case as you mentioned.

@rosa

rosa approved these changes Apr 23, 2019

Copy link
Member

left a comment

Looks great, I really like this simple approach to support the transition 馃挴Thanks @jhawthorn!

@rosa

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@kaspth I think it's the other way around, from 5.2 to 6.0, because in 6.0 we still increment executions. A job enqueued on 6.0 and attempted 4 times on 6.0 would have executions == 4 when getting picked by 5.2, and will be retried the right number of times. However, if it starts in 5.2, and it's retried 4 times, and then picked up by 6.0, it'd have exception_executions == nil, defaulting to 0 and getting retried again (up to ~2x in the worst case as you said).

@eileencodes eileencodes merged commit 9f58023 into rails:master Apr 23, 2019

2 checks passed

buildkite/rails Build #60679 passed (9 minutes, 45 seconds)
Details
codeclimate All good!
Details
@kaspth

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@rosa Oh! I hadn't picked up that Rails 6 still incremented executions. But of course it does for backwards compatibility. Thanks for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.