🐛 fix(kanban): reaper clears stale claims on dead workers in non-running lanes

PATCHES.md

@@ -69,5 +69,6 @@ default; do **not** retire such a row on a PR-merge signal. See the #44338 row.
 | fork-local (CI infra) | Daily upstream-sync GitHub Actions workflow that fetches/rebases the patch queue onto the newest upstream tag, runs tests, and opens a review PR (`.github/workflows/fork-daily-sync.yml`). Fork-only release engineering; never sent upstream. | permanent-local | v2026.6.19 |
 | fork-local (CI infra) | Secret-scan gate (gitleaks) that runs on every PR/push to `cwest/integration`, enforcing the fork's "no secrets, ever" invariant on our own commits (`.github/workflows/fork-secret-scan.yml`). Fork-only release engineering; never sent upstream. | permanent-local | v2026.6.19 |
 | fork-local (this manifest) | `PATCHES.md` itself — the manifest of everything the fork carries on top of its upstream base tag, plus the bucket model, the auto-retire rule, and the per-row behavior-keyed override. Kept indefinitely. | permanent-local | v2026.6.19 |
+| fork PR (TBD — upstream PR not yet opened) | Make the kanban crash-detector 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. `detect_crashed_workers` scanned only `WHERE t.status = 'running'`, so a card already in `review` carrying a dead `worker_pid` + `claim_lock` was invisible to the fast reaper; the review-column dispatch query gates on `claim_lock IS NULL`, so the next worker (the reviewer) could not spawn until `release_stale_claims`' TTL eventually freed it (observed live twice on PR #70 rework rounds; card `t_a29b93c2` had to be hand-cleared). The fix widens the scan to any card with a non-NULL `worker_pid` (status added to the projection) and, after the existing host-local + launch-grace + dead-PID checks pass, branches on status: a non-`running` card gets an **in-place claim clear** (`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; it is not yanked to `ready`), and NO run open/close, crash event, or failure-counter/breaker tick (those are `running`-crash semantics). The `running` path is byte-for-byte unchanged — its UPDATE stays guarded `status = 'running'` and only fires for `running` cards, since non-`running` ones `continue` out above. 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. Single-file change in `hermes_cli/kanban_db.py` + 4 regression tests in `tests/hermes_cli/test_kanban_db.py` (dead worker in `review` → claim cleared, status stays `review`, no failure count; review dispatch then permitted; within-grace claim NOT cleared; foreign-host claim untouched). Clean upstream candidate — a reaper that ignores non-running lanes is a general dispatcher bug, not homestead-specific. **Retire trigger:** carried as upstream-pending with NO upstream PR (deliberate — we do not surface this to NousResearch). Auto-retires if/when an equivalent lane-agnostic stale-claim clear appears in the base tag's `detect_crashed_workers` independently (the rebase will drop or empty the hunk); otherwise it stays as a carried fix. | upstream-pending | v2026.6.19 |
 | fork PR (TBD — upstream PR not yet opened) | Fix the kanban crash-detector reaping a re-claimed worker mid-init. `detect_crashed_workers` grants a freshly-spawned worker a launch-window grace (default 30s, `HERMES_KANBAN_CRASH_GRACE_SECONDS`) so its PID can become visible on `/proc` before liveness is checked — but it measured the grace from `tasks.started_at`, which is pinned to the task's first-ever start (`COALESCE` on every claim) and never refreshed on re-claim. A card re-claimed for its next lane (implement → review) therefore inherited a stale `started_at`, the grace had long since expired, and the new worker was reaped as `crashed (pid not alive)` before it finished initializing (plugin load); with `failure_limit=2` the card then landed in `blocked`/`gave_up` without the work ever being attempted. The fix measures the grace from the active `task_runs` row via `COALESCE(r.started_at, t.started_at)` joined on `current_run_id` — the **exact per-attempt pattern `enforce_max_runtime` already uses** (see `test_max_runtime_uses_current_run_start_after_retry`), so retries/re-claims get a fresh window; falls back to `tasks.started_at` when no run row is present (preserves first-claim behavior). Single-file change in `hermes_cli/kanban_db.py` + regression test `test_detect_crashed_workers_grace_uses_current_run_start_after_reclaim`. Proven live: under the fix the auto-dispatcher spawned a worker on a card with a 3600s-stale `started_at` and it reached its agent loop and completed (run 14, 12s) where pre-fix runs crashed mid-init. **Retire trigger:** open the upstream PR (phase 2), then auto-retire when it merges in a tagged release ≥ base; until then carry as upstream-pending. | upstream-pending | v2026.6.19 |
 | fork-local (no upstream PR) | Homestead-specific expansion of the `skills/github/github-code-review` skill for the kanban PR-review pipeline: the automated/non-interactive (webhook) review context, post-once idempotency guards (head-SHA dedup, blocked-timeout-is-unknown-not-failed, write-payload-to-file-before-POST), the humanizer/de-claude gate, and four reference files (`responding-to-and-resolving-review-threads` — the AUTHOR-resolves-threads loop incl. the Casey-2026-06-24 author-vs-reviewer decision; `webhook-triggered-reviews`; `consolidating-duplicate-reviews`; `editing-a-posted-review`) plus `scripts/commentable_lines.py`. This is the durable git home for material that previously lived ONLY in the `~/.hermes` deploy (silent drift). Deliberate divergence from upstream's generic review skill — homestead/kanban-specific, never sent upstream. **Retire trigger:** never auto-retires (permanent-local); remove only if the homestead PR-review pipeline is retired. | permanent-local | v2026.6.19 |

