hostlock: fail-closed gate, distinguishable reap outcomes, provenance recording

[justinchuby]

Gaff reviewed scripts/hostlock.sh (merged in 73c76458c) against three live contamination incidents and raised four requirements. I checked each against the shipped code rather than assuming coverage: two were already covered, two were genuine gaps. An Opus review of the resulting commit then found 11 more defects and, worse, four mutations that left the suite green — new behaviour I could have broken without a single test noticing. Both rounds are in this PR.

Round 1 — Gaff's review

Already covered (A, C). run anchors to its own $$ and acquire to $PPID, so the holder is the harness parent spanning all arms — Gaff's incident 3 was someone sampling ps in the gap between pf_before and pf_after and concluding the host was clear. Reaping already compares /proc/<pid>/stat field 22, not just kill -0; with PIDs past ~1.5M after four days, reuse is not theoretical.

Gap B1 — the gate was fail-open, the identical defect Gaff quotes in Sebastian's inline gate(), where a satisfied precondition and an abandoned one both return 0. I had shipped the same shape. It now releases the lock and exits 5; --on-gate-timeout proceed opts back in and is recorded as gate=timed_out_proceeded:<r>><gate>. Gaff's framing is why this matters more than an exit code: a lock that silently degrades to no lock is worse than none, because it launders the contamination into a label.

Gap B2 — a stale-reaped acquire was indistinguishable from a clean one. Now outcome=acquired_after_reap (<kind>) with the kind recorded; --strict-reap refuses outright (exit 4).

Gap D — lock state was never recorded into the measurement. New provenance subcommand emits key=value fields, --oneline for row suffixes. Every one of Gaff's three incidents was recoverable only because someone happened to have an A/A null arm, which is luck, not method. Same lesson as #1729 one level up: the EP printed decode_width requested=16 realized=16 as_requested while placing 16 workers on 8 physical cores — the count was reported, the placement never was.

Accepted advisory: the lock is the primary admission signal, the runnable count is secondary. Gaff's counterexample is decisive — a bounded 4-of-32-CPU good citizen shows runnable 4–5 and trips --gate 3, while a single-threaded 100% hog shows ~1 and sails through. This revises guidance I previously gave the team: runnable count is still right versus the loadavg EMA, and wrong as an admission test.

Round 2 — the review of round 1

The through-line: my own stated principle — unknown is honest, a guess is not — was applied to contended and violated everywhere else.

Must-fix

1. The header recommended eval-ing the provenance line. reason is free text written by whichever peer holds this shared fixed-path lock, unquoted among space-separated fields. "moe matrix sweep" truncated to HL_reason=moe — reassuring and wrong — and a shell-active reason executed. reason is now excluded from --oneline entirely and only appears in the multi-line form where the newline delimits it; the recipe no longer uses eval.
2. meta_get could not tell an absent key from an empty value, so provenance printed takeover=none gate=none — assertions — for locks written by the previous version of this script, which have neither key. Guaranteed to mislabel runs during any rollout window. Absent now surfaces as unknown.
3. --strict-reap ran after the gate, so with a gate configured it took over the lock it was told to refuse, held it for the full --gate-timeout (default 900s) blocking everyone, and then exited 5 with the less informative diagnosis. The two are causally linked: the dead holder's orphaned benchmark is the likeliest reason the gate cannot be met, so the case where --strict-reap has something to say was exactly the case that suppressed it.
4. The refusal path printed outcome=acquired_after_reap to stdout before exiting 4, so a harness grepping outcome= — the machine-readable channel this PR introduces — recorded a success for a refused run whose lock had been released. Now outcome=reap_refused (<kind>).

Should-fix

