review_now.py: a review lock survives its owning process dying, with no staleness signal to the caller nf-70b ← Beads

open priority 2 task unassigned

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

What happened

On PR #927, ran (mistakenly wrapped in a shell timeout, against the tool's own explicit warning — "If the trigger policy says a review is due this takes MINUTES ... don't kill it"):

``` timeout 100 ./tools/pr/review_now.py 927 ```

This killed the review_now.py wrapper at 100s, but the reviewer process it had dispatched for #927 either kept running detached or died without releasing /root/.ninefold-reviewer/locks/pr-927.lock. Every subsequent review_now.py 927 call (several, over about an hour) reported:

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

ps aux showed no process for PR #927 at all — only a dispatcher for an unrelated PR (#987). The lock file's mtime (stat) was ~62 minutes old with nothing touching it. It was a dead lock, not an in-flight review, and the "review in flight" message gave no way to tell the difference — no lock age, no liveness check against the PID that (presumably) created it.

./tools/pr/review_now.py 927 --force cleared it and got a real verdict immediately.

Why this is worth fixing
Suggested fix

review_now.py's "review in flight" outcome (or the dispatcher's lock check itself) should report the lock's age and/or check whether the PID that holds it is still alive, and say so explicitly — "lock held by a dead process, --force will clear it" reads very differently from "another invocation started 30s ago, wait for it."

Found incidentally while merging a pairing-worktree PR stack (#907). Filed unclaimed — not investigating the dispatcher internals myself.

Dependencies

None.

Comments

No comments.

Add a comment