Repository navigation
ci(hostlock): run the lock's conformance suite, which nothing ran - #1912
Merged
Merged
Conversation
`scripts/hostlock_test.sh` has 252 assertions covering mutual exclusion, atomic publish, stale-holder reaping, fail-closed admission and the CPU-efficiency verdict. Nothing in CI ran it. It ran by hand, by me, on the box it locks -- which is the same "assigned, never executed" shape the suite exists to prevent, one level up. A suite nobody runs is green because it is stale, not because the tool is correct. It has earned the job. Three of the defects it caught were introduced by the fix for the previous one: an acquire that mkdir'ed and then wrote its metadata handed the host to three simultaneous winners; the reaper's own mutex could be orphaned by a SIGKILL between mkdir and rmdir, after which no stale lock could ever be reaped again; and `reaper_release` compared only `$$`, so on a box cycling ~1.5M pids in four days it could delete a live successor's guard. Path-filtered to `scripts/hostlock*.sh` and this file, on PRs and pushes to main. The suite spends about ten core-seconds on real load -- six spinners for the occupancy reading, three ~1.5s single-cpu cells for the efficiency verdict -- which is cheap but not free and is irrelevant to a PR that does not touch the lock. Deliberately NOT added to the required set: the required checks are `Fast (Linux x86_64)` and `Rust quality`, and putting a shell job on every PR's critical path buys nothing for the PRs that never touch these two files. Sized from a measurement rather than a guess. Under `taskset -c 30,31` -- two cores, the shape of a GitHub-hosted runner -- the suite passes 252/252 in 210s while consuming 53% of a single core averaged over that window, i.e. it is mostly bounded waits. timeout-minutes: 20 is ~5x that, enough for a slower runner and far short of the six-hour default a hung wait would otherwise sit for. That two-core run is also the evidence that this is safe to run on shared CI infrastructure at all: under #1802 every cell that needs to know what a run achieved measures the host and derives its threshold instead of asserting a number only an idle box can produce. Linux-only, matching the tool: hostlock.sh reads /proc/<pid>/stat for pid start times, relies on `mv -T` for its atomic publish, and uses `stat -c`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
enabled auto-merge (squash)
August 24, 2026 00:43
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1912 +/- ##
==========================================
- Coverage 80.37% 80.34% -0.03%
==========================================
Files 401 415 +14
Lines 195530 204600 +9070
Branches 195530 204600 +9070
==========================================
+ Hits 157150 164385 +7235
- Misses 32992 34638 +1646
- Partials 5388 5577 +189
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
…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
…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
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.
Nothing ran the conformance suite
scripts/hostlock_test.shis 252 assertions covering mutual exclusion, atomic publish, stale-holder reaping, fail-closed admission and the CPU-efficiency verdict of the advisory benchmark-host lock (#1803, #1806). No CI job ran it. It ran by hand, by me, on the box it locks — which is the same assigned, never executed shape the suite exists to prevent, one level up. A suite nobody runs is green because it is stale, not because the tool is correct.It has earned the job. Three of the defects it caught were introduced by the fix for the previous one:
mkdired and then wrote its metadata handed the host to three simultaneous winners — a competitor reading in the window sees a lock with no anchor pid and reaps it as stale;SIGKILLbetweenmkdirandrmdir, after which no stale lock could ever be reaped again;reaper_releasecompared only$$, so on a box cycling ~1.5M pids in four days it could delete a live successor's guard.Scope and cost, both deliberate
Path-filtered to
scripts/hostlock*.shand this workflow file, on PRs and pushes tomain. The suite spends about ten core-seconds on real load — six spinners for the occupancy reading, three ~1.5 s single-cpu cells for the efficiency verdict — which is cheap but not free, and irrelevant to a PR that does not touch the lock.Deliberately not a required check. The required set is
Fast (Linux x86_64)andRust quality; putting a shell job on every PR's critical path buys nothing for the PRs that never touch these two files.The timeout is measured, not guessed
Under
taskset -c 30,31— two cores, the shape of a GitHub-hosted runner:210 s wall at 53% of a single core, i.e. the suite is mostly bounded waits rather than work.
timeout-minutes: 20is ~5× that — room for a slower runner, and far short of the six-hour default a genuinely hung wait would otherwise occupy.That two-core run is also the evidence that this is safe on shared CI infrastructure at all: under #1802 every cell that needs to know what a run achieved measures the host and derives its threshold, instead of asserting a number only an idle box can produce.
Linux-only, matching the tool —
hostlock.shreads/proc/<pid>/statfor pid start times, relies onmv -Tfor its atomic publish, and usesstat -c; its PORTABILITY block says so.Validation
shellcheckclean, suite 252/252 green on the current branch and 252/252 under the two-core constraint).