Fix JobCancel returning a stale row to the loser of a concurrent cancel race - #1409
Merged
brandur merged 4 commits intoSep 30, 2026
Merged
Conversation
JackDanger
force-pushed
the
fix/job-cancel-stale-return
branch
from
September 29, 2026 05:01
256fce4 to
ded4f6d
Compare
Pin the documented "Returns the up-to-date JobRow" guarantee: two callers cancel the same job at the same moment, twenty rounds, and each loser must report `cancelled` with the winner's `finalized_at` — not its own pre-commit snapshot of the row. These tests fail until the locking fallback read lands. Signed-off-by: Jack Danger <github@jackcanty.com>
`JobCancel` documents "Returns the up-to-date JobRow". When two cancels race, the loser's update matches zero rows and the query falls through to its UNION fallback arm — a plain (non-locking) scan that runs on the loser's pre-commit snapshot, so the loser is returned e.g. still "scheduled" though "cancelled" is durably committed. Lock that fallback read (FOR UPDATE) so it re-reads the committed row: the same mechanism `locked_job` already applies at the top of the same statement, so no new lock ordering, wait, or deadlock surface. At REPEATABLE READ/SERIALIZABLE the locking read turns a raced cancel into a serialization error instead of a stale row; River runs READ COMMITTED. Regenerated both Postgres dialects with sqlc; SQLite's JobCancel is a single UPDATE .. RETURNING and unaffected. Signed-off-by: Jack Danger <github@jackcanty.com>
brandur
reviewed
Sep 29, 2026
| client.producersByQueueName[QueueDefault].testSignals.QueueControlEventTriggered.RequireEmpty() | ||
| }) | ||
|
|
||
| t.Run("ConcurrentCancelSingleFinalizerAndFreshReturn", func(t *testing.T) { |
Contributor
There was a problem hiding this comment.
Same as #1410, could probably use it in the shared driver test suite.
Per review: the test now lives in riverdrivertest and runs under every driver fixture (pgxv5, both database/sql Postgres dialects, and the SQLite family). The fixed-wait negative-event assertion does not survive the move; noted inline. Signed-off-by: Jack Danger <github@jackcanty.com>
Contributor
Author
|
Addressed feedback here, too. |
brandur
marked this pull request as ready for review
September 30, 2026 18:32
brandur
approved these changes
Sep 30, 2026
Contributor
|
Excellent, thanks! |
brandur
added a commit
to JackDanger/river
that referenced
this pull request
Sep 30, 2026
Consolidates new test helpers from: * riverqueue#1409 * riverqueue#1410
brandur
added a commit
that referenced
this pull request
Sep 30, 2026
…ry race (#1410) * Test `JobRetry` return under a concurrent-retry race JobRetry documents that a retried job becomes visible on commit of the retrier's transaction. When two retries race over one finalized row the CTE gets the database semantics right — the loser's update matches zero rows after EvalPlanQual re-checks the guards against the winner's committed version — but the loser is then served by the query's fallback UNION arm, a plain non-locking re-read running on the loser's statement snapshot. That snapshot predates the winner's commit, so the loser receives the stale pre-commit row: still cancelled with finalized_at set, for a retry that is already committed. The new subtest forces the interleaving deterministically (winner holds the row lock in an open transaction; the loser is confirmed parked on a lock wait via pg_stat_activity before the winner commits), so it fails on every run rather than probabilistically. A companion fix wraps the fallback read in a locking subquery, the mechanism the query's own locked read already relies on. * Test concurrent `JobRetry` calls see the committed retried row Pin `JobRetryTx`'s contract that a caller waits on the commit, not on its pre-commit snapshot: the winner retries inside an open transaction, the loser's retry is held on the row lock (observed via pg_stat_activity), then the winner commits — the loser must see the committed `available` row, not a stale finalized one. These tests fail until the locking fallback read lands. Signed-off-by: Jack Danger <github@jackcanty.com> * Fix stale `JobRetry` return to the loser of a concurrent retry race Two concurrent `JobRetryTx` calls (or a retry racing a cancel) hit the same shape the previous `JobCancel` fix: the loser's update matches zero rows and the CTE falls through to its non-locking fallback arm, which runs on the loser's pre-commit snapshot — the loser is served e.g. still `cancelled`/finalized even though the retry just committed and the row is `available`. EvalPlanQual re-checks the update's guard correctly; only the fallback read is stale. Same fix as `JobCancel`: lock the fallback read (`FOR UPDATE`). At REPEATABLE READ/SERIALIZABLE a raced retry becomes a serialization error instead of a stale read; River runs READ COMMITTED. Regenerated both Postgres dialects with sqlc; SQLite's JobRetry is a single UPDATE .. RETURNING and unaffected. Signed-off-by: Jack Danger <github@jackcanty.com> * Tighten retry race test per review Collapse the test comment to its two load-bearing sentences, bound the loser's wait and the winner's commit with testtimeout-style budgets, and receive the loser via the house `riversharedtest.WaitOrTimeout` helper. Signed-off-by: Jack Danger <github@jackcanty.com> * Move retry race test to the shared driver suite Per review: the regression now lives in riverdrivertest (client-level exercise, Postgres-gate skipping the SQLite variants since their `JobRetry` is a single non-locking UPDATE) and runs under both pgxv5 and riverdatabasesql — the latter's regenerated CTE previously had no regression coverage. Also drops the root copy, and strips the bespoke 5s budget in favor of `riversharedtest.WaitTimeout()`. Signed-off-by: Jack Danger <github@jackcanty.com> * Add missing test helper invocation * Consolidate two very specific driver test helpers in one Consolidates new test helpers from: * #1409 * #1410 --------- Signed-off-by: Jack Danger <github@jackcanty.com> Co-authored-by: river-rs verifier <verifier@river-rs.local> Co-authored-by: Brandur <brandur@brandur.org>
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
2 participants
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.
JobCanceldocuments "Returns the up-to-date JobRow", but when two callers cancel the same job at the same time, whichever call loses the race returns the job's pre-cancel state — e.g. stillscheduled— while the cancel is durably committed. The loser's UPDATE matches zero rows and the CTE falls through to its UNION fallback arm, a plain read that runs under the loser's pre-commit snapshot.Fix (second commit): make that fallback read take a row lock (
FOR UPDATE) so it re-reads past the winner's commit — the same mechanismlocked_jobalready applies at the top of the same statement, so this adds no new locking surface. (UnderREPEATABLE READ/SERIALIZABLEa raced cancel becomes an error instead of a stale read; River's own transactions run READ COMMITTED.)First commit is the failing test: two cancels of one scheduled-far-out job, 20 rounds, each round requiring both callers to observe
cancelledwith the winner'sfinalized_at. On current master, ~2–3 rounds of 10 observe the stale row (measured over three-count=10invocations on darwin/arm64), and all rounds pass with the fix.