Repository navigation
fix(hostlock): an owner is a name, so a row cannot claim it is free while held - #1908
Merged
Merged
Conversation
…hile held
`reason` was taken out of `--oneline` because it is free text written by
whichever peer holds a shared fixed-path lock. That rationale is right,
and it applies verbatim to `owner`, which stayed in the row as `held_by`
-- earlier in the row, ahead of every field a reader uses to decide
whether the box is claimed.
$ hostlock.sh acquire --owner 'gaff hostlock_state=FREE declared=no'
$ hostlock.sh provenance --oneline
hostlock_state=HELD declared=yes held_by=gaff hostlock_state=FREE \
declared=no held_pid=none takeover=none held_uid=1002 ...
Parsed last-wins with awk -- the idiom this script's own documentation
recommends over the shell -- that row reads back `hostlock_state=FREE
declared=no`. The row physically says HELD. Reproduced against an
isolated HOSTLOCK_DIR, never the shared lock.
A lock that silently degrades to no lock is worse than none, because it
launders the contamination into a label. This is the stronger case: it
does not degrade to unknown, it asserts FREE while held.
Three things make it worth a guard rather than a doc note:
- The benign spelling is the more likely one and fails identically.
`--owner "gaff cpu team"` truncates held_by to `gaff` and injects the
keys `cpu` and `team` -- the HL_reason=moe truncation again,
reassuring and wrong.
- A newline is worse than a space. publish_lock writes `owner=${OWNER}`
into the metadata file line by line, and meta_get is
`sed -n 's/^key=//p' | head -1`, so FIRST occurrence wins: an injected
`takeover=` outranks the real one. That is the field added so a row
could not assert "no takeover" about a run that reaped a corpse.
- The fields that disclose staleness were made to travel with `held_by`.
Travelling together is not enough if one of them can rewrite the
others.
Restricting an owner to `[A-Za-z0-9_.-]+` is not a limitation; it is what
the field already meant. Validated in two places, and neither is
redundant: the flag check gives a precise message, the post-parse check
catches the same text arriving through $HOSTLOCK_OWNER or a $USER with a
space. Mutation-verified -- removing both fails 6 assertions, removing
only the post-parse check fails exactly 1, the environment one.
Nine assertions added. Three assert the parsed row rather than the raw
string, because the defect is not that the text appears but that a
consumer's reading of the row inverts. One asserts the positive case
(`--owner gaff-cpu.2` still accepted, exactly one `held_by`,
`hostlock_state` parsing to the value the row carries) so this is a
validation and not a lockout.
The `every assertion in this file ran` pin goes 249 -> 258, verified by
running the suite rather than by arithmetic: 258 passed, 0 failed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
enabled auto-merge (squash)
August 23, 2026 23:59
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1908 +/- ##
==========================================
+ Coverage 80.15% 80.33% +0.18%
==========================================
Files 413 415 +2
Lines 200757 204450 +3693
Branches 200757 204450 +3693
==========================================
+ Hits 160910 164248 +3338
- Misses 34326 34626 +300
- Partials 5521 5576 +55
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
justinchuby
added a commit
that referenced
this pull request
Aug 24, 2026
…fuse-run-ttl Both sides appended a new assertion block at the same point in the suite, so git could not tell they were independent. Resolved by keeping BOTH blocks -- this branch's `run --ttl` refusal and #1908's owner-injection refusal -- and re-deriving the pinned count by running the suite: 265. Verified per-assertion rather than by the total alone, since a total can be made to match while a block is missing: this branch's 5 TTL assertions all present, all PASS #1908's 3 owner-injection ones all present, all PASS "every assertion in this file ran" PASS Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 24, 2026
…at produces it Follow-up to #1903, fixing a defect its review found and I merged anyway because auto-merge fired on green CI while I was still writing the fix. #1903 replaced a fixed `--min-efficiency 0.8` with a threshold derived from a probe run on the same host, because 0.8 is an exclusive-host assumption and this suite has to pass co-tenanted (#1802). The correct fix, with a defect: a threshold derived from the measurement it judges is invariant to any CONSTANT scaling of that measurement. Review demonstrated it rather than suspecting it: eff = c / (w * n) * 0.5 halves every efficiency, every derived threshold and every ratio together, and all 249 assertions passed. The fixed 0.8 that #1903 removed is precisely what would have caught it. The two mutations I had run -- `eff` = constant 1.000, and dropping cstime -- both move the numbers RELATIVE to each other, which is the one class a self-referential threshold can still see, so my "coverage is preserved, verified by mutation" was a reassuring conclusion drawn from the wrong experiment. So pin the number to two anchors that do not come from that code path: * the child's own accounting of its own CPU -- os.times() inside the command, written to a file, compared against hostlock's cpu= field; * the arithmetic relating the three fields on the row it prints, efficiency ~= cpu / (wall * cores). Both hold under any load, because both compare the run against itself rather than against an expectation of the host, so neither reintroduces the quiet-host assumption #1903 removed. Mutation-verified: eff = c/(w*n) * 0.5 (constant scaling) 260/1 RED <- was 249/0 GREEN CPU_TICKS=$((cu)) (kernel time dropped) 258/3 RED (unchanged) Both re-measured after rebasing onto main at cdc7d93, because #1908 added nine assertions to this same file in the interim and a mutation count taken against the older total would be a number nobody measured. Two robustness fixes to #1903 in the same area, both observed rather than theorised. The derived-threshold slack goes 0.9 -> 0.5, and the kernel-time ratio 0.7 -> 0.5: the probe and the run it judges are separate 1.5s arms taken seconds apart, so a co-tenant arriving between them starves only the second, and at 0.9 that reddens the pair for a reason that has nothing to do with the gate under test. That is the same "goes red when somebody starts a build, so people re-run until green" trap #1903 exists to remove, and it was observed twice on this box: two assertions failing, then the identical file passing minutes later with nothing changed but the neighbours. 0.5 still leaves 2x headroom over the ~0.23 that dropping kernel time produces. And one cell that cannot be made vacuous by load at all: the verdict must follow the number printed on the same row. That holds at any starvation level and catches a gate that always says ok, always says contended, or has its comparison backwards. Suite 258 -> 261. shellcheck clean. 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
…and say why this must never be required Three corrections to #1912, all found in review of it. The comment said "252 assertions (249 before #1910)". That was a projection, not a count: 249 was the branch total and +3 was a PR that had not landed. Neither number was ever the total on main, which went 249 -> 258 (#1908) -> 265 (#1869) while #1912 sat in review. The suite already pins its own total in its last assertion, `every assertion in this file ran`, which is the only copy that cannot drift because it fails the run rather than misinforming a reader. Same defect this lock exists to catch, one level down: a figure published where nothing can falsify it. The second is load-bearing. This job is non-required by choice, but with `pull_request: paths:` a PR that does not touch these files gets NO job rather than a skipped one, so promoting it to a required check would leave every unrelated PR sitting on "Expected -- waiting for status" forever. Recorded next to the trigger so the next person to reach for the branch-protection settings reads it first. The third is that my own explanation of the escape hatch was wrong, in the same commit that complains about unchecked claims. I wrote that `ci.yml` "documents the mirror image of that trap on its `changes` job, which uses a job-level `if:`". `changes` has no `if:` at all, deliberately -- its comment records that gating it once skipped the entire workflow, because a job whose `needs` dependency is skipped is skipped whatever its own `if:` says. The job-level `if: needs.changes.outputs.docs_only != 'true'` is on the two REQUIRED jobs, and it works because a job skipped by a conditional still reports a conclusion. Workflow-level `paths:` cannot be rescued that way: there is no job to skip, so there is no conclusion to report. Also records the first real run of this job on a GitHub-hosted runner, 3m17s, against the 210s two-core emulation that sized the timeout. The emulation predicted the real number, which is the reason to keep both rather than replace one with the other. Comment-only. No behaviour change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 24, 2026
…at produces it Follow-up to #1903, fixing a defect its review found and I merged anyway because auto-merge fired on green CI while I was still writing the fix. #1903 replaced a fixed `--min-efficiency 0.8` with a threshold derived from a probe run on the same host, because 0.8 is an exclusive-host assumption and this suite has to pass co-tenanted (#1802). The correct fix, with a defect: a threshold derived from the measurement it judges is invariant to any CONSTANT scaling of that measurement. Review demonstrated it rather than suspecting it: eff = c / (w * n) * 0.5 halves every efficiency, every derived threshold and every ratio together, and all 249 assertions passed. The fixed 0.8 that #1903 removed is precisely what would have caught it. The two mutations I had run -- `eff` = constant 1.000, and dropping cstime -- both move the numbers RELATIVE to each other, which is the one class a self-referential threshold can still see, so my "coverage is preserved, verified by mutation" was a reassuring conclusion drawn from the wrong experiment. So pin the number to two anchors that do not come from that code path: * the child's own accounting of its own CPU -- os.times() inside the command, written to a file, compared against hostlock's cpu= field; * the arithmetic relating the three fields on the row it prints, efficiency ~= cpu / (wall * cores). Both hold under any load, because both compare the run against itself rather than against an expectation of the host, so neither reintroduces the quiet-host assumption #1903 removed. Mutation-verified: eff = c/(w*n) * 0.5 (constant scaling) 267/1 RED <- was 249/0 GREEN CPU_TICKS=$((cu)) (kernel time dropped) 265/3 RED (unchanged) Both re-measured against the current base rather than carried over: #1908 and #1869 added sixteen assertions to this same file while this PR was open, and a mutation count taken against a superseded total is a number nobody measured. Two robustness fixes to #1903 in the same area, both observed rather than theorised. The derived-threshold slack goes 0.9 -> 0.5, and the kernel-time ratio 0.7 -> 0.5: the probe and the run it judges are separate 1.5s arms taken seconds apart, so a co-tenant arriving between them starves only the second, and at 0.9 that reddens the pair for a reason that has nothing to do with the gate under test. That is the same "goes red when somebody starts a build, so people re-run until green" trap #1903 exists to remove, and it was observed twice on this box: two assertions failing, then the identical file passing minutes later with nothing changed but the neighbours. 0.5 still leaves 2x headroom over the ~0.23 that dropping kernel time produces. And one cell that cannot be made vacuous by load at all: the verdict must follow the number printed on the same row. That holds at any starvation level and catches a gate that always says ok, always says contended, or has its comparison backwards. Suite 265 -> 268. shellcheck clean. 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
…and say why this must never be required (#1917) Two corrections to #1912, both raised in review of it. ## 1. The assertion count was already wrong twice The comment said **"252 assertions"**. #1908 added nine to the same file before #1912 merged, and #1910 adds three more — so the number was wrong *in review* and wrong again *on merge*. A count in a comment is a number nobody re-measures. The suite already pins its own total in its last assertion — `every assertion in this file ran` — which is the only copy of that number that cannot drift, because it **fails the run** rather than misinforming a reader. This is the same defect the lock exists to catch, one level down: a figure published somewhere nothing can falsify it. The timing evidence keeps its number but now says which version produced it, since that one *was* measured. ## 2. This job must never be promoted to a required check Non-required was a cost decision in #1912; there is a harder reason, and it belongs next to the trigger. With `pull_request: paths:`, a PR that does not touch these files does **not** get a `skipped` job — it gets **no job at all**. A required check by this name would therefore sit `Expected — waiting for status` on every unrelated PR, forever. `ci.yml` documents the mirror image of that trap on its `changes` job, which uses a job-level `if:` precisely so it still reports a conclusion when there is nothing to do. Recorded here so the next person to reach for the branch-protection settings reads it before flipping the switch. ## Validation Comment-only; the `on:`, `jobs:` and step definitions are byte-identical. YAML parses. No behaviour change, so this PR's own run of the job is the regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 24, 2026
…at produces it (#1910) ## What this fixes, and how it got merged unfixed #1903's review found a real defect and I merged the PR anyway — auto-merge fired the moment required CI went green, while I was still writing the fix. This is that fix, as its own PR. #1903 replaced a fixed `--min-efficiency 0.8` with a threshold **derived from a probe run on the same host**, because 0.8 is an exclusive-host assumption and this suite has to pass co-tenanted (#1802). That was the right correction, with a defect inside it: > **A threshold derived from the measurement it judges is invariant to any constant scaling of that measurement.** Review demonstrated that rather than suspecting it, by applying the mutation: ``` eff = c / (w * n) * 0.5 ``` Every efficiency, every derived threshold and every ratio halves **together**, and all 249 assertions passed. The fixed `0.8` that #1903 removed is exactly what would have caught it. My two mutations — `eff` = constant `1.000`, and dropping `cstime` — both move the numbers *relative to each other*, which is the one class a self-referential threshold can still see. So "coverage is preserved, verified by mutation" was a reassuring conclusion drawn from the wrong experiment. That is the seventh round in a row where the fix for the previous finding carried the next one, and the direction is always the reassuring one. ## The fix: anchor the number outside the code path that produces it Two anchors, neither of which can be reached by scaling `eff`: 1. **The child's own accounting of its own CPU.** `os.times()` *inside* the command, written to a file, compared against hostlock's `cpu=` field. 2. **The internal arithmetic of the row it prints.** `efficiency ≈ cpu / (wall × cores)`. Both hold under any load, because both compare the run against **itself** rather than against an expectation of the host — so neither reintroduces the quiet-host assumption #1903 removed. | mutation | before | now | |---|---|---| | `eff = c/(w*n) * 0.5` — constant scaling | 249 / **0 GREEN** ❌ | 267 / **1 RED** ✅ | | `CPU_TICKS=$((cu))` — kernel time dropped | 246 / 3 RED | 265 / **3 RED** | Both re-measured against the current base rather than carried over: #1908 and #1869 added sixteen assertions to this same file while this PR was open, and a mutation count taken against a superseded total is a number nobody measured. ## Two robustness fixes in the same area, both observed rather than theorised **Derived-threshold slack 0.9 → 0.5, kernel-time ratio 0.7 → 0.5.** The probe and the run it judges are separate 1.5 s arms taken seconds apart, so a co-tenant arriving in between starves only the second one. At 0.9 that reddens the pair for a reason that has nothing to do with the gate under test — the same *"goes red when somebody starts a build, so people re-run until green"* trap #1903 exists to remove. Observed twice on this box: two assertions failing, then the identical file passing minutes later with nothing changed but the neighbours. 0.5 still leaves **2× headroom** over the ~0.23 that dropping kernel time produces. **One cell that load cannot make vacuous:** the verdict must follow the number printed on the same row. That holds at *any* starvation level, and catches a gate that always says `ok`, always says `contended`, or has its comparison backwards. ## Validation - Suite **265 → 268**, green on current `main`, and green under `taskset -c 30,31` — two cores, the shape of a GitHub runner. - The new `Host lock conformance` CI job (#1912) runs the suite on this PR: **pass in 3m17s** on a GitHub-hosted runner, the first time the suite has ever run outside the box it locks. - Both decisive mutations red, restored after each. - `shellcheck` clean. No crate code, no runtime behaviour change — `scripts/hostlock_test.sh` only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #1820 and #1885. Reported to @justinchuby first; opening rather than sitting on it because it is live on
mainand the row it corrupts is the one we are all being told to paste into published benchmark rows. Happy to close this if the author would rather carry it — it is their file and they have merged two rounds on it today.The defect
#1885 took
reasonout of--oneline, with this rationale in the source:That is exactly right, and it applies verbatim to
owner— which stayed in the row asheld_by, earlier in the row, ahead of every field a reader uses to decide whether the box is claimed.Parsed last-wins with awk — the idiom this script's own documentation recommends over the shell — that row reads back:
The row physically says
HELD/yes.#1885's own words for the fail-open gate apply here and land harder: a lock that silently degrades to no lock is worse than none, because it launders the contamination into a label. This does not degrade to
unknown. It asserts FREE while held.Three things that make this a guard rather than a doc note
The benign spelling is the more likely one and fails identically.
--owner "gaff cpu team"truncatesheld_bytogaffand injects the keyscpuandteam. That is theHL_reason=moetruncation again, in the field that was kept.A newline is worse than a space.
publish_lockwritesowner=${OWNER}into$METAline by line, andmeta_getissed -n "s/^$1=//p" | head -1— first occurrence wins. So--owner $'gaff\ntakeover=none'injects atakeoverkey that outranks the real one. That is the field added in fix(hostlock): give the reaper guard an owner, so a kill cannot wedge the box #1885 precisely so a row could not assert "no takeover" about a run that may have reaped a corpse.fix(hostlock): give the reaper guard an owner, so a kill cannot wedge the box #1885's point about staleness fields generalises one step further. Its conclusion was that the fields disclosing staleness must travel with
held_by. True — and they must also be unforgeable byheld_by. Travelling together is not enough if one of them can rewrite the others.The fix
require_name:[A-Za-z0-9_.-]+. An owner is a name; restricting it to one is not a limitation, it is what the field already meant.Validated in two places, and neither is redundant:
require_namecalls$HOSTLOCK_OWNERoneThe flag check gives a precise message at the point of the typo; the post-parse check catches the same text arriving through
$HOSTLOCK_OWNERor a$USERwith a space in it, which reach the published row by exactly the same route.Tests
Nine assertions in
hostlock_test.sh. Three assert the parsed row rather than the raw string — the defect is not that the text appears, it is that a consumer's reading of the row inverts, so asserting on the string would pass a fix that only escaped the display.One is the positive case:
--owner gaff-cpu.2is still accepted, its row parses back to exactly oneheld_by, andhostlock_stateparses to the value the row physically carries. This is a validation, not a lockout.The
every assertion in this file ranpin goes 249 → 258, and I verified it by running the suite rather than by arithmetic — a pin bumped to make the number match is the defect it exists to catch. 258 passed, 0 failed.bash -nclean on both files. That pin is also what made me count what I had added instead of assuming; it is a good mechanism.Note for reviewers
held_uid(from/proc/<pid>/status) is the only field in the row a peer cannot forge, and after this changeheld_byis constrained but still self-declared. Worth a header line saying which is corroborating and which is claimed, since a reader will otherwise trust the friendlier-looking name. Not done here — it is prose in someone else's file and this PR is already touching their two most recently merged areas.#1869 is also open against
scripts/hostlock.sh. This change is confined to a new helper plus two call sites and does not touchrun/--ttl, so it should merge either order, but flagging it.Auto-merge armed, no
--admin; waiting on required CI.