Repository navigation
fix(ep-cpu): stop the pool counters building the pool they report - #2125
Merged
Merged
Conversation
`testing::counters()` reached the pool through `global_pool()`, which
constructs it. So the act of observing spawned `width - 1` worker threads,
and any measurement taken around the snapshot included workers the
measurement itself created.
This was not hypothetical. Attributing thread creation by lifecycle phase
in the gap harness on an fp32 model -- which fans out on Rayon and never
touches this pool -- showed:
phase[native-session-load]: +32 -0 threads (33 live)
phase[timed-loop]: +15 -0 threads (49 live) -> 15x nxrt-task-*
The 15 `nxrt-task` workers appear in the *timed loop*, of a model whose own
report says `0.00 dispatches/iter`. They were built by the harness's
"before" snapshot, then showed up in the steady window as parks. Confirmed
directly: `counters()` in a virgin process takes the thread count from 2 to
17.
Zeroes are the honest answer for a pool that does not exist, and no
caller's delta changes: if the pool is built between two snapshots, the
"before" that is now missed was zero anyway.
The test runs in a child process, because the property is only observable
where nothing has built the pool yet -- in the shared test binary an
earlier test may have built it, and the assertion would then hold
vacuously against the very defect it exists to catch. It also asserts its
own observable is live: after the checks it builds the pool and requires
the thread count to rise, so the test cannot pass by measuring a
thread-count that never moves. Mutation-proved by restoring the old body:
`observing the counters spawned 15 thread(s)`.
Refs #2075
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2125 +/- ##
==========================================
+ Coverage 80.51% 80.93% +0.42%
==========================================
Files 413 428 +15
Lines 195487 210505 +15018
Branches 195487 210505 +15018
==========================================
+ Hits 157388 170374 +12986
- Misses 32569 34388 +1819
- Partials 5530 5743 +213
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This was referenced Aug 25, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 25, 2026
…readers (#2142) (#2147) Closes #2142. ## The defect `COUNTERS_OBSERVER_CHILD_ENV` (`task_runtime/mod.rs`) is declared **ungated** while both of its readers are `#[cfg(target_os = "linux")]`. Off-Linux the constant is dead code, and those lanes build with `-D warnings`, so it is a hard build failure: ``` error: constant `COUNTERS_OBSERVER_CHILD_ENV` is never used --> crates\onnx-runtime-ep-cpu\src\task_runtime\mod.rs:932:11 = note: `-D dead-code` implied by `-D warnings` error: could not compile `onnx-runtime-ep-cpu` (lib test) due to 1 previous error ``` Introduced by #2125 (`85565fc5b`). The fix gives the constant the same cfg predicate as its two readers, so all three appear and disappear together. ## How I found it, and why it is not the PR that surfaced it It reddened `Rust (Windows ARM64)` on my #2098. The timing discriminates cleanly — that lane on **the same PR branch** was green twice before #2125 merged and red after, with no Rust in the diff at any point: | lane run | started | vs #2125 (merged 17:13:15Z) | result | |---|---|---|---| | #2098 @ `e93532ae0` | 11:42:56Z | before | **success** | | #2098 @ `d99c48c13` | 13:11:41Z | before | **success** | | #2098 @ `45530133f` | 19:17:16Z | after | **failure** | CI builds the *merge result*, so a PR lane can be red for a defect that is entirely `main`'s. The colour moved because `main` moved. ## Verification I could not check the real target locally — `cargo check --target aarch64-pc-windows-msvc` dies in `onnx-genai-ort-sys`'s bindgen step (`fatal error: 'stdlib.h' file not found`), needing a Windows SDK. **That failure says nothing about this change**, and I am recording it rather than quietly reporting the exit code, because a cross-target check that fails for toolchain reasons is the mirror image of the trap @Gaff pinned on `check_cross_compile.sh`: one direction false-passes without a toolchain, the other false-fails. So I proved the mechanism natively instead, by making the *readers* off-target on Linux — which is exactly the shape Windows sees — and varying only the constant's gate: | arm | const | readers | rc | `is never used` | expected | |---|---|---|---|---|---| | **A** pre-fix state | ungated | absent | 101 | **yes** | yes ✓ | | **B** with this fix | gated | absent | 0 | no | no ✓ | | **C** real tree on Linux | gated | present | 0 | no | no ✓ | Arm A reproduces CI's exact error text, so B is not a pass by compiling nothing — the control is non-vacuous. Arm C shows the Linux behaviour is unchanged: the test and its child still compile and are still gated exactly as before. **No test is disabled by this change**; the constant is simply present on precisely the targets that read it. Required-lane commands, run as spelled: ``` cargo fmt --all --check -> 0 cargo clippy -p onnx-runtime-ep-cpu --all-targets --locked -- -D warnings -> 0 ``` (Read via `${PIPESTATUS[0]}`, not the pipeline's status.) ## The part worth keeping Both affected lanes are **advisory**. The required set is `Fast (Linux x86_64)` + `Rust quality`, and both are Linux — so **a Linux-only cfg mistake is structurally invisible to the gate that guards merges**. #2125 merged green and was genuinely green on everything required. That is the same tier gap as #1915 (Miri red, not required), and it is a different problem from a required check being red and merged anyway. Recorded on the audit ledger in #2056 as such rather than as a bypass. *No admin bypass; normal auto-merge, waiting on required CI.* Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 26, 2026
…it (#2121) Five `cargo test` steps in `ci.yml` — including the one in the **required** `Fast (Linux x86_64)` lane — derived their package set from an unguarded command substitution: ```yaml cargo test --locked $(python .github/scripts/workspace_test_packages.py cargo-args offline-linux) ``` If that substitution ever produces nothing, `cargo test` still runs — as a **bare** `cargo test` — and the step exits 0. ## Measured shell semantics Under GitHub's default step shell (`bash -e {0}`), not inferred: | form | resolver fails | step exit | cargo runs | |---|---|---|---| | `cargo test $(resolver …)` | exit 2, empty stdout | **0** | **yes, bare** | | `cargo test $(resolver …)` | exit 0, empty stdout | **0** | **yes, bare** | | `packages="$(resolver …)"` | exit 2 | 2 (`set -e` aborts) | no | | `packages="$(resolver …)"; test -n "$packages"` | exit 0, empty | 1 | no | Assignment is the load-bearing half: `set -e` **does** abort on a failing substitution in an assignment and **does not** abort on one inside an argument list. The `test -n` guard covers the remaining case where the resolver succeeds but prints nothing. ## What the degraded run actually tests A bare `cargo test` falls back to `default-members` (46 crates), not the lane set (49): - **14 crates silently dropped**, including the entire plugin/ABI/FFI surface. - **`onnx-genai-ort-sys` pulled in** — explicitly denylisted because its build script downloads a native ONNX Runtime — plus both PyO3 crates. **Severity.** The harm is *invariant in the outcome colour*: a **required** check silently stops exercising the 14-crate plugin/ABI surface. Whether the degraded run then goes red is contingent on network and environment — the denylisted `ort-sys` crate has a strong offline failure vector, so a **red lane with a misattributed cause** is likelier than a silent green — but what the lane *covers* is not contingent on anything. The defect is that the lane stops testing what it claims to test while its exit code stays authoritative, in either colour. (Framing corrected by the independent review; I had originally anchored on the red/green coin-flip, which is the symptom, not the defect.) ## The fix Each of the five sites becomes: ```yaml packages="$(python .github/scripts/workspace_test_packages.py cargo-args <lane>)" test -n "$packages" || { echo "::error::lane '<lane>' resolved to an empty package set"; exit 1; } cargo test --locked $packages ``` Sites: `fast-linux` (`offline-linux`, **required**), `rust-coverage` ×2, `rust-windows-arm64`, `cli-ort`. A rationale comment is added at the required site only. ## Mutation battery — 6 arms, effective Each arm names what it expects and asserts whether the *test command* was reached, discriminated by argv: | arm | exit | test cmd ran | packages | |---|---|---|---| | new_healthy | 0 | yes | 49 | | new_badlane | 2 | **no** | 0 | | new_emptyres | 1 | **no** | 0 | | old_healthy | 0 | yes | 49 | | old_badlane | **0** | **yes** | **0** ← defect | | old_emptyres | **0** | **yes** | **0** ← defect | Both defect arms fail on the old form and pass on the new one, so the guard is what changes the verdict. **Instrument bug found and fixed mid-battery, disclosed because it invalidated a first run:** my stub `cargo` also shadowed the resolver's *own* `cargo metadata` call, crashing the resolver and causing the recorder to log the resolver's invocation instead of the lane's. Every arm was contaminated — `new_healthy` reported a nonsensical `exit=1 pkgs=0`. Fixed by passing `metadata` through to the real cargo and discriminating recorded commands by argv. Same species as the errors this repo's measurement-discipline skill catalogues: the instrument resolved against itself instead of against the subject. ## Per-site verification All five shipped steps executed with a stub recorder to confirm the real argv: | site | lane | packages | exit | |---|---|---|---| | fast-linux (required) | offline-linux | 49 | 0 | | rust-coverage | offline-cross-platform | 48 | 0 | | rust-coverage (linux-only) | linux-only | 1 | 0 | | rust-windows-arm64 | offline-cross-platform | 48 | 0 | | cli-ort | ort-backed | 6 | 0 | `cli-ort`'s trailing `-- --test-threads=1` is preserved. YAML parses; 9 jobs; the `Fast (Linux x86_64)` and `Rust quality` check names are unchanged, so the ruleset's required checks still resolve. ## Related blind spot (not fixed here) `workspace_test_packages.py verify`, which runs in the required `rust-quality` lane, would catch a *crashed* resolver — but it **takes no lane argument and never reads `ci.yml`**, so a lane-name mismatch between the workflow and the script stays invisible while `verify` prints `coverage ok`. It also treats advisory and required lanes as equivalent evidence: it proves a crate is a member of *some* lane, not that any *enforced* lane runs it. That is the same cardinality-vs-identity trap as §9 of the measurement-discipline skill, and it is why the "no required lane runs these six crates' tests" gap survived. Left for a separate change, since making `verify` strict today would red `main` and the ruleset half is Justin's call. Base merged with current `main` (`0be2d23fe`) and re-validated before arming. Working as Holden (Security Engineer). --- ## Correction: the first push broke the three Windows legs The guard as first written was bash-only, and **four of the five sites also run on Windows**, where the default step shell is **pwsh**. CI failed on exactly `Rust coverage (Windows x86_64)`, `Rust (Windows ARM64)` and `CLI ORT (Windows x86_64)` — no other lane. A failure set that is exactly the Windows legs and nothing else is the signature of a shell-portability defect, and that is what it was. **Root cause, reproduced directly under pwsh** with `$ErrorActionPreference = 'stop'` and a *native* fake cargo: | form | shell | exit | test cmd | argv | |---|---|---|---|---| | inline `$( )` | pwsh | 0 | ran | `argc=14` — correct 6-crate set | | guarded assignment | pwsh | 1 | **never ran** | — | | guarded assignment | `bash --noprofile --norc -eo pipefail` | 0 | ran | 98 tokens — correct 49-crate set | **Why the original was portable and I mistook that for it being bash.** The resolver prints **one token per line**. bash word-splits that; pwsh returns it as an array and splats it element-wise into a *native* command. So `cargo test --locked $(resolver …)` happened to deliver the right argv under both shells. That portability was an accident of the output format, never a contract — and it is precisely what made the inline form look shell-agnostic while its failure mode was not. **Fix:** `shell: bash` on all five steps. `shell: bash` already appears in every affected job — including `rust-windows-arm64` itself, for "Install the pinned Rust toolchain" and "Resolve cargo cache target" — which is in-repo evidence that bash is present on `windows-11-arm`, rather than an assumption about the runner image. ### Instrument note My first pwsh reproduction used a **pwsh function** as the fake cargo and reported `argc=3`. That number is wrong for a 6-crate lane, and the discrepancy is the tell: pwsh passes an array to a *function* as a single object and splats it only for *native* commands. Re-running against a real executable gave `argc=14`. Had I not checked the number against what the lane should contain, I would have reported that pwsh mangles the argument list — a real-sounding conclusion about the wrong subject. That is the same failure as the `cargo metadata` contamination disclosed above, and the same one as the original defect: **the instrument resolved against itself instead of against the subject.** I measured `bash -e {0}` semantics, concluded "this is how the step behaves", and shipped it to four steps that were not running bash. The measurement was correct; its scope was not. A limitation is scoped to the thing that carries it. --- ## Second correction: CRLF, found only once the Windows lanes ran bash `shell: bash` landed correctly — the ARM64 log confirms `C:\Program Files\Git\bin\bash.EXE --noprofile --norc -e -o pipefail {0}` — and then the step failed on something the pwsh path had been hiding: ``` error: unexpected argument 'onnx-genai-cuda-version-guard^M ' found ``` **Windows Python writes stdout in text mode and translates `\n` into `\r\n`.** bash splits command substitution on `IFS`, which contains space, tab and newline but **not** carriage return — so every token kept a trailing `\r` and cargo saw it as part of the package name. PowerShell strips CRLF when it splits native output into an array, so this was invisible for exactly as long as the Windows lanes ran pwsh. So the inline form was portable for **two** independent accidents, not one: one-token-per-line output (which `package_args` documents), *and* pwsh's CRLF stripping (which nothing documented, because nothing depended on it). **Fixed at the producer**, not per-step, so a future call site cannot reintroduce it: the resolver now forces LF separators for the `cargo-args` path. Verified by emulating Windows text-mode stdout on Linux — a falsifier that fails without the change and passes with it: | producer | resolver bytes | bash tokens | argc | |---|---|---|---| | without fix | `-p\r\nmlas-sys\r\n` | `-p\r`, `mlas-sys\r` | 4 | | with fix | `-p\nmlas-sys\n` | `-p`, `mlas-sys` | 4 | **`argc` is 4 either way.** An argument-count assertion — the obvious way to check "did the right package set reach cargo" — passes under the defect. Only token *identity* separates them. That is the same cardinality-vs-identity trap as the `verify` blind spot described above, arriving a third time in the same change: `verify` counts crates and cannot see which lane; my first pwsh model counted `argc=3` and could not see it was measuring a function instead of a native command; and here the count is invariant across the bug. `verify` still reports `coverage ok: 55 tested, 5 denied`, and both lanes still resolve to 98 and 12 tokens. --- ## Rebased onto #2023, which changed what this PR has to satisfy `main` landed **#2023** ("run the ORT-backed crates' tests in a required lane") while this was in review. It closes the six-crate gap described above and adds two subcommands — `self-test` and `verify-required-tier`. Both conflicts were co-located edits where each side is wanted, and there is a **sixth** call site now, in `Rust quality`, which makes it the most load-bearing of the set. It is converted too. ### `verify-required-tier` refused the guarded form — correctly Its `packages_tested_by` credits a lane only when the `cargo-args` call and the `cargo test` occupy the **same fragment**. The guard puts them on separate lines, so nothing was attributed and the gate reported that no required check runs any crate's tests — all 55. That is the gate behaving as designed, not a bug in it. Its docstring anticipates exactly this class: > joining continuations moves attribution in the permissive direction, and this gate's whole thesis is that an over-read is fatal while an under-read merely fails loudly … the fix is to put the invocation on one line. Its prescribed remedy is precisely the thing this PR exists to undo, so the gate is **extended** rather than worked around — and extended in the direction its own thesis allows. ### The extension is a dataflow read, not a join A lane resolved into a shell variable is credited to a later `cargo test` **only if that command references the variable**. The reference is what does the crediting. Reassignment *replaces* a variable's lanes rather than adding to them, so shadowing the name with a non-lane value drops the credit rather than keeping it. Two self-test arms hold it there, and both are mutation-effective: | mutation | arm that fails | |---|---| | credit every assignment regardless of reference (**permissive**) | `packages_tested_by refuses a lane the cargo call never uses` | | remove the variable read (back to pre-extension) | `packages_tested_by credits a lane the cargo call uses` + `verify-required-tier (control)` | The permissive mutation is the one that matters: it is the failure mode the gate's author warned about, and the arm exists specifically to catch it rather than to confirm the happy path. ### Post-merge state ``` verify coverage ok: 55 tested, 5 denied verify-required-tier exit 0 -- 55 package(s) tested by ['Fast (Linux x86_64)', 'Rust quality'] windows ORT coverage ok: 6 package(s) via ['CLI ORT (...)'] self-test 37/37 arms behaved as stated (35 before, +2 mine) ``` Six sites guarded, zero inline substitutions remain, and the three-dot diff touches exactly two files. --- ## Fourth correction: the guard tripped a *different* gate, and that gate was right to be suspicious `Rust quality` — a **required** lane — went red on: ``` FAIL no cargo test step in ci.yml carries an unguarded name filter ``` The gate is `scripts/test_step_test.sh`, and it guards the same bug class this PR does: a bare word after `cargo test` is a **name filter**, and libtest reports `ok` / exit 0 for a filter that matches nothing. So an accidental positional silently converts "run the suite" into "run nothing, successfully" — the identical fail-open shape, one layer down. It flagged my steps because its scanner joins every line of a `run:` block with a space and reads the result as one argv. That premise held while every `cargo test` step in this file was a single line. **My guard makes three of them scripts**, so the guard's own words — `echo`, `::error::...`, `exit`, `1` — landed in the same argv as `cargo test` and were read as selectors. Worth being clear about the direction of the error: the gate produced a **false positive on a correct step**, not a false negative on a broken one. For a ban that is the right way to fail, and it is the same argument @Gaff made against word-boundarying `shape_dispatch_gate` — do not loosen a ban to accommodate a benign case if loosening it could also admit a real one. So I did not add an exemption for my steps. ### What I changed instead — the premise, in two places **1. Statement boundaries survive the accumulator.** The scanner discarded newlines, which is exactly the information needed to tell a guard *beside* the cargo call from arguments *to* it. Which style a block uses decides it: `run: |` keeps newlines as separators, `run: >-` folds them into spaces. The accumulator now records the block scalar style, splits on `;`, `&&`, `||` and real newlines, and scans only the fragment containing `cargo test`. **2. A variable is blanked only when it is a generator.** `$(...)` was already blanked as opaque, with the existing dynamic cell closing that hole by *executing* every generator and proving it emits options only. My form moves the substitution into a variable, so the variable needs the same treatment — but only when it was assigned from `$(...)`. `packages="some::filter::"` followed by `cargo test $packages` is still caught. That second point is where a widened ban usually acquires a quiet hole, so it is where I put the falsifiers. ### Falsifiers — each names the assertion it requires to be red Three steps added to the detector's own mutant fixture: a guarded step that must **not** be flagged, a guarded step with a trailing `kernels::something::` that **must** be, and `packages="kernels::something::"` (assignment that is *not* a generator) that must be. | mutation | want | got | assertion that failed | |---|---|---|---| | control | PASS | PASS | — | | record vars from **any** assignment, not only `$(...)` | FAIL | FAIL | `MUTATION: the detector finds an unguarded filtered step` | | read a literal block as folded (drop style detection) | FAIL | FAIL | same, + the real-`ci.yml` cell | | drop the statement split | FAIL | FAIL | same, + the real-`ci.yml` cell | | stop blanking generator vars (over-strict) | FAIL | FAIL | same, + the real-`ci.yml` cell | An earlier revision of the fixture passed all four mutations **for the wrong reason** — a leftover string literal from the blanked assignment happened to remain in the same fragment, so the arm went red on debris rather than on the filter. Only the real-`ci.yml` cell was actually carrying the claim. That is @Gaff's finding from #1995 arriving in my own battery: *a red-expecting check must name what it expects to be red*, and I would add — it must be red for the stated reason, which you only learn by reading which assertion failed rather than the exit code. Fixing the accumulator fixed the fixture too. `33/33` assertions pass. `shellcheck` is clean apart from the `SC1091` already present on `main` (2 occurrences on both sides). ## Blocked on a break that is not mine — #2142 `Rust (Windows ARM64)` and `Rust coverage (macOS arm64)` are red on `main` independently of this PR: `COUNTERS_OBSERVER_CHILD_ENV` is declared ungated but consumed only under `#[cfg(target_os = "linux")]`, so it is dead code off-Linux and these lanes build with `-D warnings`. From #2125 (`85565fc5b`). Filed as **#2142** with a two-arm reproduction; not fixing it here, as it is @sebastian's area and unrelated to this diff. Neither lane is required, but I am not merging while they are red. Worth naming why it survived: the required set is exactly `Fast (Linux x86_64)` + `Rust quality` and **both are Linux**, so a Linux-only `cfg` mistake is structurally invisible to required CI — the same tier gap @Gaff documented, reached from a different direction. --- ## Merged `main` again, and it had added three more of these `main` moved 13 commits, one of them **#2098** ("derive the clippy package lists so every tested crate is linted", closing #2058). It touches all three of my files, and git merged it without a conflict — which is worth saying out loud, because a clean textual merge is not a correct one. It added three new invocations in exactly the form this PR exists to remove: ```yaml - name: Clippy Linux offline crates # fast-linux (REQUIRED) run: >- cargo clippy --locked --all-targets $(python .github/scripts/workspace_test_packages.py cargo-args lint) -- -D warnings ``` plus `Run clippy on all offline crates` (`rust-quality`, **required**) and `Clippy Windows ARM64 offline crates`. The consequence is sharper here than for the test lanes. #2058 was *"21 crates are compiled and tested by CI and linted by nothing"*. If this substitution degrades, clippy lints the default members instead of the lane and exits 0 — so #2058 is silently re-opened **in the lane whose only purpose is to prove it stayed closed**. Two of the three are required. All three are now bound and guarded, matching the six `cargo test` sites. Healthy-path argv is byte-identical: | site | old vs new | argc | crates | |---|---|---|---| | `lint` (×2 sites) | **identical** | 116 | 55 | | ARM64 `offline-linux` | **identical** | 106 | 49 | | bad lane | — | exit **2**, clippy never invoked | — | `ci.yml` is now **9 guards, 0 unbound substitutions**, and the 9 job `name:` values are byte-identical to `main`. ### The same trap, third time, and it fails in the dangerous direction Binding the generator moves the lane name off the `cargo clippy` line, so `linted_packages` stopped seeing it — exactly what `verify-required-tier` did to me earlier in this PR. But the direction differs and that matters: a package then looks **unlinted**, `verify` goes red, and the obvious repair is to loosen the scanner. That would be the wrong repair for the same reason @Gaff gave for `shape_dispatch_gate`. `clippy_blocks` now reads backwards for the assignment, bounded by indentation so it cannot walk out of the step, and credits it **only when the invocation actually references the variable**. Crediting every assignment in scope would be a join rather than a dataflow read, and that scanner's docstring names over-reading as its whole risk. | mutation | arm that fails, by name | `verify` | |---|---|---| | credit every preceding assignment (**permissive**) | `linted_packages refuses a lane the clippy call never references` | **exit 0** | | remove the backwards read | `linted_packages credits a lane the clippy call reads from a variable` + `verify (control)` | exit 1 | The permissive row is the load-bearing one, and it makes the case for these arms better than I could: **the over-read is invisible to `verify`.** Production stays green, every package still looks linted, and nothing anywhere says otherwise. The new arm is the only thing standing on that edge. ### Local state ``` verify 55 tested, all linted by 10 clippy invocation(s) verify-required-tier exit 0 self-test 48/48 arms behaved as stated test_step_test.sh 36/36 assertions passed shellcheck clean apart from the SC1091 already on main ci.yml 9 guards / 0 unbound substitutions / job names identical to main ``` ## Advisory lanes red on `main`, both filed, neither mine | lane | cause | issue | |---|---|---| | `Rust (Windows ARM64)`, `Rust coverage (macOS arm64)` | `COUNTERS_OBSERVER_CHILD_ENV` ungated but consumed only under `cfg(linux)` — dead code off-Linux, `-D warnings` | **#2142** | | `CUDA compile (Linux x86_64)` | `capture_sync_contract`: NMS (#2130) and Unique (#2113) added unguarded `.synchronize()` without the reviewed allowlist entry | **#2149** | For the CUDA one the attribution is exact rather than argued: **#2130 is not in my branch at all** — it reaches CI only through the merge ref — and I reproduce the failure locally from the Unique half alone. My diff contains zero Rust files. Three advisory lanes red on `main` from three unrelated commits in one day is itself the finding. Required is `Fast (Linux x86_64)` + `Rust quality`, and **both are Linux**, so a Linux-only `cfg` mistake is not merely under-covered — it is structurally invisible to the required set, in every crate. --- ## Second independent review, and the six findings it returned The diff grew after the first review (three clippy sites from #2098, a second scanner repair), so I asked for a fresh independent Opus review of the whole thing rather than of the delta. Verdict **APPROVE**, with six non-blocking findings. All six are now fixed, and each is falsified by a named mutation arm rather than by inspection. | # | finding | repair | arm | |---|---|---|---| | a | `_generator_vars_before` could walk past a step boundary and credit the *previous* step's deeper-indented body | return `{}` when the invocation line is itself a list item or carries `run:` | `B1` | | b | `$packages` matched `$packages_extra` — a prefix collision granting credit to the wrong variable | match variable references whole | `E1` | | c | a plain assignment shadowing a generator assignment left the stale credit in place | `_ANY_ASSIGN` clears the credit | `E2` | | d | blanking a substitution to `" "` let a preceding value-taking option swallow the next word, so `--features $(...) my::filter::` lost its filter entirely | emit an `@@sub@@` sentinel on **both** the inline `$( )` and `$var` paths | `D1`, `D1b` | | e | `llvm-cov` and `+toolchain` invocations were not scanned | first-token rule, so `Install cargo-llvm-cov` steps are examined and correctly *not* flagged | `D2` | | f | a scanner that reads nothing reports a clean pass | assert the scan reached `ci.yml` and names the required lane's own test step | `F1` | ### The (d) fixture is the finding worth recording My first version of the (d) fixture exercised the `$var` path. So the arm that mutates the **inline `$( )`** path — revert `@@sub@@` back to `" "` — came back: ``` D1 expect FAIL, got PASS *** DID NOT BITE *** ``` Right colour on every other arm, and this one silently inert: the fixture never reached the code the mutation changed. The generalisation is the mirror of Gaff's rule about naming what you expect to be red: > **A mutation that does not bite is a statement about your fixture, not about your code.** It does not mean the path is safe; it means nothing you wrote goes there. That is a third distinct way a battery lies, alongside the two already on the board — Gaff's zero-selection filter, and Gaff's 18-tests-none-of-them-the-right-ones. All three produce a number that looks like a measurement. With the inline fixture added, both arms bite: ``` D1 expect FAIL, got FAIL failing: MUTATION: the detector finds an unguarded filtered step D1b expect FAIL, got FAIL failing: MUTATION: the detector finds an unguarded filtered step F1 expect FAIL, got FAIL failing: ...and the scan actually reached ci.yml (not a pass by reading nothing) ``` ### Direction-of-error, restated for these two scanners They fail in opposite directions and the asymmetry drove every one of the six repairs: - `test_step_test.sh` enforces a **ban**. A false positive fails loudly on a benign name. That is the correct direction and I did not loosen it — the same argument Gaff made against word-boundarying `shape_dispatch_gate`. - `packages_tested_by` grants **coverage credit**. Here an over-read is *fatal and silent*: it says a package is linted when nothing lints it. So it credits only variables it can prove are referenced, and stops at blank lines, at lesser indent, and at a step boundary. Findings (a), (b) and (c) are all the same species — an over-read on the credit-granting side — which is why they are the ones I would have shipped without noticing. ### Local state, re-measured on the current base ``` verify exit 0 verify-required-tier exit 0 self-test 51/51 arms behaved as stated test_step_test.sh 38/38 assertions passed ci.yml 9 guards / 9 assignments / 0 unbound substitutions / 9 jobs job names byte-identical to main shellcheck clean apart from the SC1091 already on main ``` `main` was 8 commits ahead when I finished, one of them touching the crates this branch's lanes build. Because `strict_required_status_checks_policy = false`, CI would otherwise have graded me on the stale base — so I merged `main`, re-ran everything above, and confirmed the merge touched **none** of `ci.yml`, `workspace_test_packages.py` or `test_step_test.sh`, so the mutation battery transfers unchanged. **#2142 is fixed on `main`** by #2147, which clears the `Rust (Windows ARM64)` and `Rust coverage (macOS arm64)` reds listed above. **#2149 remains open** and is not mine; my diff still contains zero Rust files. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Holden <holden@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.
testing::counters()reached the pool throughglobal_pool(), which constructs it. So observing the pool spawnedwidth - 1worker threads, and every measurement taken around that snapshot included workers the measurement itself had created.How it surfaced
I was chasing the long-standing "~2x the thread budget, mostly unnamed" question with the gap harness's per-phase thread attribution, on an fp32 model:
Fifteen
nxrt-taskworkers created in the timed loop, in a run whose own output says0.00 dispatches/iterand whose route line saysMatMulF32 -> Native— fp32 MatMul fans out on Rayon and never touches this pool.My first reading was that the EP builds two pools. It does not. The harness built the second one, by taking a "before" snapshot. Confirmed directly rather than inferred — in a virgin process,
counters()alone takes the thread count from 2 to 17.So the honest version of the thread census for that model is 32 Rayon workers, not 47, and the
parksthe harness attributed to the steady window were workers that existed only because the harness asked a question.The fix
counters()now reads through a non-constructing accessor and returns zeroes when no kernel has built the pool. No caller's delta changes: if the pool is built between two snapshots, the "before" that is now missed was zero anyway.pool_width()is deliberately left constructing — its callers are asking how wide a fan-out would be, which is a question you cannot answer without the pool, and they are asking it in order to use it.Test
Runs in a child process, because the property is only observable where nothing has built the pool yet. In the shared test binary any earlier test may have built it, and the assertion would then hold vacuously — passing just as happily against the defect it exists to catch. A child running the one test
--exactis virgin by construction. Same pattern asa_failed_build_leaves_no_workers_runningindecode_spmd.rs.It also asserts its own observable is live: after the checks it builds the pool and requires the thread count to rise. Without that, a thread-count that never moved for any reason would make the test pass while measuring nothing.
Mutation-proved by restoring the old body:
Scope
Independent of #2117 — different file, no textual overlap — but the same investigation. #2117 adds the
spin_yieldscounter; this makes reading any counter safe.One consequence worth stating: the thread and RSS figures the gap harness has reported so far are inflated by this, on any route that does not dispatch to the native pool. The yield-rate numbers on #2075 are not affected — those come from the softmax fixture, which does dispatch (8/iter), so its pool was real and would have existed regardless.
Validation: ep-cpu 1815/0 default, 1841/0
--features mlas, clippy-D warningsclean, fmt clean.Refs #2075