JobRescuer can overwrite a job that completed after the stuck-job fetch
Summary
JobRescuer selects stuck jobs while they are running, builds rescue
parameters from that snapshot, and later calls JobRescueMany. The rescue
update only checks the job id. If the original worker completes the job after
JobGetStuck returns but before JobRescueMany runs, the rescue update can
move the completed row back to retryable, or to another rescue outcome such
as discarded or cancelled.
Confirmed against River master at
4aa884e927fc07635605967729a288ac9416ebf1 on 2026-07-10 with Go 1.26.5
on Linux. I did not find an exact existing public report during a targeted
search.
Why this seems wrong
A successful worker completion should be terminal. Once a worker has committed
the row as completed, a maintenance pass based on an older running snapshot
should not rewrite the job state, append a rescue error, update
river:rescue_count, or clear/set finalized_at and scheduled_at.
The worker completion path already uses current-state guards, so adding a
current-state guard to rescue seems consistent with the surrounding design.
Relevant code
The stuck-job query filters to rows that are running only at read time:
-- name: JobGetStuck :many
SELECT *
FROM river_job
WHERE state = 'running'
AND attempted_at < @stuck_horizon::timestamptz
ORDER BY id
LIMIT @max;
The Postgres rescue update writes by id only:
-- name: JobRescueMany :exec
UPDATE river_job
SET
errors = array_append(errors, updated_job.error),
finalized_at = updated_job.finalized_at,
scheduled_at = updated_job.scheduled_at,
metadata = river_job.metadata || jsonb_build_object(...),
state = updated_job.state
FROM (...) AS updated_job
WHERE river_job.id = updated_job.id;
The SQLite rescue update has the same effective guard:
-- name: JobRescue :exec
UPDATE river_job
SET
errors = jsonb_insert(...),
finalized_at = cast(sqlc.narg('finalized_at') as text),
scheduled_at = @scheduled_at,
metadata = jsonb_set(...),
state = @state
WHERE id = @id;
Interleaving
- A job is
running and old enough for rescue.
JobRescuer calls JobGetStuck and receives the row.
- Before
JobRescueMany runs, the worker completes the row normally and
commits it as completed.
JobRescuer executes the stale rescue update by id and changes the same row
to retryable, discarded, or cancelled.
Reproduction
I reproduced the storage-level race on SQLite with a focused shared driver test
that:
- Creates a
running row old enough to be returned by JobGetStuck.
- Calls
JobGetStuck and keeps that stale fetched row id.
- Completes the row through
JobSetStateIfRunningMany, matching the normal
guarded worker completion write.
- Calls
JobRescueMany with the stale fetched row id.
- Re-reads the row and expects it to remain
completed.
Core test sequence:
stuckJobs, err := exec.JobGetStuck(ctx, &riverdriver.JobGetStuckParams{
Max: 10,
StuckHorizon: attemptedAt.Add(time.Hour),
})
require.NoError(t, err)
_, err = exec.JobSetStateIfRunningMany(ctx, setStateManyParams(
riverdriver.JobSetStateCompleted(job.ID, completedAt, nil),
))
require.NoError(t, err)
_, err = exec.JobRescueMany(ctx, &riverdriver.JobRescueManyParams{
ID: []int64{stuckJobs[0].ID},
Error: [][]byte{rescueErrorPayload},
FinalizedAt: []*time.Time{nil},
ScheduledAt: []time.Time{rescueScheduledAt},
State: []string{string(rivertype.JobStateRetryable)},
})
require.NoError(t, err)
jobAfterRescue, err := exec.JobGetByID(ctx, &riverdriver.JobGetByIDParams{ID: job.ID})
require.NoError(t, err)
require.Equal(t, rivertype.JobStateCompleted, jobAfterRescue.State)
Run:
go test ./riverdriver/riverdrivertest -run "TestDriverRiverSQLiteModernC/JobSetStateIfRunningMany_JobSetStateCompleted/JobRescueMany_DoesNotOverwriteCompletedJobFromStaleSnapshot" -count=1 -v
Current result:
Error: Not equal:
expected: "completed"
actual : "retryable"
Test: TestDriverRiverSQLiteModernC/JobSetStateIfRunningMany_JobSetStateCompleted/JobRescueMany_DoesNotOverwriteCompletedJobFromStaleSnapshot
The same update shape exists in the Postgres SQL, so the SQLite test is a
compact reproduction of the shared state-transition issue.
Expected behavior
The rescue update should only affect rows that are still running when the
rescue write executes. If a row has already completed, failed into another
final state, or otherwise left running, the rescuer should treat it as
already resolved and leave it unchanged.
Suggested fix
Add a current-state predicate to rescue updates, for example:
WHERE river_job.id = updated_job.id
AND river_job.state = 'running'
For SQLite, add the equivalent AND state = 'running' predicate to
JobRescue.
If the update returns/counts fewer rows than requested, those rows can be
treated as no-ops because they are no longer eligible for rescue.
JobRescuer can overwrite a job that completed after the stuck-job fetch
Summary
JobRescuerselects stuck jobs while they arerunning, builds rescueparameters from that snapshot, and later calls
JobRescueMany. The rescueupdate only checks the job id. If the original worker completes the job after
JobGetStuckreturns but beforeJobRescueManyruns, the rescue update canmove the completed row back to
retryable, or to another rescue outcome suchas
discardedorcancelled.Confirmed against River
masterat4aa884e927fc07635605967729a288ac9416ebf1on 2026-07-10 with Go1.26.5on Linux. I did not find an exact existing public report during a targeted
search.
Why this seems wrong
A successful worker completion should be terminal. Once a worker has committed
the row as
completed, a maintenance pass based on an olderrunningsnapshotshould not rewrite the job state, append a rescue error, update
river:rescue_count, or clear/setfinalized_atandscheduled_at.The worker completion path already uses current-state guards, so adding a
current-state guard to rescue seems consistent with the surrounding design.
Relevant code
The stuck-job query filters to rows that are
runningonly at read time:The Postgres rescue update writes by id only:
The SQLite rescue update has the same effective guard:
Interleaving
runningand old enough for rescue.JobRescuercallsJobGetStuckand receives the row.JobRescueManyruns, the worker completes the row normally andcommits it as
completed.JobRescuerexecutes the stale rescue update by id and changes the same rowto
retryable,discarded, orcancelled.Reproduction
I reproduced the storage-level race on SQLite with a focused shared driver test
that:
runningrow old enough to be returned byJobGetStuck.JobGetStuckand keeps that stale fetched row id.JobSetStateIfRunningMany, matching the normalguarded worker completion write.
JobRescueManywith the stale fetched row id.completed.Core test sequence:
Run:
Current result:
The same update shape exists in the Postgres SQL, so the SQLite test is a
compact reproduction of the shared state-transition issue.
Expected behavior
The rescue update should only affect rows that are still
runningwhen therescue write executes. If a row has already completed, failed into another
final state, or otherwise left
running, the rescuer should treat it asalready resolved and leave it unchanged.
Suggested fix
Add a current-state predicate to rescue updates, for example:
For SQLite, add the equivalent
AND state = 'running'predicate toJobRescue.If the update returns/counts fewer rows than requested, those rows can be
treated as no-ops because they are no longer eligible for rescue.