A killed review_now.py orphans its lock, and every later call then says a review is running when none is nf-ea1 ← Beads

open priority 2 task unassigned

Twinned from https://github.com/cfsh/ninefold/issues/611 by tools/beads/import_github.py's reconcile pass.

Filed unclaimed from nf-dev-history-charter. Hit while pulling Tier-0 review on #600; not covered by #581's five items.

What happened

./tools/pr/review_now.py 600 was killed mid-review by the caller's timeout (an agent's foreground command cap — 10 minutes, against a review the tool itself warns "takes MINUTES"). It left behind:

``` -rw-r--r-- 1 root root 0 2026-08-06 07:16:48 /root/.ninefold-reviewer/locks/pr-600.lock ```

Zero bytes. No PID, no start time, no owner of any kind. Every subsequent call for the next 45 minutes then returns:

``` #600: skip (review in flight, lock < 45m old)

ⓘ a review is already running (the scheduled tick got there first) — its verdict lands shortly, but not from this call. ```

No review was running. pgrep -af 'agents/ninefold-reviewer' showed dispatchers for #603 and #604 and none for #600, and no verdict ever landed.

Why it is worse than a stale lockfile

The message is confidently wrong in the one direction that costs the most time. It doesn't say "a lock exists and I can't tell if it's live" — it asserts a review *is* running and that *its verdict lands shortly*. The correct response to that message is to wait, so a session does: it waits 45 minutes for a verdict that was never coming, and the tool reaffirms the claim on every retry.

It also fires the failure mode #578 exists to prevent. review_now.py is the on-demand path *because* a PR that never got a review sits silently forever — which is why #568 (this session's whole job) went stale in the first place. An orphaned lock recreates that state for 45 minutes and papers over it with a reassuring message.

Killed mid-run is the expected case, not an edge case. Any agent driving this from a bash tool has a foreground cap shorter than a slow review on a contended box. This box has 2 cores and four live dev sessions; the review that eventually ran took long enough to be capped twice.

The lock carries nothing to reason with

Aging out at 45m is the only liveness signal there is, because the file is empty. Two cheap fixes, either one closing it:

  1. Write the owner into the lock — PID, start time, and --on-demand vs scheduled. Then a caller can check pgrep//proc/<pid> and distinguish *held* from *orphaned* instead of guessing from mtime.
  2. Hedge the message when the lock cannot be corroborated. If no dispatcher process for that PR exists, say so — "a lock is present but no reviewer process is running; it may be orphaned, --force to retake it" — rather than asserting a verdict is imminent.

--force appears in dispatch.py's own backoff message (tools/reviewer/dispatch.py:755), so the escape hatch may already exist; if so, the lock path should mention it, because the skip message currently offers no way forward at all.

Related: #581 items 1 and 2 are the same theme — review_now.py reporting a state more confidently than it can actually observe.

Workaround used here: verified no dispatch.py --on-demand 600 process existed, deleted the orphaned lock, re-ran.

Dependencies

None.

Comments

No comments.

Add a comment