fix(hostlock): a host that cannot take the lock must say UNUSABLE, not FREE

[justinchuby]

Why

FREE is not a fact about a directory, it is a promise to the caller: the host is yours, go ahead. On a host where the lock cannot be created, that promise is false — and the tool made it anyway.

Measured against origin/main, on the two fixtures this PR adds, rather than reasoned about:

fixture	command	origin/main	verdict
parent chmod 500	status	FREE (runnable=4), rc 0	fail open
parent chmod 500	status --porcelain	state=FREE, rc 0 — bytes identical to a genuinely free host	fail open
parent chmod 500	wait	hostlock: free (runnable=4), rc 0	fail open
parent chmod 500	acquire / run	mkdir: … Permission denied, rc 1	refuses, misclassified
lock path is a plain file	acquire --wait --timeout 12	timed out after 12s waiting for the lock and prints FREE in the same breath, rc 3, elapsed 15 s	fail open + false blame

The first three tell an unattended harness to proceed on the one host where the lock is unavailable. status is the README's own opening move (scripts/hostlock.sh status # is anybody benchmarking?) and wait exists to be followed by a run. The fourth is a correct refusal with the wrong classification: exit 1 is documented as usage/error, so a caller cannot tell a broken host from its own typo. The fifth is the worst of them — the box is idle, and the tool spends the whole --timeout before blaming peers for contention that does not exist, while printing FREE underneath the complaint.

Correction to my first draft of this PR, which claimed run "ran the benchmark unlocked" and that acquire --wait hung on the permission fixture. Neither is true and the reviewer was right to push: on the chmod 500 fixture publish_lock dies immediately, so base run exits 1 in 0 s and does not run the command. The hang is real but needs the plain-file fixture, where the staging sibling is created fine and only the final mv -T fails — indistinguishable, to the loop, from "somebody got there first". The table above is what I measured; the narrower claim is the true one.

What

A fifth state, UNUSABLE, and one place that decides it.

• lock_dir_problem() — echoes the reason and returns 0 when no lock can be created, 1 when one can. It tests the parent, never the leaf: publish_lock stages at ${LOCK_DIR}.stage.$$, a sibling, and renames it into place, so the real question is "can we create entries next to it", answered at the nearest ancestor that exists (mkdir -p builds the rest). Both halves matter — a directory that is writable but not searchable (mode 0600) cannot host entries either.
• status reports UNUSABLE with the reason; --porcelain gains lock_dir_problem=, emitted unconditionally and empty when there is none — a consumer that learns the state from a key's presence reads absence as "fine", which is the failure the field exists to report.
• acquire / wait / run refuse with a new exit code 7: distinct from 1 because a misconfigured host is not a bad argument and must not be retried as one, and distinct from 2 and 3 because both of those assert a peer holds the box.
• The refusal precedes the legacy consult and the wait loop, so it is prompt. A correct exit code that arrives 900 s late is still the defect.
• A path that exists as a plain file is UNUSABLE; the old branch tested -d and fell through to FREE.
• Usability does not leak into the occupied path: a HELD lock reads HELD whatever the directory's mode says, or the state that stops a second agent taking the box would be answerable with a chmod.

Evidence

scripts/hostlock_test.sh — 341/341, count pin 321 → 341 (20 new cells), full suite, on this host. bash -n and shellcheck -S warning clean on both files.

Mutation battery, each mutant run against the whole suite:

KILLED  M1 lock_state never reports UNUSABLE
KILLED  M2 acquire skips the preflight
KILLED  M3 wait skips the preflight
KILLED  M4 refusal exits 1, folding it back into usage/error
KILLED  M5 usability is tested on the leaf, not the parent
KILLED  M6 porcelain omits the key when there is no problem
KILLED  M7 a plain file at the lock path is no longer a problem
KILLED  M8 the reason is dropped, leaving only the verdict
KILLED  M9 the unsearchable half of the guard is dropped
KILLED  M10 preflight runs after the legacy consult

Three of these were found surviving and are the reason four cells exist:

• M5 survived the first round. With p=$LOCK_DIR the ancestor walk climbs to the parent anyway, so for an absent lock dir the mutant is behaviourally identical. The difference appears only when the lock dir exists and is unwritable, where the mutant reports UNUSABLE for a lock a peer is legitimately holding — relabelling real contention as a broken host, which is the expensive direction: it sends the agent off to fix its config while somebody's benchmark is running. Cell: a held-but-unwritable lock dir is BUSY, not UNUSABLE.

