Include the error in fail_many_claimed events and document recovery - #779
Open
wintan1418 wants to merge 1 commit into
Open
Include the error in fail_many_claimed events and document recovery#779wintan1418 wants to merge 1 commit into
wintan1418 wants to merge 1 commit into
Conversation
Jobs running on a process that dies without a clean shutdown are marked as failed with ProcessPrunedError (or ProcessExitError when the supervisor reaps a crashed fork) via ClaimedExecution.fail_all_with, which already emits a fail_many_claimed event with the affected job and process ids. The error itself wasn't part of the payload though, so subscribers couldn't tell these mass failures apart from each other. Add the error to the event payload and to the log line, and document how to subscribe to the event to implement a custom recovery policy for these jobs, including why Solid Queue doesn't retry them automatically. Closes rails#591
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #591
Approach
Following the discussion in #591: not automatic retries (poison-pill risk, as @rosa pointed out), but the hook @nhorton asked for, so applications that know their jobs are idempotent can implement their own recovery policy.
It turns out most of the machinery already exists:
ClaimedExecution.fail_all_with— the path that marks in-flight jobs as failed when their process is pruned or a crashed fork is reaped — already emitsfail_many_claimed.solid_queuewith the affectedjob_idsandprocess_ids. What was missing was theerrorin the payload, so subscribers couldn't distinguish a prune (ProcessPrunedError) from other mass failures, and any documentation that this hook exists.This PR:
errorto thefail_many_claimedpayload (and to its log line, using the existingformatted_errorconvention).FailedExecution#retrywith an explicit note that the application must bring its own safeguard against retry loops.No new machinery, no behavior change for anyone not subscribing to the event.
Testing
Instrumentation and log subscriber test files green on MySQL.