PR #810 follow-ups: UI-gate ledger, lock and worktree residuals nf-38p ← Beads

open priority 2 task unassigned

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

Suggestions from ninefold-reviewer's review of #810 (backgrounded UI gate + post-merge sync guard). None of these blocked that PR — each sat below the gate's severity floor across its seven review rounds and is recorded here so the tension lands in the tracker rather than the thread. Unclaimed and unlabelled; take any subset, or close this if none earns the time.

All line refs are against tools/pr/merge_pr.py as merged unless stated.

Ledger / --check-gate reporting
  1. The RUNNING arm never compares the recorded sha against current main. Every other arm does. A sweep of an older sha that is genuinely still in flight reports gate: RUNNING — it does name the sha it is sweeping, so the information is there, but the reader has to notice that it is not the head they asked about.
  2. A worker finishing while a second is queued on the lock leaves the ledger at the older sha, so --check-gate says MISSING while a sweep of the current head is genuinely queued behind the lock. Safe direction and transient (the second worker overwrites it on completion), but it reads as "nobody has swept this" when someone is about to.
  3. ui-gate.log is opened "ab" and never rotated — it accumulates the full captured output of every sweep this worktree ever launches, for the life of the session state dir.
  4. previous.get("pr", number) stores the argv *string* on a ledger re-read failure, so pr becomes "42" rather than 42 in that one path.
The lock
  1. Nothing re-touches ui-gate.lock after acquisition, so staleness is measured from when the holder started, not from whether it is making progress: a legitimately slow sweep becomes stealable at exactly GATE_RUNNING_STALE_AFTER_MIN (45) regardless. Headroom today is about 2x (design 005 measured 22m19s under one competing sweep), so this is an accepted bound rather than a live hazard — but it is the one remaining path back to round-4's two-workers-one-worktree race, and it is not written down where a reader of the lock would find it. A heartbeat os.utime between sweep phases, or a note on the constant, closes it.
  2. A steal whose lock.unlink() fails persistently busy-loops with no sleep. _acquire_gate_lock's steal path swallows the OSError and continues straight back to the O_EXCL create, which fails again immediately. Needs a permission or share-violation failure to trigger, and it lands in a detached background process where nothing would report it. One sleep(GATE_LOCK_POLL_SECONDS) on the unlink-failure path bounds it.
The -gate worktree
  1. git worktree remove --force is check=False and silent on failure. _gate_worktree's reuse branch recovers the tree on the next sweep, so this self-heals — but between the two, a surviving <topic>-gate is the phantom-session state round-2 F2 set out to remove, and nothing prints that it happened.
  2. _gate_worktree compares t.path == path on unresolved Paths where nflib.sessions uses _resolve + normcase for the same job. A mismatch fails in the safe direction (worktree add fails, the sweep is skipped loudly) but for a reason the operator would have to work out.
Docs and small surfaces
  1. tools/nflib/sessions.py:1393-1395 still quotes CLAUDE.md's old step-7 sentence — *"a 2-minute timeout kills the gate after the merge landed and reports success as failure"* — as an analogy for its own failure shape. The analogy still reads, but the sentence is no longer in CLAUDE.md; #810 replaced it. One-line requote.
  2. probe_gate()'s merge complete: <sha> print sits after the SKIP_PROBE_GATE=1 and missing-probe_gate.py early returns, so neither path prints the merge-outcome line #728 asked for. sync_main()'s merged #N covers the substance on both, which is why this never rose above a note.
  3. pid reuse can make a dead worker read as alive in both check_gate and _acquire_gate_lock. Unfixable cheaply without a start-time check; worth knowing the liveness answer is best-effort.
  4. check_gate(*, alive=_pid_alive) binds its default at def time, so patching M._pid_alive no longer reaches it. Correct today — every test injects — but it is a seam that behaves differently from the module-global lookup it replaced.

Dependencies

None.

Comments

No comments.

Add a comment