Fix `JobRetry` returning a stale row to the loser of a concurrent retry race by JackDanger · Pull Request #1410 · riverqueue/river

JackDanger · 2026-09-29T05:03:11Z

JobRetryTx has a concurrent-race version of the same bug the JobCancel fix 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 literal UNION fallback read — a plain (non-locking) scan that reads under the loser's pre-commit snapshot — and the loser is told the job is still cancelled/finalized even though the retry it waited on just committed and the row is available.

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 the JobCancel fix. Under REPEATABLE READ/SERIALIZABLE a 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.

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>

brandur · 2026-09-29T15:33:51Z

+
+		require.NoError(t, winnerTx.Commit(ctx))
+
+		loser := <-loserDone


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 uses context.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.

brandur · 2026-09-29T15:34:47Z

+		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")


@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.

brandur · 2026-09-29T15:36:57Z

+	// 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.


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.

brandur · 2026-09-29T15:37:54Z

+	// 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) {


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.

JackDanger · 2026-09-30T00:16:18Z

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>

river-rs verifier and others added 3 commits September 28, 2026 22:01

JackDanger force-pushed the fix/job-retry-stale-return branch from 71ba5ab to 2c65c11 Compare September 29, 2026 05:03

brandur reviewed Sep 29, 2026

View reviewed changes

brandur mentioned this pull request Sep 29, 2026

Fix JobCancel returning a stale row to the loser of a concurrent cancel race #1409

Draft

JackDanger force-pushed the fix/job-retry-stale-return branch from ad6f63b to 0e25e28 Compare September 30, 2026 00:25

Navigation Menu

Fix `JobRetry` returning a stale row to the loser of a concurrent retry race - #1410

Fix `JobRetry` returning a stale row to the loser of a concurrent retry race#1410
JackDanger wants to merge 4 commits into
riverqueue:masterfrom
JackDanger:fix/job-retry-stale-return

JackDanger commented Sep 29, 2026

Uh oh!

brandur Sep 29, 2026

Uh oh!

brandur Sep 29, 2026

Uh oh!

brandur Sep 29, 2026

Uh oh!

brandur Sep 29, 2026

Uh oh!

JackDanger commented Sep 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		require.NoError(t, winnerTx.Commit(ctx))

		loser := <-loserDone

Conversation

JackDanger commented Sep 29, 2026

Uh oh!

brandur Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

brandur Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

brandur Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

brandur Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

JackDanger commented Sep 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants