Fix/stale delivery reclaim - #1
Merged
Merged
Conversation
RefreshJobStatus aggregated task counts with a SELECT, computed the status
in Go, then wrote it with a separate UPDATE. Nothing wrapped the two, so
concurrent workers finishing tasks of the same job could lose an update:
W1 SELECTs (2 of 3 done) -> computes "running"
W2 SELECTs (3 of 3 done) -> computes "completed"
W2 UPDATEs status = "completed"
W1 UPDATEs status = "running" <-- stale write wins
The job then stays "running" forever, since once every task is terminal
nothing triggers another refresh. Workers call this on every task
completion and failure, so jobs with parallel tasks hit it regularly.
Compute the status in SQL with a CTE and update in the same statement.
The read of tasks and the write to jobs now share one timestamp, and as
an implicit transaction CockroachDB retries it internally on conflict.
A job with no tasks leaves the row untouched and returns n_total so the
"no tasks" and "no job row" errors stay distinguishable.
Tests (gated on CRDB_DSN, run against the three-node cluster):
- ParallelTaskCompletions completes every task of a job from parallel
workers and asserts the job ends "completed".
- StaleReadCannotWinTheLastWrite forces the interleaving above. Parallel
completions alone do not reproduce it: writes to the contended jobs
row are served in arrival order, so readers write in the order they
read. Running one refresh on a single-connection pool and queueing
behind that connection catches it between its read and its write,
which fails on the split read-then-write form every run.
- TerminalStates pins the status mapping across a rewrite of the SQL,
and UnknownJob covers the missing-row path that no longer comes from
RowsAffected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A pending marker is created when a task id is pushed to the ready list and cleared only after a worker successfully claims it. Two failure paths end a delivery without ever reaching that claim, and both leave the marker behind. The task is then queued, due, and absent from the ready list, while EnqueueDueTaskID reads the surviving marker as "already enqueued" and skips the LPUSH - so nothing re-delivers it and nothing releases the marker. Reclaim path: a worker dies mid-flight, ReclaimStaleRunningOnce returns the task to queued, but the marker from the original enqueue survives. Claim path: BRPOP takes the id off the ready list and the claim then fails to commit (during a CockroachDB node outage the worker logs "claim <id>: unexpected EOF"). The task never reaches 'running', so reclaim cannot rescue it either. Both sites now release the marker at the point the delivery is abandoned. The worker releases on a context independent of the caller's, since the task context is typically already cancelled or failing in exactly these cases. Releasing on a lost claim race is safe: the winner has already released the marker itself, and EnqueueDueTaskID re-checks status before pushing, so a running task is not re-enqueued. The 5 minute marker TTL bounded the damage - a stranded task recovered when the marker expired, not never - but a 5 minute stall on a due task is still a failure. The TTL stays as a backstop and is deliberately not raised: an early expiry only costs a duplicate BRPOP, because ClaimQueuedTask is atomic. Verified on the three-node cluster with `docker kill` under load. Before: 4 tasks stranded, all recovering exactly 5:00 later at marker expiry. After: 7354 tasks submitted, zero stranded, no markers left behind. Reclaim was exercised separately with a marker armed ahead of the task - recovery took ~10s against a live 300s marker, so the release did it, not the TTL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…invariant Reclaiming a stale running task rolls tasks.attempt back by one so losing a worker does not cost the task a retry. The next claim therefore re-issues the same attempt number, and the retried run writes a second task_runs row sharing (task_id, attempt_number) with its abandoned predecessor. That is intended, but it left history ambiguous: both rows were status 'failed', tellable apart only by matching on the reason string in error. Reclaim now closes the abandoned row as 'dead' instead of 'failed'. 'failed' means the handler ran and reported an error; 'dead' means no result was ever reported. Nothing reads task_runs.status today - RefreshJobStatus aggregates over tasks - and the column is a plain STRING with no constraint, so no migration is needed. The non-uniqueness of (task_id, attempt_number) is now stated everywhere someone might assume otherwise: a Conventions bullet and the status enum in INSTRUCTIONS.md, doc comments on ClaimQueuedTask, InsertTaskRun and ReclaimStaleRunningTask, and a comment above the task_runs DDL - the place someone stands when about to add a unique index. reclaim_dead_run_test.go asserts both halves: the abandoned run is 'dead' with finished_at and error set, attempt rolls back, the re-claim re-issues the same number, and inserting the retry's run under it succeeds. That last assertion is what fails if anyone later adds UNIQUE (task_id, attempt_number). Runs reclaimed before this change still read 'failed' and are not backfilled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TestIntegration_SubmitDelayedTask asserted on ReconcileOnce's return value and the ready list's length. Both aggregate over every due task in the database, which is shared with the rest of the suite and retains rows from earlier runs: measured on consecutive runs the count was 111, then 112, then 113, of which this test's task was one. The assertion passed on the strength of unrelated leftovers and could fail for reasons having nothing to do with delayed scheduling. Assert instead that this task id reaches the ready list, and poll to a deadline rather than sleeping a fixed 3s and reconciling exactly once: run_at is set from the test process's clock but due-ness is decided by the database's. On timeout report status, scheduled_at and now(), so a real regression does not read as a slow machine. Also assert the enqueue happens exactly once and clears the scheduled ZSET, which exercises the pending marker suppression this test did not cover before. Checked as a strengthening rather than a rewrite: with a backlog exceeding reconcileBatch and the ZSET path disabled, the old assertions pass (n=2, entirely from leftovers) while the new one fails. Both tests that submit now clean up their job row and Redis keys. They leaked one queued, due task per run - the pollution the old count assertions depended on - plus a ready list, which has no TTL and so never expired. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…urface The spec described the pending marker as "Released when a worker successfully claims the task". That sentence was the deadlock: it names only the happy path, so the two paths that end a delivery without a claim - a failed claim, and a reclaim abandoning an in-flight delivery - read as having no release obligation, which is exactly how tasks ended up queued, due, and unreachable. The marker is now documented as a lock on the delivery that whoever ends a delivery must release, with all three release paths listed, in INSTRUCTIONS.md and on ReleasePending itself. ReleasePending also notes that failure paths must pass a context the failure has not already cancelled. TryReservePending gains the TTL rationale, because "raise the TTL to be safe" is the intuitive move and the wrong one: the TTL is a backstop for a release that never happened, expiring early only costs a duplicate BRPOP that ClaimQueuedTask rejects, and expiring late stalls a task for its full duration. INSTRUCTIONS.md also states the asymmetry behind this: Redis claiming there is more work than there is self-corrects at claim time, but a marker claiming a delivery is in flight suppresses recovery instead of triggering it. EnqueueTaskIDs advertised "after TryReservePending" while ignoring the reservation result. That is correct on the submit path, where ids are new and cannot be in flight, but it reads as a dedup guarantee; the comment now says so and warns against copying it into a re-delivery path. Gaps found while sweeping, all pre-existing: - The three-node cluster was undocumented - no mention of docker-compose.multinode.yml or the make cluster-* targets anywhere. Added the multi-host DSN, port map, and the docker kill failover procedure. - WORKER_HEARTBEAT_INTERVAL is implemented and validated but appeared in no doc and no .env.example. - Two heartbeats share a name. WORKER_HEARTBEAT_INTERVAL is the per-process workers-table registry beat; the lease heartbeat that decides whether a running task gets reclaimed is Options.HeartbeatEvery, 5s, set in code, and cmd/worker passes the zero value so no environment variable reaches it. - GET /v1/workers and its active_since parameter were in neither doc. Verified against a running orchestrator rather than by reading: the endpoints answer 200/200/400 as documented, /metrics exposes the orchestrator_reconcile_* counters, HeartbeatEvery defaults to 5s. Comment-only in the Go files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
No description provided.