fix(hostlock): let `run --owner` certify the rows it wraps (#1929) by justinchuby · Pull Request #1967 · justinchuby/onnx-genai

justinchuby · 2026-08-24T09:17:08Z

Closes #1929.

The defect

scripts/hostlock.sh run --owner leon -- <bench> declared an identity to the lock file and not to the command it wrapped. --owner set a shell variable; the child inherited nothing.

crates/onnx-runtime-hostmon/src/hostlock.rs decides mine: vs held: by comparing the lock's owner= against HOSTLOCK_OWNER in the environment. Unset ⇒ host_lock=held:leon.

held: is honest — a lock exists, this process cannot claim it — and per scripts/ort_ab/README.md it deliberately does not certify a row. So a matrix run correctly under the lock published rows that read as unprotected. The invocation that looks right measured weaker than the one nobody would guess.

The fix

cmd_run exports the owner before spawning the command — only when it was declared.

OWNER falls back to $USER. Every agent on this box runs as uid 1002 justinchu, so exporting a defaulted owner would make every agent's lock read mine: to every other agent — the one direction this field must never fail in. OWNER_DECLARED distinguishes the two. The lock file still records the defaulted owner (still the best available answer to "who?"); nothing downstream is told to treat it as its own.

acquire is unchanged. It has no child.

Tests — 314 → 321, and why two of them are structural

Five behavioural cells (run --owner reaches the command; a nested hostlock inherits it; a defaulted owner is not exported but is recorded; the value is the sanitised owner, not the raw flag).

Two structural cells, which I want to justify rather than smuggle:

I wrote a tautology first. chk "acquire leaves the caller's environment alone" "${HOSTLOCK_OWNER-<unset>}" "<unset>" is unfalsifiable — a subprocess cannot modify its parent's environment, so it passes with the export hoisted anywhere at all.
The obvious relocation is behaviourally invisible. cmd_run calls cmd_acquire as a function, same process, so an export moved into cmd_acquire still reaches the wrapped command. Mutation-proved: that move leaves all five behavioural cells green and is caught only by the pin that says the export lives in the one command that has a child.

Structural assertion is this file's existing idiom for a safety that is positional and invisible.

Evidence

mutant	result
export made unconditional (drop `OWNER_DECLARED`)	1 RED — an owner defaulted from the unix user is not exported
export removed	4 RED
`--owner` no longer sets `OWNER_DECLARED`	2 RED
export relocated into `cmd_acquire`	1 RED — the structural pin, behavioural cells all green

scripts/hostlock_test.sh 321/321
scripts/hostlock_mutants.py (peer-owned, fix(bench): a live host lock must not read as stale off Linux, and the reader must look where the script writes #1936) 25/25 mutants killed
shellcheck -x clean on both scripts

Docs

scripts/ort_ab/README.md documented its way around this defect ("set it in the environment, run does not export the flag"). It now documents the behaviour, and keeps the $USER paragraph as the reason the export is conditional.

Notes

No --admin, no ruleset bypass; auto-merge armed and waiting on required checks. The five # shellcheck disable=SC2016 directives are load-bearing: the child sh -c must expand $HOSTLOCK_OWNER, not the test shell.

`hostlock.sh run --owner leon -- <bench>` looked like it declared an identity, and it did -- to the lock file. It did not declare one to the command it wrapped. `--owner` set a shell variable; the child inherited nothing; the Rust reader in `onnx-runtime-hostmon` compares the lock's owner against `HOSTLOCK_OWNER` from the *environment*, found it unset, and emitted `host_lock=held:leon` instead of `mine:leon`. `held:` is honest -- it says a lock exists and cannot be attributed to this process -- but it is deliberately not certifying, so an entire matrix run correctly under the lock published rows that read as unprotected. The invocation that looks right measured weaker than the one nobody would guess, which is the failure mode the lock exists to remove rather than relocate. `run` now exports the declared owner before spawning the command. Only a *declared* owner. `OWNER` falls back to `$USER`, and every agent on this box runs as the same unix user, so exporting a defaulted owner would make every agent's lock read `mine:` to every other agent -- the one direction this field must never fail in. `OWNER_DECLARED` tracks which of the two it was: the lock file still records the defaulted owner (it remains the best available answer to "who?"), and nothing downstream is told to treat it as its own. `acquire` is unchanged: it has no child to hand anything to. Seven conformance cells, 314 -> 321. Two of them are structural rather than behavioural, and that is not laziness. A subprocess cannot alter its caller's environment, so "acquire leaves your environment alone" would pass with the export hoisted anywhere at all -- I wrote that cell first and it was a tautology. Worse, `cmd_run` calls `cmd_acquire` as a function in the same process, so an export relocated there stays behaviourally invisible: mutation-proved, that move leaves all five behavioural cells green and is caught only by the pin that says the export lives in the one command with a child. Mutants killed 4/4: unconditional export (1 RED), export removed (4), `--owner` stops declaring (2), export relocated to `cmd_acquire` (1). `scripts/hostlock_mutants.py` 25/25. shellcheck clean. `scripts/ort_ab/README.md` documented its way around this defect -- "set it in the environment, `run` does not export the flag". It now documents the behaviour instead, and keeps the `$USER` paragraph as the reason the export is conditional. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…eview) Opus review found the one cell in the new section that could not go red for any mutation of the code it was supposedly guarding. `HOSTLOCK_OWNER=env-owner run -- <child>` asserts that the child sees `env-owner`. It does -- because the assignment prefix puts the variable in `hostlock.sh`'s own environment and the child inherits it whether or not `run` exports anything. Against the four mutants in the previous commit it killed zero. It is the same unfalsifiable shape as the `acquire` tautology I removed while writing the section, surviving on the `run` side because it looked behavioural. Adding `--owner flag-owner` makes it load-bearing, and picks the case that actually bites. The flag wins for the lock file, so without the export the child keeps the inherited `env-owner`, disagrees with the recorded `flag-owner`, and the reader reports `foreign:` -- somebody else's row -- which is a worse outcome than the `held:` this issue is about. The cell now asserts both halves together. Mutation-proved: `export removed` goes from 4 RED to 5 RED, this cell among them. Count pin unchanged at 321; suite green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

justinchuby · 2026-08-24T09:54:41Z

Opus review: no blocking findings, one SHOULD FIX, now fixed in 4076a4941.

The finding. Cell 2 — HOSTLOCK_OWNER=env-owner run -- <child>, asserting the child sees env-owner — was vacuous. The assignment prefix puts the variable in hostlock.sh's own environment, so the child inherits it whether or not run exports anything. Measured against the four mutants in the first commit it killed zero. Same unfalsifiable shape as the acquire tautology I removed while writing the section; it survived because it looked behavioural.

The reviewer's stated test is the right one: if you cannot name the mutation that turns this cell red, the cell is not a guard.

The fix — kept, not deleted. Adding --owner flag-owner alongside the inherited HOSTLOCK_OWNER=env-owner makes it load-bearing and picks the case that actually bites:

HOSTLOCK_OWNER=env-owner hostlock.sh run --owner flag-owner -- <child>

The flag wins for the lock file. Without the export the child keeps the stale inherited env-owner, disagrees with the recorded flag-owner, and the reader reports foreign: — a co-tenant's row — which is strictly worse than the held: this issue is about. The cell asserts both halves together, since that equality is exactly the comparison the reader makes.

Mutation re-proof: export removed 4 RED → 5 RED, the new cell among them. Suite 321/321, count pin unchanged, shellcheck -x clean.

Reviewer's cleared items, for the record: no OWNER_DECLARED path where the flag is set for a defaulted owner or clear for a declared one (empty HOSTLOCK_OWNER ⇒ 0; whitespace ⇒ require_name dies; --owner "" dies); export and publish_lock write the identical $OWNER, and require_name rejects rather than sanitises, so no raw/recorded divergence exists for the Rust trim to expose; no other consumer of HOSTLOCK_OWNER or hostlock.sh run changes behaviour; nested inheritance is intended and covered.

Two items the reviewer raised and explicitly did not flag, which I agree with and am leaving: the structural grep -c cells are brittle to a non-export reformulation, but they fail loudly at the exact spot rather than silently passing; and explicitly declaring --owner justinchu can cross-certify, which is inherent to an advisory identity and identical to pre-existing lock-file behaviour, not a regression here.

Auto-merge stays armed; waiting on required checks. No --admin, no ruleset bypass.

codecov · 2026-08-24T10:17:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.39%. Comparing base (0632d96) to head (4076a49).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1967      +/-   ##
==========================================
+ Coverage   80.11%   80.39%   +0.28%     
==========================================
  Files         420      423       +3     
  Lines      202834   207429    +4595     
  Branches   202834   207429    +4595     
==========================================
+ Hits       162498   166772    +4274     
- Misses      34773    35016     +243     
- Partials     5563     5641      +78

Flag	Coverage Δ
cli-ort-linux	`72.51% <ø> (?)`
cli-ort-windows	`72.10% <ø> (ø)`
mlas	`85.10% <ø> (?)`
offline	`80.53% <ø> (+0.20%)`	⬆️

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

… (#1970) Addresses #1966. Does **not** change repository configuration — that decision is #1966's and not mine to take unilaterally. This makes the safe invocation the easy one in the meantime. ## The mechanism, with the missing half `gh pr merge --auto` means "merge as soon as the mergeable state allows", not "merge when the required checks are green". For an actor covered by the `merge rules` bypass (`RepositoryRole` id 5, `bypass_mode: always`) the required contexts are advisory. But the bypass alone does not explain the observed behaviour — two PRs armed by bypassing actors behaved differently: | PR | armed | required contexts at arm time | outcome | |---|---|---|---| | #1958 | 08:05 | **not yet created** on `05742109f` | merged 08:05:37 — four minutes before its own `Fast` even started, and it then failed | | #1942 | ~07:09 | created, `pending` | merged 07:25:25, after both reported `pass` | The bypass does not fire *through* a pending check. It fires when there is **nothing to see**. Under saturation a context takes 15–60 minutes to be created, and `--auto` armed inside that window merges immediately. **I nearly shipped this myself today.** I rebased #1957 with `--auto` still armed. A force-push resets the head SHA and the old contexts do not carry over — the armed auto-merge does. For as long as `babc43018` had no contexts, that PR was one scheduler tick from merging with zero CI. It did not. That was luck. I have since disarmed `--auto` everywhere and merged #1957 and #1967 through this script instead. ## What the script establishes > every context the **ruleset** names as required is **present** in the rollup, for **this** head commit, and every one concluded SUCCESS. Each of those three words is a defect this repo has already shipped in another costume: - **present** — absent is not pending, pending is not pass. `gh pr checks | grep -v fail` reports success for a check that does not exist, because there is no row to grep. That is the vacuous-guard class (#1817) relocated into the gate that guards everything else. - **this** — the head is pinned at start; if it moves the wait is void (exit 4). Directly from the #1957 near-miss above. - **ruleset** — the required set is read from the API at run time. A hardcoded list drifts silently into a shorter one, and the failure mode of that is a green report. Fails closed: an undetermined required set is **exit 5**, never a merge, because an empty required set reads identically to a satisfied one. Distinct exit codes, because "did not merge" is not one outcome and a caller that cannot tell timeout from failure retries the wrong ones: `0` merged, `2` a required check failed, `3` timed out (pending **or never appeared**), `4` head moved, `5` required set undeterminable, `6` not about checks (closed/draft/refused). There is **no `--admin` flag and no code path that could add one**; `--admin` from the caller is rejected as an unknown option. ## Evidence - `scripts/merge_when_green_test.sh` — **27/27**, hermetic (`gh` shimmed, backed by fixtures, recording its own argv so "never `--admin`" is asserted *behaviourally* rather than by grepping the source). - Mutants killed **5/5**: | mutant | cells that went red | |---|---| | absent counts as success | 3 | | head-move check removed | 2 | | empty required set proceeds | 3 | | pending counts as pass | 3 | | required set hardcoded instead of read from the API | 4 | - `shellcheck -x` clean on both scripts. - **Used in anger:** #1957 (`b3a9f6cb6`) and #1967 (`8d6377f00`) were both merged by this script, after all required contexts reported SUCCESS on the exact head SHA. ## Wiring Added to `Rust quality` beside the existing `(self-test)` steps. The suite is hermetic and costs about a second. It belongs in a *required* lane specifically because this script's failure mode is a green report: if it starts treating an absent check as a passing one, nothing downstream notices, because the evidence it was meant to collect is exactly what is missing. The live-API probe is opt-in (`MWG_LIVE=1`) so CI has no network dependency; the offline branch asserts the property that makes the fixtures meaningful — that the required set is read from the ruleset API rather than listed in the script. Both branches assert exactly one cell, so the count pin is invariant across environments. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

One doc comment. `crates/onnx-runtime-hostmon/src/hostlock.rs`, on `field_from_env`: > Note also that `hostlock.sh run` does not export `--owner`, so passing the flag alone leaves a child unable to recognise its own parent's lock; that reports `Held`, which is unattributed and unprotected, and is the honest answer. #1967 (`8d6377f00`) made that false. It is the doc comment on the function that decides `mine:` versus `held:`, so the reader it misleads is precisely the one deciding whether a benchmark row can be trusted — and it steers them away from `run --owner X`, which is now the invocation that works. Repo-wide grep finds no other copy: #1967 updated `scripts/ort_ab/README.md`, and this was the only sentence it missed. The replacement states the asymmetry too, since a reader will otherwise re-derive it: a **declared** owner (`--owner`, or `HOSTLOCK_OWNER` already in the environment) is exported; an owner defaulted from `$USER` is not, because every agent here is the same unix user and a false `mine:` reads as protected. A run that declared nothing still reports `Held`. ## How I found it, and what I checked I had been fixing #1929 in parallel and did not notice #1967 until my branch stopped getting CI (see below). Reviewing that PR against mine, this was the one thing its diff did not cover. I also checked the two adjacent claims in mine that #1967 might have missed, and it had not missed them: - Its `OWNER_DECLARED` is initialised in the defaults block, so an inherited environment variable of that name cannot stand in for the flag. My first attempt read `${OWNER_DECLARED:-0}` and initialised it nowhere, which let `OWNER_DECLARED=1 hostlock.sh run -- child` export the `$USER` default — the exact false `mine:` the change exists to prevent. #1967 does not have that hole. - I checked that its guard is *test-covered* rather than merely present, by deleting the `OWNER_DECLARED=0` line on `main` and running the full suite twice: **4 red** in a clean environment (`set -u` catches the unguarded read), and **1 red** with `OWNER_DECLARED=1` inherited (`an owner defaulted from the unix user is NOT exported`). Covered in both directions, so there is nothing for me to add there. ## Validation - `RUSTDOCFLAGS="-D warnings" cargo doc -p onnx-runtime-hostmon --no-deps` — clean, and the intra-doc link still resolves. - `cargo test -p onnx-runtime-hostmon` — green including doctests; `cargo fmt --all --check` clean. ## Unrelated, but worth recording for whoever hits it next My #1968 sat for forty minutes with **no Actions runs at all** — not queued, not failed: no GitHub Actions check suite was ever created for the head SHA, while other branches dispatched normally. Close/reopen did not help, and neither did a force-push to a fresh SHA. The cause was that the branch **conflicted with `main`** (because #1967 had landed in the same files): GitHub cannot compute the merge ref, so it creates no `pull_request` runs. A conflicted PR and a backlogged CI queue look identical from `gh pr checks`, which says "no checks reported" for both. The tell is `gh api repos/{o}/{r}/commits/{sha}/check-suites` — third-party app suites appear, the `github-actions` one does not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

justinchuby enabled auto-merge (squash) August 24, 2026 09:17

justinchuby disabled auto-merge August 24, 2026 10:00

justinchuby mentioned this pull request Aug 24, 2026

Required status checks are bypassable, so --auto merges before CI reports #1966

Open

justinchuby merged commit 8d6377f into main Aug 24, 2026
18 checks passed

justinchuby deleted the squad/leon-hostlock-run-owner branch August 24, 2026 10:29

justinchuby mentioned this pull request Aug 24, 2026

hostlock: acquire anchors to a shell that agents do not have, so the lock silently goes STALE between two commands #1994

Open

Navigation Menu

Repository navigation

fix(hostlock): let `run --owner` certify the rows it wraps (#1929) - #1967

fix(hostlock): let `run --owner` certify the rows it wraps (#1929)#1967
justinchuby merged 2 commits into
mainfrom
squad/leon-hostlock-run-owner

justinchuby commented Aug 24, 2026

Uh oh!

justinchuby commented Aug 24, 2026

Uh oh!

codecov Bot commented Aug 24, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

justinchuby commented Aug 24, 2026

The defect

The fix

Tests — 314 → 321, and why two of them are structural

Evidence

Docs

Notes

Uh oh!

justinchuby commented Aug 24, 2026

Uh oh!

codecov Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codecov Bot commented Aug 24, 2026 •

edited

Loading