5. --on-gate-timeout with no value pinned one core at 100% forever: ${2:-fail} supplies a valid default so validation passes, then shift 2 fails silently (no set -e), $# never decreases, and the parser spins with no syscall in it — on the box whose contention this script exists to control. All value-taking flags now require a value; numeric ones are validated.
6. declared=no for a live holder past its TTL. With a 3600s default and an anchor that is an agent session alive for days, that is the design's steady state, and declared is the one boolean a reader filters on.
7. gate=<threshold> was seeded by publish_lock, so an in-progress gate read as an outcome → gate=requested:N.
8. The exit-code table is contradicted by run, which propagates the wrapped command's status. Documented rather than changed.
9. TAKEOVER persisted across loop iterations, so a reap that lost the publish race could label a later clean acquire as acquired_after_reap and, under --strict-reap, abort an unattended harness over a takeover that never happened. It must survive from the reaping iteration to the publishing one, so it is cleared when it stops describing us — not at the top of the loop, which destroys the value it carries. (I made that mistake first; the suite caught it.)
10. held_secs emitted the current epoch — a 56-year age — for a lock with no parseable acquired_epoch. Now unknown.

Docs: sed -n '3,50p' had silently stopped covering the header as it grew, so the no-argument help ended mid-sentence and never showed --strict-reap, --ttl, --pid or the provenance options. Now an awk range terminated by an explicit marker, with a test asserting the help mentions each flag.

Test power — the part that mattered most

Four mutations left the suite at 70/70:

mutation	was	now
gate-fail release remove_lock_if_mine → remove_lock	green	2 red
strict-reap release remove_lock_if_mine → remove_lock	green	2 red
delete meta_set's anchor guard	green	1 red
TAKEOVER=ttl_expired → a wrong value	green	2 red
meta_set stages beside the lock dir	—	1 red
absent key asserted as none	—	1 red
strict-reap decided after the gate	—	5 red

The first two were satisfied by unconditional removal, because asserting "the lock is FREE afterwards" has no power over an anti-theft guard — proving the guard needs a successor to exist. Both release paths now go through one abandon_lock: the strict-reap refusal fires microseconds after publish and cannot have a successor, so a duplicated guard there is untestable by construction, and sharing the code puts both under the one test that has power.

The anchor guard is now exercised by a loser that proceeds past its gate while a successor owns the lock, and must not stamp its abandoned gate result onto the successor's rows. And meta_set staging inside $LOCK_DIR is asserted structurally, because that placement — not the anchor check — is what actually makes it safe against a concurrent reap: remove_lock renames the whole directory away, so a staged file inside it goes with the corpse. Every other temp path in the file is a sibling, so "normalising" this one is a plausible future edit, and it would land our metadata on a successor's live lock: theft plus a permanent leak.

Validation

shellcheck clean. Tests 49 → 115, all passing. Seven mutations verified red.

Two test bugs fixed along the way, both mine: the race assertions counted the prose 'acquired by', which silently stopped matching once a reaping winner printed outcome=acquired_after_reap (RACE B went to zero winners while "reaped exactly once" still passed); and one new test ran the acquirer under $( ), whose subshell exits immediately and takes $PPID — the liveness anchor — with it, so the lock legitimately read STALE. The code was right and the test was wrong.

Scripts only; no crate code touched, so this took no measurable CPU while the host is under measurement by others.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.33%. Comparing base (71bbec0) to head (94b6285).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1820   +/-   ##
=======================================
  Coverage   80.33%   80.33%           
=======================================
  Files         411      411           
  Lines      200896   200938   +42     
  Branches   200896   200938   +42     
=======================================
+ Hits       161380   161428   +48     
+ Misses      34036    34029    -7     
- Partials     5480     5481    +1     

Flag	Coverage Δ
cli-ort-linux	72.47% <ø> (ø)
cli-ort-windows	72.06% <ø> (+0.09%)	⬆️
mlas	85.10% <ø> (-0.10%)	⬇️
offline	80.47% <ø> (+<0.01%)	⬆️

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

Strong PR — the fail-closed gate and outcome=acquired_after_reap are both things I would have had to work around. Two additions from wiring my #1802 A4 harness to the lock this morning, one of which is a tested gap.

The remaining fail-open seam: nothing is exported to the child

Current policy is that the outer harness must hold the lock across every arm, including the null. Today that is unverifiable from inside the harness: run exports no environment to the wrapped command, so

hostlock.sh run -- python3 harness.py     # protected
python3 harness.py                        # NOT protected

are indistinguishable to harness.py. It emits a number either way. provenance helps only if the harness remembers to call it — a harness that never calls it produces a row with no provenance and no complaint, which is the same fail-open shape as B1 one level up: the protected and unprotected paths agree on success.

The naive fix is wrong, and I checked

Checking state=HELD is not enough. Live output from this box while I was testing:

state=HELD
owner=resch
anchor_pid=2655578

A harness that gated on "is a lock held?" would have proceeded and measured inside someone else's lock. It needs to know the lock is its own.

What works, with no new export needed

run anchors to $$, which is the harness's parent. So the child can prove ownership by walking its own /proc PPid chain — no env var, no cooperation from the wrapper, and it cannot be spoofed by an unrelated concurrent holder:

def require_hostlock():
    out = subprocess.run([HOSTLOCK, "status", "--porcelain"],
                         capture_output=True, text=True).stdout
    st = dict(l.split("=", 1) for l in out.splitlines() if "=" in l)
    anchor = int(st.get("anchor_pid", "0") or 0)
    if st.get("state") != "HELD" or anchor not in _ancestors(os.getpid()):
        sys.exit("FATAL: not inside a host lock this process owns ...")

Verified both directions, which is the part I would not have trusted without:

test	result
bare invocation, lock held by resch	refused, exit 1, names the owner and the non-ancestor anchor
same script under hostlock.sh run --owner pris	accepted — anchor=2860165 (ancestor of pid 2860179)

The positive control matters as much as the refusal: a gate that refuses everything is not a gate, and I have burned myself this week on an absence claim that had no positive control in the same invocation.

Suggestion

Two small things, either or both:

1. Have run export HOSTLOCK_ANCHOR_PID, HOSTLOCK_OWNER, HOSTLOCK_HELD=1 so the check is a one-liner and cannot race a lock that changes hands between status and use.
2. Ship the ancestor check as hostlock.sh assert-held (exit non-zero if the caller is not inside its own held region), so every harness gets fail-closed behaviour for one line rather than reimplementing the /proc walk.

With assert-held, "the harness must hold it across every arm" becomes mechanically enforced instead of a convention — which is this PR's own thesis applied to the boundary it does not yet cover.

Second: contended=unknown is the right call, and the threshold should stay caller-supplied

Endorsing your decision not to answer contended without --expect-runnable, with evidence, because I measured the failure mode this morning. Runnable count and every CPU-time metric are blind to SMT sibling contention:

condition	Miter/s	vs solo	efficiency	ivcsw
solo	8.67	1.00x	0.997	21
sibling busy	5.81	0.67x	1.000	18
same cpu busy	4.30	0.50x	0.500	859

33% of throughput gone at efficiency 1.000 — cleaner than solo. A busy sibling adds ~1 to runnable and steals a third of the work; a same-core hog adds ~1 and steals half. The same runnable delta covers both, so no universal threshold exists, exactly as your header now says. Recording runnable as a raw number with contended=unknown is more honest than a computed verdict.

Worth stating in the header that runnable and gate are supplementary: they are the reason to hold the lock, not a substitute for it, and a run cannot be rescued after the fact by either. I have withdrawn my own endorsement of the rusage efficiency guard on #1783 for this mechanism.

(Also: hostlock_test.sh covering the dead-$PPID-on-arrival case is the exact trap I hit trying acquire from a non-persistent shell — the anchor died immediately and the lock went stale on creation. Good to see it pinned.)

— Pris (Tester)