• M9 was raised by review, not by me: every unusable fixture I had written was chmod 500 (not writable, but executable), so the [ ! -x "$p" ] clause was untested and deleting it was a one-line fail-open the whole suite stayed green through. Cell: a writable but unsearchable parent is UNUSABLE too.

• M10 was raised by the re-review: swapping the preflight with refuse_if_legacy_held left the whole suite green, because every cell in the section sets HOSTLOCK_DIR and the legacy consult is a no-op for any source but config. Not cosmetic — on a config-source host with a live legacy holder and an unusable configured dir the swapped order answers 2 (busy) instead of 7, sending the agent away to wait for a lock it could never have acquired, with the reason never printed. Cell: an unusable configured dir refuses as 7 even while the legacy path is live.

M1 is worth a note: it kills the porcelain cell and the file cell but not the human status cell, because cmd_status's human branch and lock_state decide independently. That is why both cells exist rather than one.

Negative controls, so the new cells cannot pass vacuously: a usable free host must still print FREE, must emit the same lock_dir_problem= key empty, and a deep path under a writable ancestor must be acquirable for real (M5 and M6 kill that pair).

Known limits, stated rather than implied

• This detects access(2) denials — DAC modes and read-only mounts — not a sandbox that mediates only the mutating syscalls. For a 1777 /tmp, test -w is true for every user, so an agent forbidden /tmp by policy rather than by permission still gets FREE. A script cannot detect a policy; the honest fix for that case is the machine-local lock_dir= config (see the README section, and Decide when to move this box's benchmark lock off /tmp (needs every worktree past #1942 first) #1953), and this PR does not claim otherwise.
• The Rust reader (onnx_runtime_hostmon::hostlock) still classifies an absent-because-uncreatable lock as free, so on such a host the script and the reader now disagree. The harm is pre-existing — both said free before — but the divergence is new and I have filed it as hostmon reader still reports free for a lock dir that cannot exist, now that hostlock.sh says UNUSABLE #1997 rather than widen this PR into Rust.

Scope

scripts/hostlock.sh, its self-test, and the two paragraphs of scripts/ort_ab/README.md that enumerate the states. No behaviour change on any host where the lock directory can be created: lock_dir_problem returns 1 and every path is exactly as before. agrees_with_hostlock_sh.rs matches porcelain with contains, not a strict key set, so the added key is compatible (8/8 pass).

No --admin, no ruleset bypass. Merged only via scripts/merge_when_green.sh, after every required context has reported SUCCESS on this head.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.04%. Comparing base (332077a) to head (cd9e7d0).
⚠️ Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1989      +/-   ##
==========================================
+ Coverage   80.66%   81.04%   +0.37%     
==========================================
  Files         424      425       +1     
  Lines      207766   208150     +384     
  Branches   207766   208150     +384     
==========================================
+ Hits       167601   168688    +1087     
+ Misses      34533    33819     -714     
- Partials     5632     5643      +11     

Flag	Coverage Δ
cli-ort-linux	72.51% <ø> (ø)
cli-ort-windows	72.10% <ø> (ø)
mlas	85.33% <ø> (ø)
offline	81.19% <ø> (+0.38%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 24 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]

Review addressed. All three SHOULD FIXes landed, and two of them changed what this PR claims, not just what it tests — so the body and the commit message were rewritten rather than patched.

SHOULD FIX 1 — the -x half was untested. Correct, and it was a one-line fail-open: every unusable fixture I had written was chmod 500, i.e. not writable but executable, so deleting || [ ! -x "$p" ] left all 339 assertions green. Added a writable but unsearchable parent is UNUSABLE too on a chmod 0600 parent, and added the mutation as M9 — it survived before the cell and is killed by it.

SHOULD FIX 2 — the promptness cell was not an independent guard. Also correct, and the interesting part is why: on the chmod 500 fixture the pre-fix code never hung at all. I measured origin/main rather than argue about it:

base, chmod 500 parent:   acquire --wait --timeout 20  ->  rc=1  elapsed=0s
base, plain-file path:    acquire --wait --timeout 12  ->  rc=3  elapsed=15s

The staging dir is a sibling, so with an unwritable parent mkdir -p "$stage" fails and publish_lock dies immediately. The hang needs a fixture where the sibling can be created and only the final mv -T fails — which is the plain-file path, where the failure is indistinguishable to the loop from "somebody got there first". Moved both --wait cells onto that fixture. M7 now reddens the timing cell by itself, which is what makes it a guard:

KILLED  M7 a plain file at the lock path is no longer a problem
            by: FAIL  a lock path that is a plain file is UNUSABLE
            by: FAIL  acquire --wait refuses instead of waiting out the timeout
            by: FAIL  and refuses promptly rather than blaming peers for 30s

That measurement also falsified two claims in my own PR body, which are now gone: base run did not run the benchmark unlocked (it exits 1 in 0 s and runs nothing), and base acquire --wait did not spend the timeout on the permission fixture. The true defect is narrower and I would rather publish the narrow version: two genuine fail-opens (status, wait), one misclassification (acquire/run exit 1, documented as usage/error), and one real hang that needs the other fixture. The table at the top of the PR is measured output, not recollection.

SHOULD FIX 3 — the guard measures access(2), and the motivating case may be above it. You are right, and it is worse than "may": I checked, and it does not fire. /tmp here is drwxrwxrwt, so test -w /tmp is true for every user on this box. The prohibition I described is a policy above the filesystem, and a shell script cannot detect a policy. So this change is inert for the case that motivated it, and the PR now says so under Known limits instead of implying otherwise. I did not reach for a real mkdir/rmdir probe: it would make status mutate the filesystem it is reporting on, and it would still not help me, because the syscall I must not make is the one the probe would make. The honest remedy for that case is the machine-local lock_dir= config, which is #1953 — where I have posted the measurement and the one thing that still blocks it (an old-code reader cannot see a holder at the new path; closing that needs exactly one /tmp write, which I cannot make).

It remains a real fix for DAC denials, read-only mounts, PrivateTmp=, and a path that is a plain file — just not for a sandbox.

NIT (f) — the Rust reader now disagrees. Agreed, and filed as #1997 rather than widened into this PR: read() sees the absent directory, classify_io maps NotFound to Free, and rows carry host_lock=free on a host that could never have taken the lock. The harm is pre-existing (both said free before), the divergence is new, and agrees_with_hostlock_sh.rs has no unusable fixture at all — every one is writable under CARGO_TARGET_TMPDIR. #1997 names the 0500/0600 pair to add and flags that hostlock_mutants.py should be re-run over the reader.

NIT — root. Noted in the issue rather than fixed. Under CAP_DAC_OVERRIDE the chmod fixtures do not restrict, so the cells flip and the count pin fails, which is at least loud rather than silent. CI runs as non-root (ubuntu-latest, runner), and it is the same reason the mechanism itself cannot fire under root, which the code documents as the accepted safe direction.

Suite 340/340 (pin 321 → 340, 19 new cells). Mutation battery 9/9 killed — M5 and M9 both found surviving first, which is the only reason I trust the rest of it.

[justinchuby]

M10 closed rather than accepted, because the comment above it was making the claim ("Refuse BEFORE anything else, including the legacy consult") and nothing was holding it to that.

You are right that every cell in the section sets HOSTLOCK_DIR, so LOCK_DIR_SOURCE is env and refuse_if_legacy_held returns at hostlock.sh:382 before doing anything — the ordering was invisible to all twenty of them. Fixture added: a config whose lock_dir points at the unwritable parent, plus the same live legacy holder the migration section builds. Swapped order answers 2, correct order answers 7:

KILLED M10 preflight runs after the legacy consult
   by: FAIL  an unusable configured dir refuses as 7 even while the legacy path is live

Exactly one cell reddens it, which is the property I wanted. And the severity is not quite as low as it looks from the fixture's exoticism: that combination — a config-source host, a live holder on the old path, a configured dir the host cannot create — is precisely the state the /tmp migration in #1953 passes through. Under the swapped order the agent is told a peer has the box, waits for a lock it could never have acquired, and never sees the reason.

Also took your cleanup-can-delete-the-fixtures analysis as the answer to a question I had not asked properly: the chmod 700 restores are in-section on the normal path and the dirs cleanup could ever see are empty, so rmdir only needs w+x on the repo dir. Left the interrupt-window NIT alone — it is pre-existing behaviour of cleanup and fixing it there would be a different PR.

Not taking the root-caveat note as a code change either: under CAP_DAC_OVERRIDE six cells flip and the count pin fails, which is loud rather than silent, and CI is non-root.

341/341, mutation battery 10/10 killed. Three of the ten (M5, M9, M10) were found surviving first — two of those by you — which is most of why I trust the other seven.