Skip to content

hostlock: fix a zombie-anchor wedge and a live-holder theft, and make the seven properties falsifiable - #1830

Merged
justinchuby merged 5 commits into
mainfrom
squad/leon-hostlock-conformance
Aug 23, 2026
Merged

justinchuby merged 5 commits into
mainfrom
squad/leon-hostlock-conformance

Conversation

@justinchuby

@justinchuby justinchuby commented Aug 23, 2026 •

Copy link
Copy Markdown
Owner

The advisory host lock is merged (73c76458c, then 6a4db3f24 from Gaff's review). This PR closes the conformance gap against the seven required properties — and, in doing so, found two live defects in the merged implementation.

Three retractions, in order

An earlier revision of this PR said "No production change. The merged implementation already satisfies all seven requirements." That was wrong, and it was wrong in the reassuring direction. I wrote it after reading the code rather than after trying to break it. Opus review rejected the claim and named two of the seven as unproven; I reproduced both, and both are real bugs that were on main.

And then the fix for one of them was itself worse than the bug. A second review round found that rejecting process state Z reports a live holder dead — see "Defect 3" below. The first bug wedged the box; my fix let one agent steal another's box mid-benchmark, which is strictly the worse error and the exact thing the tool exists to prevent.

And the fix for that had the same shape again. The anti-theft guard I added in round two returned before the holder's own TTL was consulted, so a lock with a live anchor and no readable start time outlived any ttl forever — a bounded wedge replaced by an unbounded one, while fixing a theft. See "Defect 6".

And it recurred a fourth time, in the fix for that. The class round three made newly reapable — a live anchor, no start_time, a lapsed ttl — was then announced as a dead holder: status said "is gone" about a pid the guard had just verified was running, and the reaper logged "reaping stale lock from dead pid N", which suppresses the "still alive … both sets of numbers are now suspect" warning in exactly the case it applies to. Same defect, third arm. See "Defect 7". The same round found that the guard's liveness test ([ -d /proc/$a ]) accepts a zombie, so on the run path (ttl=0) a corpse got the anti-theft protection and the box wedged forever — strictly worse than the theft the guard replaced. See "Defect 8".

I also have to correct the previous commit message: it claimed "EXPIRED would be unreachable for that whole class" was fixed by round three. It was not. EXPIRED requires holder_alive, which requires a start_time, so it stayed unreachable for that class; only the wait half of that claim was true. This commit is what actually makes it reachable.

This is the same over-claim pattern four times: the conclusion that says "your existing data is fine", the fix that says "and now it's handled", the guard that says "and nothing can take it now", and the release note that says "and this whole class is handled". Each was published without a falsification attempt, and each was caught by review rather than by me. Recording all four rather than quietly editing the body.

Defect 1 — R6 (SIGKILL recovery) was not satisfied: a zombie anchor wedges the box permanently

holder_alive decided liveness from two facts: /proc/<pid> exists, and its start time still matches. A SIGKILLed process whose parent has not wait()ed for it is a zombie — and a zombie keeps both. So the lock reports HELD on a corpse, forever.

Reproduced directly (before → after on the same script):

status acquire
before HELD indefinitely rc=2 BUSY, no escape
after STALE rc=0, takeover

It is worst on the run path, which sets ttl=0 reasoning that "its own pid is exact". Exact — and still resolving, to a dead process. With ttl=0 there is no expiry escape hatch either, so the box stays locked until someone deletes the directory by hand.

This is the common shape here, not the exotic one. Every agent harness on this host launches long commands via subprocess.Popen without an immediate wait(). Roy's prefill_ab.py is one.

The damning detail: hostlock_test.sh's own alive() helper has excluded state Z since the day it was written. The test file knew the correct semantics; the implementation did not. Fix reads state (field 3) and starttime (field 22) from /proc/<pid>/stat in one pass and rejects Z.

Defect 2 — R5: the anti-theft grace did not cover its own stated rationale

reapable has an UNPARSEABLE_GRACE window whose comment names the case it exists for: a lock written by an older or newer version of the script. But the guard tested [ ! -s "$META" ] — zero-byte only. A meta with content but no readable anchor_pid/start_time — exactly that case — skipped the grace, fell through to ! holder_alive, and had a live holder's box stolen out from under it.

Reproduced: a demonstrably live holder reported STALE, acquire returned rc=0 and took the lock. After the fix, rc=2 and the owner is preserved. Live risk right now, because we are running different revisions of this script side by side in different worktrees.

Also aligned lock_state: an unverifiable-but-ungraced lock now reports HELD, matching what acquire actually does. Still exactly four states.

Defect 3 — my own fix: state Z is not proof of death

When a thread-group leader exits via pthread_exit() while other threads keep running, /proc/<tgid>/stat reports Z for a fully live process. ps shows Zl ... <defunct>; Threads: says 2. Reproduced directly:

LIVE-but-Z leader:  pid=701267  state=Z  Threads=2   ps: Zl python3
TRUE zombie:        pid=681666  state=Z  Threads=1

So the previous commit's [ "$state" != Z ] || return 1 reported a live holder dead, and the next acquire reaped it mid-run. Reachable via acquire --pid <harness> — the flag R1 tells everyone to use. Threads: separates the two cases exactly; an unreadable status file now fails toward alive.

Defect 4 — the widened grace only deferred the theft it was meant to stop

reapable threw away a readable, running anchor_pid and fell back to lock age. The grace is 300 s; the runs it protects are 40 minutes. So a live holder running a different script revision kept its box for five minutes and lost it at 301 s — the fix covered 5 minutes of a 40-minute exposure, and my R5 test asserted at t≈0 only. It now returns "not reapable" whenever the anchor pid is present and running, and consults the clock only when there is no usable pid.

Related, same species as the retraction above: a takeover that happened only because a lock aged out of the grace now reports takeover=unverifiable, not stale_pid. The script never established the holder was dead.

Defect 5 — status told a blocked agent the box was abandoned

lock_state was fixed to say HELD for an unverifiable lock; cmd_status's human branch still keyed off holder_alive. cmd_acquire dumps that text on the BUSY path, so what a blocked agent actually read was:

hostlock: BUSY
STALE  holder alice pid=680653 is gone; next acquire will reap it

with pid 680653 alive. Telling a human the box is abandoned is how the box gets taken by hand. Driven from lock_state now.

Defect 6 — the anti-theft guard voided the holder's own TTL

Round two added: a readable, running anchor pid is never reaped on the clock. It returned before holder_expired was consulted. So a lock with a live anchor and no readable start_time outlived any ttl forever — wait blocked on a lock everyone agreed had expired, EXPIRED became unreachable for that whole class of lock, and the file's own contract (--ttl S hard expiry: a lock older than this is reapable by the next acquirer) was silently void.

The anti-theft property only needs the clock-based grace disabled, not the expiry the holder itself asked for. Verified with ttl=1, acquired 4000 s ago, live anchor: rc=2 before, rc=0 now.

Defect 7 — the third arm: a live holder announced as a dead one

Round three made the live-anchor/no-start_time/expired-ttl class reapable, and then described that takeover as a reap of a corpse:

$ hostlock.sh status                    # pid 3312 is RUNNING
STALE  holder alice pid=3312 is gone; next acquire will reap it
$ hostlock.sh acquire --owner leon
hostlock: reaping stale lock from dead pid 3312 (owner alice)

A takeover from a live holder is the one event this tool can cause that corrupts somebody's benchmark, and it is the one message that must never be downgraded to routine housekeeping. The correct output is the warning that already existed and was being skipped:

EXPIRED (held 4000s > ttl 1s; next acquire will take it over) by alice pid=3312 …
hostlock: WARNING taking over a lock held by alice (pid 3312, still alive) after its 1s TTL expired
hostlock: WARNING if alice is still benchmarking, both sets of numbers are now suspect

This is the third arm of one defect: HELD said "is gone" (fixed in commit 2), EXPIRED said "is gone" (fixed in commit 3), STALE said it in the reaper (fixed here). The root cause is that four call sites each decided "is this pid alive?" for themselves. Liveness is now one predicate, pid_is_live(), shared by holder_alive, the anti-theft guard, lock_state and the reaper, so they cannot disagree.

Defect 8 — the anti-theft guard protected a corpse, forever

The guard tested [ -d "/proc/${a}" ]. A zombie passes that: /proc/<pid> exists until the parent wait()s, and no parent here ever does. On the run path ttl=0, so holder_expired is never true — meaning the clock grace, the only thing that would ever have released the box, was disabled on behalf of a dead process. Unbounded wedge, in the guard written to prevent an unbounded wedge.

Fixed by routing that guard through the same pid_is_live() as everything else, so a dead anchor falls through to the clock. Verified with a real zombie (fork, SIGKILL, no wait) at ttl=0: BUSY inside the 300 s grace, recoverable past it.

Mutations that stayed green — my assertions were weaker than my claims

Two in the first round, five in the second, five more in the third, three more in the fourth. All found by review, none by me.

runnable_now() { echo 1; } — suite stayed green. My R2 test only pinned the occupancy figure into [1,10000] and never compared it to load the test itself created. The consequence is not a cosmetic column: a constant 1 silently satisfies every --gate N for N>=1, disabling the only mechanism that sees load from agents who never took the lock. A -f4→-f2 field slip is equally invisible. Now the test starts six bounded spinners and requires the reported occupancy to move with them.

pkill -9 -f ... inserted into reap_if_dead — suite stayed green. My R7 structural guard was grep -c '^[^#]*\bkill\b', which has three holes: \b does not match pkill/killall/killpg; -c counts lines, so a second kill appended to the sanctioned line is invisible; and ^[^#]* cannot cross a #, so a kill placed after any # anywhere on the line — including inside a string — is hidden. That last one fails in the false-PASS direction. Comments are now stripped first, then the whole kill family is counted by occurrence — except that the repair had the same false-PASS hole as the original: echo "reaping # " ; kill -9 "$p" is one shell command and no comment at all, but stripping at any # that begins a word truncates it. Only whole-line comments are stripped now, so a kill named in a trailing comment counts — a false FAIL, the safe direction — and timeout -s/timeout -k and xargs are matched. It is a net for concrete spellings, not a proof: K=kil; "${K}l" -9 $p evades any grep by construction, which is why the count is exact rather than a threshold and why the behavioural orphan test exists.

Five more green mutations, second round:

  • "the wrapped command is really stopped" was inert. It ran after the runner had exited and been reaped, and an orphan reparents to pid 1, so pgrep -P "$runner" answers 0 whether or not the child was killed. Reproduced by making run_teardown never signal: test passed, sleep 60 still ALIVE-ORPHANED under pid 1. Now the child pid is captured before signalling.
  • held_uid was compared against the test's own id -u. Everything here runs as one user, so uid=$(id -u) in the writer — reporting the reader's uid, destroying the field's only purpose — passed. Now anchored to pid 1, which is root and is never us.
  • runnable_at_acquire only had to be numeric. echo "runnable_at_acquire=1" passed, in the very block whose comment argues at length that "is a number" is satisfied by a constant. The load-based version I first wrote could not distinguish stored at acquire from re-read at print time on a host busy for other reasons — and this host is. The stored value is now forged on disk and must be echoed verbatim.
  • UNPARSEABLE_GRACE was entirely unpinned. Every dependent assertion created its lock immediately before checking, so now - mtime == 0 and any grace ≥ 1 passed; cutting 300 → 2 left the suite green. Now backdated on both sides of the window.
  • run_teardown's start-time comparison had no test at all, so it could be weakened back to "the pid exists" silently.

Five more green mutations, third round:

  • No fixture had an anchor_pid that was present and dead with no start_time. Every other one either carries a start_time or has no anchor_pid at all. So deleting [ -d "/proc/${a}" ] from the new guard — which makes every unparseable lock permanently unreapable, the exact wedge this branch exists to avoid — was invisible. The tell was already in the file: a dead_pid computed and then discarded with : "$dead".
  • The teardown start-time check was pinned by grep -c, which counts lines. Appending || [ -d "/proc/$child" ] leaves the literal intact and the count at 1, restoring "the pid still exists" at the one place this script signals anything. Now asserted as a canonicalised golden function body — brittle to legitimate refactoring, which is a false FAIL and the safe direction.
  • Only the HELD arm of cmd_status's human branch was asserted. EXPIRED is by definition a lock whose holder is alive, so putting "is gone; next acquire will reap it" there is the same false-abandonment message one arm over.
  • UNPARSEABLE_GRACE was bracketed at 30 s and 4000 s — two orders of magnitude around 300. Now 200 s and 400 s.
  • timeout --signal=, timeout --kill-after= and fuser -k evaded the R7 family regex. Unlike the acknowledged ${K}l indirection these are plausible future edits ("escalate the teardown"). Added.

Three more green mutations, fourth round:

  • cmd_status's STALE arm had no assertion at all. Replacing its entire body with echo "FREE (nobody is here)" left the suite green — every STALE check in the file went through porcelain state=. That is the one arm where saying FREE is actively dangerous: a stale lock still covers a host that may be loaded, and "next acquire will reap it" is the sentence that sends a human through the tool instead of round it.
  • The "present and dead" fixture was neither. dead_pid() is fully reaped, so /proc/<pid> does not exist — it is the absent-and-dead shape, and it silently tested a different branch than its comment claimed. The genuinely present-and-dead shape is a zombie, which is what Defect 8 turns on, and there was no fixture for it. Both shapes are now tested, and they reach the clock by different routes.
  • cmd_run's trap ordering was unpinned. The golden function body added in round three is a body, not a position: moving RUN_CHILD_START=$(…) back above the three trap lines leaves that golden text byte-identical while reopening the window it exists to close — a signal arriving during the fork that reads the child's start time takes bash's default action, leaking the lock and orphaning a forty-minute benchmark. Now asserted positionally.

Two more, found by the new assertions rather than by review:

  • <"/proc/$pid/stat" 2>/dev/null silences tr, not bash. Redirections apply left to right, so the input redirection fails and is reported before 2> is applied: every status on a stale lock printed two lines of No such file or directory in front of its output, and anything parsing that output got them too. Found because the new STALE text assertion is anchored — ^STALE against 2>&1. 2>/dev/null now comes first, and "status writes nothing on stderr for a departed pid" is asserted directly.
  • The primary assertion for the primary fix could pass vacuously. "A live holder with no start_time still honours its own ttl" asserts acquire returns 0; if any earlier assertion in that block had already let the lock be reaped, it returns 0 for the wrong reason. It now asserts the fixture's starting state first.

And the coverage-count hole itself. Two checks sit behind environment probes (/proc/1 readability, uid), and an assertion that quietly stops running is indistinguishable from one that passes — the same failure mode as the inert R1 block and the unasserted STALE arm. Both probe branches now assert something, so the total is environment-invariant, and the total is pinned: deleting any single chk from the file now fails the suite by name. Verified by deleting one.

Also fixed, and not a mutation: the pid-1 held_uid check hardcoded 0 and asserted id -u != 0. That fails outright as root — the default in most CI containers — and wherever pid 1 is not root, for reasons that have nothing to do with this script. The expectation is derived from /proc/1/status now, and the check skips itself when pid 1 is unreadable or is us, because then it discriminates nothing.

The seven requirements

# requirement before now
R1 holder is the outer harness, spanning all A/B/null arms untested asserted, and the rejected design pinned as rejected
R2 rows carry lock state, owner identity, occupancy snapshot partial occupancy must track load the test creates
R3 FREE / HELD / STALE / EXPIRED distinct isolation only distinctness asserted
R4 fail closed on wait and admission expiry admission only both, incl. acquire --wait (the path run uses)
R5 dead-holder PID + start-time reaping one direction both, plus a fixed theft bug, and takeovers from a live holder are warned about rather than logged as reaps
R6 SIGKILL recovery forged lock, never a real kill real kill and a real zombie, plus two fixed wedge bugs
R7 no process killing untested behavioural + structural, kill-family-wide

R1 — the old test was inert

The previous R1 block passed unchanged with its arms deleted and with $$ substituted for $BASHPID. Passing --pid explicitly makes a test insensitive to every anchoring decision it claims to be about. Rewritten as two harnesses differing only in which pid is the anchor, arms exiting normally, sampled in the gap between arms — which is exactly the moment Sebastian ran ps, saw no bench process, and concluded the host was clear. Per-arm anchoring must read STALE; harness anchoring must read HELD; and the two anchorings are asserted to genuinely disagree.

R6 — a forged lock never exercises the transition

Fabricating a lock from an already-dead PID asserts the reading of a stale lock, not the becoming of one. And my first attempt at a real kill still hid the zombie bug, because sig ...; wait ... reaps the zombie — it measured bash's reaping, not the lock's recovery. The test now keeps a real zombie alive (a parent that Popens, SIGKILLs, and never wait()s), confirms state Z, then requires HELD → STALE → rc=0 takeover with ttl=0.

R7 — the requirement whose violation would be catastrophic

Reclaiming a lock does not stop the load; the dead holder's benchmark is still on the cores. The tempting "fix" is for the reaper to kill it — which would make this tool capable of destroying a colleague's forty-minute run on the strength of a misparsed PID. A reap must leave the orphan running. The orphan is now named via exec -a, so a pattern-kill is caught behaviourally as well as structurally.

Also in this PR

  • provenance emits runnable_at_acquire. The data was already written by publish_lock and silently dropped on the way out, so a row could only sample occupancy after the measured window closed. It can now bracket it.
  • provenance emits held_uid, read from /proc/<anchor>/status. This is the honest half of R2's "owner identity": --owner is self-declared free text and corroborates nothing — --owner roy from my shell produces a row that says roy. The uid is the one identity the kernel vouches for. This is an advisory lock, so identity is for attribution in a published row, not enforcement, and the row should not make the two look like the same kind of fact.
  • run_teardown verifies the child's start time before signalling. It is the only place this script signals anything, and pids on this box are cycling at ~1.5M in four days. The run traps are now installed before that read, so a signal arriving in the fork window cannot leak the lock and orphan the benchmark.
  • --gate 0 is documented as unsatisfiable by construction — field 4 of /proc/loadavg counts the process doing the reading, so the floor is 1 on a perfectly idle box. Documented alongside it: the lock is the primary admission signal and the gate is secondary. A bounded good citizen on 4 of 32 cpus reads runnable 4–5 and trips --gate 3, while a single-threaded 100% hog reads ~1 and sails through. That revises guidance I gave the team earlier — still right against the loadavg EMA, wrong as an admission test.
  • .hostlock-selftest* is gitignored; an aborted run left scratch files untracked.
  • The test header no longer claims the suite "costs no measurable CPU". The R2 occupancy assertion deliberately spins 6 of 32 cpus for ~5 s, and someone running the suite next to a colleague's A/B on the strength of that sentence would inject exactly the contamination this tool exists to prevent.

Falsification

Tests 115 → 190. A conformance suite that cannot fail is worse than none, so every requirement is mutation-tested:

mutation property tests red
anchor defaults to $$ instead of $PPID R1 17
runnable_now() returns a constant 1 R2 1
held_uid echoes the declared owner R2 1
held_uid reports the reader's uid, not the holder's R2 1
runnable_at_acquire re-read at print time R2 2
STALE collapsed into EXPIRED R3 8
acquire --wait expiry returns success R4 3
grace narrowed back to zero-byte meta only R5 3
UNPARSEABLE_GRACE 300 → 2 R5 2
reapable ignores a live anchor pid R5 2
unverifiable takeover claims stale_pid R5 1
status human branch keys off holder_alive R5 2
status EXPIRED arm says the holder is gone R5 2
live-anchor guard voids the holder's own ttl R5 1
live-anchor guard drops the liveness test R5 2
start-time comparison weakened to "the pid exists" R5 2
zombie check [ "$state" != Z ] deleted R6 2
state Z treated as dead unconditionally R6 3
pkill -9 -f added to reap_if_dead R7 2
timeout -s KILL smuggled into reap_if_dead R7 1
run_teardown refuses every kill R7 1
run_teardown check weakened to "the pid exists" R7 1
run_teardown guard weakened with an ` `
fuser -k smuggled into reap_if_dead R7 1
anti-theft guard accepts a zombie anchor (-d /proc) R5/R6 1
lock_state drops the unverifiable-live arm R3/R5 3
reaper calls a live unverifiable holder dead R5 3
cmd_status STALE arm says FREE (nobody is here) R3 3
pid_is_live treats every zombie as alive R6 3
pid_is_live treats every state Z as dead R6 3
/proc read restored to <file 2>/dev/null (stderr noise) R2 2
cmd_run reads the start time before installing its traps R7 1
(meta) one chk deleted from the suite coverage 1

Baseline green at 208/208 in the same harness, immediately before and after every mutation. 33 mutations, all red.

Two mutations stay green, both recorded in the source rather than hidden, because a green mutation is otherwise indistinguishable from a missing test:

  1. Dropping the -d /proc/<pid> check in holder_alive is semantically equivalent — proc_start_time already fails for a missing /proc entry. Not a coverage gap.
  2. Flipping pid_is_live's unreadable-Threads: arm from "no evidence of death, keep the lock" toward theft. This one is a real gap and stays one. Producing a Z process with a readable stat and an unreadable status needs hidepid= or a lost microsecond race; the alternative is a HOSTLOCK_PROC hook in production, which would itself be a way to spoof liveness and steal a lock. What has changed since the last revision of this note is the argument for accepting it: it no longer rests on [ "" -le 1 ] exiting 2, which was true but incidental. The non-numeric case is now an explicit case arm that returns "alive" and says why, so the safe direction is written down rather than inherited from a test builtin's error behaviour — and, as a bonus, it no longer prints [: abc: integer expression expected on hostlock's stderr.

shellcheck clean on both files. No crate code; the suite is sleep-bound and holds no cores, apart from the one deliberate ~5 s six-thread burst that the R2 occupancy assertion needs.

Known gap, filed rather than smuggled in here: nothing runs this suite in CI — it is referenced by no workflow, and there is no shellcheck job. That is a separate concern from the conformance gap this PR closes, and it wants its own PR and its own argument about cost (the suite is ~3 minutes of mostly sleep).

@codecov

codecov Bot commented Aug 23, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.29%. Comparing base (39b6b18) to head (16058dc).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1830      +/-   ##
==========================================
- Coverage   80.61%   80.29%   -0.33%     
==========================================
  Files         414      414              
  Lines      203362   203362              
  Branches   203362   203362              
==========================================
- Hits       163941   163287     -654     
- Misses      33869    34522     +653     
- Partials     5552     5553       +1     
Flag Coverage Δ
cli-ort-linux 72.51% <ø> (ø)
cli-ort-windows 72.01% <ø> (ø)
mlas 85.10% <ø> (-0.14%) ⬇️
offline 80.42% <ø> (-0.34%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 12 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@justinchuby
justinchuby force-pushed the squad/leon-hostlock-conformance branch from 0b3feb1 to a52fe2f Compare August 23, 2026 12:07
@justinchuby justinchuby changed the title hostlock: assert the seven required properties directly hostlock: fix a zombie-anchor wedge and a live-holder theft, and make the seven properties falsifiable Aug 23, 2026
justinchuby and others added 5 commits August 23, 2026 15:46
The lock's requirements were confirmed by three host collisions, but
three of the seven had no test at all. They were properties of the
design that nothing would have noticed the loss of -- which is the same
failure mode as a benchmark that reports a count it measured and stays
silent about the placement it did not.

No production change: the merged implementation already satisfies all
seven. This makes that auditable, and falsifiable.

R1 the holder is the outer harness, spanning every arm -- WAS UNTESTED.
   The existing test only proved the lock outlives the script that took
   it. The collision it needs to encode is different: an observer ran
   `ps`, saw the benchmark gone, concluded the host was clear, and
   started work -- but that process had exited only because the harness
   had advanced from one arm of an interleaved A/B to the next. Any
   liveness notion scoped to "is a bench binary running" has that hole
   by construction. A harness now holds the lock across three arms and
   an observer samples in each gap, where no bench process exists; all
   three gaps must read HELD and every row must name the harness. The
   negative half is what makes that meaningful, so it is asserted too: a
   lock anchored to a bench CHILD goes STALE the instant that arm ends.

R2 emitted rows carry identity, state and occupancy. All ten fields are
   asserted individually, so dropping any one goes red -- previously
   only `held_by` was checked and `--oneline` survived deleting the
   rest.

R3 FREE/HELD/STALE/EXPIRED distinct. Each existed in isolation; nothing
   asserted they do not collapse. STALE and EXPIRED are the pair that
   matters: one means the box may still be loaded by an orphan, the
   other means somebody is actively using it and has overrun.

R4 both expiries fail closed. Admission expiry was covered. Wait expiry
   was covered only for the `wait` subcommand, not for `acquire --wait`
   -- which is the path `run`, and therefore every harness, goes
   through. Now asserted for both, including that `run` does not execute
   the command.

R5 reaping compares start time, not just the pid. Now asserted from both
   sides: a LIVE pid carrying the wrong start time (what pid reuse looks
   like) must read STALE, and the same pid with the right start time
   must read HELD and be unreapable.

R6 SIGKILL recovery -- WAS UNTESTED against a real kill. Crash recovery
   used a forged lock built from an already-dead pid, which never
   exercises the transition. A real holder is now really SIGKILLed --
   the one signal the design cannot trap and therefore the one it
   promises to survive.

R7 the lock never kills anything it did not start -- WAS UNTESTED.
   Reclaiming a lock does not stop the load: the dead holder's benchmark
   is still on the cores. The tempting fix is for the reaper to kill it,
   which would make this tool capable of destroying a colleague's
   forty-minute run on the strength of a misparsed pid. A reap must
   leave the orphan running, and structurally there must be exactly one
   `kill` in the file, targeting the command the script itself started.

Tests 115 -> 151. Seven mutations verified red, one rejected as
equivalent rather than counted:

  holder_alive always true                       16 red
  acquire anchors to $$ instead of $PPID         17 red
  STALE collapsed into EXPIRED                    7 red
  start-time comparison dropped                   2 red
  occupancy dropped from the row                  2 red
  acquire --wait expiry returns success           2 red
  reaper kills the dead holder's process group    1 red

Dropping the `-d /proc/<pid>` check in holder_alive left the suite green
and is NOT a coverage gap: proc_start_time already fails for a missing
/proc entry, so the check is redundant and the mutation is semantically
equivalent. Recording it because a green mutation is otherwise
indistinguishable from a missing test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…review

I claimed the merged lock already satisfied all seven required properties and
that this branch was tests-only. That was wrong, and wrong in the reassuring
direction: I had read the code rather than tried to break it. Review rejected
the claim and named two properties as unproven. Both are real defects that
were on main, and both are reproduced here.

R6 (SIGKILL recovery) was not satisfied. holder_alive decided liveness from
"/proc/<pid> exists" plus "the start time matches", and a SIGKILLed process
whose parent has not wait()ed keeps both, because it is a ZOMBIE. The lock
reported HELD on a corpse forever. Worst on the `run` path, which sets ttl=0
on the reasoning that its own pid is exact -- exact, and still resolving to a
dead process, with no expiry escape. Every agent harness on this box launches
via Popen without an immediate wait(), so this is the common shape. The test
file's own alive() helper has excluded state Z since the day it was written;
the implementation had not. proc_state_and_start() now reads state and
starttime in one pass and rejects Z. Verified: HELD/rc=2 before, STALE/rc=0
after.

R5: UNPARSEABLE_GRACE did not cover its own stated rationale. The comment
names the older-or-newer-script case, but the guard tested for a ZERO-BYTE
meta, so a meta with content and no readable anchor_pid/start_time skipped the
grace, fell through to !holder_alive, and stole a LIVE holder's box. Verified:
a demonstrably live holder read STALE and acquire returned 0; now rc=2 and the
owner is preserved. lock_state agrees with acquire on that case (HELD, not
STALE); still exactly four states.

Two of my conformance assertions were also weaker than their labels, both
found by review and neither by me:

- runnable_now() { echo 1; } left the suite green. The consequence is not a
  cosmetic column: a constant silently satisfies every --gate N for N>=1 on a
  saturated box, disabling the only mechanism that sees load from agents who
  never took the lock. The test now creates load and requires the reported
  occupancy to move with it.
- pkill -9 -f inserted into reap_if_dead left the suite green. The R7 guard
  was grep -c '^[^#]*\bkill\b': \b does not match pkill/killall/killpg, -c
  counts lines so a second kill on the sanctioned line is invisible, and
  ^[^#]* cannot cross a #, hiding a kill placed after any comment character
  -- a false-PASS. Comments are stripped first and the whole family counted
  by occurrence.

The R1 block was inert: it passed with its arms deleted and with $$ for
$BASHPID, because passing --pid explicitly makes a test insensitive to every
anchoring decision it is about. Rewritten as two harnesses differing only in
the anchor, arms exiting normally, sampled in the gap between arms -- the
moment an observer runs ps, sees no bench process and concludes the host is
clear.

Also in this change:

- provenance emits runnable_at_acquire, which publish_lock already wrote and
  the reader silently dropped, so a row could only sample occupancy after the
  measured window closed. It can now bracket it.
- provenance emits held_uid from /proc/<anchor>/status. --owner is
  self-declared free text and corroborates nothing; the uid is the one
  identity the kernel vouches for.
- run_teardown verifies the child's start time before signalling. It is the
  only place this script signals anything, and pids here are cycling at ~1.5M
  in four days.
- --gate 0 is documented as unsatisfiable by construction (field 4 of
  /proc/loadavg counts the reader), along with the lock-is-primary,
  gate-is-secondary argument: a bounded good citizen on 4 of 32 cpus reads
  runnable 4-5 and trips --gate 3, while a single-threaded 100% hog reads ~1
  and passes.
- .hostlock-selftest* is gitignored; an aborted run left it untracked.

Tests 115 -> 166. Ten mutations verified red, including the two that were
green before this change. shellcheck clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…efects

Second review round on this branch. The headline is that the fix in the
previous commit introduced a worse bug than the one it repaired, in the
direction that matters most.

State Z is not proof of death. When a thread-group leader exits via
pthread_exit() while other threads keep running, /proc/<tgid>/stat reports Z
for a fully LIVE process -- `ps` shows `Zl ... <defunct>`, Threads: says 2.
Rejecting Z outright therefore reported a live holder dead, and the next
acquire reaped it mid-benchmark. The bug I fixed only WEDGED the box; this one
STEALS it, which is the exact outcome the tool exists to prevent. Reachable
via `acquire --pid <harness>`, which is the flag R1 tells everyone to use.
Threads: separates the two cases exactly, and an unreadable status file now
fails toward "alive". Verified: pid in state Z with Threads=2 stays HELD and
a second acquirer gets rc=2.

The widened UNPARSEABLE_GRACE also only DEFERRED the live-holder theft it was
meant to stop. reapable threw away a readable, running anchor_pid and fell
back to lock age, so a live holder under a different script revision kept its
box for 300s and lost it at 301 -- against runs that are 40 minutes long. It
now returns "not reapable" whenever the anchor pid is present and running, and
uses the clock only when there is no usable pid.

Related, and the same species of over-claim as the PR body I already had to
retract: a takeover that happened only because a lock aged out of the grace
now reports takeover=unverifiable, not stale_pid. The script never established
the holder was dead; stale_pid asserts it did.

cmd_status's human branch still keyed off holder_alive while lock_state had
been fixed, and cmd_acquire dumps that text on the BUSY path -- so a blocked
agent was told "holder is gone; next acquire will reap it" about a box the
tool itself refuses to reap. Telling a human the box is abandoned is how the
box gets taken by hand. It is driven from lock_state now.

Test defects, all of which left the suite green:

- "the wrapped command is really stopped" was inert. It ran after the runner
  had exited and been reaped, and an orphan reparents to pid 1, so
  `pgrep -P "$runner"` answered 0 whether or not the child was killed.
  Capture the child pid before signalling, then assert on it.
- held_uid was compared against the test's own `id -u`. Everything here runs
  as one user, so `uid=$(id -u)` in the writer -- reporting the READER's uid
  and destroying the only purpose of the field -- passed. Now anchored to
  pid 1, which is root and is never us.
- runnable_at_acquire only had to be numeric, and the load-based version of
  the check could not distinguish "stored at acquire" from "re-read at print
  time" on a host that is busy for other reasons, which this one is. The
  stored value is now forged on disk and must be echoed verbatim.
- UNPARSEABLE_GRACE was entirely unpinned: every dependent assertion created
  its lock immediately before checking, so age was 0 and any grace >= 1
  passed. Cutting 300 to 2 left the suite green. Now backdated on both sides
  of the window.
- run_teardown's start-time comparison had no test at all.
- The R7 structural guard's repair had the SAME false-PASS hole as the
  original: `echo "reaping # " ; kill -9 "$p"` is one command and no comment,
  but stripping at any `#` that begins a word truncates it. Only whole-line
  comments are stripped now -- a kill named in a trailing comment counts,
  which is a false FAIL and the safe direction -- and timeout -s/-k and xargs
  are matched. It is a net for concrete spellings, not a proof: `K=kil;
  "${K}l"` evades any grep, which is why the count is exact and why the
  behavioural orphan test exists.

Also: install the run traps before reading the child's start time, so a
signal in that window cannot leak the lock and orphan the benchmark. And the
test header no longer claims the suite "costs no measurable CPU" -- the R2
occupancy assertion deliberately spins 6 of 32 cpus for ~5s, and someone
running the suite next to a colleague's A/B on the strength of that sentence
would inject exactly the contamination this tool exists to prevent.

Tests 166 -> 184. Nineteen mutations verified red across both commits.
shellcheck clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… test gaps

Third review round. Same shape as the last two: the fix was correct about the
thing it was aimed at and wrong one step to the side.

The `reapable` short-circuit added last commit -- "a readable, running anchor
pid is never reaped on the clock" -- returned before `holder_expired` was ever
consulted. So a lock with a live anchor and no readable start_time outlived
any ttl FOREVER: `wait` blocked on a lock everyone agreed had expired, EXPIRED
became unreachable for that whole class, and I had replaced a bounded wedge
with an unbounded one while fixing a theft. The anti-theft property only needs
the CLOCK-BASED grace disabled, not the expiry the holder itself declared.
Verified: ttl=1, acquired 4000s ago, live anchor -- rc=2 before, rc=0 now.

Test gaps, all of which left the suite green:

- No fixture had an anchor_pid that was PRESENT and DEAD with no start_time.
  Every other one either carries a start_time or has no anchor_pid at all. So
  deleting `[ -d "/proc/${a}" ]` from the new guard -- which makes every
  unparseable lock permanently unreapable, the exact wedge this branch exists
  to avoid -- was invisible. The tell was already in the file: a `dead_pid`
  computed and then discarded with `: "$dead"`.
- The teardown start-time check was pinned by `grep -c`, which counts LINES,
  so appending `|| [ -d "/proc/$child" ]` left the literal intact, the count
  at 1, and R7 back to "the pid still exists" at the one place this script
  signals anything. Now asserted as a canonicalised golden function body.
  Brittle to legitimate refactoring, which is a false FAIL and the safe
  direction.
- Only the HELD arm of cmd_status's human branch was asserted. EXPIRED is by
  definition a lock whose holder is ALIVE, so putting "is gone; next acquire
  will reap it" there is the same false-abandonment message one arm over.
- The pid-1 uid check hardcoded 0 and asserted `id -u != 0`. That fails
  outright as root -- the default in most CI containers -- and wherever pid 1
  is not root, for reasons that have nothing to do with this script. The
  expectation is derived from /proc/1/status now, and the check skips itself
  when pid 1 is unreadable or is us, because then it discriminates nothing.
- `UNPARSEABLE_GRACE` was bracketed at 30s and 4000s, two orders of magnitude
  around 300. Now 200s and 400s.

Also: `timeout --signal=`, `timeout --kill-after=` and `fuser -k` evade the
R7 family regex and are plausible future edits ("escalate the teardown"),
unlike the acknowledged `${K}l` indirection. Added.

One mutation stays GREEN and is recorded in the file rather than hidden:
`[ "${threads:-1}" -le 1 ]` flips the unreadable-status default from "no
evidence of death, keep the lock" to "dead", i.e. back toward theft. Producing
a Z process with a readable stat and an unreadable status needs hidepid= or a
lost microsecond race; the alternative is a HOSTLOCK_PROC hook in production,
which would itself be a way to spoof liveness and steal a lock. It is
acceptable because every ACCIDENTAL form is safe: dropping the `-n` guard
gives `[ "" -le 1 ]`, which exits 2 and leaves the holder treated as alive.
Only an explicit numeric default flips it, and that is a rewrite, not drift.

Tests 184 -> 190. Twenty-five mutations verified red across three commits.
shellcheck clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… wedge

Review round 3 on 69cf4a8. Two blocking findings, both mine, both the
same shape as the two I already fixed in this PR.

1. The class 69cf4a8 made newly reapable -- live anchor, no start_time,
   lapsed ttl -- was announced as a DEAD holder. lock_state called it
   STALE, `status` printed "holder N is gone; next acquire will reap it"
   about a pid the guard immediately above had just verified was running,
   and the reaper logged "reaping stale lock from dead pid N", which
   SUPPRESSED the "still alive ... both sets of numbers are now suspect"
   warning in exactly the case it applies to. A takeover from a live
   holder is the one event this tool can cause that corrupts somebody's
   benchmark; it is the one message that must never be downgraded to
   routine housekeeping. Fixed in HELD (commit 2), fixed in EXPIRED
   (commit 3), and it landed in the third arm.

2. The anti-theft guard tested `[ -d /proc/$a ]`, and a ZOMBIE passes
   that. On the `run` path ttl=0, so holder_expired is never true and the
   clock grace was disabled on behalf of a corpse: the box would be wedged
   forever. That is strictly worse than the theft the guard replaced.

Liveness is now one predicate, pid_is_live(), used by holder_alive, by the
anti-theft guard, by lock_state and by the reaper, so the four cannot
disagree about whether a pid is running -- which is what produced both
defects. unverifiable_live_anchor() names the "running but unverifiable"
class explicitly rather than inferring it from a directory test.

Also fixed, found by the new assertions rather than by reading:

- `<"/proc/$pid/stat" 2>/dev/null` silences tr but not bash. Redirections
  apply left to right, so a departed pid printed two lines of shell error
  in front of every `status` on a stale lock. 2>/dev/null now comes first.
- cmd_status's STALE arm had NO assertion: replacing its body with
  `echo "FREE (nobody is here)"` left the suite green. Every STALE check
  went through porcelain.
- The "present and dead" fixture was not: dead_pid() is fully reaped, so
  /proc/<pid> is absent. The real present-and-dead shape is a zombie, and
  it is now built from one (fork, SIGKILL, no wait) -- with the keeper's
  stdout closed, because a background child inside $(...) holds the
  substitution's pipe open and the corpse is reaped before the caller
  resumes.
- The primary assertion for the primary fix could pass vacuously if an
  earlier assertion had already let the lock be reaped. It now asserts the
  fixture's starting state first.
- cmd_run's trap ordering was unpinned: the golden body is a body, not a
  position, so moving RUN_CHILD_START back above the traps left it
  byte-identical while reopening the signal-in-fork window.
- Both environment-probed branches now assert something, and the total
  assertion count is pinned, so a check that stops running fails loudly
  instead of quietly reducing coverage.

Corrections to my own record, since both were stated in 69cf4a8:

- "EXPIRED would be unreachable for that whole class" was FALSE as
  written. Before this commit EXPIRED remained unreachable for it either
  way, because holder_alive needs a start_time; only the `wait` half of
  that claim was true. This commit is what actually makes it reachable.
- The "lock written by an older version of this script" rationale is
  withdrawn. start_time has been written since 73c7645, so no revision
  that ever existed produces an anchor without one. The reachable causes
  are a meta caught mid-write and on-disk damage, which is what the
  comments now say.

Suite 190 -> 208, all green.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby force-pushed the squad/leon-hostlock-conformance branch from c281b4e to 16058dc Compare August 23, 2026 15:52
@justinchuby
justinchuby merged commit 65024b2 into main Aug 23, 2026
14 of 17 checks passed
@justinchuby
justinchuby deleted the squad/leon-hostlock-conformance branch August 23, 2026 16:24
@justinchuby

Copy link
Copy Markdown
Owner Author

A question came up about whether run should keep ttl=0, prompted by a real incident: a genuinely-running aarch64 job that held ~1778% CPU for 5h40m after the work item motivating it had been abandoned — the owner moved HEAD off the branch at 03:28:16 and never looked back. The proposal was to pass an explicit finite --ttl on run for qemu jobs, on the reasoning that "a never-expiring lock held by a job whose owner has moved on is exactly the shape of what just happened".

I think run should keep ttl=0, and that a finite TTL there would make things worse rather than better. Laying out why, because the reasoning generalises past this script.

What ttl=0 on run actually guarantees

The rationale at :1015-1017 is exact:

run anchors to itself: that pid is exact and dies with the command on every exit path, so it needs no expiry.

Verified — cmd_run acquires, backgrounds the child, waits, and releases on the normal path (:891) and on INT/TERM/HUP (:883-885). So on the run path:

lock lifetime ≡ job lifetime. That is a true statement, and ttl=0 is what makes it true.

Why the incident does not argue against it

The incident was not a lock outliving its job. It was a job outliving its purpose while remaining perfectly alive. Had it been run under hostlock run, the lock would have been held for 5h40m and every second of that would have been accurate: the box really was occupied, by 18 cores of really-running work.

The lock was never the thing that was wrong. The job was.

Why a finite --ttl on run would be a regression

Set run --ttl 1800 against a 5-hour job and here is what you have built: at t=30min the lock becomes reapable while the job keeps burning 18 cores. The next acquirer takes it over — loudly, naming the holder (:394), which is good — and then starts measuring on a box that is 60% occupied by a process the lock no longer mentions.

That converts a true statement into a false one. The lock would report free while the host is anything but, which is a false-green of exactly the kind this repo has spent the day cataloguing. And it is the same observation made earlier in a different context — reclaiming the lock does not stop the load. A TTL bounds the claim; it cannot bound the work, and on the run path those are currently the same object, which is the property worth protecting.

--strict-reap (:64) blunts the edge by letting an acquirer refuse a host it had to reap, and --gate catches load with no lock behind it. But the default outcome of a finite run TTL is a lock that lies, and the default is what matters for a safety mechanism.

The mechanism that actually fits the failure

A job that outlives its purpose needs a bound on the job, not on the lock:

setsid timeout -k 30 5400 taskset -c 16-23 <cmd>

timeout kills the work; the run anchor then dies with it and the lock releases through the path it already has. Both statements stay true throughout, and the box is genuinely free at the moment the lock says so. That posture is already in use for qemu runs — taskset + --test-threads=2 + hard timeout + setsid + a pgrep -g reap check — and it is the correct fix for this failure mode. The lock never needed to change.

So: no TTL on run, and no per-call-site --ttl either. The instinct behind the ask was right — something must be bounded — but the thing to bound is the process, and it already is.

One thing genuinely worth having

acquire gets 3600 and run gets 0 (:1018-1023), matching the documented behaviour at :72. That asymmetry is correct and well-reasoned — acquire anchors to $PPID, a shell that can outlive the benchmark by days, so it does need a clock.

What is missing is any way to notice the case above from outside: a run lock held far longer than anything of its class should be. status prints the age (:648), so the information is present — but nothing draws attention to it. A soft advisory in status — held longer than N with ttl=0, printed as an observation and not a takeover — would have surfaced a 5h40m hold to anyone who looked, without giving anything the right to reap a live job. That keeps the honesty property intact and adds the only thing the incident actually lacked, which was visibility, not authority.

justinchuby added a commit that referenced this pull request Aug 23, 2026
…s/--min-efficiency) (#1864)

Roy found a hole in the guard I shipped, using it. This closes it.

## The defect this fixes is in the *premise* of the merged lock, not its
code

`hostlock.sh` records an occupancy snapshot into every row —
`runnable_at_acquire`, `runnable`, `contended=yes/no` — and `--gate N`
refuses to start until the instantaneous runnable count drops. Both are
**admission controls**, and admission controls sample *instants*.

Roy gated exactly that way, sampling before and after each arm, and it
reported **"runnable peak 2–4, clean"** for runs that were in fact
getting **50–70% of a core**. A 2-second arm has ample room for a burst
that begins after the opening sample and ends before the closing one.
His A/A null — the same binary against itself — was **52% per cell**.

So a row emitted by the merged lock can say `contended=no` about a
ruined measurement, and nothing in the tool contradicts it. That is the
same failure the lock exists to prevent, one level up: **a number that
was reported without being measured.**

## What this adds

`run` now measures the CPU the wrapped command actually consumed and
compares it to `cores × wall`:

Both blocks below are **real output from this branch on this box**, not
illustrations. A 16-wide load that got its cores:

```
$ hostlock.sh run --owner leon --reason "16-wide example" \
      --expect-cores 16 --min-efficiency 0.90 -- python3 load.py 16 5
hostlock: outcome=acquired by leon (anchor pid 3086636) — 16-wide example
hostlock: released (command exit 0)
hostlock: cpu wall=5.048s cpu=80.020s cores_expected=16 efficiency=0.991 verdict=ok
rc=0
```

and the same command with one competitor pinned to the same cpu — a
*real* half-a-core run, not a simulated one:

```
$ taskset -c 30 python3 load.py 1 6 &          # the competitor
$ hostlock.sh run --owner leon --reason "one cpu, shared" \
      --expect-cores 1 --min-efficiency 0.90 -- taskset -c 30 python3 load.py 1 4
hostlock: cpu wall=4.090s cpu=2.040s cores_expected=1 efficiency=0.499 verdict=contended
hostlock: WARNING measured CPU efficiency 0.499 is below --min-efficiency 0.90
hostlock: WARNING the command did not have 1 core(s) to itself for the whole run; treat its numbers as untrusted
hostlock: WARNING the runnable gate cannot see this -- it samples instants, this measures the whole window
rc=6
```

`0.499` against a single competitor, and an unattended harness stops
instead of publishing. Note what the admission controls would have said
about that second run: **one** extra runnable process. Any `--gate 2` or
looser admits it, and the row would have carried `contended=no`.

**The technique is Roy's, not mine.** He derived it from `os.wait4`
rusage after his gate failed him, and it took his A/A null from **52% to
0.04–0.56%** on the same busy box. Its real virtue is that it needs no
quiet host: the gate decides whether to *start*, this decides whether to
*believe*. It composes with the lock rather than replacing it.

## Design decisions, each with the failure it avoids

**`--expect-cores` is required by `--min-efficiency` and has no
default.** Defaulting it to 1 would report a 16-thread benchmark at
`efficiency=16.0` and pass every threshold — the reassuring direction,
which is the one that propagates. Missing denominator is a usage error
*before* the command runs, not a surprise after forty minutes.

**Both knobs are refused outside `run`.** They have nothing to measure
there. Accepting them silently is how a knob comes to be believed in
while being inert — the exact defect filed against the EP's
`ONNX_GENAI_CPU_DECODE_AFFINITY`, where every setting produced
byte-identical placement.

**A failing command's own status always wins.** Reporting 6 for a
benchmark that crashed would bury the crash under a host-quality
complaint. Only a command that *succeeded* can be overridden, and only
when a threshold was explicitly requested — so `run`'s documented
"returns the wrapped command's status" contract is unchanged unless you
opt out of it.

**A run too short to measure is refused, not certified.** Below ~50 ms
the clock-tick quantum is a large fraction of the measurement; a ratio
computed from it is noise wearing a decimal point. With a threshold set,
unmeasurable ⇒ `NOT verified` ⇒ exit 6. Fail closed on the *evaluation*,
not just on the result.

**The claim is deliberately narrow.** Low efficiency means *"the command
did not have the cores"*, which is **not** the same as *"somebody stole
them"*: a benchmark that sleeps, blocks on I/O, or leaves a deliberate
inter-token gap is legitimately below 1.0 and is not contended. Only the
caller knows which, which is why there is no default threshold and why
the header says to set it from a measured quiet-host run rather than
from an ideal.

## The one implementation subtlety, which is a trap

CPU time comes from this shell's own reaped-children accounting,
`cutime+cstime` in `/proc/self/stat`, so it covers the wrapped command
**and every descendant it waited for**, costs nothing, and needs no
sampling.

But it must be read **without forking**: `fork()` zeroes a child's
`RUSAGE_CHILDREN`, so `t=$(children_cpu_ticks)` — and equally `times |
awk` — reads a freshly zeroed counter *in the subshell* and always
answers `0`. Every run would then measure as perfectly idle and, with a
threshold, every run would be rejected. `read -r line < /proc/self/stat`
is a builtin with a redirection and does not fork; the value is parsed
afterwards, where forking is harmless. The mutation battery includes
this exact mistake (`line=$(cat /proc/self/stat)`) as N4.

## Falsification

Suite **208 → 231**, all green. Nine mutations, all red:

| mutation | what it breaks | tests red |
|---|---|---|
| N1 | efficiency check never enforces | 3 |
| N2 | efficiency hardcoded to `1.000` | 3 |
| N3 | denominator ignores `--expect-cores` | 1 |
| N4 | child CPU read through a fork (rusage resets to 0) | 2 |
| N5 | too-short guard disabled | 1 |
| N6 | missing denominator defaults to 1 instead of dying | 2 |
| N7 | inert knobs accepted outside `run` | 3 |
| N8 | efficiency verdict overrides a failing command | 1 |
| N9 | `CPU_TICKS=$((cu))` — kernel time dropped | 2 |

N2 is the one that matters most: it is the *same* defect as
`runnable_now() { echo 1; }` from the previous round — a reported number
that is a label rather than a measurement. Both the sleeping-command
cell and the one-core-against-two-cores cell exist to kill it, because
"efficiency is a number in [0,2]" is satisfied by a constant.

The pinned total-assertion count moves 208 → 231 in the same commit, so
a check that silently stops running still fails by name.

**N9 came from review, and it is the interesting one.** The first eight
mutations were mine. Review pointed out that every efficiency cell I had
written was a pure user-mode busy loop, so dropping `cstime` from the
tick sum — half the measurement, and the half a real benchmark spends
loading models and writing tensors — left all 229 assertions green. That
is the same shape as the defects in #1830: the fix was right, and the
suite proving it could not tell. The added cell is an
`os.write`-to-`/dev/null` loop; under N9 it reads `cpu=0.35s eff=0.23
contended rc=6` instead of `eff≈1.0 ok`.

`shellcheck` clean. No crate code. The suite's deliberate CPU use is ~6
× 1 s (R2 spinners, killed as soon as occupancy is read) plus 3 × ~1.5 s
on one cpu (R2b) — about ten core-seconds. The header previously
mis-stated this as ~5 s × 6; corrected here, since a cost note that
overstates by 3× is no more useful than one that understates.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 23, 2026
… the box (#1885)

Closes Gaff's blocking item on #1806.

## The defect, in his words and then in mine

> `REAP_DIR` is unguarded. … One kill in that window orphans the
directory, after which `reap_if_dead` returns 1 forever and **no stale
lock can ever be reaped again** — the box wedges behind a dead holder
and `acquire` just reports busy, indistinguishable from legitimate
occupancy.

Correct, and the framing is the part worth keeping: **the wedge this
script exists to prevent, relocated into the mechanism that prevents
it.** The lock has an anchor pid, a start time and a reaper. Its own
mutex had none of the three — a bare `mkdir "$REAP_DIR"` released by
`rmdir`.

#1811 bounded it by age (`REAPER_GRACE=60`). That recovers, but it
recovers *by waiting*, and it bought the mirror-image defect: **age is
not evidence of death.** A reaper that is merely slow — loaded box, a
qemu leg, a stalled `stat` — could have its guard cleared while it was
still inside the critical section. Two acquirers then both reap and both
believe they own the host. That is worse than a wedge, because a wedge
is loud and this is silent, and the numbers on both sides are ruined
without either party knowing.

## What it is now

The guard is built the same way as the lock, for the same reasons:

| property | mechanism | why |
|---|---|---|
| published atomically **with** metadata | stage a populated dir, `mv
-T` | `rename(2)` onto a non-empty dir fails `ENOTEMPTY` → test-and-set,
not last-writer-wins. No kill can produce an unattributable guard,
because there is no instant at which one exists |
| owned | `anchor_pid` + `start_time` in `$REAP_DIR/meta` | pids
recycle; "a process with this number exists" is a different question
from "the reaper is still running" |
| dead owner reclaimed **immediately** | `anchor_alive` | no grace, no
waiting, nothing to wedge behind |
| live owner **never** disturbed | `anchor_alive` outranks age | kills
the double-reap that the age rule introduced |
| released only by its owner | pid check in `reaper_release` | a process
whose guard was reclaimed while descheduled must not delete its
successor's guard on the way out |

`anchor_alive` is now **one** predicate, shared with `holder_alive`.
Four call sites each deciding "is this pid alive?" for themselves
produced two of the four defects in #1830; I am not doing that again in
a second module.

`REAPER_GRACE` survives for exactly one residual class — a guard that
exists with no readable anchor. Staging makes that unreachable from this
script; a stray directory at that path from an older version or a
hand-run `mkdir` can still present it, and for that class no liveness
evidence exists, so age is all there is. The existing test for it stays.

## The kill-in-window test is deterministic, and it cannot pass
vacuously

Requested explicitly, and it is the cell I would have written badly:

```sh
HOSTLOCK_REAPER_STALL=30 $HL acquire --owner victim ... &   # seam holds the section open
...wait for $LOCK.reaper/stalled_pid...                     # the pid INSIDE the window
sig "$stalled" 9                                            # SIGKILL lands in the window every run
chk "killing it in the window really does orphan the guard" ...        # ← the orphan EXISTS
chk "and the orphan still names its dead owner"            ...
$HL acquire --owner roy ...
chk "a guard orphaned by SIGKILL does not wedge the next acquirer" "$(st owner)" "roy"
chk "and recovery is immediate, not after REAPER_GRACE"    "$(...)" "clear"
```

Three things this gets right that the obvious version does not:

1. **Deterministic, not opportunistic.** Racing a real reaper's
microsecond window means the kill lands inside it when the scheduler
feels like it. The seam makes the window as wide as I ask.
2. **It asserts the orphan exists first.** Without that line, all three
recovery checks pass equally well when nothing ever leaked — the
vacuous-coverage failure Gaff flagged on Resch's #1805 the same night,
where every assertion sat behind a condition that could silently be
false.
3. **`cleanup` is not called between the kill and the recovery.** The
harness sweeps `$LOCK.reaper`, so a tidy-looking cleanup there would
erase the orphan and the cell would pass **against the defect it exists
to catch.** Cleanup must not mask the defect; here it is a deliberate
omission with a comment saying so.

The seam is production code, so its inertness is asserted too (R8.4):
unset, the reap completes in under 3 s.

## Falsification

Suite **231 → 247**, green. `shellcheck` clean. Nine mutations, all red:

| # | mutation | result |
|---|---|---|
| P1 | dead owner never reclaimed (the original wedge) | 242 passed /
**2 failed** |
| P2 | age-only rule restored, liveness ignored | 240 / **4** |
| P3 | release without the ownership check | 243 / **1** |
| P4 | guard created in place instead of renamed into place | 242 /
**2** |
| P5 | stall seam always fires | 199 / **45** |
| P6 | `anchor_alive` ignores `start_time` (recycled pid) | 242 / **2**
|
| P7 | rename-then-remove keeps the corpse (clear path) | 246 / **1** |
| P8 | rename-then-remove keeps the corpse (release path) | 246 / **1**
|
| P9 | release checks pid but not start time | 246 / **1** |

**P7–P9 came from review, and both findings are the shape this file
keeps producing: the fix was right and the suite proving it could not
tell.**

*P9* — `reaper_release` compared only `$$` while `reaper_clear_if_dead`
compares pid **and** start time. The asymmetry sat exactly where the
consequence is worst: a recycled pid landing on our number makes us
delete a **live successor's** guard, rather than merely failing to tidy
up our own. This box is at ~1.5M pids in four days, so recycling is not
theoretical. R8.3b forges a successor guard carrying the stalled
reaper's own pid with a start time that is not its, and requires it to
survive.

*P7/P8* — dropping the `rm -rf` from either rename-then-remove left all
244 assertions green, because the harness `cleanup()` sweeps those globs
between cells. In production that is unbounded `.dead.*`/`.rel.*` growth
beside the lock. The new assertion runs **before** cleanup,
deliberately: placed after it, it would pass whether or not the code
tidies up at all — the same "cleanup masks the defect" failure the
kill-in-window cell was written to avoid, sitting one cell to the left
of where I was looking for it.

P4 is a structural assertion rather than a behavioural one, and the PR
says so rather than dressing it up: killing between a `mkdir` and a meta
write is a microsecond window, and a seam wide enough to test it would
be wider than the bug. The suite asserts instead that the guard is never
created in place — weaker evidence, honestly labelled.

## The other two items

**`--gate` stays a start admission only.** No change to its semantics.
The README already carries the split — the lock and the gate decide
whether to **start**, `--expect-cores/--min-efficiency` (#1864) decides
whether to **believe** — with Roy's 52% A/A null as the evidence for why
one instrument cannot do both.

**The harness holds the lock across all arms**, and rows carry
`held_by`, `contended`, `runnable_at_acquire` and now `efficiency`.
Unchanged here, restated in the README because "I sampled `ps` and the
host looked free" was a *between-arms* sample of somebody else's
interleaved A/B.

Also documents that this script is **Linux-only by construction**
(`/proc`, `mv -T`, `stat -c`) instead of leaving the question open. A
portability fallback that degraded liveness to `kill -0` would reap live
holders on whichever platform took it — the one error this script must
never make.

No crate code.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 24, 2026
Closes #1924.

`scripts/hostlock.sh` has been on `main` for a while and nothing reads
it:

```
$ git grep -l hostlock -- crates/ scripts/ .github/
.github/workflows/hostlock.yml
scripts/hostlock.sh
scripts/hostlock_test.sh
```

Every consumer is the script or its own test. A capability with no
caller is
indistinguishable in the output from one that was never built, so a row
printed
on a host somebody else had declared looks exactly like a row printed on
a quiet
one. This adds the reader and puts the answer in the row.

## What lands

`onnx_runtime_hostmon::hostlock` — a read-only consumer — plus a
`host_lock=`
field on `bench_generic` rows and a provenance line on the
`decode_gap_park_ab` matrix.

| value | meaning |
|---|---|
| `free` | nobody declared the host for the whole window |
| `mine:<owner>` | held throughout by a live anchor whose owner matches
`HOSTLOCK_OWNER` — **the only value that certifies a row** |
| `foreign:<owner>` | held throughout by somebody else |
| `held:<owner>` | held throughout, `HOSTLOCK_OWNER` unset, so the
reader cannot attribute it |
| `unverified:<owner>` | held, liveness unprovable (no anchor PID, or no
recorded `start_time`) |
| `stale:<owner>` | held by an anchor that is provably gone |
| `changed` | the two readings disagree; the row spans a change of
custody |
| `unknown` | the lock could not be read — *not* the same as free |

## Five decisions, and why

**Read-only, and it stays that way.** It never acquires or releases.
Taking a
lock as a side effect of formatting a field would be far worse than no
lock at
all.

**Read at both ends, not once.** `changed` is the whole point. A single
reading
after the runs would report one credible holder for a window that
changed hands
halfway through — the same stale-snapshot error as checking `ps` once
before
starting, only moved into the row where it is harder to notice.
Demonstrated end
to end below.

**Liveness mirrors the script rather than being decided again.**
`hostlock.sh`'s
comment on `anchor_alive` records that the last time two call sites
decided this
question independently the answers disagreed, and two of the four
defects in
\#1830 came out of the gap. A Rust reader deciding it independently
would be a
third call site, disagreeing in the worst possible place: the published
row.

**Ignorance is never resolved in the flattering direction.** `free` and
`unknown` format differently; `held` and `mine` format differently;
`unverified` and `stale` format differently. `is_protected()` is
`Mine(_)`
alone — an unattributable lock, an unverifiable anchor and a dead one
all
refuse to certify a row, even when the owner string matches.

**Owners are sanitised.** The script's own header documents this hazard
against
its provenance line: an owner of `gaff hostlock_state=FREE declared=no`
splices
two extra key/value pairs into the output, and a consumer reading the
first
`hostlock_state=` gets `FREE` for a held lock. A result row is a
`key=value`
list too, so it inherits the hazard verbatim. The test asserts the
*field count*
of the rendered row, not a string.

## The integration test found two real defects in this reader

`tests/agrees_with_hostlock_sh.rs` drives the actual script — acquires,
releases, and reads back — rather than parsing a fixture I wrote myself.
The
unit tests are pure functions over strings of my own construction, so
they prove
the decision table is internally consistent and prove nothing about
whether it
describes the file `hostlock.sh` writes.

It failed on first run, twice, both times on this reader and not on the
script:

1. **A zombie has a `/proc/<pid>` entry** and a matching start time, so
my
original `Path::exists` liveness check reported a held lock on a corpse
forever. The script calls this the common shape rather than the exotic
one,
because every agent harness on this box launches long commands without
an
immediate `wait()`. The test reproduces a genuine zombie — a spawned
child
that is never reaped, with the `Z` state asserted before the lock is
taken —
   so it also confirms `proc_info` parses a real `/proc/<pid>/stat`.
2. **A recycled PID passes an existence check too.** Fixed by carrying
the
   recorded `start_time`, which is exactly why the script records it.

State `Z` is not itself proof of death — a thread-group leader that
exited via
`pthread_exit` while its threads keep running reports `Z` for a fully
live
process, and reaping that one takes a live holder's machine
mid-benchmark. So
`Threads:` decides, and an unreadable `Threads:` is no evidence of
death.

The test **fails rather than skips** if the script is missing. A skip
would be
the same defect one level up: the run prints `ok`, the agreement is
unchecked,
and the output is indistinguishable from a real pass. The platform gate
is
`#[cfg]`, so on non-Linux the tests do not exist rather than passing
vacuously.

## The field prints on the unmeasured path too

That is the row with the least other evidence about the conditions it
was taken
under. Dropping the declaration exactly there would leave the least
trustworthy
rows looking the least suspicious.

## It stays advisory

`decode_gap_park_ab` prints an `UNPROTECTED` warning to stderr and then
prints
the matrix anyway. Refusing to publish because nobody took a lock would
mostly
teach people to stop taking the lock.

## Validation

**End-to-end**, on a scratch `HOSTLOCK_DIR`, each cross-checked against
`hostlock.sh status --porcelain`:

| configuration | `bench_generic` printed | script agreed |
|---|---|---|
| lock held by `sebastian`, `HOSTLOCK_OWNER=sebastian` |
`host_lock=mine:sebastian` | `state=HELD` |
| same lock, `HOSTLOCK_OWNER=roy` | `host_lock=foreign:sebastian` |
`state=HELD` |
| same lock, `HOSTLOCK_OWNER` unset | `host_lock=held:sebastian` |
`state=HELD` |
| anchor killed, lock left behind | `host_lock=stale:sebastian` |
`state=STALE` |
| released 2 s into a 5 s run | **`host_lock=changed`** | — |
| no lock | `host_lock=free` | `state=FREE` |

The fifth row is the one that justifies the design: a single end-of-run
read
would have printed `mine:sebastian` there, and looked entirely normal.

Also read against the *real* lock while a co-tenant held it, and against
it
after they released — reader and script agreed both times.

**Tests:** 19 unit + 2 integration in `hostmon`, 39 in `bench_generic`
(3 new).
`cargo fmt --all --check` clean; `clippy -D warnings` clean on all three
changed crates.

**Mutation:** 21 mutants, 21 killed — including zombie-ignored,
zombie-leader-reaped, start-time-ignored, no-two-ended-read,
owner-passthrough,
io-error-is-free, and every widening of `is_protected`.

## Not in scope

Holding the lock from inside a harness. That is a decision a harness
makes and
this deliberately does not make it.

## Coordination

This is the reader half of the mechanical-lock idea @roy raised. I
claimed it
after finding the writer already existed on `main`; the widening of the
realized-width assertion in `matmul_nbits.rs` is his and is untouched
here.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 27, 2026
…e doc at the command that emits it

Independent review found the header sent readers to the wrong subcommand: it
said `status` reports `held_owner_source`, but `held_*` is the `provenance
--oneline` row schema and `status` never emitted it. The one comment whose job
is discoverability pointed at the command where the field cannot be found.

Fixing only the comment would have left the gap that made it plausible to
write. `status --porcelain` already re-emits the attribution set for machine
consumers -- owner, reason, worktree, cmd -- and omitting the qualifier there
hands a porcelain reader `owner` as bare free text, which is precisely the
defect #2260 describes. It now emits `owner_source`, defaulting to `unknown`
for a lock predating the key, matching provenance.

Three assertions, pinned count 468->471. Mutation-proved: deleting the
porcelain line fails "porcelain reports unknown for a lock predating the key"
and "and porcelain reports a declared owner as declared", while the companion
`owner=leon` guard still passes -- so the qualifier assertions cannot go
vacuous on a failed acquire.

Also corrects the reference the review flagged in the pin-count comment. The
review attributed the inert R1 block and the vacuous STALE arm to #2252;
`git log -S 'vacuous STALE arm'` says they were fixed in #1830, so the comment
names #1830.

shellcheck clean, 471/471, all 37 hostlock mutants still killed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 27, 2026
…e doc at the command that emits it

Independent review found the header sent readers to the wrong subcommand: it
said `status` reports `held_owner_source`, but `held_*` is the `provenance
--oneline` row schema and `status` never emitted it. The one comment whose job
is discoverability pointed at the command where the field cannot be found.

Fixing only the comment would have left the gap that made it plausible to
write. `status --porcelain` already re-emits the attribution set for machine
consumers -- owner, reason, worktree, cmd -- and omitting the qualifier there
hands a porcelain reader `owner` as bare free text, which is precisely the
defect #2260 describes. It now emits `owner_source`, defaulting to `unknown`
for a lock predating the key, matching provenance.

Three assertions, pinned count 468->471. Mutation-proved: deleting the
porcelain line fails "porcelain reports unknown for a lock predating the key"
and "and porcelain reports a declared owner as declared", while the companion
`owner=leon` guard still passes -- so the qualifier assertions cannot go
vacuous on a failed acquire.

Also corrects the reference the review flagged in the pin-count comment. The
review attributed the inert R1 block and the vacuous STALE arm to #2252;
`git log -S 'vacuous STALE arm'` says they were fixed in #1830, so the comment
names #1830.

shellcheck clean, 471/471, all 37 hostlock mutants still killed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant