Repository navigation
Retry batched completeJobs/failJobs instead of exiting the retry loop on first failure - #645
Open
ujjawal-yadav wants to merge 1 commit into
Open
ujjawal-yadav wants to merge 1 commit into
ujjawal-yadav wants to merge 1 commit into
Conversation
batch()'s retry loop (governed by BATCH_RETRY_OPTIONS) ended its catch with `break`, so the retry code never ran: a single transient error (deadlock / serialization failure / dropped connection) on a batched completion tore the worker pool down and left the jobs locked, so they were re-executed later (duplicate execution). See graphile#636. Fall through to the next iteration on a *retryable* error so the completion is retried with backoff, and fail fast on a non-retryable error (throwing after a single attempt) rather than looping maxAttempts times - matching makeEnhancedWithPgClient.withRetries. Also clear `backpressure` in allgood() after resolving it so the pending refcount stays balanced across a retry-then-succeed sequence. Adds regression tests: a transient 40P01 deadlock on the batched completion is retried and the runner survives (asserting the retry actually fired), and a non-retryable error fails fast with a single attempt. Fixes graphile#636.
This branch has not been deployed
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.
Description
Fixes #636.
batch()retries batchedcompleteJobs/failJobsvia a loop governed byBATCH_RETRY_OPTIONS(20 attempts, backoff, per-code delays fromRETRYABLE_ERROR_CODES). But itscatchended with an unconditionalbreak, so the loop always exited after the first attempt — the backoff branch andBATCH_RETRY_OPTIONSwere dead code, and a batched completion/failure was never retried.A single transient error (e.g. a
40P01deadlock, a40001serialization failure, or a dropped connection) therefore tore the worker pool down and left the jobs locked. Since those jobs had already run their task, they were re-executed once the lock expired (duplicate execution).The fix makes the
catchbehave likemakeEnhancedWithPgClient.withRetries(used by the rest of the SQL layer): a retryable error falls through and is retried with backoff (up tomaxAttempts, then it throws); a non-retryable error fails fast. It also resets thebackpressuredeferred inallgood()so thependingrefcount stays balanced now that a hold/release cycle can repeat across retries.Tests
__tests__/batchRetry.test.ts, against real Postgres via the existing harness: a40P01deadlock on the first attempt is retried and the runner survives (asserting the completion was attempted ≥2 times) — fails onmain; and a non-retryable error fails fast with a single attempt.Notes
Complementary to #643 (fixes #641): that PR improves error reporting on the unbatched path and touches the
batch()call sites, whereas this fixes the retry loop insidebatch()— a different region ofmain.ts. Apatchchangeset is included.