Repository navigation
fix(hostlock): export a declared owner to the wrapped command - #1968
justinchuby wants to merge 2 commits into
Conversation
`run` acquired the lock as OWNER and then never told the command about it.
A harness that reads the lock therefore saw a holder it could not attribute
and labelled its own protected run UNPROTECTED: `run --owner leon` produced
`host_lock=held:leon`, never `mine:leon`. The one state the field exists to
certify was unreachable by the one invocation that earns it, and the reader
was right both times -- `held:` is the honest answer when the child has been
told nothing.
`--owner` was a shell variable, invisible across the fork; only the
environment form worked, and only incidentally, by inheritance. The README
documented around it. It now says what the code does instead.
Only a DECLARED owner is exported -- `--owner`, or `HOSTLOCK_OWNER` in the
environment. The `$USER` default is not, and that asymmetry is the point:
every agent on this box is the same unix user, so a child told to answer to
`$USER` would claim a co-tenant's lock as its own. A false `mine:` is worse
than a missing one, because it reads as protected. The lock file still
records the default, so `acquire` is unchanged.
`OWNER` cannot distinguish the two sources by the time anything reads it, so
each is checked at its origin: the environment directly, and `--owner` via
OWNER_DECLARED set in the parse loop.
Six assertions, each shown to fail under its own defect rather than assumed
to cover one:
* never export (the defect being fixed) kills four of the six;
* export unconditionally, including the `$USER` default, kills exactly the
one cell that holds that asymmetry down, and nothing else;
* scrub the variable for the child kills five, including the environment
cell -- which the first mutant does NOT kill, since an exported variable
is inherited whether `run` re-exports it or not. That cell is annotated
with what it does and does not exclude, because an assertion whose defect
nobody identified is indistinguishable from one that cannot fail.
The load-bearing cell compares the exported name against the name written in
the lock, not merely that the variable exists: the reader says `mine:` by
comparing those two, so an assertion on presence alone would pass with them
disagreeing. Nested inheritance is asserted deliberately rather than left
incidental, on a separate lock directory -- a nested acquire on the same lock
is contention, not inheritance.
Suite: 320/320 with the assertion-count pin updated; shellcheck clean.
Closes #1929.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1968 +/- ##
==========================================
+ Coverage 80.21% 80.84% +0.63%
==========================================
Files 406 423 +17
Lines 188213 207425 +19212
Branches 188213 207425 +19212
==========================================
+ Hits 150974 167695 +16721
- Misses 31842 34083 +2241
- Partials 5397 5647 +250
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Review of the first commit falsified its headline claim. `run_owner_is_declared`
read `${OWNER_DECLARED:-0}` and nothing ever assigned that name, so bash's
import of inherited variables made it answerable from the environment: a caller
exporting `OWNER_DECLARED=1` with no `--owner` and no `HOSTLOCK_OWNER` got the
`$USER` default exported, and the child then matched a lock it had never
claimed. That is precisely the false `mine:` the change exists to prevent,
reintroduced by the mechanism meant to prevent it.
The comment that argued for reading it with a default -- "keeps this
self-contained" -- was the defect's own justification, which is why it is gone
rather than edited. `OWNER_DECLARED=0` now sits in the defaults block with
every other option variable, and the read is unguarded, so `set -u` would catch
a future path that skipped it.
Asserted by a seventh cell, and the assertion is attributed: removing the
initialisation kills that cell and nothing else (320/321), so it is testing the
severance and not something incidental.
Also from review:
* `field_from_env`'s rustdoc still told readers `run` does not export the
owner. It is the doc comment attached to the function whose meaning this
change alters, so it would have misled exactly the reader who most needed
it. Repo-wide grep finds no other copy.
* The probes used `${HOSTLOCK_OWNER:-<unset>}` under a comment claiming it
distinguishes exported-empty from never-exported. The colon form reports
both as unset. Switched to the no-colon form, which does what the comment
says.
Full mutation table, each arm a complete suite run:
* baseline 321/321
* never export kills 4
* export unconditionally, $USER included kills 2
* scrub HOSTLOCK_OWNER for the child kills 5
* drop the OWNER_DECLARED initialisation kills 1 (the new cell)
Every one of the seven cells is killed by at least one arm.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3e30bb6 to
29a777f
Compare
|
Closing this as superseded by #1967 ( I did not check for an existing PR on #1929 before starting — I checked the issue, which was open, and assumed that meant unclaimed. It does not, and that is my error to carry: #1967 is the better implementation, and I checked that rather than assuming itIts Its test cells are also stronger than mine in two places I want to name, because I would otherwise be tempted to re-propose them:
Its structural pin (that the export lives in the one command with a child) covers a mutation mine did not test at all: relocating the export into What I verified about it before closingI checked that its
Covered in both directions. So the one cell I could have contributed is redundant, and I am not proposing it. What survivesExactly one thing, now in #1981: One diagnostic worth keepingThis PR sat for forty minutes with no Actions runs at all — no |
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>
1 participant
Closes #1929.
The defect
hostlock.sh run --owner leon -- <bench>acquires the lock asleonand then runs the command without telling it.--owneris a shell variable, invisible across the fork. A benchmark that reads the lock therefore cannot tell its own parent's declaration from a co-tenant's, and prints:held:is deliberately not a protected label. So the obvious invocation was the one that silently produced the weaker answer, and the only way to getmine:was the environment form — which worked by inheritance, incidentally, not by design.scripts/ort_ab/README.mddocumented around it; it now documents what the code does.The fix
Export
HOSTLOCK_OWNER="$OWNER"for the child incmd_run— only when the owner was declared, by--owneror byHOSTLOCK_OWNERin the environment.The
$USERdefault is deliberately not exported. Every agent on this box is the same unix user, so a child told to answer to$USERwould claim a co-tenant's lock as its own. A falsemine:is worse than a missing one: it reads as protected. The reader in #1925 has no$USERfallback for exactly this reason, and exporting one fromrunwould have reintroduced it through the back door. The lock file still records the default, soacquireis unchanged.OWNERcannot answer "was this declared?" — by the time anything reads it, the$USERdefault is indistinguishable from an explicit--owner "$USER". Each source is therefore checked at its origin: the environment directly, and--ownerthroughOWNER_DECLARED, which is initialised in the defaults block and set by the parse loop.The second commit exists because review falsified the first
The first version read
${OWNER_DECLARED:-0}and initialised that name nowhere, with a comment arguing that this "keeps this self-contained". Bash imports inherited environment variables as shell variables, so the flag was answerable from the environment:OWNER_DECLARED=1 hostlock.sh run -- child, with no--ownerand noHOSTLOCK_OWNER, exported the$USERdefault, and the child then matched a lock it had never claimed. The mechanism meant to prevent the falsemine:reintroduced it. The comment was the defect's own justification, which is why it is gone rather than edited.Review also found the reader's own rustdoc (
field_from_env) still telling readers thatrundoes not export the owner — the doc comment attached to the function whose meaning this change alters — and that the probes'${VAR:-...}sat under a comment claiming it distinguishes exported-empty from never-exported, which the colon form does not do. Both fixed.Evidence
Seven cells in
hostlock_test.sh. Rather than assume they cover the defect, each was shown to fail under a defect of its own. Every arm below is a full suite run on an isolated copy:$USERincludedHOSTLOCK_OWNERfor the childOWNER_DECLAREDinitialisationEvery one of the seven cells is killed by at least one arm.
The environment cell is not killed by the first arm: an exported variable is inherited whether
runre-exports it or not. It is killed by the third, and it is annotated in place with what it does and does not exclude — an assertion whose defect nobody identified is indistinguishable from one that cannot fail. Same class as the latched-OnceLockA/B in #1736 and thecompile_faildoctest that passed on a typo in #1950.The load-bearing cell compares the exported name against the name in the lock, not merely that the variable is set: the reader decides
mine:by comparing those two, so an assertion on presence alone would pass with them disagreeing.Nested inheritance (a wrapped script taking a second lock) is asserted deliberately, on a separate lock directory — a nested acquire on the same lock is contention, not inheritance, and would have tested the wrong thing.
Every cell controls
HOSTLOCK_OWNERexplicitly (env -uor an explicit value). The agents that run this suite export that variable, so an ambient value would otherwise decide these cells instead of the code under test.Validation
bash scripts/hostlock_test.sh→ 321/321, count pin 314 → 321.shellcheck scripts/hostlock.sh scripts/hostlock_test.sh→ clean, exit 0. The fourSC2016suppressions are load-bearing (stripping them yields exactly four info findings and a non-zero exit) and none masks an unrelated warning.RUSTDOCFLAGS="-D warnings" cargo doc -p onnx-runtime-hostmon --no-deps→ clean;cargo test -p onnx-runtime-hostmon→ green.Host lock conformanceis path-filtered to these files, so this PR triggers it.Not verified
No benchmark has yet been run under
hostlock.sh run --owner Xto observe ahost_lock=mine:Xrow end to end — the host has been held by another agent. The equality that produces that label is asserted here at the shell level (the child reads the real lock and compares) and at the Rust level in #1950's child probe, but the two halves have not been joined in one live run.Coordination
Touches
scripts/hostlock.shandscripts/hostlock_test.sh, which #1926 (gaff) also modifies. My hunks are a new helper abovecmd_run, the"$@" &line inside it, two lines in the defaults/parse region, and a new test section — no overlap with #1926's changes except the assertion-count pin, which is a one-line mechanical resolution in whichever order they land.