hermes_cli/kanban_db.py

@@ -5648,12 +5648,21 @@ def detect_crashed_workers(conn: sqlite3.Connection) -> list[str]:
     crash_details: list[tuple[str, int, str, bool, str]] = []
     # (task_id, pid, claimer, protocol_violation, error_text)
     with write_txn(conn):
+        # Scan ANY card carrying a worker_pid, not just ``running`` ones. A
+        # worker can die while its card sits in a non-``running`` lane (e.g.
+        # ``review`` after the implementer opened a PR and the card moved on),
+        # leaving a stale ``claim_lock`` + ``worker_pid``. Those cards must
+        # have the dead claim cleared too — otherwise the lane wedges for the
+        # full stale-claim TTL because dispatch gates on ``claim_lock IS
+        # NULL``. The status drives WHAT we do (running -> crash/requeue;
+        # non-running -> in-place claim clear, no lane change), not WHETHER we
+        # look. See the per-status branch below.
         rows = conn.execute(
-            "SELECT t.id, t.worker_pid, t.claim_lock, "
+            "SELECT t.id, t.status, t.worker_pid, t.claim_lock, "
             "       COALESCE(r.started_at, t.started_at) AS active_started_at "
             "FROM tasks t "
             "LEFT JOIN task_runs r ON r.id = t.current_run_id "
-            "WHERE t.status = 'running' AND t.worker_pid IS NOT NULL"
+            "WHERE t.worker_pid IS NOT NULL"
         ).fetchall()
         host_prefix = f"{_claimer_id().split(':', 1)[0]}:"
         for row in rows:
@@ -5683,6 +5692,37 @@ def detect_crashed_workers(conn: sqlite3.Connection) -> list[str]:
             if _pid_alive(row["worker_pid"]):
                 continue
 
+            # Non-``running`` lane: the worker that held this card died while
+            # it was parked somewhere other than ``running`` (most commonly
+            # ``review`` after a PR was opened). Clear the dead claim IN PLACE
+            # so the next worker for that lane can spawn on the following tick
+            # — but do NOT change the lane (a dead worker in ``review`` must
+            # STAY in ``review`` so the reviewer re-spawns; yanking it back to
+            # ``ready`` would lose the lane and re-trip respawn guards). This
+            # is a stale-claim cleanup, NOT a crash against the work item: it
+            # does not open/close a run, emit a ``crashed`` event, or touch
+            # the failure counter / circuit breaker (all of which are
+            # ``running``-crash semantics). The launch-grace and host-local
+            # checks above already protect a freshly-spawned worker here.
+            if row["status"] != "running":
+                cleared = conn.execute(
+                    "UPDATE tasks SET claim_lock = NULL, "
+                    "claim_expires = NULL, worker_pid = NULL "
+                    "WHERE id = ? AND status = ? AND worker_pid = ?",
+                    (row["id"], row["status"], row["worker_pid"]),
+                )
+                if cleared.rowcount == 1:
+                    _append_event(
+                        conn, row["id"], "stale_claim_cleared",
+                        {
+                            "pid": int(row["worker_pid"]),
+                            "claimer": row["claim_lock"],
+                            "lane": row["status"],
+                            "reason": "dead_worker_in_nonrunning_lane",
+                        },
+                    )
+                continue
+
             pid = int(row["worker_pid"])
             kind, code = _classify_worker_exit(pid)
             rate_limited_exit = False

