Fix JobRetry returning a stale row to the loser of a concurrent retry race - #1410
Conversation
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.
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>
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>
71ba5ab to
2c65c11
Compare
|
|
||
| require.NoError(t, winnerTx.Commit(ctx)) | ||
|
|
||
| loser := <-loserDone |
There was a problem hiding this comment.
A little Codex suggestion here:
- Bound the second retry’s execution and result wait.
[loser := <-loserDone](https://github.com/riverqueue/river/blob/2c65c111f01820b96e282793a06004d1857513c8/client_test.go#L5849)is unbounded, and its query usescontext.Background(). Add a timeout context and a bounded receive so a locking regression produces a useful failure instead of reaching the package timeout.
Basically, in case this were not to be sent, you'd end up blocking the test forever.
Did you see the riversharedtest.WaitOrTimeout helper? This is a fairly common convention we use to work around this.
| require.Eventually(t, func() bool { | ||
| var waitEventType string | ||
| err := bundle.dbPool.QueryRow(ctx, | ||
| "SELECT COALESCE(wait_event_type, '') FROM pg_stat_activity WHERE pid = $1", loserPID). | ||
| Scan(&waitEventType) | ||
| return err == nil && waitEventType == "Lock" | ||
| }, 5*time.Second, 10*time.Millisecond, "loser retry never entered a lock wait on the job row") |
There was a problem hiding this comment.
@bgentry Wanted to flag this require.Eventually helper — not sure if it's new, but I've seen it crop up in a few tests now (currently there's 4x in the codebase).
Not sure I completely love it because it feels a bit too much like a sleep to me, but not sure.
| // Forces the interleaving deterministically: the winner's retryTx is held | ||
| // open while the loser's retry parks on the row lock (observed via | ||
| // pg_stat_activity). RED without a locking fallback read. | ||
|
|
||
| // JobRetryTx contract: "A retried job isn't visible to be worked until the | ||
| // transaction commits, and if the transaction rolls back, so too is the | ||
| // retried job" — a caller that waits for that transaction must therefore | ||
| // observe the committed outcome, not a pre-commit snapshot. | ||
| // | ||
| // The retry CTE (river_job.sql) serializes concurrent retries on a | ||
| // `SELECT ... FOR UPDATE`. When two retries race over one finalized row, the | ||
| // loser's update correctly matches zero rows (EvalPlanQual re-checks the | ||
| // "already available with a prior scheduled_at" guard against the winner's | ||
| // committed version), but the query's fallback UNION arm — `id NOT IN | ||
| // (SELECT id FROM updated_job)` — is a plain, non-locking re-read. It runs | ||
| // on the loser's statement snapshot, which predates the winner's commit, so | ||
| // the loser is handed back the stale pre-commit row: still `cancelled`, | ||
| // `finalized_at` still set, even though the retry it waited for is | ||
| // committed and the row is `available`. | ||
| // | ||
| // Unlike a wall-clock race, this interleaving is forced deterministically | ||
| // here: the winner retries inside an open transaction, the loser is parked | ||
| // on the row lock (confirmed via pg_stat_activity before proceeding), and | ||
| // only then does the winner commit. Red without a locking (or otherwise | ||
| // post-EPQ) read in the fallback arm. |
There was a problem hiding this comment.
Could you have your LLM collapse this a bit more?
Some descriptive info on a test case definitely doesn't hurt, but we're finding with LLMs it's way too easy to write these massive walls of text, and with no natural predators, we could expect them to proliferate hugely until they're so common that no one bothers reading any long-form comments anymore because they're not indicative of anything particularly special.
Should be easy to ask the LLM to digest it, keep the meat, but only the meat.
| // on the row lock (confirmed via pg_stat_activity before proceeding), and | ||
| // only then does the winner commit. Red without a locking (or otherwise | ||
| // post-EPQ) read in the fallback arm. | ||
| t.Run("ConcurrentRetryLoserSeesWinnersCommitNotStaleSnapshot", func(t *testing.T) { |
There was a problem hiding this comment.
Could you move this test into the shared driver suite (LLM should be able to do this quite easily)? It's mostly Postgres specific, but if it's a problem, it'd be a good idea to verify we're okay in every database backend.
|
Feedback incorporated - sorry for the LLM text-walls, those snuck by me. |
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>
ad6f63b to
0e25e28
Compare
No worries! Just one more — do you want to see if that test case can go in the shared driver suite? (https://github.com/riverqueue/river/pull/1410/changes#r4135363003) |
|
Okay, moved to Only on Postgres: SQLite |
0e245d0 to
43a7c11
Compare
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>
43a7c11 to
a441091
Compare
Consolidates new test helpers from: * riverqueue#1409 * riverqueue#1410
|
A pushed a couple of other changes up to add a missing call to the new test helper and consolidate the test helper a little with the new one from #1409. |
JobRetryTxhas a concurrent-race version of the same bug theJobCancelfix addressed (#1409). Two retriers (or a retry racing a cancel) hit the CTE: the loser's UPDATE matches zero rows, control falls through to the literalUNIONfallback read — a plain (non-locking) scan that reads under the loser's pre-commit snapshot — and the loser is told the job is stillcancelled/finalized even though the retry it waited on just committed and the row isavailable.Fix (second commit): make that fallback read take a row lock (
FOR UPDATE) so it re-reads past the winner's commit — same mechanism, same shape as theJobCancelfix. UnderREPEATABLE READ/SERIALIZABLEa raced retry becomes an error instead of a stale read; River's own transactions run READ COMMITTED.First commit is the failing test. It doesn't need timing luck to hit the window: the winner retries inside an open transaction, the loser's retry parks on the row lock (verified through
pg_stat_activity), then the winner commits and the loser returns — deterministic FAIL on current master, deterministic pass with the fix.