Repository navigation
Finish the heartbeat: recover the boot, and the worker as well as the row - #117
Merged
Merged
Conversation
… row Two blockers the heartbeat left behind, and the two smaller things beside them. All of it is one change, because all of it is #108's own machinery. Boot recovery had quietly stopped recovering anything (#111). The beat rewrites `started_at` every minute, so a crash leaves rows whose last sign of life is under a minute old, and the fifteen-minute window matches none of them -- the comment above the call, which says a restart is the one moment we know nothing is running, had become false. `reclaimAllRunning` says what boot actually means: every `running` row, with no clock in the query at all. Crash recovery goes back to instant from about 61 minutes. The lifetime bound freed the row and left the worker (#112). Nothing abandoned the handler, so a wedged job kept its worker parked in `await` for ever and the released row let a second worker come and hang on the same job -- one slot per cycle until the pool was gone and an ordinary job never ran again, while the row sat `queued` beside four idle-looking workers. The lifetime now races the handler, and a beat that matches no row ends the wait too, since the job is somebody else's by then. Abandonment is the worker leaving and nothing more: a promise cannot be cancelled, so the handler goes on holding its memory, its file handles and any child it spawned until it returns or the process ends, and a retry runs beside it. What is recovered is the slot, which is the part that was never coming back on its own. Handler-side deadlines are filed separately. The attempt-scoped writes also have to agree that the row is still `running`. `reclaimStale` requeues without touching the attempt count, so scoping to the attempt alone did not tell "somebody else has it" from "nobody has it yet", and a late `done` marked a requeued job finished. And zero rows is no longer discarded: it is the one clean signal that a job was taken away mid-flight, which is the case the scoping exists for. Finally, the retry backoff asks Postgres for the time. It wrote `run_at` from the app's clock while `claim` compares it against the database's -- the skew #108 moved the reclaim cutoff to Postgres to remove, relocated rather than removed. Closes #111 Closes #112 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the commit before this one. Abandonment went through `fail`, which requeues while attempts remain -- and the run it would be retrying is still going, because nothing can stop it. Five attempts at a handler that hangs is five live copies of it, five children and five copies of its memory, one every six hours. Slot starvation traded for a resource leak. So the lifetime bound is the one place the queue gives up on a job it could retry: the row is failed, with the reason in `last_error` and the attempts it did not use still showing, and a person decides whether to start it again. The test moves off `maxAttempts: 1`, which was the one setting that could not have caught this. Abandonment carries whether the row is still this worker's, too. A job taken away mid-flight is somebody else's, so there is nothing to write -- where before, the write went out anyway and warned that it had matched nothing, on the one path where matching nothing is expected. And two notes that were claiming more than the code does: the boot reclaim assumes a single instance, which is the only thing in this file that does, and the attempt-scoped writes are evidence of ownership rather than proof of it while an administrator can rewind the counter (#115). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review notes on the commit before this one, all of them about a comment that had stopped matching the code. The constant a reader looks up still described being let go of and reclaimed a quarter of an hour later, which is no longer what happens to a job that runs past it. The vigil also stops before the write that fails the row rather than after it, so a beat cannot land in the middle of that round trip and report a row it has just been failed out of. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third review round, and the one finding in it that matters: the write that fails a hung job sat inside the handler's own `try`, so a dropped connection under it fell through to the `catch`, which calls `fail`, which requeues while attempts remain. A database blip would have undone exactly the decision the branch above it exists to make, and sent a second copy after a handler that is still running. It has its own `catch` now: a row that cannot be marked failed is left `running` with its beat stopped, which is indistinguishable from a worker that died, and the hourly reclaim has known what to do with that since #107. The vigil now stops as soon as the race is decided rather than per branch, so no terminal write races a beat. A beat landing during the completion write found the row already `done` and warned that the job had been taken away -- on an ordinary success, which is the one place that warning must never appear if it is to mean anything. Two beats can be in flight at once when a round trip outlasts the interval, so the test no longer asks for exactly one of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A thrown abandonment update left the row running with its heartbeat stopped, so reclaim would start a second worker beside the handler still in this process.
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
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.
Closes #111. Closes #112. Both are blockers already on
mainfrom #108, found by anindependent confirming review of it.
latesttracksmain, so both are live for anyonetracking it.
They are one change: both are consequences of the heartbeat, both live in
jobs/queue.ts(plus one line of
index.ts), and the two significants from the same review are in thesame file and the same machinery.
What changed
#111 — the boot reclaim. The beat rewrites
started_atevery minute, so a crashleaves rows whose last sign of life is under a minute old, and the fifteen-minute window
matched none of them.
reclaimAllRunning()says what boot means: everyrunningrow, withno clock in the query at all. Crash recovery goes back to instant from about 61 minutes,
and the comment above the call is true again.
#112 — the wedged worker. The lifetime bounded the row and not the worker, so a hung
handler kept its slot for ever and the released row let a second worker come and hang on
the same job — one slot per cycle until the pool was gone. The lifetime now races the
handler, and a beat that matches no row ends the wait too.
Significant: the retry backoff asks Postgres for the time. It wrote
run_atfrom theapp's clock while
claimcompares it against the database's — the skew #108 moved thereclaim cutoff to Postgres to remove, relocated rather than removed.
Significant: a write that matches nothing is no longer discarded.
done,failandbeatall filtered on the attempt and none looked at the result. The guard is now the row,the attempt and
status = 'running'— the attempt alone could not tell "somebody else hasit" from "nobody has it yet", because
reclaimStalerequeues without touching the attemptcount, so a late
donemarked a requeued job finished. Zero rows is logged, and a beat thatmatches nothing stops the worker waiting, since the job is somebody else's by then.
What abandoning a handler does, and does not, do
A promise cannot be cancelled. Abandonment is the worker leaving, and nothing else:
memory, its file handles and any child process it spawned.
running, and by then the row isfailed. That is the reasoning Give the maintenance chores a cadence, and a rescuer the queue cannot strand #108 relied on, and itneeded the
runningpredicate to actually hold.on its own. That is the invariant: a slot is always recoverable without restarting the
process.
An abandoned job is failed, not requeued — the one place the queue gives up on a job it
could retry. Everywhere else a retry costs another run; here the run it would replace is
still going, so five attempts at a handler that hangs is five live copies of it. A job that
hung past its whole lifetime is for a person to look at.
Two edges that follow from that, neither hidden:
once its row is
failed, so the next tick asks for it and a fresh copy runs beside thehung one. The alternative is a chore that stops happening, which was The maintenance chores run once per boot and never again, so the trash is never swept #107.
failed, which is the set the bulk "retry failed" sweeps backto
queued. An operator clicking it while a hung handler is still live re-creates thesecond copy. Same family as Admin retry rewinds a running job's attempt count, so a worker can write over another worker's run #115.
Handler-side deadlines — the model fetch, the two
Bun.spawns — are what would stop anorphan holding real resources, and they are #116. The queue-level recovery does not depend
on them, and the next unbounded
awaitwould reintroduce #112 without it.Tests
Written first, watched fail (6 red against
main, two of them by timing out at fiveseconds, which is the pool death itself):
hourly window that frees nothing;
has to run anyway;
failedwith attempts to spare;done;bun install && bun run verify && bun run --filter '@imogen/web' build: 968 pass, 0 fail,web bundle built. No
bun.lockdiff and the SDK pin is untouched.Review
Three rounds at
medium, which is the cap. Round 1: three mediums — the boot reclaim'ssingle-instance premise (now stated where it is relied on), the abandonment requeueing a
hung handler (now
failed), and a write on a row already given away (now skipped). Round 2:documentation that had stopped matching the code. Round 3 found one that mattered — the
abandonment write sat inside the handler's
try, so a database blip under it fell intofailand requeued the job the branch had just decided must never be retried — plus a beatthat could warn "taken away" on an ordinary success.
Round 3 was not clean, so this is not self-merged. Its findings are fixed in b321a29 and
verify is green, but the cap says three rounds and a fourth is exactly what went wrong in
#108. It wants a human's eye before it merges.
Found, not fixed
another worker's run. It is what keeps the guard here evidence of ownership rather than
proof of it, and the fix belongs in the admin service, not the queue.
Left as nits:
reclaimStale's predicate has no supporting index now that it runs hourly,and
stop()still waits up to a whole lifetime for a wedged handler at shutdown.🤖 Generated with Claude Code