tests/hermes_cli/test_kanban_db.py

@@ -912,6 +912,199 @@ def test_resolve_crash_grace_seconds_handles_bad_env(monkeypatch):
         )
 
 
+# ---------------------------------------------------------------------------
+# Stale-claim cleanup for dead workers in NON-running lanes (review wedge).
+#
+# A worker can die (or be killed) while its card sits in a non-``running``
+# lane — e.g. ``review`` after the implementer opened a PR and moved it on.
+# The dead worker leaves ``claim_lock`` + ``worker_pid`` populated. Because
+# the review-column dispatch query gates on ``claim_lock IS NULL``, the next
+# worker (the reviewer) can't spawn until the 1h stale-claim TTL frees it —
+# the lane wedges. ``detect_crashed_workers`` is the fast path that should
+# clear it, but it historically scanned only ``status='running'`` and so
+# never inspected the review card. The fix: clear a dead host-local
+# ``worker_pid``'s claim regardless of lane, WITHOUT a spurious lane change
+# (a dead worker in ``review`` keeps status=``review`` so the reviewer
+# re-spawns; it is NOT yanked back to ``ready``).
+# ---------------------------------------------------------------------------
+
+
+def test_reaper_clears_stale_claim_on_dead_worker_in_review_lane(
+    kanban_home, monkeypatch,
+):
+    """A dead host-local worker_pid on a ``review`` card has its claim
+    cleared in place, with status STAYING ``review`` (no lane change), so the
+    next reviewer can spawn on the following tick instead of waiting out the
+    1h TTL."""
+    import hermes_cli.kanban_db as _kb
+
+    monkeypatch.setattr(_kb, "_pid_alive", lambda _pid: False)
+    # No grace suppression: started_at far in the past.
+    monkeypatch.setenv("HERMES_KANBAN_CRASH_GRACE_SECONDS", "0")
+
+    with kb.connect() as conn:
+        host = _kb._claimer_id().split(":", 1)[0]
+        tid = kb.create_task(conn, title="review wedge", assignee="reviewer")
+        # Card sits in ``review`` with a dead host-local worker still holding
+        # the claim (the exact state an exited rework worker leaves behind).
+        conn.execute(
+            "UPDATE tasks SET status='review', worker_pid=?, claim_lock=?, "
+            "claim_expires=?, started_at=? WHERE id=?",
+            (
+                64646,
+                f"{host}:dead-worker",
+                int(time.time()) + 3600,  # TTL not yet expired
+                int(time.time()) - 3600,  # outside any launch grace
+                tid,
+            ),
+        )
+        conn.commit()
+
+        kb.detect_crashed_workers(conn)
+
+        task = kb.get_task(conn, tid)
+        assert task is not None
+        # Claim state fully cleared so review dispatch can re-claim it.
+        assert task.claim_lock is None, "claim_lock must be cleared"
+        assert task.worker_pid is None, "worker_pid must be cleared"
+        assert task.claim_expires is None, "claim_expires must be cleared"
+        # CRITICAL: no spurious lane change — stays in review.
+        assert task.status == "review", (
+            f"dead worker in review must STAY review, got {task.status}"
+        )
+        # The clear is not a crash against the work item: the failure
+        # counter / breaker must not be touched for a non-running clear.
+        assert task.consecutive_failures == 0, (
+            "clearing a non-running stale claim must not count a failure"
+        )
+
+
+def test_reaper_clears_stale_claim_then_review_dispatch_permitted(
+    kanban_home, monkeypatch,
+):
+    """After the reaper clears a dead worker's claim on a ``review`` card, the
+    card matches the review-column dispatch query (``status='review' AND
+    claim_lock IS NULL``) — i.e. a fresh reviewer spawn is now permitted."""
+    import hermes_cli.kanban_db as _kb
+
+    monkeypatch.setattr(_kb, "_pid_alive", lambda _pid: False)
+    monkeypatch.setenv("HERMES_KANBAN_CRASH_GRACE_SECONDS", "0")
+
+    with kb.connect() as conn:
+        host = _kb._claimer_id().split(":", 1)[0]
+        tid = kb.create_task(conn, title="review respawn", assignee="reviewer")
+        conn.execute(
+            "UPDATE tasks SET status='review', worker_pid=?, claim_lock=?, "
+            "claim_expires=?, started_at=? WHERE id=?",
+            (
+                64647,
+                f"{host}:dead-worker",
+                int(time.time()) + 3600,
+                int(time.time()) - 3600,
+                tid,
+            ),
+        )
+        conn.commit()
+
+        # Before the reaper: the stale claim hides the card from review
+        # dispatch (this is the wedge).
+        wedged = conn.execute(
+            "SELECT id FROM tasks "
+            "WHERE status = 'review' AND claim_lock IS NULL AND id = ?",
+            (tid,),
+        ).fetchone()
+        assert wedged is None, "precondition: stale claim wedges review dispatch"
+
+        kb.detect_crashed_workers(conn)
+
+        # After the reaper: the card is eligible for review dispatch again.
+        eligible = conn.execute(
+            "SELECT id FROM tasks "
+            "WHERE status = 'review' AND claim_lock IS NULL AND id = ?",
+            (tid,),
+        ).fetchone()
+        assert eligible is not None, (
+            "after reaper, review card must be re-claimable (claim cleared)"
+        )
+
+
+def test_reaper_respects_launch_grace_for_nonrunning_card(
+    kanban_home, monkeypatch,
+):
+    """A freshly-spawned worker on a ``review`` card (within the launch-grace
+    window) must NOT have its claim cleared, even if its PID isn't visible
+    yet — same protection as the running path."""
+    import hermes_cli.kanban_db as _kb
+
+    monkeypatch.setattr(_kb, "_pid_alive", lambda _pid: False)
+    monkeypatch.delenv("HERMES_KANBAN_CRASH_GRACE_SECONDS", raising=False)
+
+    now = 3_000_000.0
+    monkeypatch.setattr(_kb.time, "time", lambda: now)
+
+    with kb.connect() as conn:
+        host = _kb._claimer_id().split(":", 1)[0]
+        tid = kb.create_task(conn, title="grace review", assignee="reviewer")
+        conn.execute(
+            "UPDATE tasks SET status='review', worker_pid=?, claim_lock=?, "
+            "claim_expires=?, started_at=? WHERE id=?",
+            (64648, f"{host}:fresh", int(now) + 3600, int(now), tid),
+        )
+        conn.commit()
+
+        # Within the default 30s grace: claim must be preserved.
+        kb.detect_crashed_workers(conn)
+        task = kb.get_task(conn, tid)
+        assert task is not None
+        assert task.claim_lock is not None, (
+            "within launch grace, claim must NOT be cleared"
+        )
+        assert task.worker_pid == 64648
+        assert task.status == "review"
+
+        # Past the grace window: now the stale claim is cleared.
+        monkeypatch.setattr(_kb.time, "time", lambda: now + 60)
+        kb.detect_crashed_workers(conn)
+        task = kb.get_task(conn, tid)
+        assert task is not None
+        assert task.claim_lock is None, "past grace, stale claim must clear"
+        assert task.status == "review", "still no lane change after grace clear"
+
+
+def test_reaper_ignores_other_host_claim_in_nonrunning_lane(
+    kanban_home, monkeypatch,
+):
+    """A claim owned by a DIFFERENT host on a ``review`` card is left alone —
+    its PID is meaningless to this host's liveness check."""
+    import hermes_cli.kanban_db as _kb
+
+    monkeypatch.setattr(_kb, "_pid_alive", lambda _pid: False)
+    monkeypatch.setenv("HERMES_KANBAN_CRASH_GRACE_SECONDS", "0")
+
+    with kb.connect() as conn:
+        tid = kb.create_task(conn, title="other host", assignee="reviewer")
+        conn.execute(
+            "UPDATE tasks SET status='review', worker_pid=?, claim_lock=?, "
+            "claim_expires=?, started_at=? WHERE id=?",
+            (
+                64649,
+                "some-other-host:worker",
+                int(time.time()) + 3600,
+                int(time.time()) - 3600,
+                tid,
+            ),
+        )
+        conn.commit()
+
+        kb.detect_crashed_workers(conn)
+        task = kb.get_task(conn, tid)
+        assert task is not None
+        assert task.claim_lock == "some-other-host:worker", (
+            "foreign-host claim must not be touched"
+        )
+        assert task.worker_pid == 64649
+
+
 # ---------------------------------------------------------------------------
 # Rate-limit requeue: a worker that bails on a provider quota wall must be
 # released back to ``ready`` WITHOUT counting a failure, so a long (e.g.