🐛 fix(kanban): reaper clears stale claims on dead workers in non-running lanes - #16
Conversation
…ing lanes A worker can die or be killed while its card sits in a non-`running` lane — most commonly `review`, after the implementer opened a PR and the card moved on. The dead worker leaves `claim_lock` + `worker_pid` populated, and because the review-column dispatch query gates on `claim_lock IS NULL`, the next worker (the reviewer) cannot spawn until the 1h stale-claim TTL in `release_stale_claims` eventually frees it — the lane wedges. Root cause: `detect_crashed_workers` scanned only `WHERE t.status = 'running'` and its release UPDATE was guarded `status = 'running'`, so a dead worker whose card had already moved to `review` was invisible to the fast reaper. The TTL was the only path that ever cleared it. Widen the scan to any card carrying a non-NULL `worker_pid` (status added to the projection). After the existing host-local, launch-grace, and dead-PID checks pass, branch on status: a non-`running` card has its claim cleared in place (`claim_lock`/`claim_expires`/`worker_pid` = NULL) with a `stale_claim_cleared` event and NO lane change — a dead worker in `review` stays in `review` so the reviewer re-spawns, rather than being yanked back to `ready`. The clear opens no run, emits no `crashed` event, and does not touch the failure counter or circuit breaker; those are `running`-crash semantics and stay byte-for-byte unchanged (non-`running` cards `continue` out before that machinery). Launch-grace (measured from the active `task_runs` row) and the host-local claim check still apply, so a freshly-spawned worker is not reaped mid-init and a foreign-host claim is left alone. Adds 4 regression tests: a dead worker in `review` has its claim cleared with status staying `review` and no failure counted; the card is then eligible for review dispatch; a within-grace claim is NOT cleared; a foreign-host claim is untouched. Recorded in PATCHES.md as upstream-pending — a reaper that ignores non-running lanes is a general dispatcher bug, a clean upstream candidate.
22962ba to
dc458d0
Compare
cwest
left a comment
There was a problem hiding this comment.
The fix lands where the wedge actually was. detect_crashed_workers only ever looked at running cards, so a dead worker whose card had already moved to review sat there holding claim_lock until the 1h TTL — and the review-column dispatch gates on claim_lock IS NULL, so no reviewer could spawn in the meantime. Widening the scan to any non-NULL worker_pid and branching on status is the right shape.
The branch ordering is correct: host-local, launch-grace, and dead-PID checks all run before the new code, so a freshly-spawned worker still gets its grace window and a foreign-host claim is left alone. The non-running clear sits behind a continue, so it never reaches the run open/close, the crashed event, or the failure-counter and breaker — those stay running-only. The running path's UPDATE is unchanged, still guarded on status = 'running'.
The in-place UPDATE is guarded on id, status, and worker_pid together, so if the row moved between the SELECT and the UPDATE the rowcount won't be 1 and no stale_claim_cleared event fires. No lane change means a dead worker in review stays in review and the reviewer re-spawns instead of getting yanked back to ready.
Verified by running the suite in a throwaway clone at the head commit: the four new tests pass and the full tests/hermes_cli/test_kanban_db.py is 230 passed. ruff is clean on both changed files. mergeable is MERGEABLE, every check is success, and there are no unresolved threads.
No changes needed.
…g lanes Clear a dead worker's stale claim regardless of lane, so a worker that dies while its card sits in a NON-running lane (most commonly review, after the implementer opened a PR and the card moved on) no longer wedges that lane for the full 1h stale-claim TTL. Widens the crash scan to any card with a non-NULL worker_pid and, for a non-running card, does an in-place claim clear with a stale_claim_cleared event and NO lane change; the running path is byte-for-byte unchanged. upstream-pending: fork PR #16
…g lanes Clear a dead worker's stale claim regardless of lane, so a worker that dies while its card sits in a NON-running lane (most commonly review, after the implementer opened a PR and the card moved on) no longer wedges that lane for the full 1h stale-claim TTL. Widens the crash scan to any card with a non-NULL worker_pid and, for a non-running card, does an in-place claim clear with a stale_claim_cleared event and NO lane change; the running path is byte-for-byte unchanged. upstream-pending: fork PR #16 (cherry picked from commit b5420f3)
Summary
A worker can die or be killed while its kanban card sits in a non-
runninglane — most commonly
review, after the implementer opened a PR and the cardmoved on. The dead worker leaves
claim_lock+worker_pidpopulated, and thereview-column dispatch query gates on
claim_lock IS NULL, so the next worker(the reviewer) cannot spawn until the 1h stale-claim TTL in
release_stale_claimseventually frees it. The lane wedges in the meantime.Root cause
detect_crashed_workersscanned onlyWHERE t.status = 'running'and itsrelease UPDATE was guarded
status = 'running'. A dead worker whose card hadalready moved to
review(or any non-runninglane) was invisible to the fastreaper — it never inspected the card, never cleared the stale lock. The TTL was
the only path that ever freed it. The reaper already keys liveness on
worker_pid(correct), but the status filter excluded the card before thatcheck ran.
Fix
Widen the scan to any card with a non-NULL
worker_pid(status added to theprojection). After the existing host-local, launch-grace, and dead-PID checks
pass, branch on status:
runningcard → clear the dead claim in place(
claim_lock/claim_expires/worker_pid= NULL) with astale_claim_clearedevent and no lane change — a dead worker inreviewstays in
reviewso the reviewer re-spawns, rather than being yanked back toready. No run is opened/closed, nocrashedevent is emitted, and thefailure counter / circuit breaker is not touched (those are
running-crashsemantics).
runningcard → unchanged. The crash/requeue/breaker machinery runsexactly as before; its UPDATE stays guarded
status = 'running'and only everfires for
runningcards (non-runningonescontinueout above).Launch-grace (measured from the active
task_runsrow) and the host-local claimcheck still apply, so a freshly-spawned worker is not reaped mid-init and a
foreign-host claim is left alone.
Tests
4 new regression tests in
tests/hermes_cli/test_kanban_db.py, E2E-styleagainst a temp
HERMES_HOMEwith the realkanban_dbimport:worker_pidon areviewcard → claim cleared, status staysreview, no failure counted;permitted);
reviewcard is not cleared;reviewcard is left untouched.Full
tests/hermes_cli/test_kanban_db.pygreen (230 passed); the 35crash/reaper/stale/grace/rate-limit tests — including the breaker-trip and
systemic-block paths — confirm the
running-crash semantics are unchanged.Scope
Single-file change in
hermes_cli/kanban_db.py+ tests + a PATCHES.mdupstream-pendingrow. A reaper that ignores non-running lanes is a generaldispatcher bug, not homestead-specific — a clean upstream candidate.