From cb45ed62323c890a7a14189812e9d887d4a685f0 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Thu, 17 Sep 2026 03:40:51 -0400 Subject: [PATCH 01/11] feat(platform-probes): add the placement probes, and derive both cost renderings from one source Peeled from `mikegrier/deferred-namespace-ops`, which carries 382 commits main does not. This is the whole of what that branch still changes in `windows-platform-probes`, taken as current state rather than replayed: the history is tangled with workspace-wide refactors that already landed, so the commits do not separate. **Two new probes, both thin renderers over library code already on main.** `probe-core-affinity` measures what placement costs and refuted the hypothesis it was written to test; `probe-peer-index-cache` records a result that inverts by host, which is why it is kept rather than discarded. The measurement itself lives in `windows-placement-probe`, which is byte-identical between the two branches -- these add the `probe-*` tier entries over it. **Both cost probes now derive their prose and their NDJSON from one source.** `a_cost_probe_tells_both_readers_the_same_thing` is the test; the mechanism is a label-to-field relation in `doorbell_cost.rs` and `request_cost.rs` that makes a disagreement between the two renderings *unrepresentable* rather than detectable. The arrangement it replaces had already gone wrong twice -- the prose iterated the measured timings while the NDJSON named each field by hand, which is how `"efficiency_classes":1` came to sit beside a prose `[0]`. It panics on a label it does not know, deliberately: adding a timing without naming it fails at the render rather than silently omitting it from the line a fleet survey would read. **Scope decisions, each made against something checkable rather than by preference:** - The crate builds and tests green against main on its own -- verified in a scratch worktree at `origin/main` with only this crate applied, before any of it was committed. No other crate is required. - `.github/workflows/ci.yml` **is** taken. It was left out of the first cut, and that made `M2.7` in `COMPLETED-CHECKLIST.md` false: the archived item says the probes run in CI behind a build gate, and nothing on main ran them. Taking the workflow is what makes the archive's claim true rather than aspirational -- twelve probe steps, each gated on `steps.build.outcome == 'success'`. - `crates/windows-topology-sys/src/granularity.rs` carries a one-line doc-link change on the source branch. It is **not** taken: the link points into `topology-planner`, which exists there as three documents and is not a workspace member, so taking it would either dangle or drag that in. - `COMPLETED-PLANS.md` is **not** taken. It declares this crate's `CHECKLIST.md` completed on 2026-09-09, and main's has twelve open items -- the two lineages diverged, and importing it would assert something false about main. - `design-sessions/DESIGN-SESSION-2026-09-02-cache-locality-model.md` is **not** taken. It reads as context for this work, but it references `topology-planner` and a branch-only root checklist, so it is not free either. Two dead links in `COMPLETED-CHECKLIST.md` are repaired in passing: both pointed at `src/claim_layout.rs`, a file the same archive records `CW-1.6` deleting. They were already broken on the source branch. Now inline code rather than links, which changes no claim the archive makes. **A review of this cut found defects in it, fixed here.** Three were in the code and two of those were observed misfiring on this host: `probe-core-affinity` printed `VERDICT FLIPS` when both placements agreed, because it compared magnitudes without checking the sign, and its distance table never reached its own single-pair guard, because it counted `a->b` and `b->a` as two pairs. `probe-peer-index-cache` attributed a win to batch depth on a run whose batch was 1.4 items. A fourth was silent: `Observation`'s `#[derive(Debug, Clone)]` had been absorbed into the doc comment above it, so both impls were gone. Fixing the third of those introduced a fourth, caught before it shipped. Making the NDJSON loop walk a fixed label list closes "a host without `IoRing` drops the key", and opens "a timing added to `measure` but not to the list is dropped on every host" -- the same split, in the other direction, and silent where the first was merely ambiguous. So the relation is now checked in the direction the loop cannot check itself: every measured label must be one the line emits, asserted at render. Verified by sabotage -- removing a label from the list makes the probe fail by name rather than quietly emit one figure fewer. A second review found three more, and the first two are the same sign-error class as the verdict flip -- which is the useful finding here, because it means that defect was never one site. `probe-core-affinity` listed six of `Placement`'s eight variants, dropping the two that carry a measured handoff on a host whose topology never reported the partitioning level; those rows vanished from the table, and because neither cache-separating placement was then expressible, the `confounded` test fired and announced that the classes and the cache domains "coincide exactly" -- a definite claim about cache structure on the one host where the cache relationship was never observed. Its comment already warned that omitting a variant "produced a table that disagreed with the interpretation printed directly beneath it"; it had happened again, in the same function. The same probe counted `Absent` and `NotObserved` as cache domains, so a host reporting no cache level printed "1 cache domain(s)". `probe-peer-index-cache` took its calibration gap through `abs()` and then argued a floor from it, so the argument printed just as readily when the model was the slower of the two, where it does not hold. The third was mine, and it undid a guard `main` had. `main` looked each cost figure up by name and `expect`ed it, with a comment saying why it must be loud: a lookup that misses means a label was renamed in one place and not the other. Deriving the NDJSON from a label list replaced every one of those with `null`, so a renamed timing would publish `null` and read exactly like a host that could not run it -- in both cost probes. `EVERY_LABEL` now carries a `Presence` per label, `null` is reachable only for the one timing gated on `IoRing`, and `request_cost` has the census its unconditional list needs. All three are verified by sabotage rather than by argument, including the one this host cannot produce: a simulated unobserved-cache topology prints the "coincide exactly" claim under the old test and does not under the new one. A third review found four more, and two of them were introduced by the second round's fixes -- which is the finding worth keeping. Correcting a rule obliges a re-check of what was built against the old one, and nothing does that automatically. `attribution` gained a `measured` parameter here, and it was substituted for the first of the two lines the disclaimers describe. So a disagreeing bracket printed the measured reading and the `after` reading under "the two readings above bracket the measurement and differ" -- two byte-identical lines, with the reading that actually differed absent; and a FAILED bracket read printed two well-formed `host:` lines under "at least one of the two readings failed", discarding the `io::Error` that would have said why. `is_attribution_shaped` accepts both shapes, so nothing downstream would have caught either. The arms that carry a disclaimer now print the two readings it names. The "domains coincide exactly" over-claim had a third route, missed because the second round closed the two it could see rather than the reason they were reachable. A host whose topology reports no partitioning cache gets `Observed::Absent`, which compares EQUAL, so every pair classifies `SameCacheSameClass` and no `Unknown*` variant appears -- and on a homogeneous SMT VM both separators are then vacuously inexpressible. The claim printed directly beneath the same report's "0 reported cache domain(s)" line. Requiring `CrossCacheCrossClass` to be expressible closes all three at once, because a sentence about cross-class-and-cross-cache pairs is empty on a host that has none. Verified in both directions: the false claim is gone and genuine confounding is still reported. The NUMA spread sweep recorded each extreme by hop while the table it summarises measures every hop twice, once per ring placement -- so both extremes could be the same hop and the spread between them was then offered as evidence that hops differ. It also printed the directed pair count beside the undirected hop count the round-1 guard computes, describing one machine with two numbers. This host has one NUMA node, so that renderer returns before any of it: compile-verified and read, not executed, and said plainly rather than claimed. `DESIGN-NOTES.md` still said the placement table has six rows, which the second round's own fix made eight -- the commit's headline defect class, reintroduced by a fix for it, in the sentence that counts the thing that changed. The count is gone rather than corrected; it was never the finding. The cost-rendering test asserted `!rows.is_empty()` over a heuristic row recovery, so a renderer printing one of five rows passed. It now names the labels that must appear. Sabotage and its counterfactual, which is the part that makes it a check rather than a hope: with four of five rows dropped, the old assertion passes and the new one fails. One corruption is repaired: two comment lines in `ci.yml` had been welded into one by a lost newline during the peel. Swept the change set for the class -- no other welded line, and no stray control byte in any changed file. A fourth review found five more, three of them in the third round's own fixes. That pattern has now held for three consecutive rounds, and it is the most reliable thing observed about this change: the correction is where the next defect is. `attribution` had three readings and two line slots, and each round traded one for the other. Round 3 showed both brackets and dropped the measured read, so a report that disclaimed its own attribution no longer showed the reading its body came from. Widening the shape to three lines when there is a measured read ends the trade instead of moving it: every disclaimed banner now carries every reading the run holds. That also gave the fourth arm a home -- brackets that agree with each other and differ from the middle read, which is the flaky enumeration this file names as the survey population, and which until now printed an unqualified banner while the run held the evidence against it. All six arms verified by execution, each accepted by the shape recogniser. The directed/undirected confusion round 1 fixed in the hop guard reappeared in round 3's extrema comparison: `0 -> 1` against `1 -> 0` compared unequal, so a spread produced by reversing one hop was reported as the hops not being interchangeable. It is now compared as the same undirected hop the guard counts, with a separate arm for the case where one row is both extremes. The cost test's census asked a static list which labels to require, so it had to exclude the conditional one -- on every host, including those that measure it. Dropping that row from the prose while keeping its NDJSON field therefore passed. The census now asks the NDJSON: a label it reports a number for was measured, so the prose owes a row. That is the NDJSON -> prose direction, which nothing checked before. Sabotage and counterfactual: round 3's test passes with the row dropped, this one fails. Two more, both prose making a claim the arithmetic does not support. `peer_index_cache` said the shared read "is a minority of what the shipping queue spends per item, so removing it cannot be the large win" -- but the shared read sits inside the model baseline, so what the calibration bounds is the model's SHARE, and a 25% gap bounds it at 75%. Measured on this host in one run: a 39% gap, a 61% share, printed under the word minority. Correcting the sign last round left the arithmetic wrong, because the sign was only half of what made the claim unsupported. It now reports the share and stops. Separately, the cache-domain summary counted `Absent` and `NotObserved` together and called both "reported none", which states a negative about a machine where nothing was observed; the two now read differently, verified by simulating each. A fifth review found one, again in the previous round's fix, and this time the two errors are the same error with opposite signs. `is_attribution_shaped` enforces a cardinality, and the rule was written against "was there a disclaimer" rather than "which disclaimer". Round 3 made that two lines for all of them, which forced a three-reading banner to drop a reading. Round 4 made it two-or-three for all of them, which admitted a three-reading `MEASURED_UNCONFIRMED` body -- a shape `attribution` cannot emit, since that arm is reached only when the brackets are equal and so prints the measured read and one bracket. A shape the renderer cannot produce passing the recogniser is the precise hole that function's own rustdoc spends several paragraphs arguing was closed. The count is now per disclaimer. Verified as a matrix over every disclaimer at one to four readings, and against all seven banners `attribution` actually emits: every emitted shape accepted, nothing else. **That function has now been wrong twice in the same place, so the rule is a test rather than a comment.** `a_banner_shape_the_renderer_cannot_produce_is_contained` binds it where it matters -- what `preamble` writes through against what it contains -- and binds both directions, because a recogniser that accepted nothing would pass a one-directional test. Sabotage-verified: restoring the round-4 blanket makes it fail. Worth recording what this round did NOT find. The NUMA hop renderer had been changed three times without ever running, because this host has one node; driven directly against synthetic multi-node observations it rendered the right distinct message in all three cases, including the reversed-hop case that motivated the last change. The cost-probe censuses, the `confounded` condition, the share arithmetic and the cache-domain caveats were re-examined and stand. A sixth review found one thing, and it is queued rather than fixed. The pinning helper in `windows-placement-probe` asserts when `SetThreadGroupAffinity` fails, and `core_affinity::measure()` reaches it; a job object or container restricting the process is the documented cause. **The decision to stop is right and is not in question** -- the assert argues it itself, that an unpinned thread "would produce a plausible number that answers a different question, and nothing in the output would say so." What is wrong is the channel: a panic bypasses `emit_report`, so stdout carries a banner and nothing else, which a fleet survey cannot tell from a job that died for an unrelated reason. That is the same defect `report_unmeasured` closes for discovery failure, left open for pinning failure. It is not fixed here because the fix changes a public signature in `windows-placement-probe`, which this commit deliberately does not touch and which `check-commit-scope.ps1` would flag as a second released crate in one `feat`. Queued as `M4.9` with that blocker recorded, per the mono-repo policy of raising rather than either working around it or silently widening the peel. Worth noting for whoever takes it: the assert predates these binaries and `main` carries it untouched, but `main` has no binary that reaches it -- so this commit makes it newly reachable rather than newly written. The rest were a single class, and it is the one this kind of peel invites: prose lifted from a branch asserts things that are false against main. `DESIGN-NOTES.md` counted three probes where main has two, cited an `M34.5` that main has no `M34` for, and did arithmetic over a probe list main does not have; `CHECKLIST.md` carried five items whose work is on main already or archived here. The same contradiction was the stated reason `COMPLETED-PLANS.md` was left out -- checked in one file and missed in its sibling. Verified: `cargo clippy --all-targets --all-features` and `cargo fmt --check` clean, `cargo doc --workspace --all-features` clean under CI's deny flags, the crate's tests pass with every probe binary present, encoding clean, and no relative link in the changed documents dangles. The two misfiring probes were re-run and no longer misreport. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/ci.yml | 52 +- crates/windows-platform-probes/CHECKLIST.md | 152 +--- .../COMPLETED-CHECKLIST.md | 456 +++++++++- crates/windows-platform-probes/Cargo.toml | 36 +- .../windows-platform-probes/DESIGN-NOTES.md | 380 ++++++++- crates/windows-platform-probes/README.md | 17 + .../src/bin/core_affinity.rs | 790 ++++++++++++++++++ .../src/bin/doorbell_cost.rs | 118 ++- .../src/bin/peer_index_cache.rs | 368 ++++++++ .../src/bin/request_cost.rs | 62 +- .../src/bin/topology.rs | 24 +- .../src/completion_port.rs | 12 +- .../src/doorbell_cost.rs | 76 ++ crates/windows-platform-probes/src/ioring.rs | 6 +- .../src/pool_growth.rs | 9 +- .../src/request_cost.rs | 62 +- crates/windows-platform-probes/src/tests.rs | 86 +- .../windows-platform-probes/src/topology.rs | 41 +- .../src/topology_report.rs | 172 +++- ...probe_tells_both_readers_the_same_thing.rs | 161 ++++ .../tests/a_real_report_agrees_with_itself.rs | 21 +- 21 files changed, 2826 insertions(+), 275 deletions(-) create mode 100644 crates/windows-platform-probes/src/bin/core_affinity.rs create mode 100644 crates/windows-platform-probes/src/bin/peer_index_cache.rs create mode 100644 crates/windows-platform-probes/tests/a_cost_probe_tells_both_readers_the_same_thing.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index be304a743..786565a2c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -221,6 +221,17 @@ jobs: - uses: actions/checkout@v7 - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 + # Explicit, and ahead of the tests, purely so that every probe step below + # has something to gate on. `cargo test` would build these targets anyway, + # but its outcome cannot distinguish "did not compile" from "compiled and + # a test failed" -- and that distinction is the whole point of the guard: + # a failing test is exactly when a probe's diagnostics are wanted, while a + # broken build is when `cargo run` cannot produce any. Incremental and + # cached, so the tests below reuse these artifacts rather than repeating + # the work. + - name: cargo build (probes) + id: build + run: cargo build -p windows-platform-probes --all-targets --locked - name: cargo test (probes, including the ignored tier) env: RUST_BACKTRACE: 1 @@ -252,18 +263,25 @@ jobs: # Splitting also names the failing probe in the Actions UI, which matters # for a job whose output is per-probe magnitudes. - name: probe magnitudes (error mode) + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-error-mode --locked - name: probe magnitudes (handle state) + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-handle-state --locked - name: probe magnitudes (worker context) + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-worker-context --locked - name: probe magnitudes (pool growth) + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-pool-growth --locked - name: probe magnitudes (device map) + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-device-map --locked - name: probe magnitudes (IoRing) + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-ioring --locked - name: probe magnitudes (completion port) + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-completion-port --locked # Printed on every build on purpose. Hosted runners are a heterogeneous # fleet, so accumulating this across builds turns ordinary CI into a slow @@ -273,20 +291,30 @@ jobs: # emits one `x-probe-topology` JSON line so the results can be mined out # of logs mechanically rather than read by eye. # - # `if: '!cancelled()'` is what makes "on every build" true. The test step - # above asserts the cross-check reaches `Agree`, and the host where that + # The `if:` is what makes "on every build" true. The test step above + # asserts the cross-check reaches `Agree`, and the host where that # assertion FAILS is the one whose report is worth the most -- yet Actions # skips later steps in a failed job, so without this the report is # suppressed in exactly the case it exists for. The test's own failure # carries the `CrossCheck` and nothing else: not the domain counts, the # enumeration anomalies, or the machine-readable JSON line. # + # EVERY probe step carries the same guard, in the two-part form. The + # `!cancelled()` half is what runs the step when an earlier step failed. + # The `steps.build.outcome` half is what keeps that from being a nuisance: + # a plain `!cancelled()` also runs the step when the BUILD failed, where + # `cargo run` cannot compile and the step turns from skipped (grey) into + # failed (red), trading quieter broken-build logs for better broken-test + # ones. Gating on the build takes both -- a failing test still emits its + # diagnostics, and a broken build still goes quiet -- so the trade the + # first three of these steps accepted is no longer necessary. + # # It is a SECOND measurement, not a rendering of the one that failed -- # the test and this binary each call `measure()`. So it reproduces a # condition the host holds persistently, which is the case worth # investigating, and does not recover a transient one. - name: probe magnitudes (topology) - if: '!cancelled()' + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-topology --locked # `--release` on the next two, and ONLY on those two, because they are the # only probes here that report nanoseconds. Everything above measures @@ -308,24 +336,24 @@ jobs: # count signals and the waiter's count never caught up. A probe that can # hang is a probe that can hang a build. # - # `if: '!cancelled()'` for the same reason the topology step above carries - # it, and the reason is not specific to topology: a probe step exists to - # produce diagnostic output, so skipping it because an earlier step failed - # suppresses it in precisely the run that wanted it. The test step above - # covers both of these probes, and a host where those tests fail is a host - # whose timings are worth reading. + # Guarded for the same reason the topology step above is, and the reason + # is not specific to topology: a probe step exists to produce diagnostic + # output, so skipping it because an earlier step failed suppresses it in + # precisely the run that wanted it. The test step above covers both of + # these probes, and a host where those tests fail is a host whose timings + # are worth reading. # # It matters more for these two than for a pass/fail probe. Both now assert # every status they take and panic on a missing NDJSON label, and both go # through `emit_report`, which prints what was already established before a # panic -- machinery that only reaches a reader if the step runs at all. - name: probe magnitudes (doorbell cost) - if: '!cancelled()' + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-doorbell-cost --locked --release # Read with the doorbell probe above: together they say whether the # queue's mechanics or the request's own cost deserves the attention. - name: probe magnitudes (request cost) - if: '!cancelled()' + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-request-cost --locked --release # Both halves of the long-path pair, deliberately. Either alone says # nothing: the finding is the *difference* between two executables that @@ -333,8 +361,10 @@ jobs: # so a run that reported one of them would be reporting a number with no # baseline to read it against. - name: probe magnitudes (long path, manifest-aware) + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-long-path-aware --locked - name: probe magnitudes (long path, manifest-unaware) + if: "!cancelled() && steps.build.outcome == 'success'" run: cargo run -p windows-platform-probes --bin probe-long-path-unaware --locked # The NUMA questions the 2026-08-30 design session could not answer, run diff --git a/crates/windows-platform-probes/CHECKLIST.md b/crates/windows-platform-probes/CHECKLIST.md index bca71d524..812aa8ae5 100644 --- a/crates/windows-platform-probes/CHECKLIST.md +++ b/crates/windows-platform-probes/CHECKLIST.md @@ -18,27 +18,6 @@ so renumbering would leave dangling references in a file that may not be edited Stable IDs cost a mismatch between an item number and its milestone; renumbering would cost correctness in the archive. -- [ ] **M2.4** -- Explore, with the sparse matrix as the instrument, whether `Coherence`, - `BracketOutcome` and `Verdict` carry invariants the row does not yet publish -- as VALUES on the - observation, not as correspondences between two renderings -- and whether the sibling probes' - renderers have the same gaps. Expect the matrix to be mostly empty; that is the expected shape and - not a sign the exercise failed. **Record the vacuous results as well as the findings** -- "X and Y - were examined and need not be related" is what stops the next person re-exploring the same cells, - and is the half that normally evaporates. Promote only what proves meaningful into the invariant - set from M3.2. - - Re-scoped by M3.2; the note at the top of this milestone gives the reasoning. **The item text - above was rewritten when M3 was archived, to match**: it still asked for "the same correspondence - failures" and for promotion "into the oracle from M2.1", both retired by M3, so a reader working - the list linearly would have been sent after the half that no longer exists. Found by a review -- - and the lesson generalises, since a re-scoping note 25 lines above an item does not reach someone - executing the item. - -> **-> OPEN QUESTION for the engineer:** M2.4 may show this generalises past this crate, in which case -> the oracle belongs somewhere shared and the question becomes a repository-wide convention rather than -> a probe-crate one. That is a design decision, not a mechanical follow-on, and is deliberately left -> unanswered here. - - [ ] **M4.1** -- Model the observation-readable `ParseIncomplete` conditions that `blocking_states` currently omits, so a deleted push site in `cross_check` is caught for all of them rather than for the subset. @@ -149,48 +128,6 @@ correctness in the archive. - [x] **M4.6** -- Make the start gate releasable, so a failed thread spawn cannot deadlock the probe. -> [completed 2026-09-16 UTC-04:00](COMPLETED-CHECKLIST.md#m46) -- [ ] **M2.5** -- Make the banner describe the read the body describes. - - Gated by M3.1 and M3.3, both landed: establishing that the middle of three discoveries agreed - produces a new FACT, which M3.1 says must reach the row rather than only the banner, and M3.3 - changed how the banner is built. Written before those, it would have been written into machinery - about to move. - - A probe run performs - **three** independent `MachineMemoryTopology::discover()` calls: `Fingerprint::discover()` for the - banner, `measure()`'s own discovery for the body, and `Fingerprint::discover()` again. `attribution` - compares only the two endpoints, so equal endpoints print an unqualified banner without establishing - that the middle read agreed with them. - - **The uncovered window is narrow, and worth stating precisely so it is not over- or under-sold.** - `measure()` brackets its counters around its own discovery, so a processor, group or NUMA change - during the middle read is already caught as `BracketOutcome::Changed`. What no counter reaches is - cache and efficiency-class structure. So the reachable case is a run where the cache structure - differs between the endpoint reads and the middle read while the processor, group and NUMA counts - stay identical -- near-impossible on real hardware, since caches do not change without processors - changing, but reachable on a hypervisor returning inconsistent `GetLogicalProcessorInformationEx` - results, which is exactly the population this probe exists to survey. - - Prefer **construction over comparison**: return the measured topology from `measure()` (as a sibling - function, so the six existing `measure()` callers are untouched) and build the banner with the - already-public `Fingerprint::from_topology`. The banner then describes the body's read *by - construction* and the contradiction becomes unrepresentable, rather than detected by a third - comparison that is itself new prose able to drift. The endpoint reads still earn their place: they - catch structural change across the wider window that the counter bracket cannot see. - - **Express it where M3 put the invariants, not in the M2.1 oracle.** "The banner describes the - measured read" is an invariant over the OBSERVATION, so it belongs in the invariant set from M3.2 - and, if the fact reaches the artifact, in the row schema -- checked on every rendered report through - the renderer binding rather than asserted once in a single test. **These two paragraphs were - rewritten when M3 landed**: they asked for the relation to be expressed in the M2.1 prose oracle, - which M3.4 deleted, so an executor would have gone looking for machinery that no longer exists. - Found by a review, and the same defect M2.4 carried. - - Reviewer disagreement is recorded deliberately, because it is evidence about the instrument rather - than noise: across two rounds one reader raised this twice while two others cleared it, one of them - explicitly after being pointed at the question. Nothing in the suite decides it either way, which is - itself the argument for making it an invariant rather than a test. - - [x] **M4.7** -- Make the queue-contention report renderer testable, by taking the observation as an argument instead of measuring inside it. -> [completed 2026-09-16 UTC-04:00](COMPLETED-CHECKLIST.md#m47) - [ ] **M2.15** -- Run the probe suite on a second architecture in CI. @@ -258,6 +195,35 @@ correctness in the archive. **Blocker recorded when queued:** none. The dependency exists next door and is already proven by that crate's own tests. +- [ ] **M4.9** -- Route a placement probe's refusal-to-measure through the report sink instead of a + panic, so a host that cannot be pinned is a reported observation rather than a crash. + + **Gap:** `windows-placement-probe`'s pinning helper asserts on `SetThreadGroupAffinity`, and + `core_affinity::measure()` reaches it through `time_model_on` / `time_model_placed`. **The + decision to stop is correct and is not what this item changes** -- the assert's own message makes + the argument, that an unpinned thread "would produce a plausible number that answers a different + question, and nothing in the output would say so." That is this repository's position and it + should stand. What is wrong is the *mechanism*: a panic bypasses `emit_report`, so the refusal + lands on stderr while stdout carries a truncated report with no row saying why it stopped. A + fleet survey mining stdout sees a probe that produced a banner and then nothing, which is + indistinguishable from a job that died for an unrelated reason -- the same "cannot tell a + measured absence from a missing run" defect that `report_unmeasured` exists to close for + discovery failure, left open for pinning failure. + + **Reachability changed with the probe binaries.** The assert predates them and `main` carries it + untouched, but `main` has no binary that reaches it; `probe-core-affinity` and + `probe-peer-index-cache` are the first consumers, so this is newly reachable rather than newly + written. + + **Target:** the pinning helper returns a domain error instead of asserting, `measure()` + propagates it, and the binaries render it after the banner the way they already render a + topology-discovery failure -- with an `x-probe-*` row carrying the refusal, so the absence is + mineable. The assert's message text is kept; it is the right message, in the wrong channel. + + **Blocker recorded when queued:** this changes a public signature in `windows-placement-probe`, + which the branch that introduced the probe binaries deliberately does not touch. Raised rather + than taken unilaterally, per the mono-repo bug policy. + ## M5 -- Carried over from M2: unblocked hygiene @@ -268,66 +234,6 @@ gated these, is complete and archived. IDs keep their M2 numbers, for the reason given under M4. -- [ ] **M2.7** -- Decide whether the other nine probe steps in CI should carry `if: '!cancelled()'`, - and apply or record the decision. - - **Measured 2026-09-09:** twelve probe steps in [ci.yml](../../.github/workflows/ci.yml), of which - three are guarded -- topology, and the doorbell/request pair added with this note. The other nine - (`error mode`, `handle state`, `worker context`, `pool growth`, `device map`, `IoRing`, - `completion port`, and both halves of the long-path pair) are skipped whenever an earlier step in - the job fails, because Actions defaults to `if: success()`. - - The argument for guarding is already written at the topology step and is not specific to it: a - probe step exists to emit diagnostics, so skipping it on failure suppresses it in exactly the run - that wanted it. **The long-path pair is the sharpest case** -- its own comment says either half - alone "says nothing", since the finding is the difference between two executables, so a partial - run of that pair is worse than useless. - - **It is queued rather than done because there is a real tradeoff, and it is an operational call.** - `!cancelled()` also runs the step when the *build* failed, where `cargo run` cannot compile and - the step turns from skipped (grey) into failed (red). That trades quieter broken-build output for - better broken-test output. The topology step already took that trade; whether all twelve should is - a judgement about how the CI log is read, not something to settle by consistency alone. - -- [ ] **M2.8** -- Carry the OS error in the remaining Win32 assertion messages. - - `last_os_error()` (or a raw `GetLastError`) is in the messages in `doorbell_cost`, `request_cost` - and `handle_state`, and missing from four sites in probes this peel did not touch: - `completion_port.rs:224` and `:234` ("create a completion port"), `ioring.rs:320` ("create the - probe pipe"), and `pool_growth.rs:62` ("create the gate event"). Each says what was being attempted - and not why it failed, which is the whole of what a CI log can offer someone who cannot rerun under - a debugger. - - Two rules worth carrying over, both learned the expensive way in this peel. Read the error - **immediately after the single call whose failure is reported** -- a code attached to a condition - spanning two calls belongs to whichever ran last, not whichever failed, and can print "The - operation completed successfully" under a message saying something failed. And attach it only to a - condition that is genuinely an OS failure: a call that returned a size rather than an error should - not carry one, since `GetLastError` says nothing about it. - -- [ ] **M2.9** -- Stop `request_cost` calling a cross-host ratio "the finding". - - [src/request_cost.rs](src/request_cost.rs) ends its module doc with "Absolute values are - host-specific; the **ratios against the doorbell and the atomic** are the finding." The ratios that - [src/bin/request_cost.rs](src/bin/request_cost.rs) actually prints divide THIS host's measurement by - `DOORBELL_NS_REFERENCE` / `ATOMIC_NS_REFERENCE`, which are constants measured on the Snapdragon X2 - development machine. A ratio with this host's numerator and another host's denominator is neither a - same-host ratio nor a portable finding, and the emitted report says as much two lines later: - "re-read that probe on this host before trusting them". So the module doc promotes to "the finding" - exactly the number its own output tells the reader not to trust. - - **Pre-existing, and deliberately not fixed in the `GetFullPathNameW` peel (PR #86) that found it.** - It arrived in `ae1e39f`, is already on `main`, and is outside that branch's diff; folding it in - would have put an unrelated behavioural change into a documentation peel that had already run to - nineteen review rounds. - - The fix is a decision, not a sweep, which is why this is queued rather than taken: either compute - both figures on the same host and run (the probe would have to measure the doorbell itself, or read - a companion artifact), or keep the fixed references and demote them in the prose from "the finding" - to a labelled cross-host comparison. The first is more useful and more work; the second is honest - and cheap. Same defect class as PR #86's subject -- a claim stated more strongly than the evidence - supports -- so whichever is chosen, the wording has to end up matching what the numbers can carry. - - [ ] **M2.13** -- Lint the completed-checklist archive mechanically in CI. Three bookkeeping defects reached review on this branch, and all three are decidable by a script: a diff --git a/crates/windows-platform-probes/COMPLETED-CHECKLIST.md b/crates/windows-platform-probes/COMPLETED-CHECKLIST.md index 06a962a08..ceb316629 100644 --- a/crates/windows-platform-probes/COMPLETED-CHECKLIST.md +++ b/crates/windows-platform-probes/COMPLETED-CHECKLIST.md @@ -2,7 +2,221 @@ Append-only. Newest groups at the bottom. -## Moved 2026-09-09 19:00:17 -04:00 -- M1: a probe's report streams as it is measured +## Moved 2026-09-05 -- claim-word layout: measured the apportionment, then shipped it as a caller's choice + +# Checklist: claim-word layout + +Measures how the `reserving_mpsc` claim word's bit apportionment and width +affect push throughput, then offers the layouts as documented, caller-selectable +options in `windows-waitable-queues`. + +Design decisions land in [DESIGN-NOTES.md](DESIGN-NOTES.md) for the measurement +and in the queue crate's +[DESIGN-NOTES.md](../windows-waitable-queues/DESIGN-NOTES.md) for the API +(`D-36`, `D-37`). + +## Background + +`reserving_mpsc` packs `reserved` and `position` into one `AtomicU64` because +the claim protocol needs a single compare-and-swap to update both (`D-17`, +`D-34`). The split is 32/32, which caps positions at 2^32 and is the whole +source of the `SH-14.1` recurrence hazard disclosed by `D-36`. + +**The 32/32 split is not forced by the platform.** It follows from a capacity +ceiling of 2^31, because the `reserved` half must be able to hold the entire +capacity. Two independent constraints bound the capacity: + +- ring arithmetic: `capacity <= 2^(POSITION_BITS - 1)` +- packing: `capacity <= 2^(64 - POSITION_BITS) - 1` + +`BOUNDS_MAX` is currently derived from the first alone and the second is only +*asserted*, so widening the position raises the ceiling while shrinking the +field obliged to hold it -- which is why widening trips the assertion instead of +working. + +## M1: measure the layouts -- done + +Built as a duplicated path in this crate so the measurement added no third-party +dependency to a publishable crate and could not disturb the +`windows-waitable-queues` branch being peeled off PR #56. + +- [x] **CW-1.1** -- Add `portable-atomic` with `default-features = false` to + this crate only, and record whether `AtomicU128` exists and is lock-free. + Measured: `is_always_lock_free()` is true and `cmpxchg16b` is a default target + feature here, so no CPUID branch was timed as though it were the algorithm. + +- [x] **CW-1.2** -- Implement the three claim-word layouts as self-contained + `u64`-item queues in `claim_layout.rs`. + +- [x] **CW-1.3** -- Wire the three layouts into `probe-queue-contention` as + named shapes in both regimes. + +- [x] **CW-1.4** -- Run the probe and capture the report. Result: + re-apportioning is free (16/48 tracks 32/32 within noise in both regimes); + widening to `u128` costs 2-3x isolated and 5-12% drained, and the drained + figure understates it because a slower producer earns fewer refusals. + +- [x] **CW-1.5** -- Record the measurement and the rollover table in + [DESIGN-NOTES.md](DESIGN-NOTES.md). + +## M2: offer the layouts as options + +**Decided: offer a set of named layouts rather than one, on the condition that +each carries its own ramifications.** The engineer's direction was that options +are right "as long as quality is maintained" and "the ramifications of the +choices are available". Both halves are binding, and the second is the one an +options API usually fails: a caller who cannot see what a layout costs will pick +by name, and the names are the least informative thing about them. + +Three obligations apply to every item in this milestone: + +- **Each layout states its own consequences where it is named** -- reservation + ceiling, capacity ceiling, and time-to-recurrence at a stated push rate. The + rollover figures in [DESIGN-NOTES.md](DESIGN-NOTES.md) are the source; the + crate documentation restates them once and nothing else does. +- **Quality is per-layout, not per-crate.** Every layout gets the same const + assertions, tests, and mutation coverage as the shipping one. A layout + exercised only by a doctest is worse than no option, because its presence + claims a support level nothing verifies. +- **Adding a layout must not weaken the default.** The layout parameter must + not leak into the signatures of callers who do not use it. If it cannot be + kept out, say so rather than accepting the churn. + +**This milestone is no longer parked.** It was gated on the peel merging, on the +reasoning that touching `windows-waitable-queues` would re-grow a branch under +review. That reasoning expired: `mikegrier/waitable-queues` has no pull request +open, so there is no review to disturb, and the `u64` layouts need no new +dependency at all -- only 64/64 does, which is `CW-2.3`. + +- [x] **CW-2.1** -- Introduce the layout as a compile-time parameter, widen the + position to 64 bits, and decouple the reservation ceiling from the capacity. + + **Merged from two items during execution, because they cannot be verified + apart.** Decoupling the ceiling is numerically invisible at 32/32: + `MAX_RESERVED` is 2^32-1 while `BOUNDS_MAX` is 2^31, so a cap on outstanding + reservations can never bind and no test can reach it. It becomes observable + only once a layout makes the reservation half narrow. Landing them separately + would have meant committing a branch nothing could exercise and calling it + done. + + The three parts: + + - Cap *outstanding reservations* at `MAX_RESERVED` in `reserve`, and drop the + `BOUNDS_MAX <= MAX_RESERVED` const assertion that ties the capacity to the + reservation field. `BOUNDS_MAX` then follows from ring arithmetic and the + crate-wide bound alone. + - Widen `position`, `head`, and the per-slot `sequence` to 64 bits for every + layout, since a position of more than 32 bits cannot be read out through + `position_of`'s `u32`. Uniform 64-bit metadata is measured-safe rather than + assumed: `CW-1.4` compared 32/32 with 32-bit metadata against 16/48 with + 64-bit metadata and found no difference, and for a `u64` payload the slot is + 16 bytes either way once alignment is applied. + - Add the layout parameter with a default preserving today's behaviour. + Generic defaults are permitted on types but not on functions, so `bounded` + keeps its signature and returns the defaulted types, and a second entry + point names a layout explicitly. + + **This is a contract change**: the shipping shape promises every slot may be + reserved at once, and this replaces that with a fixed reservation ceiling. A + different promise rather than a broken one, but it must be stated, not slipped + in. + +- [x] **CW-2.3** -- Decide whether a 128-bit claim word ships at all. + + **Decided: yes, behind an opt-in `dwcas` feature.** The `Wide` layout packs a + `u128` divided 64 / 64. Without the feature the crate depends on + `windows-sys` alone and every layout uses `AtomicU64`; with it, + `portable-atomic` appears. So a caller who does not want the dependency does + not carry it, and one who wants a guarantee rather than a twenty-year + argument can have it. + + This resolves `CW-1.6`'s scope the other way from what the item anticipated: + the shipping crate *can* now express a 128-bit layout, so the probe does not + need to keep its own `wide` implementation to measure one. + + **Not a dependency question.** An earlier form of this item framed it as + whether `portable-atomic` becomes a dependency of a published crate, which was + wrong: `core::arch::x86_64::cmpxchg16b` is stable on the pinned toolchain, so + a 64/64 layout needs no third-party crate. `D-7`'s and `D-37`'s dependency + cost does not apply, and the decision must not be made on it. + + What it actually costs: hand-written `unsafe` with manual orderings in the + file where that is worst to get wrong, x86-64 only (no ARM64 `casp`, no + i686), and a `target-feature` or runtime-detection decision. Against that, + `CW-1.4` measured the 128-bit exchange 2-3x slower on the claim in the + isolated regime, and `CW-2.1` has since made `Perpetual` reach about 20 years + before recurrence on a plain `AtomicU64` at no measured cost. + + So the question is narrow: is going from unreachable-in-any-deployment to + unreachable-in-principle worth that? The engineer has said 32-bit Windows + deployment is not a present concern, which changes `D-18`'s premise and must + be recorded rather than assumed. + + **`CW-1.6`'s scope is decided by this item**: if `portable-atomic` is + declined, this crate must keep its `wide` implementation, because a layout the + queue crate cannot express is one the probe cannot instantiate. + +- [x] **CW-2.4** -- Document the layouts as a choice, in the crate documentation + and the README, with the rollover table and the two axes a caller trades + between: outstanding reservations against time-to-recurrence. Lead with what + `CW-1.4` measured -- re-apportioning is free, widening is not -- so a caller + is not left assuming the safest option must be the slowest. State the push + rate the figures assume, and that a draining queue cannot sustain the fastest + of them. + + **Compiled, not merely written.** Any README example naming a layout is a + doctest per this repository's CONTRACT INTEGRITY rule, so a renamed or removed + layout breaks the build instead of leaving the documentation teaching a name + that no longer exists. + +- [x] **CW-2.5** -- Reopen `D-36` with the measurement in hand, then sweep every + statement of the hazard. + + **`D-36`'s premise is falsified, and that is the finding, not the sweep.** It + decided 0.1.0 ships `SH-14.1` disclosed rather than fixed *because the fix is + a claim-protocol replacement (`D-35`) gated on an open question*. Re- + apportionment is a second fix that neither `D-36` nor `D-37` considered, and + `CW-1.4` measured it free. It does not eliminate the recurrence -- only moves + it -- but 8/56 moves it from about 37 seconds to about 20 years at the + disclosed rate, which takes `D-36`'s "computed exposure" from reachable in + under a minute to unreachable in any real deployment. + + So the question is whether the crate ships this hazard at all. Answer that + first; the sweep follows from the answer. + + The sweep is blast-radius, not an edit of one reported site: `D-36` states the + hazard in the crate documentation, the README, and `reserving_mpsc`'s module + documentation, each leading with "on every target, not only 32-bit ones", and + `lib.rs` separately claims the shape is "sound below the wrap". Every one is + scoped to a 32-bit position. Grep the distinguishing terms across `src/`, + `tests/`, `examples/` and `*.md` for the crate and its dependents, fix every + hit or say why it is out of scope, and record the sweep in the commit message. + +## M3: retire the duplicate + +- [x] **CW-1.6** -- Delete the duplicated *implementation* in + `claim_layout.rs`, keeping only what `CW-2.3` leaves no + other way to measure. + + **This is not a decision about which layouts to offer.** That is settled -- + multiple layouts ship as caller-selectable options, per `M2`. This item is + only about the private copy of the reserving protocol in this crate, which + existed so the layouts could be measured without touching + `windows-waitable-queues`. + + **`M2` makes the copy obsolete.** Once the shipping crate takes the layout as + a compile-time parameter, the probe instantiates the *real* type at any layout + it wants to compare, including candidates that are not defaults -- so + exploring a new apportionment no longer needs a duplicate. + + Deleting it is not tidiness. A second implementation of the same protocol + drifts, and this one already did: `CW-1.4`'s first run measured 3.7x against + the shipping shape on an entirely different scaling curve, because the + duplicate had not cache-padded `head` and the claim word. Corrected, it still + sits about 1.26x off. A duplicate that diverges silently produces a + measurement that looks healthy and describes something nobody ships. + +## Moved 2026-09-09 -- M1: a probe's report streams as it is measured ## M1 -- Stream a probe's report as it is measured @@ -31,6 +245,10 @@ piece of work rather than a correction to that one. [DESIGN-NOTES.md](DESIGN-NOTES.md#d-streaming-report). **The estimate in this item was wrong, and re-measuring it decided the question.** It said "upwards + of 160" `writeln!` sites; there are **504** across the production renderers, written into the `&mut + String` of about twenty functions. Option (a) -- a `Report` method taking `fmt::Arguments` plus a + macro -- is the most explicit and would have rewritten all 504; that is affordable at 160 and is not + at 504. Option (b) moves the twenty signatures and leaves the 504 untouched, because `String` of 160" `writeln!` sites; there are **332** across this crate's production renderers, written into the `&mut String` of 18 functions. Option (a) -- a `Report` method taking `fmt::Arguments` plus a macro -- is the most explicit and would have rewritten all 332; that is affordable at 160 and is not @@ -47,11 +265,26 @@ piece of work rather than a correction to that one. by reading. - [x] **M1.2** -- Convert every renderer to write into the sink as it measures, and simplify + `emit_report` accordingly. All sixteen probes now take `out: &mut dyn std::fmt::Write`; the `emit_report` accordingly. All thirteen probes now take `out: &mut dyn std::fmt::Write`; the `catch_unwind`/`resume_unwind` pair is deleted, because with lines leaving as they are produced there is no buffer to rescue and keeping it would imply partial output still depends on the panic unwinding. `Captured` is unchanged and its tests pass untouched. + **Three probes needed more than a signature change**, because they never went through + `emit_report` at all -- `core_affinity`, `peer_index_cache` and `queue_contention` each composed a + `String` and called `emit` directly. They are branch-local and so missed the round that fixed the + same bypass in the peeled probes, which means the crate's "every probe routes through this" claim + was false in three places. `core_affinity` additionally measured in `main`'s argument list, ahead + of the renderer, so a topology read that failed produced no banner at all; it now measures after + the banner and reports the failure as a failure to observe rather than as a finding. + + **Verified with a control, because these probes are not deterministic.** A direct before/after + comparison flagged nine of fifteen reports, which is not evidence -- they print measured + nanoseconds and branch their verdicts on them. Running the *same* build twice differed by as much + or more (`peer-index-cache`: 22 lines between two runs of one build, against 20 across the + conversion), and the twelve deterministic reports were structurally identical. A before/after diff + on a probe means nothing without that control. **Every probe in this crate needed only the signature change**, because each already went through `emit_report` rather than composing a `String` and calling `emit` itself. That is what the one-sink refactor bought, and it is why converting thirteen probes is one function plus one line @@ -82,6 +315,9 @@ piece of work rather than a correction to that one. the mechanism and gone red only on the missing catch. **The interruption half is measured, with a control**, and recorded in + [DESIGN-NOTES.md](DESIGN-NOTES.md). `probe-queue-contention` (~65 s), stdout redirected, killed at + 8 s: the streaming build had **114 bytes** on disk (banner and heading), the pre-M1.2 build built + from `246687e` had **0**. The control is what makes it evidence rather than an observation. [DESIGN-NOTES.md](DESIGN-NOTES.md). `probe-doorbell-cost` (~0.8 s, the longest-running probe here), stdout redirected, killed at 300 ms, six runs of each build with every run confirmed still alive at the kill: the streaming build captured **129 characters** (banner and heading) on all @@ -94,6 +330,185 @@ piece of work rather than a correction to that one. newline even when redirected -- had stdout been block-buffered this milestone would have needed a per-line flush too. +## Moved 2026-09-09 -- M2: the report's parts are checked against each other + +## M2 -- Check correspondence between the report's parts, not just each part + +A pull-request review found a state where [src/topology_report.rs](src/topology_report.rs) printed +`BUG IN THIS PROBE ... Nothing below about cache partitioning can be trusted` while `cross_check` had +no branch for that state, so the verdict could print `=> agree` two paragraphs below. Twenty-eight +rounds of per-artifact review and a zero-surviving-mutant `cargo-mutants` result had both passed over +it, because every function involved was correct on its own terms and the defect lived in the relation +between two of them. + +See [DESIGN-NOTES.md](DESIGN-NOTES.md) -> [The defects that survived were correspondence +failures](DESIGN-NOTES.md#d-correspondence-failures) for why each instrument was structurally +incapable of finding it, and for the matrix-as-exploration / oracle-as-durable split this milestone +implements. + +The three correlations below are known to be real because each was violated. They are not a +speculative list to extend by imagination -- a fourth is added when a fourth contradiction is found. + +- [x] **M2.1** -- Add a report oracle to this crate. [src/report_oracle.rs](src/report_oracle.rs), + seeded with the three known invariants and modelled on `ContractChecker`, including its + as-many-must-accept-as-must-reject discipline. Reasoning in + [DESIGN-NOTES.md](DESIGN-NOTES.md#d-correspondence-failures). + + It relates two things **already visible in the report** and re-derives nothing, because a second + implementation of the rendering rules would be a check of the copy rather than of the contract. + The gating rule is the interesting boundary: `parse_in_doubt` is + `!disagreements.is_empty() || !parse_incomplete.is_empty()`, and the NDJSON publishes + `parse_incomplete` as a *count*, so the oracle reads that count and the `disagree` verdict -- the + two visible shadows of the definition. That coupling is what M2.2's sabotage must confirm. + + **Validated against a real rendered report, not only fixtures.** The prose labels were confirmed + against a live `probe-topology` run, a test corrupts each double-rendered value in turn so a + drifted label fails loudly rather than silently reading nothing, and the historical defect injected + into a real report is reported twice -- once for the prose verdict, once for the NDJSON. + + **The first injection silently did nothing and nearly inverted the conclusion.** Its anchor, + `cross-check:`, does not occur -- the real text is `cross-check against independently read Win32 + counters:` -- so the "defective" report was identical to the clean one and the oracle correctly + found nothing, which read as the oracle being blind. A sabotage that fails to apply is + indistinguishable from an instrument that fails to fire unless the injection asserts it changed + something. + +- [x] **M2.2** -- Route every test that renders a report through the oracle. Bound inside + `topology_report::report` and `report_unmeasured` under `cfg(test)`, so all **26** existing call + sites inherit it without being touched and every future one does too -- no author has to remember. + + `cfg(test)` rather than always-on: a real probe run must still print a contradictory report rather + than panic, because a self-contradicting report is a finding *about this probe* and suppressing it + would destroy the evidence. The real-host path is M2.3. + + **The sabotage measured both directions, which is what makes it evidence rather than a gesture.** + Emitting the processor count where the core count belongs -- a pure correspondence defect, both + renderings individually well-formed -- turns **13 tests red**, all in `tests` and none in + `report_oracle::tests`. Among them is `every_report_carries_the_banner_and_title`, written for + something else entirely, which is exactly the point: the cases most likely to catch the next + contradiction are the ones nobody aimed at it. + + With the same defect in place and the binding removed, **all 173 tests pass**. The existing suite + cannot see the defect at all, so the detection is the oracle's and the binding is what delivers + it. Had only `report_oracle::tests` gone red, the binding would have been cosmetic. + +- [x] **M2.3** -- Add the missing integration test. + [tests/a_real_report_agrees_with_itself.rs](tests/a_real_report_agrees_with_itself.rs) composes the + report exactly as `probe-topology` does -- two fingerprint reads, a real `measure()`, `attribution` + -- and applies the oracle. Explicitly, because an integration test links the lib without + `cfg(test)`, so M2.2's binding does not reach it. + + **It asserts nothing about this machine.** A test expecting a processor count or a cache level + would fail on the next runner shape rather than on a defect, and would be loosened until it + asserted nothing. It checks only that the report's parts agree *with each other*, which every host + must satisfy -- including one whose topology cannot be read at all. + + **A second test exists because the first can pass vacuously**, and that is not hypothetical. If the + renderer drifts from the oracle's prose labels, every lookup returns `None`, every comparison is + skipped, and the real assertion passes having checked nothing. So each of the four double-rendered + counts is corrupted in this host's own report and a violation is required. Verified by widening a + prose label by one space: the guard failed naming `"packages":`, **while the primary test still + passed** -- exactly the vacuous green it exists to expose. + + The corruption asserts it changed something first, per M2.1's lesson. + +- [x] **M2.4** -- Explore with the sparse matrix. Full record, findings and vacuous cells both, in + [DESIGN-NOTES.md](DESIGN-NOTES.md#d-correspondence-failures). + + **The prediction held on the axis it named and failed on one it did not.** `Coherence` and + `BracketOutcome` are each rendered exactly once -- `Coherence` never appears in prose or NDJSON at + all, `BracketOutcome` only in the banner -- so neither can contradict itself and both cells are + genuinely empty. `Verdict` was already covered. + + The productive axis turned out to be **facts, not state enums**: six more were rendered twice with + nothing comparing them, and all six are now promoted -- NUMA domains and those without processors, + cache domains per level, the outermost partitioning level, domains per policy, and the two + independently-read Win32 counters against the enumeration. + + Those last two are a **different rule shape** and the closest to what this probe is for: the + counters exist so a mismatch is a finding, so one contradicting the enumeration under an `agree` + verdict is the original defect in its purest form. `GetNumaHighestNodeNumber` is deliberately + excluded and the exclusion is pinned by a test -- it is the largest node *number*, not a count, so + comparing it would manufacture a disagreement on any sparsely-numbered machine. + + Reading `caches` and `policies` forced the field reader to balance brackets rather than stop at the + first closer; `caches` is an array *of objects*, so the naive read saw only its first entry and + would have skipped every later cache level in silence. + + All eight promoted cells are proved live against this host's real report by the M2.3 guard. + + > **-> SCOPE FINDING:** `probe-doorbell-cost` and `probe-request-cost` render **every measured + > figure twice**, prose table and NDJSON, with nothing comparing them -- the same class, in two + > more probes. Queued as M2.9 rather than taken here, because extending the oracle past one + > renderer is a design question about where it should live, not a mechanical follow-on. + +- [x] **M2.9** -- **Decided: if two renderings must match, they come from a common source.** Not a + third oracle rule set -- both cost probes now walk the same `Observation::timings` the prose table + walks, with a `json_key` function deciding only what the machine-readable rendering calls each + entry. Reasoning in [DESIGN-NOTES.md](DESIGN-NOTES.md#d-correspondence-failures). + + This is strictly stronger than extending the oracle, and cheaper. An oracle rule finds a + contradiction that already exists; deriving both renderings from one value means there is none to + find. It also deleted code rather than adding it: ten hand-named NDJSON fields and a `get` closure + went, because naming each figure separately was what made the two renderings independent. + + The `json_key` gate panics on a label it does not know, so a figure added to `measure` reaches both + renderings or fails loudly -- it cannot reach one only. Verified by adding an unnamed timing: the + probe printed its prose row and then died naming the missing key. (The row appearing before the + panic is M1.2's streaming, which is how a reader sees how far it got.) + + One test remains, and its job is narrow: the derivation is structural in the source, so what is + left to check is that the structure survives rendering, formatting and the process boundary. It + reuses the crate's own `json_key` rather than restating the pairing, since a test carrying its own + copy would be checking the copy. + + **Its emptiness guard fired on the first run**, and that is worth recording: `request_cost`'s table + has ratio columns after the figure, so a row parser requiring exactly two tokens matched nothing + and the test would have passed having compared zero rows. + + > **-> REMAINING SCOPE:** this closes the class in the two cost probes. Whether the same + > common-source rule should be applied to `topology_report`, whose prose and NDJSON are still + > written separately and are guarded by the M2.1 oracle instead, is a larger change and is not + > queued yet -- the oracle covers it today, and the eight promoted cells are what make that + > coverage real. + +> **-> ANSWERED (M2.9):** it did generalise, and the answer was not to move the oracle. If two +> renderings must match they come from a common source, so the pairing is structural and there is +> nothing for an oracle to check. That is a repository-shaped answer -- it is the same rule the root +> [DESIGN-NOTES.md](../../DESIGN-NOTES.md) states as preferring a derived fact to a restated one -- +> but it needed no shared code to apply, because what generalises is the principle rather than a +> mechanism. + +- [x] **M2.5** -- Make the banner describe the read the body describes. A probe run performs + **three** independent `MachineMemoryTopology::discover()` calls: `Fingerprint::discover()` for the + banner, `measure()`'s own discovery for the body, and `Fingerprint::discover()` again. `attribution` + compares only the two endpoints, so equal endpoints print an unqualified banner without establishing + that the middle read agreed with them. + + **The uncovered window is narrow, and worth stating precisely so it is not over- or under-sold.** + `measure()` brackets its counters around its own discovery, so a processor, group or NUMA change + during the middle read is already caught as `BracketOutcome::Changed`. What no counter reaches is + cache and efficiency-class structure. So the reachable case is a run where the cache structure + differs between the endpoint reads and the middle read while the processor, group and NUMA counts + stay identical -- near-impossible on real hardware, since caches do not change without processors + changing, but reachable on a hypervisor returning inconsistent `GetLogicalProcessorInformationEx` + results, which is exactly the population this probe exists to survey. + + Prefer **construction over comparison**: return the measured topology from `measure()` (as a sibling + function, so the six existing `measure()` callers are untouched) and build the banner with the + already-public `Fingerprint::from_topology`. The banner then describes the body's read *by + construction* and the contradiction becomes unrepresentable, rather than detected by a third + comparison that is itself new prose able to drift. The endpoint reads still earn their place: they + catch structural change across the wider window that the counter bracket cannot see. + + **This belongs to M2 rather than beside it:** "the banner describes the measured read" is a + correspondence invariant, so it should be expressed in the M2.1 oracle and checked on every rendered + report, not asserted once in a single test. + + Reviewer disagreement is recorded deliberately, because it is evidence about the instrument rather + than noise: across two rounds one reader raised this twice while two others cleared it, one of them + explicitly after being pointed at the question. Nothing in the suite decides it either way, which is + itself the argument for the oracle. ## Moved 2026-09-09 -- M2.6: what `GetFullPathNameW` does, and whether it stays ### M2.6 -- Say what `GetFullPathNameW` does, in the crate that owns it, and whether it stays. *(completed 2026-09-09 22:54:01 UTC-04:00)* @@ -154,6 +569,45 @@ request as it was written, and quotes the module doc as it read before the corre say what that is made of; the owning crate could say, and a reader of either would then stop guessing. +- [x] **M2.7** -- Decide whether the other nine probe steps in CI should carry `if: '!cancelled()'`, + and apply or record the decision. + + **Measured 2026-09-09:** twelve probe steps in [ci.yml](../../.github/workflows/ci.yml), of which + three are guarded -- topology, and the doorbell/request pair added with this note. The other nine + (`error mode`, `handle state`, `worker context`, `pool growth`, `device map`, `IoRing`, + `completion port`, and both halves of the long-path pair) are skipped whenever an earlier step in + the job fails, because Actions defaults to `if: success()`. + + The argument for guarding is already written at the topology step and is not specific to it: a + probe step exists to emit diagnostics, so skipping it on failure suppresses it in exactly the run + that wanted it. **The long-path pair is the sharpest case** -- its own comment says either half + alone "says nothing", since the finding is the difference between two executables, so a partial + run of that pair is worse than useless. + + **Decided and applied: gate on the build's outcome, not on `!cancelled()`.** The tradeoff above is + real for `!cancelled()`, which also runs the step when the *build* failed, turning skipped (grey) + into failed (red) exactly when `cargo run` cannot compile anything. Gating on + `steps.build.outcome == 'success'` takes neither horn: a broken build still skips quietly, and a + failing *test* -- the run that wanted the diagnostics -- still emits them. All twelve probe steps + now carry it, and an explicit `cargo build` step ahead of the tests exists only to give them + something to gate on, since `cargo test`'s outcome cannot separate "did not compile" from + "compiled and a test failed". + +- [x] **M2.8** -- Carry the OS error in the remaining Win32 assertion messages. + + `last_os_error()` (or a raw `GetLastError`) is in the messages in `doorbell_cost`, `request_cost` + and `handle_state`, and missing from four sites in probes this peel did not touch: + `completion_port.rs:224` and `:234` ("create a completion port"), `ioring.rs:320` ("create the + probe pipe"), and `pool_growth.rs:62` ("create the gate event"). Each says what was being attempted + and not why it failed, which is the whole of what a CI log can offer someone who cannot rerun under + a debugger. + + Two rules worth carrying over, both learned the expensive way in this peel. Read the error + **immediately after the single call whose failure is reported** -- a code attached to a condition + spanning two calls belongs to whichever ran last, not whichever failed, and can print "The + operation completed successfully" under a message saying something failed. And attach it only to a + condition that is genuinely an OS failure: a call that returned a size rather than an error should + not carry one, since `GetLastError` says nothing about it. ## Moved 2026-09-10 -- M2.1: the report oracle ### M2.1 -- Add a report oracle to this crate. *(completed 2026-09-10 19:45:50 UTC-04:00)* diff --git a/crates/windows-platform-probes/Cargo.toml b/crates/windows-platform-probes/Cargo.toml index 8b92ee564..487a58ca4 100644 --- a/crates/windows-platform-probes/Cargo.toml +++ b/crates/windows-platform-probes/Cargo.toml @@ -38,6 +38,10 @@ path = "src/lib.rs" # parse. See the `[dependencies]` entry for why that is not hand-written. oracle-in-renderer = ["dep:serde_json", "dep:serde"] +[[bin]] +name = "probe-core-affinity" +path = "src/bin/core_affinity.rs" + [[bin]] name = "probe-error-mode" path = "src/bin/error_mode.rs" @@ -70,10 +74,6 @@ path = "src/bin/ioring.rs" name = "probe-pool-growth" path = "src/bin/pool_growth.rs" -[[bin]] -name = "probe-topology" -path = "src/bin/topology.rs" - [[bin]] name = "probe-doorbell-cost" path = "src/bin/doorbell_cost.rs" @@ -82,10 +82,18 @@ path = "src/bin/doorbell_cost.rs" name = "probe-request-cost" path = "src/bin/request_cost.rs" +[[bin]] +name = "probe-topology" +path = "src/bin/topology.rs" + [[bin]] name = "probe-queue-contention" path = "src/bin/queue_contention/main.rs" +[[bin]] +name = "probe-peer-index-cache" +path = "src/bin/peer_index_cache.rs" + # These two are the same code, and that is the measurement: they differ only in # whether `build.rs` embeds the `longPathAware` manifest, which is not a runtime # switch and so cannot be a flag on one binary. @@ -130,7 +138,8 @@ serde_json = { version = "1.0", optional = true } # A `version` beside a `path` is consulted only when the depending crate is # packaged, but cargo still requires the path crate's own version to satisfy it # at every build -- so a pin left behind by a bump breaks the whole workspace's -# resolution rather than only this crate's. +# resolution rather than only this crate's. With six such pins, that was six +# standing chances to break `main` in exchange for nothing. # # The pool-growth probe measures the shipping API rather than a # reimplementation of the SDK's inline environment helpers, so it depends on the @@ -140,10 +149,16 @@ windows-threadpool-sys = { path = "../windows-threadpool-sys" } # a second parse written here, which would only measure itself. The raw Win32 # counters it cross-checks against are read independently through windows-sys. windows-topology-sys = { path = "../windows-topology-sys" } -# Every probe's report opens with a line naming the machine that produced it and -# whether the measurement is tainted, so a captured finding cannot be pasted -# somewhere and compared against something it does not describe. That banner is -# `windows-placement-probe`'s to render, not a second copy here. +# The placement measurement moved out to its own crate so it could be shared +# with people running it on hardware this workspace does not own. The probes +# here call into it rather than keeping a second copy: two renderings of one +# measurement disagreeing is a defect this investigation has already hit. +# +# It renders the banner too: every probe's report opens with a line naming the +# machine that produced it and whether the measurement is tainted, so a captured +# finding cannot be pasted somewhere and compared against something it does not +# describe. That banner is `windows-placement-probe`'s to render, not a second +# copy here. windows-placement-probe = { path = "../windows-placement-probe" } # The request-cost probe measures the real request type the design would put on # a queue, not a stand-in, for the same reason the topology probe reads the @@ -156,6 +171,9 @@ windows-namespace-request-sys = { path = "../windows-namespace-request-sys" } # The experimental permit claim is enabled here because this probe is what # decides its fate (SH-15.5): it must be measured against the shipping shapes on # the same host, in the same run, by the same harness. +# +# `dwcas` is NOT enabled here: it is taken from the target-gated tables below, +# so a target without a native 128-bit exchange still builds. windows-waitable-queues = { path = "../windows-waitable-queues", features = [ "experimental-permit-claim", ] } diff --git a/crates/windows-platform-probes/DESIGN-NOTES.md b/crates/windows-platform-probes/DESIGN-NOTES.md index 70356fb77..851c48193 100644 --- a/crates/windows-platform-probes/DESIGN-NOTES.md +++ b/crates/windows-platform-probes/DESIGN-NOTES.md @@ -359,6 +359,26 @@ knowledge of what the crate *should* have said -- only two readings and whether they agree. A probe that grew a hard-coded expectation would be back on the wrong side of the rule above. +## This crate is never distributed, and its dependencies carry no versions + +Not to a registry, and not as a released binary either -- unlike +`windows-placement-probe`, which ships a CI-built binary to people running it on +hardware this workspace does not own. These probes are a development +instrument, run from a checkout by someone who has the checkout. `publish = +false` is the whole story, and it is permanent rather than "not yet". + +**The consequence is that every workspace dependency here is path-only.** A +`version` beside a `path` exists to tell a registry what to resolve when the +depending crate is packaged. Nothing packages this crate, so those pins named a +version no one would ever consult -- while still having to be correct, because +cargo requires the path crate's own version to satisfy the pin **at every +build**, not merely at publication. + +That is not a theoretical tidy-up. Measured on 2026-09-02: bumping +`windows-topology-sys` to 0.2.0 while this crate pinned `"0.1.0"` failed +`cargo metadata` for the whole workspace. Six such pins were six standing +chances to break `main` on someone else's release, in exchange for nothing, +and they are gone. ## The earlier probes are migrated, and two of them corrected in the move @@ -756,6 +776,153 @@ count from the queue's own `Observable` counters precisely so that is visible as mistaken for contention: the sixteen- and thirty-two-producer drained rows show millions of refusals and should be read as measurements of the consumer. +## `probe-core-affinity`: placement costs 5.6x, and it refuted the hypothesis it was written to test + +This probe pins an SPSC producer and consumer to chosen logical processors and measures the handoff +under each placement the machine can express. It exists because +[`probe-peer-index-cache`](#probe-peer-index-cache-a-result-that-inverts-by-host-which-is-why-it-is-kept) +gave opposite answers on two hosts, and the obvious suspect was *placement*: a machine with two +efficiency classes might be decoupling the two threads in a way a homogeneous one does not. + +**The plain answer, which is the useful one.** On the ARM64 development host the unoptimised handoff +costs **38.5 ns/item within a domain and 215.3 ns/item across domains -- 5.6x, for no change but where +the two threads run.** Within a class, the performance cores (class 1) run the same handoff at 30.4 ns +against the efficiency cores' 38.7, about 27% apart, which is a real but far smaller effect than +crossing the boundary. Medians of three, stable across three invocations. + +**The hypothesis was refuted, and backwards.** The prediction was that mismatched core speeds would +decouple the two sides, letting a backlog form and giving peer-index caching the deep batch it needs. +Measured, threads placed *together* batch **~135x deeper** than threads placed apart (49.6 against 0.4 +items per shared read). A coherent reading is that a cheap handoff lets the producer race ahead and +build a backlog while an expensive one throttles it into lockstep -- so cost drives depth rather than +core speed driving it -- but **this run does not test that**, and the probe says so rather than +recording a replacement conclusion it did not earn. What is established is only that the original +prediction is wrong. + +**It also failed to explain the host disagreement, which was its main purpose.** Caching wins at *both* +placements here (14.4x together, 3.0x apart), so placement alone does not account for x64 rejecting the +technique while ARM64 accepts it. That question stays open under `D-28` and M-inf.4. + +**A confound this machine cannot escape, stated because it bounds every reading above.** Its efficiency +classes and its cache domains coincide exactly -- processors 0-5 are class 0 behind one L2, 6-11 are +class 1 behind the other -- so every cross-class pair is also a cross-cache pair. The 5.6x is +"across domains", and attributing it to core speed *or* to cache would need a machine whose classes and +caches cut differently. The probe detects this and prints a CAUTION rather than letting a reader draw +the finer conclusion; on such a host several of its placement rows come back `n/a`, and reporting +a placement as +inexpressible is deliberately not the same as reporting that it made no difference. + +Two construction notes. **Pinning failures panic** rather than warn: a silently unpinned thread turns a +placement experiment into a measurement of the scheduler's preferences while still printing a confident +number. And **batch depth is read from the cached runs only** -- the baseline strategy reads the shared +line on every operation by definition, so its depth is ~1 at every placement and carries no +information. An earlier revision compared the baseline depths and duly reported 0.8 against 0.4, which +is noise around a constant being read as a finding. + +## `probe-peer-index-cache`: a result that inverts by host, which is why it is kept + +This probe measures peer-index caching -- each side of an SPSC ring keeping a plain copy of the other +side's position, so the shared line is read once per batch instead of once per item -- against the +`windows-waitable-queues` `spsc` shape. **It gives opposite answers on our two architectures**: roughly +1.8x slower on x64, roughly 17x faster on ARM64, because the batch depth it amortises over is set by how +the two threads interleave on that host rather than by our code. The full reasoning lives with the queue +as [DESIGN-NOTES.md](../windows-waitable-queues/DESIGN-NOTES.md) -> `D-28`. + +This section previously described the probe as recording a settled rejection, on x64 evidence alone. + +Three things about its construction are deliberate and worth keeping if it is ever edited. + +**It counts shared reads, not just time.** A timing-only result would have been unreadable: "caching is +slower" is indistinguishable from "the caching was implemented wrongly and never engaged". The read +counters settle that directly, and they are also what made the two hosts comparable -- the reads reveal +a batch depth near 1 on x64 against roughly 150 on ARM64, which is the mechanism rather than the +symptom. Any future variant added here must keep the counters for the same reason. + +**Its interpretation is derived from the run, and must never go back to prose.** It used to print the +x64 conclusion as a fixed paragraph -- "the technique WORKED and still lost", "roughly 3.6x", "the +producer count goes UP" -- with only the speedup ratio computed. Run on ARM64 it printed all three while +its own table three lines above showed the opposite, and the contradiction was noticed by a reader +rather than by the tool. A probe that states its finding regardless of what it measured is worse than no +probe, because it is believed. The interpretation now computes the batch depths and says outright that +this verdict is host-dependent. + +**It carries a calibration row and a warming control.** The calibration times the real shipping `spsc` +beside the model, and the probe prints a CAUTION when they diverge by more than 25% -- which they +currently do, so the probe says out loud that its rows describe the model rather than the shipped +queue. That guard earned its place immediately: the first run's 3x gap would otherwise have been read +straight past. The warming variant is a control for the hypothesis that a discarded prefetch could +substitute for the real thing; it removes no read and moves no time, which is exactly what a control +that confirms the null should do. + +Like `probe-queue-contention`, this probe is **absent from the CI probe job**, and for the same +measured reason: the effects it studies are coherence effects that a debug build's overhead buries. + +## The fingerprint carries provenance inside the string, not beside it + +The fingerprint is a **canonical summary of a machine's marginal shape**: two hosts rendering the +same string have the same processor, core, cache-domain, class and node sizes, so string equality +is a supported way to group results by shape. (It does *not* mean the two can express the same +placements -- the sizes are recorded without how the partitions intersect. See +[`Fingerprint::provenance`](../windows-placement-probe/src/fingerprint.rs).) That the string is +compared at all is what forces the provenance marker to live *inside* the rendered form. A marker kept alongside -- a separate field, a +second printed line, a note in the surrounding prose -- would leave a fabricated machine claiming the +exact shape of a real one **comparing equal to it**. That is a concrete bug rather than a display +preference, and it has a test named for it. + +Three details are deliberate: + +- **A measured host renders exactly as before, with no prefix.** Every fingerprint already recorded in + a checklist or design note came from a real machine, so those strings stay valid and comparable + rather than being silently reinterpreted by this change. +- **The prefix leads**, so a reader scanning a column of pasted results cannot skip it, and it is + removable -- stripping `!!SYNTHETIC!! ` yields exactly the measured rendering, so a synthetic host + can still be compared against a real one on purpose. +- **`RESTORED` and `SYNTHETIC` are distinguished** rather than collapsed into one "untrusted". They + are different claims: one describes some real machine, the other describes none, and a reader + deciding how far to believe a number needs to know which. + +`Fingerprint::from_topology` exists so provenance *flows* from the topology rather than being stamped +on afterwards. `discover` is now a thin wrapper over it, which means there is no path that invents an +answer -- whatever the topology says is what the fingerprint reports. + +`print_banner` was split so the line is available as a string. The taint marker reaching that line is +the entire point of carrying provenance, and a property that load-bearing should not rest on someone +having read a format string correctly. + +## Which seams are safe: data may be injected, labels may not reach hardware + +Two topology-injection seams were considered during this work and they were decided opposite ways. +The rule that separates them is worth stating on its own, because "add a seam for testability" reads +as unambiguously good and here it is only half true: + +**A seam that only moves data is safe. A seam that lets fabricated labels reach real hardware is +not.** + +- [`places_from_topology`](../windows-placement-probe/src/fingerprint.rs) **has** a seam. It is a pure conversion -- topology in, + processor positions out, nothing pinned and nothing timed. A synthetic topology yields synthetic + positions, which is what the caller asked for and cannot be mistaken for a measurement. +- [`measure`](../windows-placement-probe/src/core_affinity.rs) **must not**, and its documentation says so at the definition. + A synthetic topology's processor *numbers* are still valid on the real host, so every pin would + succeed and the run would produce genuine timings filed under fabricated node ids -- output + indistinguishable from a real NUMA measurement that measured no such thing. The pin assertion does + not catch it: it rejects a processor that does not exist, not a label that is wrong. + +The absence of the second seam is also what lets `Slice` carry no provenance marker of its own, so +the two decisions hold each other up. + +### The hole this closed, and how it was proven + +`discover_places` took no argument and appeared in no test. It was untestable, not merely untested, +and it carries the rules for the partitioning cache level, core and class membership, and the NUMA +node. The NUMA lookup in particular was **unverifiable on every host available to this workspace**: +with a single node, a correct map and a completely broken one both yield node 0. + +Replacing the entire lookup with a hardcoded `0` was run against the suite as it stood before this +change. **It passed everything.** Against the suite now, three tests fail. That is the difference the +seam bought, and it is why the existing `ProcessorPlace` fixtures were kept rather than treated as +sufficient: they encode what a test author assumed the conversion produces, which is precisely the +thing that cannot catch the conversion being wrong. + ## The claim word's width costs 1.1x to 3.8x in isolation, and the drained figure is withdrawn Measured by `probe-queue-contention` on one host, `x86_64-pc-windows-msvc`. @@ -1294,40 +1461,24 @@ and a `Drop` that writes can panic while unwinding, which aborts and replaces a diagnosable failure with one that explains nothing. An unterminated fragment is not a finding, so the trade is one-sided. -Every probe in this crate needed only the signature change, because each already -went through `emit_report` rather than composing a `String` and calling `emit` -itself. That is worth stating because it was not free: it is what the one-sink -refactor bought, and it is why converting thirteen probes to stream is a -mechanical change to one function plus one line per renderer. - -Three further probes are being developed on a branch and do **not** hold that -property -- they compose a `String` and call `emit` directly, so the crate's -"every probe routes through this" claim is false for them. They are converted -where they land rather than here, since they do not exist in this crate yet. - -**Verifying that no report changed needed a control, because several of these -probes are not deterministic.** Comparing before and after directly showed four -of the thirteen reports differing -- which proves nothing on its own, since +Two probes needed more than a signature change, because they were composing a +`String` and calling `emit` directly rather than going through `emit_report` at +all: `core_affinity` and `peer_index_cache`. They are the probes this change +adds, and they had never been through the round that fixed the same bypass in +the probes already here -- the crate's "every probe routes through this" claim +was false for both until now. `core_affinity` also measured in +`main`'s argument list, so a failure to read the topology produced no banner and +no indication of which probe had died; it now measures inside the renderer, +after the banner, and reports a failed read as a failure to observe rather than +as a finding. + +**Verifying that no report changed needed a control, because most of these +probes are not deterministic.** Comparing before and after directly showed +differences in most reports -- which proves nothing on its own, since these probes print measured nanoseconds and render verdicts branching on them. - -Running the *same* build twice is the control, and it differs in **five**, by -the same amount or more in every case: - -| probe | lines differing, same build twice | lines differing, across the change | -|---|---|---| -| `probe-doorbell-cost` | 34 | 30 | -| `probe-request-cost` | 32 | 32 | -| `probe-pool-growth` | 14 | 14 | -| `probe-device-map` | 4 | 4 | -| `probe-cancel-io` | 2 | **0** | - -`probe-cancel-io` is the one that makes the point sharpest: it is *not* -deterministic, yet it happened to match across the change. Had the before/after -diff been read on its own, that would have counted as evidence of no change -- -from a probe whose output varies run to run regardless. The eight reports the -control showed to be genuinely deterministic were byte-identical across the -conversion, and those are the eight that carry the argument. - +Running the *same* build twice showed differences of the same size or larger +(`peer-index-cache` 22 lines between two runs of one build, against 20 across +the conversion). The reports that are deterministic were structurally identical. A before/after diff on a probe is not evidence without that control. ### Measured: an interrupted probe keeps what it had already measured @@ -1336,8 +1487,14 @@ M1.3 asked for this to be measured once rather than assumed, because it is the property the whole milestone exists for and no unit test reaches it -- a test cannot terminate its own process without taking the harness with it. -`probe-doorbell-cost` is the longest-running probe in this crate at about 0.8 -seconds, which makes it the subject. Started with stdout redirected, left for +`probe-doorbell-cost` runs for about 0.8 seconds, which makes it the subject. +(It was **the** longest-running probe when this was measured, on a crate that +did not yet have `probe-queue-contention`'s ~65 seconds. The merge that brought +the branch-local probes back invalidated the superlative, not the measurement: +the numbers below are unchanged and were taken against the shorter probe, which +is the harder case -- a 300 ms window against 800 ms leaves far less room for a +slow start to masquerade as buffering than it would against 65 seconds.) +Started with stdout redirected, left for 300 milliseconds, then terminated -- **six runs of each build**, with every run confirmed to have still been alive at the moment it was killed, since a probe that had already exited would be measuring nothing: @@ -1955,3 +2112,156 @@ The work this implies was M3, which is complete and archived in CHECKLIST.md" until a review pointed out that the canonical design note was advertising landed work as pending.) The session that produced it is [design-sessions/DESIGN-SESSION-2026-09-12-what-the-oracle-should-read.md](design-sessions/DESIGN-SESSION-2026-09-12-what-the-oracle-should-read.md). + +## Decisions carried in from the deferred-namespace-ops branch + +These were recorded on `mikegrier/deferred-namespace-ops` while the probe work ran in parallel on +`main`, and the merge brought them across because `main` has no copy of them: M2.5 and M2.9 are still +OPEN items there, and M2.7 was archived on the branch alone. + +**The branch's other design sections were deliberately NOT carried in.** Its oracle-era notes -- the +oracle's refusal to know, the M2.4 correspondence matrix, the binding measurement -- already exist on +`main` in [DESIGN-RATIONALE.md](DESIGN-RATIONALE.md), where M3 relocated them as Tier 2 history. +Re-inserting them here would have put one decision in two tiers at once, which is the restatement +drift this component spends its review rounds on. + +Read them against M3: the row is the machine contract and the prose is reviewed rather than parsed, +so where one of these notes says "correspondence between two renderings", the surviving form is an +invariant over the observation or a rule in the row schema. +### M2.9: two renderings that must match come from a common source + +The M2.4 finding was that both cost probes rendered every measured figure twice +with nothing comparing the two. The obvious response was a third oracle rule +set. **The decision was that a fact rendered twice must be derived once**, so +both renderings now walk the same `Observation::timings` and a `json_key` +function decides only what the machine-readable one calls each entry. + +That is strictly stronger than an oracle rule and it is cheaper. An oracle finds +a contradiction that already exists; deriving both from one value means there is +none to find. It also **deleted** code -- ten hand-named NDJSON fields and a +`get` closure went, because naming each figure separately was exactly what made +the two renderings independent restatements. + +`json_key` panics on a label it does not know, which is the whole safety of the +scheme: a figure added to `measure` reaches both renderings or fails loudly, and +cannot reach one only. Verified by adding an unnamed timing -- the probe printed +its prose row and then died naming the missing key. (That the row appeared +before the panic is M1.2's streaming; the two milestones compose.) + +**What is left to test is narrow, and that is the mark of the right fix.** The +derivation is structural in the source, so the only remaining question is +whether the structure survives rendering, formatting and the process boundary. +One integration test per probe runs the real binary and compares each table row +against its NDJSON field, reusing the crate's own `json_key` rather than +restating the pairing -- a test carrying its own copy would be checking the +copy, which is the defect this milestone is about. + +Its emptiness guard fired on the first run: `request_cost`'s table has ratio +columns after the figure, so a parser requiring exactly two tokens matched +nothing and the test would have passed having compared zero rows. That is the +third time in M1-M2 that a check written to prevent a vacuous pass caught one +immediately. + +#### Why the topology report is not converted too + +Its prose and NDJSON are still written separately, guarded by the M2.1 oracle. +That is a real inconsistency and it is deliberate rather than overlooked: the +topology renderer's two sides are not one list rendered twice but many +individually-formatted claims, several with prose that has no NDJSON counterpart +and vice versa, so a common source is a much larger change than a `json_key` +map. The oracle covers it today and the eight cells M2.4 promoted are what make +that coverage real. Converting it is a decision available later, not a gap left +by accident. + + +### M2.5: the banner is built from the read the body describes + +A `probe-topology` run makes **three** independent discoveries of the machine -- +one before, `measure`'s own, and one after -- and the banner naming the host was +built from an *endpoint*. `attribution` compared only those two endpoints, so +when they agreed it printed their fingerprint unqualified, with nothing having +established that the middle read agreed with either. The line naming the machine +could therefore describe a different topology from the body beneath it, and the +report would say so nowhere. + +**The uncovered window is narrow, and stating it exactly is the point.** +`measure` already brackets counter reads around its own discovery, so a +processor, group or NUMA change during the middle read is caught as +`BracketOutcome::Changed`. What no counter reaches is cache and +efficiency-class structure. The reachable case is a run whose cache structure +differs between the endpoints and the middle read while processor, group and +NUMA counts stay identical -- near-impossible on real hardware, since caches do +not change without processors changing, and entirely reachable on a hypervisor +returning inconsistent `GetLogicalProcessorInformationEx` results, which is +precisely the population this probe exists to survey. + +**Construction, not a third comparison** -- the same choice M2.9 made, for the +same reason. A third comparison would be new prose able to drift from what it +compares; a banner built from the body's own read cannot disagree with it, +because there is no second value to disagree. `measure_observed` is a sibling of +`measure` returning the observation *and* `Fingerprint::from_topology` of the +very topology it parsed, so `measure`'s six existing callers are untouched. + +The endpoint reads keep their job rather than being deleted: they bracket a +**wider** window than `measure`'s counter bracket, which spans only its own +discovery, so they still detect structural change the counters cannot see. They +simply no longer supply the banner. + +**The fix nearly reintroduced the defect it removes.** The first attempt +formatted `host: {fingerprint}` inline -- a second copy of a line whose owning +function documents, in the crate that owns it, that a probe's banner is +comparable with every other probe's only while exactly one place produces it. It +now routes through `banner_line_for`, wrapping in `Ok` to do so. + +Measured, not read: sabotaging the banner back to the endpoint turns exactly one +test red. The real-host test does **not** catch that sabotage -- on a stable host +all three fingerprints are equal -- and its comment now says so. What it does +catch is the seam construction leaves open: `Fingerprint::from_topology` and +`observe` are two derivations from that one topology, each with its own filter +for which processors count, and they have already disagreed once, when +`from_topology` summed core-domain membership and printed `0p` for a machine +about to be measured on four processors. + +### M2.7: probe steps are gated on the build, not on the job + +All twelve probe steps in [ci.yml](../../.github/workflows/ci.yml) now carry +`if: "!cancelled() && steps.build.outcome == 'success'"`, against three before. + +**The item posed this as a trade and it turned out not to be one.** Its argument +for guarding was already settled -- a probe step exists to emit diagnostics, so +Actions' default `if: success()` suppresses it in exactly the run that wanted it, +and the long-path pair is the sharpest case since either half alone "says +nothing". What kept it queued was the cost: a plain `!cancelled()` also runs the +step when the *build* failed, where `cargo run` cannot compile, turning a skipped +grey step into a failed red one. + +Gating on the build takes both halves. A failing test still emits its +diagnostics; a broken build still goes quiet. The trade the first three steps +accepted is no longer necessary, so the decision the item reserved for an +engineer was answered by removing the thing being traded rather than by choosing +a side. + +**Two defects found while implementing it, both by verification rather than by +reading.** + +The first: `id: build` was added to the workspace build step, which lives in job +`build-test`, while every probe runs in `platform-probes`. `steps.build` does not +cross a job boundary, so the expression would have evaluated against an empty +context, made the condition permanently false, and **silently skipped all twelve +probes** -- a guard that reads as more careful while disabling everything it +guards. The probes job had no build step at all (its first step is `cargo test`, +which builds implicitly but whose outcome cannot separate "did not compile" from +"a test failed"), so one was added there. + +The second was pre-existing and unrelated: a conflict resolution in merge +`1abcaaf` had welded a step's `if:` and `run:` onto one line, which is not valid +YAML. It survived the merge and the repository's own workflow gate, which checks +references by regex without parsing the document. The welded step is fixed here. +**The gate's own gap is recorded and not yet queued**: giving it an owner means +deciding where a workflow-parsing check belongs, which is a repository-level call +rather than this crate's, and no checklist item exists for it. Naming the absence +is the point -- a design note cannot schedule work, so an unqueued gap has to be +visible as unqueued rather than described as though something will pick it up. + +Both are the same lesson this milestone keeps producing: the failure mode of a +check is to pass. diff --git a/crates/windows-platform-probes/README.md b/crates/windows-platform-probes/README.md index 0f7caba1d..bd93f7d48 100644 --- a/crates/windows-platform-probes/README.md +++ b/crates/windows-platform-probes/README.md @@ -149,3 +149,20 @@ once set at process scope -- irreversible, so no test performs it. its source, with the control that makes that attributable; that closing a duplicate leaves the source usable; and that single-shot metadata queries do not disturb an enumeration in progress, on the handle or on a duplicate. + +**Queue claim contention, and the claim word's layout.** How `slotwise_mpsc`, +`reserving_mpsc`, and the experimental permit claim scale as producers are +added, against an uncontended `fetch_add` floor, in two regimes: producers alone +so the compare-and-swap is the only thing happening, and producers against a +continuously draining consumer. Reports each run's refusal count, so a run that +was bounded by the consumer rather than by the claim is visible as a fact rather +than mistaken for contention. + +Also measures the four apportionments of `reserving_mpsc`'s claim word -- +32/32, 16/48, 8/56, and the 128-bit 64/64 -- which is what established that +re-apportioning the bits is free while widening the word is not. That decided +how the layouts ship. The probe instantiates the shipping type at each layout +rather than a stand-in, and the reason is recorded in +[DESIGN-NOTES.md](DESIGN-NOTES.md): an earlier version carried its own copy of +the protocol and was found to be *understating* the cost of the layout it +existed to evaluate. diff --git a/crates/windows-platform-probes/src/bin/core_affinity.rs b/crates/windows-platform-probes/src/bin/core_affinity.rs new file mode 100644 index 000000000..ef232349f --- /dev/null +++ b/crates/windows-platform-probes/src/bin/core_affinity.rs @@ -0,0 +1,790 @@ +// Copyright (c) Mike Grier. + +//! Prints whether it matters where the two ends of a queue run. + +use windows_placement_probe::core_affinity::{Observation, Placement, measure}; +use windows_placement_probe::peer_index_cache::Strategy; +use windows_platform_probes::report::emit_report; +use windows_topology_sys::Observed; + +fn main() { + // The probe's whole output policy, and it is one line: hand the renderer to + // the sink. Nothing here or below names a stream -- that is chosen once, in + // `report`, so retargeting a probe is not a rewrite. + emit_report(render); +} + +/// The probe's whole report, as text. +fn render(out: &mut dyn std::fmt::Write) { + // First line of the report, and part of the returned text rather than + // written out here: a captured report must carry the line naming the + // machine that produced it, and the taint marker with it. + let _ = writeln!( + out, + "{}", + windows_placement_probe::fingerprint::banner_line() + ); + let _ = writeln!( + out, + "== does it matter where the two ends of a queue run? ==\n" + ); + + // Measured HERE, after the banner and heading are already out, rather than + // in `main`'s argument list where it used to sit. A measurement called + // before the renderer is entered is outside the sink entirely, so a host + // where it fails gives a reader no banner and no indication of which probe + // died. `measure` reads the topology and can fail, which is exactly the + // case worth naming. + let observation = &match measure() { + Ok(observation) => observation, + Err(error) => { + let _ = writeln!(out, "could not read this machine's topology: {error}"); + let _ = writeln!( + out, + "\nNothing below could be measured, so nothing below is reported. This is\n\ + a failure to observe the host, not a finding about it." + ); + return; + } + }; + + let _ = writeln!(out, "processors, as discovered:"); + let _ = writeln!( + out, + " {:>8} {:>16} {:>13}", + "cpu", "efficiency class", "cache domain" + ); + for place in &observation.processors { + // Group and number together: a number is unique only within its group, + // so two distinct processors on a machine with more than 64 of them + // would otherwise both render as `cpu5`. + let _ = writeln!( + out, + " {:>8} {:>16} {:>13}", + format!("g{}/cpu{}", place.group, place.number), + place.efficiency_class, + match place.cache_domain { + Observed::Known(id) => id.to_string(), + Observed::Absent => "none".to_owned(), + Observed::NotObserved => "unknown".to_owned(), + } + ); + } + + let classes: Vec = { + let mut seen: Vec = observation + .processors + .iter() + .map(|p| p.efficiency_class) + .collect(); + seen.sort_unstable(); + seen.dedup(); + seen + }; + // Count only domains that were actually reported, and keep the two reasons a + // processor has none apart. `Absent` means the topology reported no cache + // level that partitions this machine; `NotObserved` means this processor was + // left out of the level that does. Reporting both as "reported none" states + // a negative about a machine where the truth is that nothing was observed -- + // the same conflation of missing data with a finding that this probe's + // placement classification keeps separate, and that the caution below is + // built around. + let (known_domains, absent, unobserved) = { + let mut seen: Vec = Vec::new(); + let mut absent = 0usize; + let mut unobserved = 0usize; + for processor in &observation.processors { + match processor.cache_domain { + Observed::Known(id) => seen.push(id), + Observed::Absent => absent += 1, + Observed::NotObserved => unobserved += 1, + } + } + seen.sort_unstable(); + seen.dedup(); + (seen.len(), absent, unobserved) + }; + let mut caveats: Vec = Vec::new(); + if absent > 0 { + caveats.push(format!("{absent} in no reported cache domain")); + } + if unobserved > 0 { + caveats.push(format!("{unobserved} not observed at that level")); + } + let _ = writeln!( + out, + "\n {} efficiency class(es), {} reported cache domain(s){}", + classes.len(), + known_domains, + if caveats.is_empty() { + String::new() + } else { + format!(" ({})", caveats.join(", ")) + } + ); + + if !observation.by_class.is_empty() { + let _ = writeln!( + out, + "\n-- the same handoff, within each efficiency class --" + ); + let _ = writeln!( + out, + "{:<12} {:>8} {:>8} {:>12} {:>12} {:>10}", + "class", "prod", "cons", "base ns/it", "cached ns/it", "cach depth" + ); + let mut classes: Vec = observation + .by_class + .iter() + .map(|m| m.producer.efficiency_class) + .collect(); + classes.sort_unstable(); + classes.dedup(); + for class in classes { + let base = observation + .by_class + .iter() + .find(|m| m.producer.efficiency_class == class && m.strategy == Strategy::Baseline); + let cached = observation + .by_class + .iter() + .find(|m| m.producer.efficiency_class == class && m.strategy == Strategy::Cached); + if let (Some(base), Some(cached)) = (base, cached) { + let _ = writeln!( + out, + "{:<12} {:>8} {:>8} {:>12.1} {:>12.1} {:>10.1}", + format!("class {class}"), + format!("g{}/cpu{}", base.producer.group, base.producer.number), + format!("g{}/cpu{}", base.consumer.group, base.consumer.number), + base.nanos_per_item, + cached.nanos_per_item, + cached.consumer_batch + ); + } + } + let _ = writeln!( + out, + " (Windows numbers efficiency classes with the FASTER cores higher, so\n \ + the highest class here is the performance one.)" + ); + } + + let _ = writeln!(out, "\n-- the handoff, by placement --"); + let _ = writeln!( + out, + "{:<26} {:>8} {:>8} {:>12} {:>12} {:>10} {:>10}", + "placement", "prod", "cons", "base ns/it", "cached ns/it", "base depth", "cach depth" + ); + + // Every variant, tightest coupling first. `SameCoreSiblings` MUST be here: + // it is the placement the caching hypothesis is about, and on an SMT host it + // is where the interesting result lives. Omitting it once already produced a + // table that disagreed with the interpretation printed directly beneath it. + // + // The two `UnknownCache*` variants MUST be here for the same reason, and + // were omitted in exactly the way this comment warns about. They are real + // measurements -- a handoff timed between two named processors -- on a host + // whose topology did not report the partitioning level. Leaving them out + // drops measured rows from the table silently. + let all = [ + Placement::SameCoreSiblings, + Placement::SameCacheSameClass, + Placement::SameCacheCrossClass, + Placement::CrossCacheSameClass, + Placement::CrossCacheCrossClass, + Placement::CrossNumaNode, + Placement::UnknownCacheSameClass, + Placement::UnknownCacheCrossClass, + ]; + + for placement in all { + let (Some(base), Some(cached)) = ( + observation.get(placement, Strategy::Baseline), + observation.get(placement, Strategy::Cached), + ) else { + // Absent is a finding, not a gap: it means this machine cannot + // express the placement at all. + let _ = writeln!( + out, + "{:<26} {:>8} {:>8} {:>12} {:>12} {:>10} {:>10}", + placement.label(), + "-", + "-", + "n/a", + "n/a", + "-", + "-" + ); + continue; + }; + let _ = writeln!( + out, + "{:<26} {:>8} {:>8} {:>12.1} {:>12.1} {:>10.1} {:>10.1}", + placement.label(), + format!("g{}/cpu{}", base.producer.group, base.producer.number), + format!("g{}/cpu{}", base.consumer.group, base.consumer.number), + base.nanos_per_item, + cached.nanos_per_item, + base.consumer_batch, + cached.consumer_batch + ); + } + + let _ = writeln!(out, "\nthe slice each row was measured on:"); + for placement in all { + if let Some(base) = observation.get(placement, Strategy::Baseline) { + let _ = writeln!(out, " {:<26} {}", placement.label(), base.slice); + } + } + + render_node_distances(out, observation); + + let _ = writeln!( + out, + " +interpretation: +" + ); + + let expressible = observation.placements(); + if expressible.len() < 2 { + let _ = writeln!( + out, + " This machine expresses only one placement, so it cannot answer" + ); + let _ = writeln!( + out, + " the question. That is a fact about the host, not a null result:" + ); + let _ = writeln!( + out, + " a homogeneous single-cache machine has nowhere else to put the" + ); + let _ = writeln!(out, " two threads."); + return; + } + + // Whether the two factors can be told apart at all on this host. If every + // cross-class pair is also cross-cache, they are perfectly confounded and + // no amount of measurement here separates them -- which is a fact to state, + // not to reason past. + // + // **Confounding is a property of structure that EXISTS, never of structure + // that is missing, and testing it by inexpressibility alone got that + // backwards twice.** Both bugs printed the same sentence -- classes and + // cache domains "coincide exactly" -- on a host that had established no + // cache structure at all, and they arrived by different routes: + // + // - Topology omitted a processor from the partitioning level, so every pair + // became `UnknownCache*`. `cache_unobserved` covers that one. + // - Topology reported no partitioning cache at all, so `Fingerprint` marked + // every processor `Observed::Absent`. `Absent == Absent` compares equal, + // so those pairs classify as `SameCacheSameClass` and NO `Unknown*` + // variant appears -- `cache_unobserved` is false and cannot see it. On a + // homogeneous SMT VM (one class, no cache partition) both separators are + // then vacuously inexpressible and the claim fired, directly beneath this + // same report's "0 reported cache domain(s)" line. + // + // Requiring `CrossCacheCrossClass` to be expressible is what closes both, + // and closes them for the right reason rather than by naming each route: the + // sentence is about a pair that is cross-class AND cross-cache, so unless + // the host can express one, there is nothing for the two factors to be + // confounded IN. A vacuous truth is not a measurement result. + let cache_unobserved = expressible.contains(&Placement::UnknownCacheSameClass) + || expressible.contains(&Placement::UnknownCacheCrossClass); + let confounded = !cache_unobserved + && expressible.contains(&Placement::CrossCacheCrossClass) + && !expressible.contains(&Placement::SameCacheCrossClass) + && !expressible.contains(&Placement::CrossCacheSameClass); + if cache_unobserved { + let _ = writeln!( + out, + " CAUTION: this machine's topology did not report the level that" + ); + let _ = writeln!( + out, + " partitions it, so some pairs above carry no cache relationship." + ); + let _ = writeln!( + out, + " Their rows are labelled 'unknown cache' rather than filed under a" + ); + let _ = writeln!( + out, + " relationship nobody established. The class comparison below still" + ); + let _ = writeln!( + out, + " holds -- the efficiency class was observed -- but nothing here" + ); + let _ = writeln!(out, " says what the cache did."); + } + if confounded { + let _ = writeln!( + out, + " CAUTION: on this machine the efficiency classes and the cache" + ); + let _ = writeln!( + out, + " domains coincide exactly, so every cross-class pair is also a" + ); + let _ = writeln!( + out, + " cross-cache pair. The two effects are perfectly CONFOUNDED here" + ); + let _ = writeln!( + out, + " and nothing below separates them. Read the rows as 'within a" + ); + let _ = writeln!( + out, + " domain' versus 'across domains', and do not attribute the" + ); + let _ = writeln!( + out, + " difference to core speed or to cache without a machine whose" + ); + let _ = writeln!(out, " classes and caches cut differently.\n"); + } + + // Batch depth is read from the CACHED runs, never the baseline ones. + // Baseline reads the shared line on every operation by definition, so its + // depth is ~1 whatever the placement and carries no information at all. An + // earlier version of this probe compared the baseline depths and duly + // reported ~0.8 against ~0.4, which is noise around a constant being read + // as a finding. + let same_class: Vec<_> = expressible + .iter() + .filter(|p| { + matches!( + p, + Placement::SameCacheSameClass | Placement::CrossCacheSameClass + ) + }) + .filter_map(|p| observation.get(*p, Strategy::Cached)) + .collect(); + let cross_class: Vec<_> = expressible + .iter() + .filter(|p| { + matches!( + p, + Placement::SameCacheCrossClass | Placement::CrossCacheCrossClass + ) + }) + .filter_map(|p| observation.get(*p, Strategy::Cached)) + .collect(); + + let mean = |runs: &[_], f: fn(&_) -> f64| -> Option { + if runs.is_empty() { + None + } else { + Some(runs.iter().map(f).sum::() / runs.len() as f64) + } + }; + + if let (Some(same), Some(cross)) = ( + mean( + &same_class, + |m: &windows_placement_probe::core_affinity::Measurement| m.consumer_batch, + ), + mean( + &cross_class, + |m: &windows_placement_probe::core_affinity::Measurement| m.consumer_batch, + ), + ) { + let within = if confounded { + "within a domain " + } else { + "same-class " + }; + let across = if confounded { + "across domains " + } else { + "cross-class " + }; + let _ = writeln!( + out, + " batch depth with caching on, {within}: {same:.1} items per shared read" + ); + let _ = writeln!( + out, + " batch depth with caching on, {across}: {cross:.1} items per shared read" + ); + if cross > same * 2.0 { + let _ = writeln!( + out, + "\n SEPARATION DEEPENS THE BATCH. The two sides decouple: one runs" + ); + let _ = writeln!( + out, + " ahead, a real backlog forms, and each shared read is amortised" + ); + let _ = writeln!( + out, + " over it. That is the condition peer-index caching needs, and it" + ); + let _ = writeln!( + out, + " is a property of PLACEMENT -- not of the architecture." + ); + } else if same > cross * 2.0 { + let _ = writeln!( + out, + "\n THE HYPOTHESIS IS REFUTED, AND BACKWARDS. Threads placed" + ); + let _ = writeln!( + out, + " TOGETHER batch {:.0}x deeper than threads placed apart, where the", + same / cross.max(0.001) + ); + let _ = writeln!( + out, + " prediction was the reverse -- that mismatched cores would" + ); + let _ = writeln!(out, " decouple and batch deeply."); + let _ = writeln!( + out, + " A coherent reading: a cheap handoff lets the producer race ahead" + ); + let _ = writeln!( + out, + " and build a backlog, while an expensive one throttles it into" + ); + let _ = writeln!( + out, + " lockstep, so each side arrives to find exactly one item. Cost" + ); + let _ = writeln!( + out, + " drives depth, rather than depth being set by core speed." + ); + let _ = writeln!( + out, + " That is a hypothesis this run does not test, and it must not be" + ); + let _ = writeln!( + out, + " recorded as a finding -- what IS established is that the" + ); + let _ = writeln!(out, " original prediction is wrong."); + } else { + let _ = writeln!( + out, + "\n Placement does NOT move batch depth here ({:.2}x).", + cross / same + ); + let _ = writeln!( + out, + " The hypothesis that unequal core speeds drive the batching is" + ); + let _ = writeln!( + out, + " not supported, and the difference between hosts needs another" + ); + let _ = writeln!( + out, + " explanation. Recording a refutation is the point of running it." + ); + } + } + + // The plainest answer to "does placement matter", independent of caching. + // "Near" falls back to SMT siblings, because a host whose outermost + // partitioning cache is per-core has no same-cache-different-core pair at + // all -- its nearest expressible placement IS the sibling pair. + if let (Some(near), Some(far)) = ( + observation + .get(Placement::SameCacheSameClass, Strategy::Baseline) + .or_else(|| observation.get(Placement::SameCoreSiblings, Strategy::Baseline)), + observation + .get(Placement::CrossCacheCrossClass, Strategy::Baseline) + .or_else(|| observation.get(Placement::CrossCacheSameClass, Strategy::Baseline)), + ) { + let _ = writeln!( + out, + "\n the unoptimised handoff costs {:.1} ns/item together and {:.1} ns/item", + near.nanos_per_item, far.nanos_per_item + ); + let _ = writeln!( + out, + " apart -- {:.1}x for crossing the boundary, with no code change.", + far.nanos_per_item / near.nanos_per_item + ); + } + + let _ = writeln!( + out, + "\n does the verdict on caching depend on placement?\n" + ); + let mut verdicts = Vec::new(); + for placement in expressible { + let (Some(base), Some(cached)) = ( + observation.get(placement, Strategy::Baseline), + observation.get(placement, Strategy::Cached), + ) else { + continue; + }; + let speedup = base.nanos_per_item / cached.nanos_per_item; + let verdict = if speedup >= 1.1 { + "caching WINS" + } else if speedup <= 0.9 { + "caching LOSES" + } else { + "no effect" + }; + let _ = writeln!( + out, + " {:<26} {:>7.2}x {verdict}", + placement.label(), + speedup + ); + verdicts.push(verdict); + } + verdicts.sort_unstable(); + verdicts.dedup(); + + // A SIGN flip, not merely disagreement. `no effect` beside `caching WINS` + // is one magnitude larger than the other, which is not a technique whose + // direction depends on placement -- and saying it is, is the failure this + // crate's `probe-peer-index-cache` note names: an instrument that states + // its finding regardless of what it measured is worse than none, because + // it is believed. Observed on a 16-processor host printing the flip over + // 1.04x and 1.31x, which share a sign. + let wins = verdicts.contains(&"caching WINS"); + let loses = verdicts.contains(&"caching LOSES"); + if wins && loses { + let _ = writeln!( + out, + "\n THE VERDICT FLIPS WITHIN ONE MACHINE. A technique whose sign" + ); + let _ = writeln!( + out, + " depends on where two threads are scheduled cannot be adopted or" + ); + let _ = writeln!( + out, + " rejected by a fixed decision. Any answer has to name the" + ); + let _ = writeln!(out, " placement it holds for."); + } else if verdicts.len() > 1 { + let _ = writeln!( + out, + "\n The verdicts differ in MAGNITUDE across placements but not in" + ); + let _ = writeln!( + out, + " sign, so placement changes how much the technique is worth here" + ); + let _ = writeln!(out, " rather than whether it helps at all."); + } else { + let _ = writeln!( + out, + "\n The verdict is the same at every placement on this host, so" + ); + let _ = writeln!( + out, + " placement alone does not explain the disagreement between hosts." + ); + } +} + +/// Print the per-node-pair handoff cost, when the host has nodes to cross. +/// +/// Silent on a single-node machine: there is nothing to say, and a header over +/// an empty table invites the reader to wonder what went wrong. +fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observation) { + let pairs = observation.node_pairs_measured(); + if pairs.is_empty() { + return; + } + + let _ = writeln!(out, "\n-- the handoff, by NUMA node pair --"); + // A ring-placement column, because a pair and a strategy no longer identify + // one row: every hop is measured once with the ring on the producer's node + // and once on the consumer's. Rendering one of them would drop half the + // measurements and, worse, could pair a baseline taken at one placement + // against a cached run taken at the other. + let _ = writeln!( + out, + "{:<14} {:>8} {:>8} {:>8} {:>12} {:>12} {:>10}", + "prod -> cons", "ring on", "prod", "cons", "base ns/it", "cached ns/it", "cach depth" + ); + // Stated rather than left as a mystery glyph. `ring on` names the node the + // run asked for, since that is what identifies the row; a `!` means the + // memory did not land there, so that row does not measure the placement it + // names. + let _ = writeln!( + out, + " (`ring on` is the node requested; `!` means it landed elsewhere)" + ); + + // Keyed on the ring placement as well as the pair, because the table above + // measures every hop TWICE -- once with the ring on the producer's node and + // once on the consumer's -- and the comment there says so. Recording only + // the pair swept the min and the max over both placements while labelling + // each with a hop alone, so the cheapest and dearest rows could be the SAME + // hop at two ring placements, and the spread between them was then reported + // as evidence that hops differ. That is a difference attributed to a factor + // that did not vary. + let mut slowest: Option<(f64, (u32, u32), Option)> = None; + let mut fastest: Option<(f64, (u32, u32), Option)> = None; + + for pair in &pairs { + for base in observation.node_pair_rows(*pair, Strategy::Baseline) { + // Matched on the ring placement as well, so the two columns + // describe the same configuration -- and on the placement that was + // *requested*, not the one that was achieved. Windows may redirect + // an allocation, so two rows can share an achieved node while + // describing different placements; keyed on that, this pairs a + // baseline taken at one placement against a cached run taken at the + // other, which is the exact error the comment above says the key + // exists to prevent. + let Some(cached) = + observation.node_pair(*pair, Strategy::Cached, base.requested_memory_node) + else { + continue; + }; + let _ = writeln!( + out, + "{:<14} {:>8} {:>8} {:>8} {:>12.1} {:>12.1} {:>10.1}", + // `->`, not `<->`: hops are directed, because the producer + // writes and the consumer reads. The probe crate's own report + // was corrected for this and this second renderer of the same + // data was not, which is how two views of one measurement drift + // apart. + format!("{} -> {}", pair.0, pair.1), + // The requested node, matching the key above and the probe + // crate's own report. A trailing `!` marks a row whose memory + // did not land where it was asked to go, so a redirected run + // is not read as a measurement of the placement it names. + match (base.requested_memory_node, base.memory_node) { + (Some(asked), Some(got)) if asked == got => format!("node {asked}"), + (Some(asked), _) => format!("node {asked}!"), + (None, _) => "unspecified".to_owned(), + }, + format!("g{}/cpu{}", base.producer.group, base.producer.number), + format!("g{}/cpu{}", base.consumer.group, base.consumer.number), + base.nanos_per_item, + cached.nanos_per_item, + cached.consumer_batch + ); + let seen = (base.nanos_per_item, *pair, base.requested_memory_node); + if slowest.is_none_or(|(worst, _, _)| seen.0 > worst) { + slowest = Some(seen); + } + if fastest.is_none_or(|(best, _, _)| seen.0 < best) { + fastest = Some(seen); + } + } + } + + // Directed pairs, so a host with two nodes yields `(0,1)` and `(1,0)` -- + // two entries for one hop measured both ways. Counting entries would make + // this guard unreachable, which it was: `pairs.len()` is 0 on a single-node + // host and even and >= 2 otherwise, so `== 1` never held and a two-node + // machine fell into the spread analysis below, where it compared a hop + // against its own reverse and reported the two as different hops. + let mut hops: Vec<(u32, u32)> = pairs + .iter() + .map(|pair| { + if pair.0 <= pair.1 { + (pair.0, pair.1) + } else { + (pair.1, pair.0) + } + }) + .collect(); + hops.sort_unstable(); + hops.dedup(); + + if hops.len() == 1 { + let _ = writeln!( + out, + "\n One node pair, so this restates the `cross NUMA node` row above\n \ + rather than adding to it. The table earns its place from three\n \ + nodes upward, where the hops stop being interchangeable." + ); + return; + } + + let (Some((worst, worst_pair, worst_ring)), Some((best, best_pair, best_ring))) = + (slowest, fastest) + else { + return; + }; + let ring = |node: Option| match node { + Some(node) => format!("ring on node {node}"), + None => "ring unspecified".to_owned(), + }; + // `hops.len()`, not `pairs.len()`. `pairs` is directed, so a three-node host + // has six entries for the three hops the guard above just counted -- and + // printing the directed count here made two adjacent paragraphs describe one + // machine with two different numbers. + let _ = writeln!( + out, + "\n {} node hop(s). Cheapest {} -> {} ({}) at {:.1} ns/item; dearest\n \ + {} -> {} ({}) at {:.1} ns/item -- a spread of {:.1}x.", + hops.len(), + best_pair.0, + best_pair.1, + ring(best_ring), + best, + worst_pair.0, + worst_pair.1, + ring(worst_ring), + worst, + worst / best + ); + // What varied between the two extremes decides what the spread is evidence + // ABOUT, and it is not always the hop. + // + // **Compared as UNDIRECTED hops, because that is the unit the guard above + // just counted.** Directed equality made `0 -> 1` and `1 -> 0` different + // hops, so a spread that came entirely from reversing one hop was reported + // as the hops not being interchangeable -- the same directed/undirected + // confusion the guard above exists to fix, reintroduced in the code that + // fixed it. + let canonical = |pair: (u32, u32)| { + if pair.0 <= pair.1 { + (pair.0, pair.1) + } else { + (pair.1, pair.0) + } + }; + if best_pair == worst_pair && best_ring == worst_ring { + // One row was both extremes, so there is no spread to describe: this is + // a single measurement, not a comparison. + let _ = writeln!( + out, + " Both extremes are the same row, so the table holds one\n \ + measurement per hop and this restates it rather than comparing." + ); + } else if canonical(best_pair) == canonical(worst_pair) { + let _ = writeln!( + out, + " Both extremes are the same hop, so that spread is within it --\n \ + between its two directions, its two ring placements, or both -- and\n \ + says nothing about how the hops compare." + ); + } else if worst / best < 1.2 { + let _ = writeln!( + out, + " That spread is small enough that this host's nodes are close to\n \ + equidistant, so the single `cross NUMA node` row above is a fair\n \ + summary of it." + ); + } else { + let _ = writeln!( + out, + " The hops are NOT interchangeable, so the single `cross NUMA node`\n \ + row above reports whichever one was enumerated first and should not\n \ + be read as 'the' cost of leaving a node." + ); + } + let _ = writeln!( + out, + " This measures the handoff between two nodes; it is not a distance\n \ + matrix read from firmware. Windows exposes no NUMA distance table, so\n \ + these numbers are the observable rather than a restatement of ACPI." + ); +} diff --git a/crates/windows-platform-probes/src/bin/doorbell_cost.rs b/crates/windows-platform-probes/src/bin/doorbell_cost.rs index 543d9e67e..b0ba13155 100644 --- a/crates/windows-platform-probes/src/bin/doorbell_cost.rs +++ b/crates/windows-platform-probes/src/bin/doorbell_cost.rs @@ -12,7 +12,11 @@ //! adequate and the more delicate protocol -- publish intent, re-check, park -- //! can wait for evidence that it is worth its lost-wakeup risk. -use windows_platform_probes::doorbell_cost::{measure, measure_park_and_wake}; +use std::fmt::Write as _; + +use windows_platform_probes::doorbell_cost::{ + EVERY_LABEL, Presence, json_key, measure, measure_park_and_wake, +}; use windows_platform_probes::report::emit_report; @@ -264,23 +268,80 @@ fn render(out: &mut dyn std::fmt::Write) { // into unparseable output for any consumer doing what this format exists // for. // - // Note the contrast with the three fields below, which correctly emit - // `null`. That is right for *them*: a parked handshake can time out and an - // `IoRing` may be unavailable, so absent is a real outcome those fields - // must be able to say. Absent is not a real outcome for these four, and - // giving them a way to say it only hid the bug. - let atomic = observation - .get("atomic_fetch_add") - .expect("measure always records atomic_fetch_add"); - let already = observation - .get("set_event_already_signalled") - .expect("measure always records set_event_already_signalled"); - let cycle = observation - .get("set_reset_event") - .expect("measure always records set_reset_event"); - let wait0 = observation - .get("wait_zero_signalled") - .expect("measure always records wait_zero_signalled"); + // Note the contrast with `park_and_wake_round_trip_ns` below, which emits + // `null` when the handshake times out, and with `submit_io_ring_empty_ns` + // in the loop, which emits `null` when this host has no `IoRing`. Absent is + // a real outcome for both, and the row's shape stays fixed across hosts so a + // mining pass can tell "measured, absent" from "field not present". Absent + // is not a real outcome for the other four, and an earlier revision that + // omitted a key entirely -- rather than saying `null` -- is what made this + // distinction worth stating. + // Built by walking the SAME labels the prose table above walked, with + // `json_key` deciding only what each entry is called here. + // + // That is the point, and it replaces four hand-written fields. Prose and + // NDJSON were previously independent restatements of one measurement -- the + // prose iterating what was measured, this line naming each figure by hand in + // a format string -- so nothing stopped them disagreeing about a value, or + // one carrying a figure the other omitted. The M2.4 matrix found exactly + // that class unchecked in this probe. + // + // Deriving both from one source makes the disagreement **unrepresentable** + // rather than detectable, which is better than any oracle rule: a rule finds + // a contradiction that already exists, and there is now none to find. A + // figure added to `measure` reaches both renderings or fails loudly -- in + // `json_key` if it is named nowhere, in the `Presence::Always` arm below if + // it is named but stopped being measured, and in the census after the loop + // if it is measured but unnamed; it cannot reach one only. + let mut fields = String::new(); + for (label, presence) in EVERY_LABEL { + let measured = observation + .timings + .iter() + .find(|timing| timing.label == label); + match (measured, presence) { + (Some(timing), _) => { + let _ = write!( + fields, + r#""{}":{:.1},"#, + json_key(timing.label), + timing.nanos_per_op + ); + } + // Absent is a real outcome for `submit_io_ring_empty`, so the key + // says so rather than vanishing. See `EVERY_LABEL`. + (None, Presence::WhenAvailable) => { + let _ = write!(fields, r#""{}":null,"#, json_key(label)); + } + // Absent is NOT a real outcome for the rest, and publishing `null` + // here would make a renamed timing indistinguishable from a host + // that could not run one. This is the `expect` the label-driven + // loop would otherwise have quietly dropped. + (None, Presence::Always) => { + panic!( + "`{label}` is recorded on every host, so its absence means \ + it was renamed or dropped in one place and not the other; \ + the NDJSON line must not publish `null` for it" + ); + } + } + } + + // The loop above walks `EVERY_LABEL`, not `timings`, so it renders a fixed + // set of keys -- which is the point, and also the way it can go wrong. A + // timing added to `measure` but not to `EVERY_LABEL` would be iterated by + // the prose and skipped here, silently reinstating the split this whole + // arrangement removes. So the relation is checked in the direction the loop + // cannot check itself: every measured label must be one the line emits. + for timing in &observation.timings { + assert!( + EVERY_LABEL.iter().any(|(label, _)| *label == timing.label), + "`{}` was measured but is not in `EVERY_LABEL`, so the NDJSON line \ + would omit a figure the prose reports", + timing.label + ); + } + // `doorbell_over_empty_submit`, not `doorbell_share_of_submit`. The prose // above tells a human that an empty submit is not a fair denominator and // that any figure derived from it is a confident wrong answer -- and this @@ -296,23 +357,22 @@ fn render(out: &mut dyn std::fmt::Write) { // reach -- and a reviewer duly read the method as promising a meaningful // share. A rename for precision is not finished until every name for the // quantity moves; the field and the method are one fact with two spellings. + // + // The two below stay separate because neither is a row of that table: the + // handshake is measured by a different function and can time out, and the + // ratio is derived rather than measured. `null` is a real outcome for the + // handshake, and for `submit_io_ring_empty_ns` in the loop above, which is + // conditional on this host having an `IoRing`; the other three timings + // always run and never say it. let _ = writeln!( out, concat!( - r#"{{"reason":"x-probe-doorbell-cost","arch":"{}","atomic_ns":{:.1},"#, - r#""set_event_already_signalled_ns":{:.1},"set_reset_event_ns":{:.1},"#, - r#""wait_zero_signalled_ns":{:.1},"park_and_wake_round_trip_ns":{},"#, - r#""submit_io_ring_empty_ns":{},"doorbell_over_empty_submit":{}}}"# + r#"{{"reason":"x-probe-doorbell-cost","arch":"{}",{}"#, + r#""park_and_wake_round_trip_ns":{},"doorbell_over_empty_submit":{}}}"# ), std::env::consts::ARCH, - atomic, - already, - cycle, - wait0, + fields, park.map_or("null".to_string(), |n| format!("{n:.1}")), - observation - .submit_nanos - .map_or("null".to_string(), |n| format!("{n:.1}")), observation .doorbell_over_empty_submit() .map_or("null".to_string(), |s| format!("{s:.4}")), diff --git a/crates/windows-platform-probes/src/bin/peer_index_cache.rs b/crates/windows-platform-probes/src/bin/peer_index_cache.rs new file mode 100644 index 000000000..1d91c3446 --- /dev/null +++ b/crates/windows-platform-probes/src/bin/peer_index_cache.rs @@ -0,0 +1,368 @@ +// Copyright (c) Mike Grier. + +//! Prints what caching the peer's index buys an SPSC ring. +//! +//! **An experiment, not a component.** These probes measure platform behaviour +//! and are not for production use. Do not call them from production code, and +//! do not lift a technique out of here. See this crate's DESIGN-NOTES.md. + +use windows_placement_probe::peer_index_cache::{CAPACITY, ITEMS, Strategy, measure}; +use windows_platform_probes::report::emit_report; + +fn main() { + // The probe's whole output policy, and it is one line: hand the renderer to + // the sink. Nothing here or below names a stream -- that is chosen once, in + // `report`, so retargeting a probe is not a rewrite. + emit_report(render); +} + +/// The probe's whole report, as text. +fn render(out: &mut dyn std::fmt::Write) { + // First line of the report, and part of the returned text rather than + // written out here: a captured report must carry the line naming the + // machine that produced it, and the taint marker with it. + let _ = writeln!( + out, + "{}", + windows_placement_probe::fingerprint::banner_line() + ); + let _ = writeln!( + out, + "== what does caching the peer's index buy an SPSC ring? ==\n" + ); + + let observation = measure(); + + let _ = writeln!( + out, + "{:<24} {:>10} {:>14} {:>14} {:>14}", + "configuration", "ns/item", "items/sec", "cons. reads", "prod. reads" + ); + for run in std::iter::once(&observation.calibration).chain(&observation.strategies) { + let _ = writeln!( + out, + "{:<24} {:>10.1} {:>14.0} {:>14} {:>14}", + run.label, + run.nanos_per_item, + run.items_per_second, + run.consumer_refreshes, + run.producer_refreshes + ); + } + let _ = writeln!( + out, + " + ({ITEMS} items, capacity {CAPACITY}. The two read columns count how often each + \ + side actually loaded the *other* side's position -- the shared line the + \ + technique exists to avoid touching.)" + ); + + let _ = writeln!(out, "\ninterpretation:\n"); + + let Some(baseline) = observation.get(Strategy::Baseline) else { + return; + }; + + // The model has to reproduce the shipping queue before anything it says + // about variants is worth reading. + // + // `signed` and `drift` are kept apart deliberately. The magnitude decides + // whether to caution at all; the SIGN decides what may then be said, and + // conflating them printed a conclusion backwards. The floor argument below + // reads "the shipping queue spends much more per item than this model, so + // whatever the shared read costs it is a minority of that" -- which holds + // only when the model is the FASTER of the two. Taken on `abs()` alone it + // fired just as readily when the model was slower, where the same words + // assert a floor the measurement does not support. + let signed = (baseline.nanos_per_item - observation.calibration.nanos_per_item) + / observation.calibration.nanos_per_item; + let drift = signed.abs(); + let _ = writeln!( + out, + " calibration: the model's baseline differs from the shipping spsc by + \ + {:.0}% ({:.1} vs {:.1} ns/item).", + drift * 100.0, + baseline.nanos_per_item, + observation.calibration.nanos_per_item + ); + if drift > 0.25 && signed < 0.0 { + let _ = writeln!( + out, + " CAUTION: that is a wide gap, so the rows below describe the MODEL" + ); + let _ = writeln!( + out, + " and not the shipping queue. The model has only the ring mechanics;" + ); + let _ = writeln!( + out, + " the shipping push also consults the reservation count, updates the" + ); + let _ = writeln!( + out, + " depth metric and rings the doorbell. This probe does NOT attribute" + ); + let _ = writeln!( + out, + " the gap between those, and no such attribution should be read into" + ); + // **The bound is the model's SHARE of shipping cost, not a verdict about + // it.** This said the shared read "is a minority of what the shipping + // queue spends per item, so removing it cannot be the large win", which + // does not follow from the gap being wide: the shared read sits inside + // the model baseline, so what the calibration bounds is the share the + // whole model occupies. At a 39% gap that share is 61% -- a majority, + // under a sentence asserting a minority. Correcting the SIGN in the + // previous round left the arithmetic wrong, because the sign was only + // half of what made the claim unsupported. + // + // So report the share and stop. A reader with a threshold in mind can + // apply it; this run does not have one. + let _ = writeln!( + out, + " it. What the gap does bound is a share: the shared read sits" + ); + let _ = writeln!( + out, + " inside the model's baseline, so whatever it costs, it is at most" + ); + let _ = writeln!( + out, + " {:.0}% of what the shipping queue spends per item.", + (baseline.nanos_per_item / observation.calibration.nanos_per_item) * 100.0 + ); + } else if drift > 0.25 { + let _ = writeln!( + out, + " CAUTION: that is a wide gap, so the rows below describe the MODEL" + ); + let _ = writeln!( + out, + " and not the shipping queue -- and the model is the SLOWER of the" + ); + let _ = writeln!( + out, + " two, which is the direction that carries no floor argument. The" + ); + let _ = writeln!( + out, + " model is the shipping queue with work stripped out, so costing" + ); + let _ = writeln!( + out, + " MORE per item than the thing it strips from means it is not" + ); + let _ = writeln!( + out, + " measuring what it was built to measure. Nothing below can be read" + ); + let _ = writeln!(out, " as a statement about the shipping queue."); + } else { + let _ = writeln!( + out, + " Close enough to treat the model as a stand-in for the real ring." + ); + } + + for strategy in [Strategy::Cached, Strategy::Warmed] { + let Some(run) = observation.get(strategy) else { + continue; + }; + let speedup = baseline.nanos_per_item / run.nanos_per_item; + let _ = writeln!( + out, + "\n {:<22} {:.2}x the baseline ({:.1} -> {:.1} ns/item)", + match strategy { + Strategy::Cached => "peer-index caching:", + Strategy::Warmed => "warming load only:", + Strategy::Baseline => unreachable!(), + }, + speedup, + baseline.nanos_per_item, + run.nanos_per_item + ); + } + + // Everything below is DERIVED from this run's numbers, and none of it may + // go back to being prose. + // + // It used to be a fixed paragraph concluding that the technique "WORKED and + // still lost", that consumer reads fell "roughly 3.6x", and that producer + // reads "go UP". Those were true of the x64 host it was written on. Run on + // an ARM64 host they were all three false -- caching was 17x FASTER, and + // producer reads fell by ~580x -- and the probe printed the old conclusion + // anyway, contradicting the table directly above it. An instrument that + // states its finding regardless of what it measured is worse than no + // instrument, because it is believed. + let Some(cached) = observation.get(Strategy::Cached) else { + return; + }; + + // The batch depth is the mechanism, so compute it rather than assert it: it + // is how many items each shared read is amortised over, and it is what + // decides whether trading freshness for fewer reads pays. + let consumer_batch = ITEMS as f64 / cached.consumer_refreshes.max(1) as f64; + let producer_batch = ITEMS as f64 / cached.producer_refreshes.max(1) as f64; + let consumer_reduction = + baseline.consumer_refreshes as f64 / cached.consumer_refreshes.max(1) as f64; + let producer_reduction = + baseline.producer_refreshes as f64 / cached.producer_refreshes.max(1) as f64; + let speedup = baseline.nanos_per_item / cached.nanos_per_item; + + let _ = writeln!(out); + let _ = writeln!( + out, + " how far each shared read was amortised, with caching on:" + ); + let _ = writeln!( + out, + " consumer: {consumer_batch:.1} items per read ({consumer_reduction:.1}x fewer reads than baseline)" + ); + let _ = writeln!( + out, + " producer: {producer_batch:.1} items per read ({producer_reduction:.1}x fewer reads than baseline)" + ); + let _ = writeln!(out); + + let engaged = consumer_reduction > 1.5; + if !engaged { + let _ = writeln!( + out, + " The technique did NOT engage: the consumer's shared reads barely" + ); + let _ = writeln!( + out, + " moved. Any throughput difference below is noise about something" + ); + let _ = writeln!(out, " else, and says nothing about peer-index caching."); + } else if speedup >= 1.1 { + let _ = writeln!(out, " The technique engaged AND won, by {speedup:.2}x."); + let _ = writeln!( + out, + " Peer-index caching trades freshness for fewer reads. The depths" + ); + let _ = writeln!( + out, + " above say how far each side's reads were amortised; which side's" + ); + let _ = writeln!( + out, + " depth carries the win is not separated by this measurement." + ); + } else if speedup <= 0.9 { + let _ = writeln!( + out, + " The technique engaged and still LOST, at {speedup:.2}x the baseline." + ); + let _ = writeln!( + out, + " This is a real result about the shape rather than a failed" + ); + let _ = writeln!( + out, + " implementation. Caching trades freshness for fewer reads; at the" + ); + let _ = writeln!( + out, + " batch depths above, each side idles on a stale bound it could" + ); + let _ = writeln!( + out, + " have refreshed, and that idling costs more than the reads saved." + ); + if producer_reduction < 1.0 { + let _ = writeln!( + out, + " Note the producer count went UP: a cached index is consulted" + ); + let _ = writeln!( + out, + " only when it says 'no room', so a blocked producer refreshes on" + ); + let _ = writeln!(out, " every spin and gains nothing."); + } + } else { + let _ = writeln!( + out, + " The technique engaged and changed throughput by {speedup:.2}x, which" + ); + let _ = writeln!( + out, + " is inside the noise of this probe. Treat it as no effect." + ); + } + + let _ = writeln!(out); + let _ = writeln!( + out, + " BATCH DEPTH IS THE VARIABLE, AND IT IS NOT A CONSTANT OF THE CODE." + ); + let _ = writeln!( + out, + " It depends on how the producer and consumer interleave, which" + ); + let _ = writeln!( + out, + " depends on the host: core count, whether siblings share a core," + ); + let _ = writeln!( + out, + " and how the scheduler places the two threads. The same binary has" + ); + let _ = writeln!( + out, + " measured a depth near 1 on one machine and in the hundreds on" + ); + let _ = writeln!( + out, + " another, and the verdict inverted with it. Do not carry a" + ); + let _ = writeln!( + out, + " conclusion from one host to another -- run it on the host you" + ); + let _ = writeln!(out, " intend to make the decision for."); + + let Some(warmed) = observation.get(Strategy::Warmed) else { + return; + }; + let warm_reduction = + baseline.consumer_refreshes as f64 / warmed.consumer_refreshes.max(1) as f64; + let _ = writeln!(out); + let _ = writeln!( + out, + " control (warming load): {:.2}x throughput, {:.2}x fewer consumer reads.", + baseline.nanos_per_item / warmed.nanos_per_item, + warm_reduction + ); + if warm_reduction < 1.5 { + let _ = writeln!( + out, + " It removed no shared read, which is what a control should do. A" + ); + let _ = writeln!( + out, + " discarded load cannot help: the authoritative load still happens," + ); + let _ = writeln!( + out, + " and in a tight handoff loop the prefetch has no time to land." + ); + let _ = writeln!( + out, + " So the technique works by REMOVING the load, not by warming it." + ); + } else { + let _ = writeln!( + out, + " UNEXPECTED: the control removed shared reads, so it is not acting" + ); + let _ = writeln!( + out, + " as a control. Distrust the comparison above until that is explained." + ); + } +} diff --git a/crates/windows-platform-probes/src/bin/request_cost.rs b/crates/windows-platform-probes/src/bin/request_cost.rs index 58a74b74f..d3b42bb2a 100644 --- a/crates/windows-platform-probes/src/bin/request_cost.rs +++ b/crates/windows-platform-probes/src/bin/request_cost.rs @@ -10,8 +10,10 @@ //! Read alongside `probe-doorbell-cost`: together they say whether the queue's //! mechanics or the request's allocation model deserves the attention. +use std::fmt::Write as _; + use windows_platform_probes::report::emit_report; -use windows_platform_probes::request_cost::measure; +use windows_platform_probes::request_cost::{EVERY_LABEL, json_key, measure}; /// Measured by `probe-doorbell-cost` on a **Snapdragon X2 (ARM64)** machine, /// and recorded in [the 2026-08-30 design session]. Restated here only to @@ -389,26 +391,48 @@ fn render(out: &mut dyn std::fmt::Write) { // and none of these can be. Letting them say "absent" would have produced a // partially populated record that parses cleanly and reads, to a mining // pass, as a host on which the measurement did not apply. - let get = |label: &str| { - let ns = observation - .get(label) - .unwrap_or_else(|| panic!("measure always records {label}")); - format!("{ns:.1}") - }; + // Built by walking the SAME `timings` the prose table walked, with + // `json_key` deciding only what each entry is called here. + // + // This replaces six hand-named fields and the `get` closure that fetched + // them. Both are gone for the same reason: naming each figure separately + // made prose and NDJSON independent restatements of one measurement, so + // nothing stopped them disagreeing or one omitting a figure the other + // showed. + // + // The `expect`-per-field the closure existed for is NOT gone -- it moved, + // and it had to. Iterating what was measured cannot ask for something + // absent, so this loop cannot fail on a missing label; it just emits one + // key fewer, shrinking the schema with nothing saying so. `main` caught + // that per field. The census below is the same guard in one place. + let mut fields = String::new(); + for timing in &observation.timings { + let _ = write!( + fields, + r#""{}":{:.1},"#, + json_key(timing.label), + timing.nanos_per_op + ); + } + + for label in EVERY_LABEL { + assert!( + observation + .timings + .iter() + .any(|timing| timing.label == label), + "`{label}` is recorded on every host, so its absence means it was \ + renamed or dropped in one place and not the other; the NDJSON line \ + must not quietly ship one field fewer" + ); + } + // The trailing comma the loop leaves is trimmed rather than avoided with a + // separator dance, because every field here is unconditional. + let fields = fields.trim_end_matches(','); + let _ = writeln!( out, - concat!( - r#"{{"reason":"x-probe-request-cost","arch":"{}","prepare_short_cycle_ns":{},"#, - r#""prepare_long_cycle_ns":{},"build_open_request_cycle_ns":{},"#, - r#""clone_prepared_units_cycle_ns":{},"capture_handle_ns":{},"#, - r#""close_handle_ns":{}}}"# - ), + r#"{{"reason":"x-probe-request-cost","arch":"{}",{fields}}}"#, std::env::consts::ARCH, - get("prepare_short_path"), - get("prepare_long_path"), - get("build_open_request"), - get("clone_prepared_units"), - get("capture_handle"), - get("close_handle"), ); } diff --git a/crates/windows-platform-probes/src/bin/topology.rs b/crates/windows-platform-probes/src/bin/topology.rs index d908c00ff..bbc23a7e2 100644 --- a/crates/windows-platform-probes/src/bin/topology.rs +++ b/crates/windows-platform-probes/src/bin/topology.rs @@ -15,7 +15,7 @@ use windows_placement_probe::fingerprint::Fingerprint; use windows_platform_probes::report::emit_report; -use windows_platform_probes::topology::measure; +use windows_platform_probes::topology::measure_observed; use windows_platform_probes::topology_report::{attribution, report, report_unmeasured}; fn main() { @@ -48,12 +48,26 @@ fn render(out: &mut dyn std::fmt::Write) { // `banner_line_for`, which keeps this probe's banner the same shape as every // other probe's. let before = Fingerprint::discover(); - let measured = measure(); + let measured = measure_observed(); let after = Fingerprint::discover(); - let banner = attribution(&before, &after); + + // The banner names the read the BODY describes, not an endpoint. The + // endpoints still bracket, and still report a disagreement across the wider + // window they span; what they no longer do is supply the line naming the + // machine, which they could do while describing a different topology from + // the one measured between them. let text = match measured { - Ok(observation) => report(&banner, &observation), - Err(error) => report_unmeasured(&banner, &error), + Ok((observation, fingerprint)) => { + let banner = attribution(Some(&fingerprint), &before, &after); + report(&banner, &observation) + } + Err(error) => { + // No measured read to name, so the banner falls back to the first + // endpoint -- which is honest here, because the body describes no + // topology either. + let banner = attribution(None, &before, &after); + report_unmeasured(&banner, &error) + } }; let _ = write!(out, "{text}"); } diff --git a/crates/windows-platform-probes/src/completion_port.rs b/crates/windows-platform-probes/src/completion_port.rs index 9d1438368..fecd6e7bd 100644 --- a/crates/windows-platform-probes/src/completion_port.rs +++ b/crates/windows-platform-probes/src/completion_port.rs @@ -221,7 +221,11 @@ pub fn measure() -> IoRingSupport { // creates a fresh one. let port = unsafe { CreateIoCompletionPort(associated_raw, std::ptr::null_mut(), COMPLETION_KEY, 0) }; - assert!(!port.is_null(), "create a completion port"); + assert!( + !port.is_null(), + "create a completion port: {}", + std::io::Error::last_os_error() + ); let after_iocp_association = attempt(associated_raw); // Case 3: ring first (which must pass), then associate, then ring again. @@ -231,7 +235,11 @@ pub fn measure() -> IoRingSupport { // SAFETY: as above. let late_port = unsafe { CreateIoCompletionPort(late_raw, std::ptr::null_mut(), COMPLETION_KEY, 0) }; - assert!(!late_port.is_null(), "create the second completion port"); + assert!( + !late_port.is_null(), + "create the second completion port: {}", + std::io::Error::last_os_error() + ); let after_late_association = attempt(late_raw); // Case 4, the second control: is the associated handle still usable through diff --git a/crates/windows-platform-probes/src/doorbell_cost.rs b/crates/windows-platform-probes/src/doorbell_cost.rs index 00062dddc..489d26528 100644 --- a/crates/windows-platform-probes/src/doorbell_cost.rs +++ b/crates/windows-platform-probes/src/doorbell_cost.rs @@ -78,6 +78,82 @@ use windows_sys::Win32::System::Threading::{ use crate::ioring; +/// The machine-readable name for a timing's label. +/// +/// **The one place a figure's two names are related**, so the prose row and the +/// NDJSON field cannot drift apart or disagree about a value: both renderings +/// walk [`Observation::timings`] and this decides what the second one calls each +/// entry. +/// +/// It exists because the alternative had already gone wrong twice. The prose +/// iterated the measured timings while the NDJSON named each field by hand in a +/// format string, so the two were independent restatements of one fact -- the +/// arrangement that produced `"efficiency_classes":1` beside a prose `[0]` in +/// the topology report, and that the M2.4 matrix found unchecked in both cost +/// probes. Relating the names here makes a disagreement unrepresentable rather +/// than detectable, which is strictly better than an oracle rule: there is +/// nothing left to check. +/// +/// # Panics +/// +/// Panics on a label it does not know. That is deliberate and is the whole +/// safety of the scheme: adding a timing to `measure` without naming it here +/// fails loudly at the render rather than silently omitting it from the +/// machine-readable line, which is the failure a fleet survey would never +/// notice. +#[must_use] +pub fn json_key(label: &str) -> &'static str { + match label { + "atomic_fetch_add" => "atomic_ns", + "set_event_already_signalled" => "set_event_already_signalled_ns", + "set_reset_event" => "set_reset_event_ns", + "wait_zero_signalled" => "wait_zero_signalled_ns", + "submit_io_ring_empty" => "submit_io_ring_empty_ns", + other => panic!( + "`{other}` is measured but has no machine-readable name; add it to \ + `json_key` so it reaches the NDJSON line too" + ), + } +} + +/// Whether a label's absence from an observation is a real outcome or a defect. +/// +/// The distinction is the whole reason this type exists. `main` guarded the four +/// unconditional timings with `expect`, deliberately -- a lookup that misses +/// means a label was renamed in one place and not the other, and the comment +/// there said so. A renderer that emits `null` for every absent label throws +/// that guard away: a renamed timing publishes `null` and looks exactly like a +/// host that legitimately could not run it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Presence { + /// `measure` records this on every host; absence is a defect, not a result. + Always, + /// `measure` records this only when the platform offers it, so absence is a + /// measurement outcome and is published as `null`. + WhenAvailable, +} + +/// Every label [`measure`] can produce, in the order the report renders them, +/// each with whether it may legitimately be missing. +/// +/// **A timing that did not run must still say so.** `submit_io_ring_empty` is +/// pushed only when `IoRing` is available, so a renderer that walks +/// [`Observation::timings`] alone omits the key entirely on a host without it -- +/// and a fleet-mining pass then cannot tell "measured, absent" from "this build +/// did not have the field". Emitting `null` for such a label keeps the row's +/// shape fixed across hosts, which is what makes the shape mineable at all. +/// +/// The [`Presence`] marker is what keeps that from costing the guard it +/// replaced: only a [`Presence::WhenAvailable`] label renders as `null`, and a +/// missing [`Presence::Always`] label still fails loudly. +pub const EVERY_LABEL: [(&str, Presence); 5] = [ + ("atomic_fetch_add", Presence::Always), + ("set_event_already_signalled", Presence::Always), + ("set_reset_event", Presence::Always), + ("wait_zero_signalled", Presence::Always), + ("submit_io_ring_empty", Presence::WhenAvailable), +]; + /// Nanoseconds per operation for one timed loop. #[derive(Debug, Clone, Copy, PartialEq)] pub struct Timing { diff --git a/crates/windows-platform-probes/src/ioring.rs b/crates/windows-platform-probes/src/ioring.rs index f6813758f..433a2392b 100644 --- a/crates/windows-platform-probes/src/ioring.rs +++ b/crates/windows-platform-probes/src/ioring.rs @@ -317,9 +317,13 @@ impl PipePair { std::ptr::null(), ) }; + // Both halves of the condition are ways `CreateNamedPipeW` reports + // failure, and it is the only call between here and the code being + // read, so the code belongs to it either way. assert!( !raw.is_null() && raw != INVALID_HANDLE_VALUE, - "create the probe pipe" + "create the probe pipe: {}", + std::io::Error::last_os_error() ); // SAFETY: `raw` is a fresh, valid handle this type now owns solely. diff --git a/crates/windows-platform-probes/src/pool_growth.rs b/crates/windows-platform-probes/src/pool_growth.rs index 822947da9..e33441728 100644 --- a/crates/windows-platform-probes/src/pool_growth.rs +++ b/crates/windows-platform-probes/src/pool_growth.rs @@ -59,7 +59,14 @@ impl Gate { // SAFETY: null attributes and name are valid; TRUE selects manual // reset, FALSE leaves it unsignalled. let handle = unsafe { CreateEventW(std::ptr::null(), 1, 0, std::ptr::null()) }; - assert!(!handle.is_null(), "create the gate event"); + // The code is read only on failure, and nothing runs between + // `CreateEventW` and this format but a null test, so it belongs to that + // call and no other. + assert!( + !handle.is_null(), + "create the gate event: {}", + std::io::Error::last_os_error() + ); Self(handle) } diff --git a/crates/windows-platform-probes/src/request_cost.rs b/crates/windows-platform-probes/src/request_cost.rs index bcaa957a1..35c178ab9 100644 --- a/crates/windows-platform-probes/src/request_cost.rs +++ b/crates/windows-platform-probes/src/request_cost.rs @@ -99,14 +99,9 @@ //! The owning crate now settles both halves rather than leaving them to be //! re-derived from a probe: see `windows-namespace-request-sys`' //! [DESIGN-NOTES.md](../../windows-namespace-request-sys/DESIGN-NOTES.md) -> -//! `D-18`, which states what the call actually does and records keeping it over -//! the canonicalizers that do not root. It claims no cost comparison against -//! those alternatives, because nothing here measures them -- but it does settle -//! the mechanism question this module once left open: resolving a -//! drive-relative path for another drive checks that drive's recorded entry -//! against the filesystem, and writes the entry back **when that check rejects -//! it** -- an accepted entry is left alone, so the write is conditional rather -//! than part of every such resolution. +//! `D-18`, which states what the call actually does, records keeping it over +//! the cheaper lexical alternative, and says plainly that whether it enters the +//! kernel is not established. //! //! The two schemes that might reduce it recover different halves. **Inline //! storage** removes the allocation and copy, which is what @@ -146,6 +141,57 @@ pub struct Timing { pub nanos_per_op: f64, } +/// The machine-readable name for a timing's label. +/// +/// **The one place a figure's two names are related.** Both renderings walk +/// [`Observation::timings`] and this decides only what the machine-readable one +/// calls each entry, so the prose row and the NDJSON field cannot disagree about +/// a value or exist without each other. +/// +/// Written for the same reason as `doorbell_cost::json_key`: the prose iterated +/// what was measured while the NDJSON named each field by hand, making them two +/// independent restatements of one measurement. The M2.4 matrix found that class +/// unchecked here, and deriving both from one source makes the disagreement +/// unrepresentable rather than merely detectable. +/// +/// # Panics +/// +/// Panics on a label it does not know, so a timing added to `measure` without a +/// name here fails at the render rather than silently missing from the +/// machine-readable line -- the omission a fleet survey would never notice. +#[must_use] +pub fn json_key(label: &str) -> &'static str { + match label { + "prepare_short_path" => "prepare_short_cycle_ns", + "prepare_long_path" => "prepare_long_cycle_ns", + "build_open_request" => "build_open_request_cycle_ns", + "clone_prepared_units" => "clone_prepared_units_cycle_ns", + "capture_handle" => "capture_handle_ns", + "close_handle" => "close_handle_ns", + other => panic!( + "`{other}` is measured but has no machine-readable name; add it to \ + `json_key` so it reaches the NDJSON line too" + ), + } +} + +/// Every label [`measure`] records, in the order the report renders them. +/// +/// All six are unconditional -- unlike `doorbell_cost`, nothing here is gated on +/// a platform capability -- so this list needs no presence marker. It exists for +/// the direction a renderer that walks [`Observation::timings`] cannot check: +/// iterating what was measured cannot notice that something *stopped* being +/// measured, so a dropped timing would quietly shrink the NDJSON schema. `main` +/// caught that with a per-field panic; this is the same guard, stated once. +pub const EVERY_LABEL: [&str; 6] = [ + "prepare_short_path", + "prepare_long_path", + "build_open_request", + "clone_prepared_units", + "capture_handle", + "close_handle", +]; + /// Every timing taken by [`measure`]. #[derive(Debug, Clone)] pub struct Observation { diff --git a/crates/windows-platform-probes/src/tests.rs b/crates/windows-platform-probes/src/tests.rs index 2ad53f00c..f51f6d58e 100644 --- a/crates/windows-platform-probes/src/tests.rs +++ b/crates/windows-platform-probes/src/tests.rs @@ -3585,12 +3585,49 @@ fn fingerprint(processors: usize) -> windows_placement_probe::fingerprint::Finge } } +#[test] +fn the_banner_names_the_measured_read_not_an_endpoint() { + // The correspondence this milestone closes. Both ENDPOINTS agree, so the + // old code printed their fingerprint unqualified -- while the read the body + // describes, taken between them, was a different machine entirely. Nothing + // in the report said so. + // + // Reachable rather than theoretical: `measure` brackets counters around its + // own discovery, so a processor, group or NUMA change there is already + // caught. Cache and efficiency-class structure is what no counter reaches, + // and a hypervisor returning inconsistent + // `GetLogicalProcessorInformationEx` results is this probe's survey + // population. + let text = crate::topology_report::attribution( + Some(&fingerprint(4)), + &Ok(fingerprint(8)), + &Ok(fingerprint(8)), + ); + assert!( + text.contains("4p/"), + "the banner names the topology the body describes: {text}" + ); + assert!( + !text.lines().next().unwrap_or_default().contains("8p/"), + "and not the endpoint reads, which describe a different machine: {text}" + ); +} + +#[test] +fn a_run_whose_discovery_failed_still_gets_the_endpoint_banner() { + // The `None` case is not a degenerate leftover: `report_unmeasured` has no + // measured read to name, and its body describes no topology either, so + // falling back to the first endpoint is the honest line there. + let text = crate::topology_report::attribution(None, &Ok(fingerprint(8)), &Ok(fingerprint(8))); + assert!(text.contains("8p/"), "{text}"); +} + #[test] fn an_unchanged_host_prints_one_banner_line() { // The equal case must render exactly as `banner_line` always did, so every // fingerprint string already recorded elsewhere stays comparable with this // probe's -- and so this probe's banner matches every other probe's. - let text = crate::topology_report::attribution(&Ok(fingerprint(8)), &Ok(fingerprint(8))); + let text = crate::topology_report::attribution(None, &Ok(fingerprint(8)), &Ok(fingerprint(8))); assert_eq!( text, windows_placement_probe::fingerprint::banner_line_for(&Ok(fingerprint(8))), @@ -3604,7 +3641,7 @@ fn a_host_that_changed_across_the_run_says_so_and_keeps_both_readings() { // The fingerprint is a topology rendering, not a name, so two readings that // disagree mean neither identifies the machine the body describes. Both are // kept: which one is stale is exactly what cannot be known here. - let text = crate::topology_report::attribution(&Ok(fingerprint(8)), &Ok(fingerprint(4))); + let text = crate::topology_report::attribution(None, &Ok(fingerprint(8)), &Ok(fingerprint(4))); assert!(text.contains("8p/"), "{text}"); assert!(text.contains("4p/"), "{text}"); assert!(text.contains("HOST READINGS DISAGREE"), "{text}"); @@ -3634,7 +3671,7 @@ fn a_failed_host_read_is_not_reported_as_a_host_that_changed() { ("failed second", Ok(fingerprint(8)), failed()), ("failed both", failed(), failed()), ] { - let text = crate::topology_report::attribution(&before, &after); + let text = crate::topology_report::attribution(None, &before, &after); assert!( !text.contains("HOST READINGS DISAGREE"), "{label}: nothing established that the host changed: {text}" @@ -3652,6 +3689,7 @@ fn two_failed_host_reads_with_different_messages_are_still_not_a_change() { // failed, so nothing about the machine was established at all, yet the two // rendered lines differ because the errors do. let text = crate::topology_report::attribution( + None, &Err(std::io::Error::other("first")), &Err(std::io::Error::other("second")), ); @@ -5028,3 +5066,45 @@ fn the_two_row_shapes_are_distinguishable_by_their_keys() { "and the measured shape must not carry it" ); } + +#[test] +fn a_banner_shape_the_renderer_cannot_produce_is_contained() { + // `is_attribution_shaped` has now been wrong twice in the same way, in + // opposite directions: once too strict (two lines for every disclaimer, so a + // three-reading banner had to drop a reading), once too loose (two-or-three + // for every disclaimer, so a three-reading `MEASURED_UNCONFIRMED` body + // passed -- a shape `attribution` cannot emit, because that arm is reached + // only when the brackets are equal). Both are one mistake: a rule about + // cardinality that does not name which shape it is the cardinality OF. + // + // So this binds the rule at the boundary that matters -- what `preamble` + // writes through verbatim versus what it contains behind `host: ` -- and it + // binds BOTH directions, because a recogniser that accepted nothing would + // pass a one-directional test. + let unconfirmed = crate::topology_report::attribution( + Some(&fingerprint(4)), + &Ok(fingerprint(8)), + &Ok(fingerprint(8)), + ); + let genuine = + crate::topology_report::report_unmeasured(&unconfirmed, &std::io::Error::other("no")); + assert!( + genuine.starts_with(&unconfirmed), + "a banner `attribution` DID produce must pass through verbatim\n{genuine}" + ); + + // The same disclaimer under a third reading. Every line is `host:`-prefixed + // and the disclaimer is intact, so only the cardinality rule can reject it. + let forged = unconfirmed.replacen( + "host:", + "host: test-arch 1p/1c smt- L2[1] ec[0:1] numa[1]\nhost:", + 1, + ); + let contained = + crate::topology_report::report_unmeasured(&forged, &std::io::Error::other("no")); + assert!( + !contained.starts_with(&forged), + "a three-reading `MEASURED_UNCONFIRMED` banner is a shape `attribution` \ + cannot emit, so it must be contained rather than written through\n{contained}" + ); +} diff --git a/crates/windows-platform-probes/src/topology.rs b/crates/windows-platform-probes/src/topology.rs index 6ac61ebd4..451443551 100644 --- a/crates/windows-platform-probes/src/topology.rs +++ b/crates/windows-platform-probes/src/topology.rs @@ -33,6 +33,7 @@ use std::io; +use windows_placement_probe::fingerprint::Fingerprint; use windows_sys::Win32::System::Threading::{ ALL_PROCESSOR_GROUPS, GetActiveProcessorCount, GetActiveProcessorGroupCount, GetNumaHighestNodeNumber, @@ -1044,6 +1045,44 @@ pub enum Verdict { /// /// Propagates a failure from [`MachineMemoryTopology::discover`]. pub fn measure() -> io::Result { + measure_observed().map(|(observation, _)| observation) +} + +/// [`measure`], also returning a fingerprint of the topology it actually parsed. +/// +/// **The banner and the body describe one read, by construction.** A probe run +/// makes three independent discoveries -- one for the banner, `measure`'s own, +/// and one more for the closing bracket -- and the banner was built from an +/// *endpoint*. Equal endpoints therefore printed an unqualified banner without +/// anything establishing that the middle read agreed with them, so the line +/// naming the machine could describe a different topology from the body under +/// it. +/// +/// The window is narrow and worth stating exactly rather than over- or +/// under-selling. `measure` brackets its counters around its own discovery, so a +/// processor, group or NUMA change during the middle read is already caught as +/// [`BracketOutcome::Changed`]. What no counter reaches is cache and +/// efficiency-class structure. The reachable case is a run whose cache structure +/// differs between the endpoint reads and the middle read while the processor, +/// group and NUMA counts stay identical -- near-impossible on real hardware, +/// since caches do not change without processors changing, and entirely +/// reachable on a hypervisor returning inconsistent +/// `GetLogicalProcessorInformationEx` results, which is exactly the population +/// this probe exists to survey. +/// +/// Construction rather than a third comparison, for the reason M2.9 chose a +/// common source over a third oracle rule: a comparison is new prose that can +/// drift from what it compares, while a banner built from the body's own read +/// cannot disagree with it. The endpoint reads keep their job -- they catch +/// structural change across the wider window the counter bracket does not span. +/// +/// A sibling rather than a changed signature, so `measure`'s existing callers +/// are untouched. +/// +/// # Errors +/// +/// Propagates a failure from [`MachineMemoryTopology::discover`]. +pub fn measure_observed() -> io::Result<(Observation, Fingerprint)> { // Bracketed, because the parse and the counters are separate reads of a // machine that can change between them: Windows supports processor hot-add, // and a machine that gained one mid-run would have both readings correct @@ -1061,7 +1100,7 @@ pub fn measure() -> io::Result { after.2, bracket_outcome(before, after), ); - Ok(observation) + Ok((observation, Fingerprint::from_topology(&topology))) } /// Whether two bracketing counter reads establish that the machine changed. diff --git a/crates/windows-platform-probes/src/topology_report.rs b/crates/windows-platform-probes/src/topology_report.rs index 69fd14864..67e9b8a36 100644 --- a/crates/windows-platform-probes/src/topology_report.rs +++ b/crates/windows-platform-probes/src/topology_report.rs @@ -142,20 +142,49 @@ use crate::topology::{ /// [`preamble`], which has to recognise an attribution-shaped banner to pass it /// through. A second copy of this sentence in the recogniser would be a /// restatement that could drift out of step with the one that writes it. -const READINGS_DISAGREE: &str = "HOST READINGS DISAGREE: the two readings above bracket the measurement\n\ - and differ, so which of them names the machine the body below describes\n\ +const READINGS_DISAGREE: &str = "HOST READINGS DISAGREE: the two readings that bracket the measurement\n\ + differ, so which of them names the machine the body below describes\n\ was not established."; /// The disclaimer `attribution` renders when at least one bracket read failed. const NOT_ESTABLISHED: &str = "HOST NOT ESTABLISHED: at least one of the two readings that bracket the measurement\n\ failed, so nothing confirmed the machine held still under it."; +/// The disclaimer `attribution` renders when the brackets agree with each other +/// but not with the read the body describes. +/// +/// **Agreeing endpoints are not agreement.** `measured` was added so the banner +/// would name the read the body came from rather than an endpoint -- but naming +/// it silently is not the same as establishing it. Two endpoints can agree with +/// each other and both differ from the middle read, and that is exactly the +/// flaky-enumeration case `attribution`'s own rustdoc calls this probe's survey +/// population: `Fingerprint::discover` returns `Ok` on a parse that dropped a +/// record, so three reads of one unchanged machine can disagree. With no +/// disclaimer for it, that host printed an unqualified banner while the run +/// held the evidence against it. +const MEASURED_UNCONFIRMED: &str = "HOST READING UNCONFIRMED: the two readings that bracket the measurement agree\n\ + with each other but not with the read the body below describes, so one of\n\ + the three discoveries did not see what the others saw."; + /// Whether `banner` is a shape [`attribution`] can produce. /// -/// [`attribution`] has exactly three outputs, and the count of `host:` lines is -/// not free in any of them: one reading prints ONE line and no disclaimer, and -/// both of the two-reading arms print TWO lines and a disclaimer. So the -/// cardinality is part of the shape, and this checks it. +/// The count of `host:` lines is not free: an undisclaimed banner carries +/// exactly ONE reading, and a disclaimed one carries the readings its own +/// disclaimer is about. So the cardinality is part of the shape, and this checks +/// it against the disclaimer that was found rather than against the mere fact +/// that one was. +/// +/// **The count is per disclaimer, not per disclaimed-or-not.** The two bracket +/// disclaimers carry every reading the run holds, so two without a measured read +/// and three with one; `MEASURED_UNCONFIRMED` is reached only when the brackets +/// are equal, so it carries exactly two. An earlier form accepted only two for +/// all of them, which forced a disclaimed banner to drop one of its three +/// readings -- it dropped the measured one, so a report that disclaimed its own +/// attribution no longer showed the reading its body came from. Widening that to +/// a flat two-or-three for every disclaimer then admitted a three-line +/// `MEASURED_UNCONFIRMED` body, which this renderer cannot produce. Both are the +/// same mistake: a rule about cardinality that does not name which shape it is +/// the cardinality OF. /// /// **A looser recogniser let the banner state a disagreement while denying /// there was one.** It accepted any number of `host:` lines with the disclaimer @@ -183,19 +212,31 @@ fn is_attribution_shaped(banner: &str) -> bool { // // Exactly one newline rather than `trim_end_matches`: `attribution` emits // one, and accepting several would admit another shape it cannot produce. - let (body, disclaimed) = [READINGS_DISAGREE, NOT_ESTABLISHED] - .iter() - .find_map(|disclaimer| { - banner - .strip_suffix(disclaimer) - .and_then(|head| head.strip_suffix('\n')) - }) - .map_or((banner, false), |head| (head, true)); + // **The admissible count depends on WHICH disclaimer, not merely on whether + // there was one.** Widening this to a flat `2..=3` for every disclaimer was + // half a rule: it admitted a three-reading body under `MEASURED_UNCONFIRMED`, + // and `attribution` cannot emit that -- that arm is reached only when the + // brackets are equal, so it prints the measured read and ONE bracket, the + // second being a copy of the first. A shape this renderer cannot produce + // passing the recogniser is the precise hole the rustdoc above argues was + // closed, reopened by the fix that widened it. + let (body, admissible) = [ + (READINGS_DISAGREE, 2..=3), + (NOT_ESTABLISHED, 2..=3), + (MEASURED_UNCONFIRMED, 2..=2), + ] + .into_iter() + .find_map(|(disclaimer, admissible)| { + banner + .strip_suffix(disclaimer) + .and_then(|head| head.strip_suffix('\n')) + .map(|head| (head, admissible)) + }) + .unwrap_or((banner, 1..=1)); let lines = body.lines().collect::>(); - let expected = if disclaimed { 2 } else { 1 }; - lines.len() == expected && lines.iter().all(|line| line.starts_with("host:")) + admissible.contains(&lines.len()) && lines.iter().all(|line| line.starts_with("host:")) } /// Caller-supplied text, reduced to something that cannot create a line. @@ -301,8 +342,38 @@ fn preamble(banner: &str) -> String { /// that changed. That is a claim about the machine drawn from a gap in the /// measurement: a failed read establishes neither that the host moved nor that /// it held still. +/// **`measured` names the read the body describes, and that is the point.** +/// The banner used to be built from `before` -- an *endpoint* read -- so equal +/// endpoints printed an unqualified banner without anything having established +/// that the middle read agreed with them. `measure_observed` returns the +/// fingerprint of the topology it actually parsed, so passing it here makes the +/// banner describe the body by construction. +/// +/// **Naming it is not establishing it, and the two were conflated.** Making the +/// banner name the measured read fixes *which* machine the body is attributed +/// to; it says nothing about whether the brackets confirmed that read. Equal +/// endpoints that both differ from it are a real and documented case -- see +/// `MEASURED_UNCONFIRMED` -- and it now has a disclaimer of its own rather than +/// printing unqualified. That is a comparison, and deliberately so: the +/// alternative is not "no comparison", it is an unstated one. +/// +/// `None` for a run whose discovery failed: there is no measured read to name, +/// so the banner falls back to the first endpoint as before. That is the +/// `report_unmeasured` path, where the body describes no topology either. +/// +/// **Every disclaimed banner shows every reading the run holds**, so a reader +/// who is told the attribution is in doubt can see what the readings were. With +/// a measured read that is three lines, not two. +/// +/// The endpoints keep their job. They bracket a **wider** window than +/// `measure`'s counter bracket -- which covers only its own discovery -- so they +/// still catch structural change the counters cannot see. #[must_use] -pub fn attribution(before: &io::Result, after: &io::Result) -> String { +pub fn attribution( + measured: Option<&Fingerprint>, + before: &io::Result, + after: &io::Result, +) -> String { // **Each reading is flattened HERE, because a banner line is a line.** // `banner_line_for` interpolates a failed read's `io::Error` verbatim, and // an OS error is free to contain a newline -- so a reading could arrive as @@ -317,17 +388,74 @@ pub fn attribution(before: &io::Result, after: &io::Result format!( + "{}\n{}\n{}", + measured_line(), + bracket(before), + bracket(after) + ), + None => format!("{}\n{}", bracket(before), bracket(after)), + }; match (before, after) { - (Ok(one), Ok(two)) if one == two => first, - (Ok(_), Ok(_)) => format!("{first}\n{}\n{READINGS_DISAGREE}", second()), + (Ok(one), Ok(two)) if one == two => match measured { + // The brackets agree and the body's own read agrees with them, so + // there is one reading to report and nothing to qualify. + Some(fingerprint) if fingerprint == one => measured_line(), + // They agree with each other and not with it. One bracket is shown + // rather than both: they are equal here, so the second would be a + // copy of the first, and the disclaimer says which relation failed. + Some(_) => format!( + "{}\n{}\n{MEASURED_UNCONFIRMED}", + measured_line(), + bracket(before) + ), + None => measured_line(), + }, + (Ok(_), Ok(_)) => format!("{}\n{READINGS_DISAGREE}", every_reading()), // Covers (Err, Ok), (Ok, Err) AND (Err, Err), so the text says "at // least one". "One of the two readings failed" understates the case // where both did -- a small thing, but the same shape as every other // sentence corrected here: claiming a more specific state than the run // established. - _ => format!("{first}\n{}\n{NOT_ESTABLISHED}", second()), + _ => format!("{}\n{NOT_ESTABLISHED}", every_reading()), } } diff --git a/crates/windows-platform-probes/tests/a_cost_probe_tells_both_readers_the_same_thing.rs b/crates/windows-platform-probes/tests/a_cost_probe_tells_both_readers_the_same_thing.rs new file mode 100644 index 000000000..934286b58 --- /dev/null +++ b/crates/windows-platform-probes/tests/a_cost_probe_tells_both_readers_the_same_thing.rs @@ -0,0 +1,161 @@ +// Copyright (c) Mike Grier. + +//! Every figure a cost probe prints reaches its machine-readable line too, with +//! the same value. +//! +//! # Why this is not an oracle rule +//! +//! The M2.4 matrix found both cost probes rendering every measured figure twice +//! -- once in a prose table, once in NDJSON -- with nothing comparing the two. +//! The obvious fix was a third oracle rule set. The chosen fix was to make the +//! two renderings **derive from one source**: both walk `Observation::timings`, +//! and `json_key` decides only what the machine-readable one calls each entry. +//! +//! That makes a disagreement unrepresentable rather than detectable, which is +//! strictly stronger. A rule finds a contradiction that already exists; there is +//! now none to find. +//! +//! # So what is left to test +//! +//! That the derivation actually holds end to end, on the real binaries: a figure +//! in the table appears in the JSON, under the name `json_key` gives it, with +//! the same digits. The property is structural in the source, and this is the +//! check that the structure survives rendering, formatting and the process +//! boundary. +//! +//! It reuses the crate's own `json_key` rather than restating the mapping. A +//! test carrying its own copy of the pairing would be checking the copy, which +//! is the defect this whole milestone is about. + +use std::process::Command; + +/// Every `label value` row of a probe's table, and its NDJSON line. +fn run(binary: &str) -> (Vec<(String, f64)>, String) { + let output = Command::new(binary).output().expect("run the probe"); + assert!( + output.status.success(), + "the probe exited with {:?}; stderr: {}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).expect("a probe's report is UTF-8"); + + let ndjson = stdout + .lines() + .find(|line| line.starts_with('{')) + .expect("a cost probe emits one NDJSON line") + .to_owned(); + + // A table row starts at column 0 with a bare label and has a number next. + // Later columns are allowed: `request_cost` prints two ratio columns after + // the figure, and requiring exactly two tokens found zero rows there -- + // caught by the emptiness assertion below rather than by passing quietly, + // which is the whole reason that assertion exists. + // + // Prose is excluded by the two conditions together: indented lines fail the + // first, and sentences fail the second because their second word is not a + // number. + let rows = stdout + .lines() + .filter(|line| line.starts_with(|c: char| c.is_ascii_alphabetic())) + .filter_map(|line| { + let mut parts = line.split_whitespace(); + let label = parts.next()?; + let value: f64 = parts.next()?.parse().ok()?; + Some((label.to_owned(), value)) + }) + .collect(); + + (rows, ndjson) +} + +/// The value NDJSON gave `key`, if it carries one. +fn ndjson_number(line: &str, key: &str) -> Option { + let needle = format!("\"{key}\":"); + let start = line.find(&needle)? + needle.len(); + let rest = &line[start..]; + let end = rest.find([',', '}']).unwrap_or(rest.len()); + rest[..end].trim().parse().ok() +} + +fn assert_table_matches_ndjson( + binary: &str, + key_for: impl Fn(&str) -> &'static str, + every_label: &[&str], +) { + let (rows, ndjson) = run(binary); + + // **`!rows.is_empty()` was the only guard on the recovery, and one row + // satisfies it.** The filter above recovers rows heuristically -- column-0 + // alphabetic start, second token parses as a number -- so a renderer that + // printed ONE of the table's rows and dropped the rest passed this test + // unchanged: the surviving row matched its NDJSON field, the set was + // non-empty, and nothing related the count to anything. + // + // **The census asks the NDJSON which labels to require, rather than holding + // a list of its own.** A static list had to be filtered down to the + // unconditional labels, since a conditional one is legitimately absent on a + // host that cannot measure it -- and that filter was the hole: it excluded + // `submit_io_ring_empty` on every host, including the ones where it IS + // measured, so dropping its prose row while keeping its NDJSON field passed. + // Deriving the requirement from the line under test has no such gap: a label + // the NDJSON reports a NUMBER for was measured, so the prose owes a row for + // it; a label it reports `null` for was not, so nothing is owed. This is the + // NDJSON -> prose direction, which nothing checked before; the loop below is + // the prose -> NDJSON one. + for label in every_label { + let key = key_for(label); + if ndjson_number(&ndjson, key).is_some() { + assert!( + rows.iter().any(|(read, _)| read == label), + "the NDJSON carries a number for `{key}` but the prose table did \ + not print `{label}`, so a figure reached a mining pass and not a \ + reader\n{ndjson}" + ); + } + } + + for (label, prose) in rows { + let key = key_for(&label); + let json = ndjson_number(&ndjson, key).unwrap_or_else(|| { + panic!( + "the table printed `{label}` but the NDJSON carries no `{key}`, \ + so a figure reached a reader and not a mining pass\n{ndjson}" + ) + }); + + // Compared as rendered, not as floats. Both renderings format to one + // decimal place from the same `f64`, so equality here is exact; an + // epsilon would hide precisely the formatting drift worth catching. + assert_eq!( + format!("{prose:.1}"), + format!("{json:.1}"), + "`{label}` reads {prose} in the table and {json} under `{key}`" + ); + } +} + +#[test] +fn the_doorbell_probe_prints_every_figure_to_both_readers() { + // Every label, not just the unconditional ones: the census asks the NDJSON + // which of them were measured, so a conditional label is required exactly on + // the hosts that measured it. + let every: Vec<&str> = windows_platform_probes::doorbell_cost::EVERY_LABEL + .iter() + .map(|(label, _)| *label) + .collect(); + assert_table_matches_ndjson( + env!("CARGO_BIN_EXE_probe-doorbell-cost"), + windows_platform_probes::doorbell_cost::json_key, + &every, + ); +} + +#[test] +fn the_request_probe_prints_every_figure_to_both_readers() { + assert_table_matches_ndjson( + env!("CARGO_BIN_EXE_probe-request-cost"), + windows_platform_probes::request_cost::json_key, + &windows_platform_probes::request_cost::EVERY_LABEL, + ); +} diff --git a/crates/windows-platform-probes/tests/a_real_report_agrees_with_itself.rs b/crates/windows-platform-probes/tests/a_real_report_agrees_with_itself.rs index 9592ddf7d..1432cc730 100644 --- a/crates/windows-platform-probes/tests/a_real_report_agrees_with_itself.rs +++ b/crates/windows-platform-probes/tests/a_real_report_agrees_with_itself.rs @@ -42,7 +42,7 @@ use windows_placement_probe::fingerprint::Fingerprint; use windows_platform_probes::report_oracle; -use windows_platform_probes::topology::{invariant, measure}; +use windows_platform_probes::topology::{invariant, measure_observed}; use windows_platform_probes::topology_report::{attribution, report, report_unmeasured}; /// The report exactly as `probe-topology` composes it. @@ -53,14 +53,25 @@ use windows_platform_probes::topology_report::{attribution, report, report_unmea /// deliberately -- a report assembled some other way would be checking an /// artifact no one ships. fn real_report() -> (String, bool) { + // **`measure_observed`, because the banner is built from the read the body + // describes.** The binary this mirrors passes the fingerprint of the + // topology it actually parsed, so the banner names the middle read by + // construction rather than naming an endpoint and hoping. Mirroring that + // here is the whole point of composing the report by hand -- a report + // assembled some other way would be checking an artifact no one ships. let before = Fingerprint::discover(); - let measured = measure(); + let measured = measure_observed(); let after = Fingerprint::discover(); - let banner = attribution(&before, &after); match measured { - Ok(observation) => (report(&banner, &observation), true), - Err(error) => (report_unmeasured(&banner, &error), false), + Ok((observation, fingerprint)) => { + let banner = attribution(Some(&fingerprint), &before, &after); + (report(&banner, &observation), true) + } + Err(error) => { + let banner = attribution(None, &before, &after); + (report_unmeasured(&banner, &error), false) + } } } From ba95bcd0d7534d6535cc4e261f0ed6676918612c Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Thu, 17 Sep 2026 14:56:13 -0400 Subject: [PATCH 02/11] fix(platform-probes): report a refusal to pin instead of crashing the probe `pin_current_thread` asserted when `SetThreadGroupAffinity` failed, and `core_affinity::measure()` reaches it through `time_model_on` / `time_model_placed`. A process restricted by a job object, a container or `start /affinity` is the documented cause. **The decision to stop has not changed, and is not in question.** The assert's own message makes the argument -- an unpinned thread "measures wherever the scheduler happened to put it, which would produce a plausible number that answers a different question, and nothing in the output would say so." That text is kept verbatim. What changed is the channel it travels on. A panic bypasses `emit_report`. Measured on this host with every pin forced to fail, before and after: the old binary wrote three lines to stdout -- banner, heading, blank -- and exited 101, with the explanation reaching only stderr. A survey mining stdout sees a probe that produced a banner and then nothing, which is indistinguishable from a job that died for an unrelated reason. That is the same "cannot tell a measured absence from a missing run" gap `report_unmeasured` already closes for discovery failure, left open for pinning failure. It now writes the refusal itself to stdout and exits 0, matching how the same binary reports a topology-discovery failure. `pin_current_thread` returns `io::Result`; `time_model`, `time_model_on`, `time_model_placed`, `median` and `peer_index_cache::measure` propagate it. Both probe binaries render it after the banner. **The `usize::BITS` check became an error too, and that is the part that makes the new path testable.** A group affinity mask is a `usize`, so on a 32-bit target a processor numbered 32 or above cannot be named -- a limit of the mask rather than a caller mistake. It is also the only pin failure an ordinary developer host can produce on demand, so leaving it a panic would have left the fallible path with no test able to reach it. The two pinning tests consequently drop `catch_unwind` and ask for the error directly, which is a stronger check: an unwind proves only that something panicked. **`probe-core-affinity`'s error arm had to change with it.** It said "could not read this machine's topology" for whatever came back, so a pin refusal would have been reported as a discovery failure -- a specific, checkable claim about the machine, made from an error that says something else. `PinSignal` still publishes `PIN_FAILED`, now on the early-return path as well as on an unwind, so the consumer never waits on a producer that has left. **A review of the first attempt at this found three defects in it, all one mistake: a claim made about the code rather than from it.** `check_group_support` (then `assert_group_support`) tests the *same* predicate as the `usize::BITS` branch, over every discovered processor, and runs before any pin. Converting only the branch therefore changed nothing a binary can reach -- the probe still died with banner, heading and nothing else, exit 101. The before/after measurement recorded below did not catch it because it forced the pin *argument* rather than the discovered set, so it exercised the one site that had been fixed. Fixing one site of a rule and leaving its twin is the blast-radius miss this repository has a rule against. Both sites now refuse; verified by narrowing the mask bound so this host trips it, which now yields nineteen stdout lines and exit 0 where it yielded three and 101. The refusal text was shared by both causes while being written for one. For the mask-width cause it claimed the processor "was reported by this machine's own topology, so this is unexpected rather than a limit of the tool" and closed by inviting a bug report -- the exact reverse of that cause, which *is* a limit of the tool, as this message itself argues three paragraphs later. The explanation is now a parameter, and both directions are verified: the environment cause still names job objects and asks for a report, the mask-width cause says the host is fine and does not. `peer_index_cache::measure` **cannot** refuse. It starts only unpinned runs, so no host can make it return `Err` -- yet its `# Errors` section described a failure mode, and an earlier draft of this message claimed "both probe binaries render it after the banner" as though both could decline. Only `probe-core-affinity` pins. The signature stays `Result` because its helpers are fallible in general; the documentation now says that rather than the opposite. **The two `?`s this commit added to `median` had no test**, which is the same gap the `usize::BITS` conversion was argued on. Two now drive it with an `Err`-returning timer and assert the call count, because short-circuiting and collecting-then-failing both return `Err` and only one of them stops asking a machine that has already refused. Two corrections to the item as I wrote it when queuing. The `x-probe-*` row it called for is deferred to `M4.10`, which on inspection is a larger question than these two probes: three of sixteen binaries emit such a row, so prose-only is the majority by four to one and the crate has two conventions with no stated rule for which applies. And the blocker I recorded was **wrong** -- `windows-placement-probe` is `publish = false` and absent from the release-please manifest, and `tools/check-commit-scope.ps1` says in terms that such a crate "cannot be poisoned, because it is never released -- so it is not a finding." I recorded a blocker the repository's own tooling documents as a non-issue. The two crates change together because the signature change and its consumers must land together to compile. Completed item: M4.9: Route a placement probe's refusal-to-measure through the report sink instead of a panic, so a host that cannot be pinned is a reported observation rather than a crash. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/core_affinity.rs | 54 +++- .../src/peer_index_cache.rs | 289 ++++++++++++------ .../src/peer_index_cache/tests.rs | 137 +++++++-- crates/windows-platform-probes/CHECKLIST.md | 97 ++++-- .../src/bin/core_affinity.rs | 18 +- .../src/bin/peer_index_cache.rs | 27 +- 6 files changed, 454 insertions(+), 168 deletions(-) diff --git a/crates/windows-placement-probe/src/core_affinity.rs b/crates/windows-placement-probe/src/core_affinity.rs index 66a4d7639..219ca2c90 100644 --- a/crates/windows-placement-probe/src/core_affinity.rs +++ b/crates/windows-placement-probe/src/core_affinity.rs @@ -551,24 +551,48 @@ pub fn representative_pairs( /// and not available again: a wrong answer there is not a wrong answer we get to /// correct. A refusal costs one message. /// -/// # Panics +/// # Errors /// /// If the discovered processors cannot be measured as they are. -fn assert_group_support(processors: &[ProcessorPlace]) { - assert!( - !processors.is_empty(), - "no processors were discovered, so there is nothing to measure" - ); +/// +/// **This is the same predicate `pin_current_thread` checks, and converting only +/// that one left this one deciding the outcome.** It runs over every discovered +/// processor *before* any pin is attempted, so a machine with a processor too +/// wide for an affinity mask died here -- with the banner, the heading and +/// nothing else on stdout -- and never reached the branch that had just been +/// made to report it. Fixing one site of a rule and leaving its twin is how the +/// fix came to be measured as working while the binary's behaviour was +/// unchanged. +fn check_group_support(processors: &[ProcessorPlace]) -> std::io::Result<()> { + if processors.is_empty() { + return Err(std::io::Error::other( + "no processors were discovered, so there is nothing to measure", + )); + } // Every discovered processor must be pinnable. A number at or above the // width of an affinity mask cannot be expressed in one, and measuring the // rest while dropping it would report a machine smaller than the real one. for place in processors { - assert!( - u32::from(place.number) < usize::BITS, - "processor {place} has a number no affinity mask can express; \ - this machine cannot be measured honestly and the run is stopping" - ); + if u32::from(place.number) >= usize::BITS { + return Err(std::io::Error::other(format!( + " +This run is stopping, and no measurement was taken. + +Processor {place} has a number no affinity mask can express: a group +affinity mask on this target holds {width} processors. + +A group affinity mask is one machine word wide, so this is a limit of this +build on this target rather than a fault in the machine. The host is fine; +this build cannot name that processor. + +Measuring the rest while dropping it would report a machine smaller than +the real one, so the run stops instead. +", + width = usize::BITS + ))); + } } + Ok(()) } /// Choose one representative processor pair for each *distinct pair of NUMA @@ -673,7 +697,7 @@ pub fn measure() -> std::io::Result { std::io::Error::new(ErrorKind::InvalidData, unplaceable.to_string()) })?; let host = Fingerprint::from_topology(&topology); - assert_group_support(&processors); + check_group_support(&processors)?; let pairs = representative_pairs(&processors); let mut measurements = Vec::new(); @@ -681,7 +705,7 @@ pub fn measure() -> std::io::Result { for strategy in [Strategy::Baseline, Strategy::Cached] { let mut samples: Vec<_> = (0..REPETITIONS) .map(|_| time_model_on(strategy, Some(producer.id()), Some(consumer.id()))) - .collect(); + .collect::>>()?; samples.sort_by(|a, b| a.nanos.total_cmp(&b.nanos)); let median = samples[samples.len() / 2]; @@ -715,7 +739,7 @@ pub fn measure() -> std::io::Result { for strategy in [Strategy::Baseline, Strategy::Cached] { let mut samples: Vec<_> = (0..REPETITIONS) .map(|_| time_model_on(strategy, Some(producer.id()), Some(consumer.id()))) - .collect(); + .collect::>>()?; samples.sort_by(|a, b| a.nanos.total_cmp(&b.nanos)); let median = samples[samples.len() / 2]; by_class.push(Measurement { @@ -751,7 +775,7 @@ pub fn measure() -> std::io::Result { Some(memory_node), ) }) - .collect(); + .collect::>>()?; samples.sort_by(|a, b| a.nanos.total_cmp(&b.nanos)); let median = samples[samples.len() / 2]; by_node_pair.push(Measurement { diff --git a/crates/windows-placement-probe/src/peer_index_cache.rs b/crates/windows-placement-probe/src/peer_index_cache.rs index 8c3c23bdd..4e1599a4c 100644 --- a/crates/windows-placement-probe/src/peer_index_cache.rs +++ b/crates/windows-placement-probe/src/peer_index_cache.rs @@ -171,15 +171,34 @@ impl Observation { } /// Time the shipping queue and every model strategy. -#[must_use] -pub fn measure() -> Observation { - Observation { - calibration: median("shipping spsc", time_real_spsc), +/// +/// # Errors +/// +/// **Cannot refuse today, and the signature says `Result` anyway.** Every run +/// this starts is unpinned -- `time_model` passes `None` for both sides, and +/// `pin_current_thread(None)` returns before it touches anything -- so no host +/// can make this return `Err`. The `Result` is here because the helpers it +/// calls are fallible in general, not because this path has a failure mode; an +/// earlier version of this section claimed it "refuses when a thread cannot be +/// confined to the processor it was given", which describes [`time_model_on`]'s +/// pinned callers rather than anything reachable from here. +/// +/// The probe that *can* refuse is `core_affinity::measure`, which chooses +/// processors and pins to them. +pub fn measure() -> std::io::Result { + Ok(Observation { + // Neither arm pins, so neither can refuse -- the `?`s below propagate a + // failure that no caller of this function can currently produce. Said + // plainly because the previous note here contrasted the calibration + // with the strategies as though only the first were unpinned, which + // would have sent the next reader looking for a refusal path in the + // second. + calibration: median("shipping spsc", || Ok(time_real_spsc()))?, strategies: [Strategy::Baseline, Strategy::Cached, Strategy::Warmed] .into_iter() .map(|strategy| median(strategy.label(), || time_model(strategy))) - .collect(), - } + .collect::>>()?, + }) } /// One timed pass, with the shared-read counts that pass performed. @@ -199,22 +218,27 @@ pub struct Sample { pub producer_refreshes: u64, } -fn median(label: &'static str, mut timer: impl FnMut() -> Sample) -> Run { +fn median( + label: &'static str, + mut timer: impl FnMut() -> std::io::Result, +) -> std::io::Result { // One untimed pass: first touch of a fresh allocation faults pages in, and // that belongs to the allocator rather than to the ring. - let _ = timer(); + let _ = timer()?; - let mut samples: Vec = (0..REPETITIONS).map(|_| timer()).collect(); + let mut samples: Vec = (0..REPETITIONS) + .map(|_| timer()) + .collect::>>()?; samples.sort_by(|left, right| left.nanos.total_cmp(&right.nanos)); let sample = samples[REPETITIONS / 2]; - Run { + Ok(Run { label, nanos_per_item: sample.nanos / ITEMS as f64, items_per_second: ITEMS as f64 / (sample.nanos / 1e9), consumer_refreshes: sample.consumer_refreshes, producer_refreshes: sample.producer_refreshes, - } + }) } /// The shipping queue, driven the same way the model is. @@ -618,7 +642,7 @@ fn working_set_flags(address: *mut c_void) -> Option { Some(unsafe { info.VirtualAttributes.Flags }) } -fn time_model(strategy: Strategy) -> Sample { +fn time_model(strategy: Strategy) -> std::io::Result { time_model_on(strategy, None, None) } @@ -635,11 +659,19 @@ fn time_model(strategy: Strategy) -> Sample { /// `None` leaves a side unconstrained, which is what the unpinned entry points /// pass and is deliberately not the same thing as pinning it to every /// processor: an unconstrained thread can migrate mid-run. +/// +/// # Errors +/// +/// Refuses, rather than measuring, when either side cannot be confined to the +/// processor it was given -- a process restricted by a job object, a container +/// or `start /affinity` being the usual cause. An unpinned run would produce a +/// plausible number that answers a different question, so the refusal is the +/// result; returning it lets the caller report it rather than die on it. pub fn time_model_on( strategy: Strategy, producer_cpu: Option<(u16, u8)>, consumer_cpu: Option<(u16, u8)>, -) -> Sample { +) -> std::io::Result { time_model_placed(strategy, producer_cpu, consumer_cpu, None) } @@ -649,71 +681,77 @@ pub fn time_model_on( /// writes locally and the consumer reads remotely; moving it to the consumer's /// node reverses exactly that, and those are different costs rather than two /// samples of one. +/// +/// # Errors +/// +/// As [`time_model_on`]. pub fn time_model_placed( strategy: Strategy, producer_cpu: Option<(u16, u8)>, consumer_cpu: Option<(u16, u8)>, memory_node: Option, -) -> Sample { +) -> std::io::Result { let ring = Ring::new_on(CAPACITY, memory_node); let placed_on = ring.memory_node(); - // **Pinned before anything is spawned.** `pin_current_thread` panics on + // **Pinned before anything is spawned.** `pin_current_thread` refuses on // failure, and this used to run *after* the producer was already started: - // the unwind then reached `thread::scope`'s cleanup, which waits for a + // the failure then reached `thread::scope`'s cleanup, which waits for a // producer that is itself blocked forever on a ring nobody is draining. A // failure that should stop the run hung it instead. Nothing is running yet - // here, so the panic simply propagates. - let _consumer_pinned = pin_current_thread(consumer_cpu); + // here, so the refusal simply returns. + let _consumer_pinned = pin_current_thread(consumer_cpu)?; // Outside the scope so it outlives every borrow the spawned thread takes. let producer_pin = AtomicU8::new(PIN_PENDING); let started = Instant::now(); - let (consumer_refreshes, producer_refreshes) = thread::scope(|scope| { - let shared = ˚ - let producer_pin = &producer_pin; - let producer = scope.spawn(move || { - // Armed *before* the pin attempt, so an unwind out of it still - // publishes an answer. Without that the consumer below waits on a - // thread that has already died. - let signal = PinSignal(producer_pin); - // Bound, not discarded: an unbound guard drops at the end of its - // own statement, which would unpin the thread immediately and - // measure the scheduler's choice while claiming to measure this - // processor. `#[must_use]` makes that mistake a warning. - let _pinned = pin_current_thread(producer_cpu); - producer_pin.store(PIN_READY, Ordering::Release); - // Its work is done; dropping it now cannot overwrite `PIN_READY`. - drop(signal); - produce(shared, strategy) - }); - - // **Neither side enters the transfer until both pins are settled.** - // Spinning rather than parking because the wait is a pin call long, - // and because this thread is already pinned and must not be handed to - // another processor by a blocking primitive. - while producer_pin.load(Ordering::Acquire) == PIN_PENDING { - std::hint::spin_loop(); - } + let (consumer_refreshes, producer_refreshes) = + thread::scope(|scope| -> std::io::Result<(u64, u64)> { + let shared = ˚ + let producer_pin = &producer_pin; + let producer = scope.spawn(move || -> std::io::Result { + // Armed *before* the pin attempt, so leaving it without success + // still publishes an answer -- by an early return now, and by an + // unwind out of anything below. Without that the consumer waits + // on a thread that has already finished. + let signal = PinSignal(producer_pin); + // Bound, not discarded: an unbound guard drops at the end of its + // own statement, which would unpin the thread immediately and + // measure the scheduler's choice while claiming to measure this + // processor. `#[must_use]` makes that mistake a warning. + let _pinned = pin_current_thread(producer_cpu)?; + producer_pin.store(PIN_READY, Ordering::Release); + // Its work is done; dropping it now cannot overwrite `PIN_READY`. + drop(signal); + Ok(produce(shared, strategy)) + }); + + // **Neither side enters the transfer until both pins are settled.** + // Spinning rather than parking because the wait is a pin call long, + // and because this thread is already pinned and must not be handed to + // another processor by a blocking primitive. + while producer_pin.load(Ordering::Acquire) == PIN_PENDING { + std::hint::spin_loop(); + } - // On failure `consume` is skipped entirely: it would spin forever on - // items no living producer will write. `join` then surfaces the - // producer's panic, which is the outcome the caller should see. - let consumer_refreshes = if producer_pin.load(Ordering::Acquire) == PIN_READY { - consume(&ring, strategy) - } else { - 0 - }; - let producer_refreshes = producer.join().expect("the producer must not panic"); - (consumer_refreshes, producer_refreshes) - }); - Sample { + // On failure `consume` is skipped entirely: it would spin forever on + // items no living producer will write. `join` then surfaces the + // producer's refusal, which is the outcome the caller should see. + let consumer_refreshes = if producer_pin.load(Ordering::Acquire) == PIN_READY { + consume(&ring, strategy) + } else { + 0 + }; + let producer_refreshes = producer.join().expect("the producer must not panic")?; + Ok((consumer_refreshes, producer_refreshes)) + })?; + Ok(Sample { nanos: started.elapsed().as_nanos() as f64, consumer_refreshes, producer_refreshes, memory_node: placed_on, - } + }) } /// The producer has not yet reached the end of its pin attempt. @@ -723,12 +761,13 @@ const PIN_READY: u8 = 1; /// The producer left its pin attempt without succeeding, so no data is coming. const PIN_FAILED: u8 = 2; -/// Publishes [`PIN_FAILED`] if the producer unwinds before it reports success. +/// Publishes [`PIN_FAILED`] if the producer leaves without reporting success. /// -/// **The point is the unwind path, not the success path.** A plain store after -/// `pin_current_thread` would never run when that call panics, and the consumer -/// would then wait on a producer that no longer exists. A guard armed before -/// the attempt runs either way, so the wait always ends. +/// **The point is the path that skips the success store, not the success path.** +/// A plain store after `pin_current_thread` never runs when that call refuses -- +/// the `?` returns first -- and the consumer would then wait on a producer that +/// has already finished. A guard armed before the attempt runs on every exit +/// from the closure, early return and unwind alike, so the wait always ends. struct PinSignal<'a>(&'a AtomicU8); impl Drop for PinSignal<'_> { @@ -751,10 +790,12 @@ impl Drop for PinSignal<'_> { /// the return value -- unpins the thread at once, so the work that follows /// measures wherever the scheduler puts it while the row claims a processor. /// -/// Panics rather than warns on failure. A silently unpinned thread would turn +/// Refuses rather than warns on failure. A silently unpinned thread would turn /// a placement experiment into a measurement of the scheduler's preferences, /// and the run would still print a confident number -- the same failure mode as -/// a probe that asserts its conclusion. +/// a probe that asserts its conclusion. The refusal is returned rather than +/// raised, so the caller can render it into a report instead of dying on it; +/// what it must never do is continue. /// /// # Why not `SetThreadAffinityMask` /// @@ -763,14 +804,29 @@ impl Drop for PinSignal<'_> { /// processors that is not a matter of widening the mask; the call has no way to /// express the target at all. `SetThreadGroupAffinity` takes the group /// explicitly, and is the only way to pin across the whole machine. -fn pin_current_thread(cpu: Option<(u16, u8)>) -> AffinityGuard { +fn pin_current_thread(cpu: Option<(u16, u8)>) -> std::io::Result { let Some((group, number)) = cpu else { - return AffinityGuard { previous: None }; + return Ok(AffinityGuard { previous: None }); }; - assert!( - u32::from(number) < usize::BITS, - "processor number {number} does not fit a group affinity mask" - ); + // **An error rather than an assert, and for the same reason the OS failure + // below is one.** A group affinity mask is a `usize`, so on a 32-bit target + // a processor numbered 32 or above cannot be named at all -- that is a + // limit of the mask, not a caller mistake, and it is the condition the + // pinning tests use because it is the only one reachable deterministically + // on an ordinary host. Leaving it a panic would have left the fallible path + // with no test that could reach it. + if u32::from(number) >= usize::BITS { + return Err(refusal( + group, + number, + &format!( + "a group affinity mask on this target holds {} processors, so \ + processor {number} cannot be named in one", + usize::BITS + ), + MASK_TOO_NARROW, + )); + } let affinity = GROUP_AFFINITY { Mask: 1_usize << number, @@ -795,38 +851,83 @@ fn pin_current_thread(cpu: Option<(u16, u8)>) -> AffinityGuard { Reserved: [0; 3], }; let ok = unsafe { SetThreadGroupAffinity(GetCurrentThread(), &affinity, &raw mut previous) }; - // A raw string rather than an escaped-continuation one: `cargo fmt` - // reindents a multi-line string literal and the backslash continuations - // then swallow the blank lines, which turns a carefully laid-out message - // into one paragraph. This is the message a stranger sees when the tool - // gives up, so its shape matters. - assert!( - ok != 0, + if ok == 0 { + return Err(refusal( + group, + number, + &std::io::Error::last_os_error().to_string(), + ENVIRONMENT_REFUSED, + )); + } + + Ok(AffinityGuard { + previous: Some(previous), + }) +} + +/// The refusal a failed pin carries, as an error rather than a panic. +/// +/// **The decision to stop is unchanged; only the channel is.** The message +/// below is the one this helper has always produced, and the paragraph about +/// measuring without pinning is still the argument for refusing. What changed +/// is that a panic bypassed the probe's report sink: stdout carried a banner +/// and nothing else, so a survey mining it could not tell a host that declined +/// to be measured from a job that died for an unrelated reason. Returned as an +/// error, the same words reach the report, and the refusal becomes an +/// observation the fleet can count. +/// +/// **`explanation` is a parameter because the two causes are opposites, and one +/// message for both told a reader the wrong thing.** A `SetThreadGroupAffinity` +/// failure names a processor the topology reported, so something about the +/// environment is unexpected and a bug report is welcome. A number too wide for +/// an affinity mask is the exact reverse -- a limit of this build on this +/// target -- and the shared text told that reader the machine was misbehaving +/// and invited a report about a documented limitation. +/// +/// A raw string rather than an escaped-continuation one: `cargo fmt` reindents +/// a multi-line string literal and the backslash continuations then swallow the +/// blank lines, which turns a carefully laid-out message into one paragraph. +/// This is the message a stranger sees when the tool gives up, so its shape +/// matters. +fn refusal(group: u16, number: u8, cause: &str, explanation: &str) -> std::io::Error { + std::io::Error::other(format!( r" This run is stopping, and no measurement was taken. Could not confine a thread to processor {number} in group {group}: - {error} + {cause} -That processor was reported by this machine's own topology, so this is -unexpected rather than a limit of the tool. A process restricted to a -subset of processors -- by a job object, a container, or a `start /affinity` --- is the usual cause. +{explanation} The run stops rather than measuring without pinning. An unpinned thread measures wherever the scheduler happened to put it, which would produce a plausible number that answers a different question, and nothing in the output would say so. +" + )) +} + +/// Why a `SetThreadGroupAffinity` failure is worth reporting. +/// +/// The processor came from the machine's own topology, so the environment is +/// doing something the topology did not describe. +const ENVIRONMENT_REFUSED: &str = + "That processor was reported by this machine's own topology, so this is +unexpected rather than a limit of the tool. A process restricted to a +subset of processors -- by a job object, a container, or a `start /affinity` +-- is the usual cause. -Reporting this is genuinely useful: please include this message. -", - error = std::io::Error::last_os_error() - ); +Reporting this is genuinely useful: please include this message."; - AffinityGuard { - previous: Some(previous), - } -} +/// Why a processor too wide for an affinity mask is not worth reporting. +/// +/// The opposite case, and it must not borrow the sentence above: nothing is +/// wrong with the host, and inviting a bug report for a documented limit of the +/// tool wastes the reader's time and ours. +const MASK_TOO_NARROW: &str = + "A group affinity mask is one machine word wide, so this is a limit of this +build on this target rather than a fault in the machine. The host is fine; +this build cannot name that processor."; /// This thread's group affinity as the system currently reports it. /// @@ -848,10 +949,10 @@ fn current_affinity() -> Option { /// Puts the calling thread's affinity back when it goes out of scope. /// -/// A guard rather than a call at the end of the timed section, so an unwind -/// restores it too: a panic between pinning and restoring would otherwise leave -/// the thread confined for the rest of the process, and this crate's pinning -/// failure path panics by design. +/// A guard rather than a call at the end of the timed section, so an early +/// return or an unwind restores it too: either one between pinning and +/// restoring would otherwise leave the thread confined for the rest of the +/// process, and this crate's pinning failure path returns early by design. #[must_use = "the thread is unpinned as soon as this guard is dropped"] struct AffinityGuard { /// What to restore, or `None` when nothing was changed. diff --git a/crates/windows-placement-probe/src/peer_index_cache/tests.rs b/crates/windows-placement-probe/src/peer_index_cache/tests.rs index 46c511ac7..0e5d38de1 100644 --- a/crates/windows-placement-probe/src/peer_index_cache/tests.rs +++ b/crates/windows-placement-probe/src/peer_index_cache/tests.rs @@ -308,9 +308,12 @@ fn asking_for_no_pin_leaves_the_affinity_alone() { /// A processor number no group can hold, so `pin_current_thread` always fails. /// -/// 200 is past `usize::BITS`, which the pin asserts on before it ever reaches +/// 200 is past `usize::BITS`, which the pin refuses on before it ever reaches /// Windows -- deterministic on every machine rather than dependent on which -/// processors happen to be online. +/// processors happen to be online, and the only pin failure an ordinary +/// developer host can produce on demand. That makes it the only route by which +/// a test can reach the refusal path at all, which is why that condition +/// returns an error rather than asserting. const UNPINNABLE: (u16, u8) = (0, 200); /// Well inside the time either case takes when it works (both return in @@ -319,48 +322,119 @@ const MUST_FINISH_WITHIN: std::time::Duration = std::time::Duration::from_secs(2 #[test] fn a_failed_producer_pin_stops_the_run_rather_than_hanging_it() { - // **The defect this guards.** `pin_current_thread` panics on failure. When - // the producer was the one to fail, the consumer still entered `consume` - // and spun forever on items no living thread would ever write -- an - // unbounded loop with no deadline, so the process simply stopped making - // progress. A run that should have failed loudly hung instead, which in CI - // is a job timeout rather than a diagnosis. + // **The defect this guards.** When the producer was the one to fail, the + // consumer still entered `consume` and spun forever on items no living + // thread would ever write -- an unbounded loop with no deadline, so the + // process simply stopped making progress. A run that should have failed + // loudly hung instead, which in CI is a job timeout rather than a + // diagnosis. + // + // No `catch_unwind` any more: the refusal is a value, so the test asks for + // it directly instead of inferring it from an unwind. That is a stronger + // check, because an unwind proves only that *something* panicked. let started = std::time::Instant::now(); - let outcome = std::panic::catch_unwind(|| { - super::time_model_on(super::Strategy::Baseline, Some(UNPINNABLE), None) - }); + let outcome = super::time_model_on(super::Strategy::Baseline, Some(UNPINNABLE), None); - assert!( - outcome.is_err(), - "an impossible pin must not report success" - ); + let error = outcome.expect_err("an impossible pin must not report success"); assert!( started.elapsed() < MUST_FINISH_WITHIN, "the run did not terminate: {:?}", started.elapsed() ); + // The refusal has to say what it could not do, because it is now rendered + // into a report rather than printed by the panic handler. + let text = error.to_string(); + assert!( + text.contains("Could not confine a thread to processor 200 in group 0"), + "the refusal must name the processor it could not pin: {text}" + ); + assert!( + text.contains("no measurement was taken"), + "the refusal must say that nothing was measured: {text}" + ); } #[test] fn a_failed_consumer_pin_stops_the_run_rather_than_hanging_it() { // The other direction, and it hung for a different reason: the consumer - // was pinned *after* the producer had been spawned, so the panic unwound + // was pinned *after* the producer had been spawned, so the failure unwound // into `thread::scope`'s cleanup, which waits for a producer that is // itself blocked forever on a ring nobody is draining. let started = std::time::Instant::now(); - let outcome = std::panic::catch_unwind(|| { - super::time_model_on(super::Strategy::Baseline, None, Some(UNPINNABLE)) - }); + let outcome = super::time_model_on(super::Strategy::Baseline, None, Some(UNPINNABLE)); - assert!( - outcome.is_err(), - "an impossible pin must not report success" - ); + let error = outcome.expect_err("an impossible pin must not report success"); assert!( started.elapsed() < MUST_FINISH_WITHIN, "the run did not terminate: {:?}", started.elapsed() ); + assert!( + error + .to_string() + .contains("Could not confine a thread to processor 200 in group 0"), + "the refusal must name the processor it could not pin: {error}" + ); +} + +#[test] +fn a_refusal_to_pin_reaches_the_caller_of_measure() { + // `time_model_placed` is the deepest frame that can refuse, so this pins + // down the error it produces before the propagation tests below use it. + let error = super::time_model_placed(super::Strategy::Baseline, Some(UNPINNABLE), None, None) + .expect_err("an impossible pin must not report success"); + + assert_eq!( + error.kind(), + std::io::ErrorKind::Other, + "the refusal is a domain error, not an OS error code: {error}" + ); +} + +#[test] +fn a_refusal_during_the_warm_up_pass_stops_median_before_it_times_anything() { + // `median` takes an untimed warm-up sample and then `REPETITIONS` more. The + // warm-up's `?` is one of the two propagation points this commit added, and + // nothing reached either: the other `median` tests all supply `Ok`, and the + // pinning tests stop one frame below `median`. A fallible path with no test + // able to reach it is exactly what the `usize::BITS` conversion was argued + // on, so leaving these two untested would have repeated it. + let mut calls = 0_u32; + let outcome = super::median("under test", || { + calls += 1; + Err(std::io::Error::other("refused")) + }); + + assert!(outcome.is_err(), "the refusal must reach the caller"); + assert_eq!( + calls, 1, + "a refusal on the warm-up must stop there rather than going on to time \ + REPETITIONS samples against a machine that already declined" + ); +} + +#[test] +fn a_refusal_after_the_warm_up_stops_median_at_the_sample_that_refused() { + // The second propagation point: `collect::>>()` short + // circuits, so the samples after the refusal are never taken. Asserting the + // call count is what distinguishes short-circuiting from collecting every + // sample and returning the first error afterwards -- both return `Err`, and + // only one of them stops asking a machine that has already refused. + let mut calls = 0_u32; + let outcome = super::median("under test", || { + calls += 1; + if calls <= 2 { + Ok(sample_of(100.0)) + } else { + Err(std::io::Error::other("refused")) + } + }); + + assert!(outcome.is_err(), "the refusal must reach the caller"); + assert_eq!( + calls, 3, + "one warm-up, one good sample, then the refusal -- and nothing after it" + ); } // --------------------------------------------------------------------------- @@ -396,12 +470,13 @@ fn the_median_is_the_middle_sample_not_the_first_or_the_last() { // repetitions: the first is consumed and must not reach the result. let mut supplied = [900.0, 500.0, 100.0, 400.0, 300.0, 200.0].into_iter(); let run = super::median("under test", || { - sample_of( + Ok(sample_of( supplied .next() .expect("the median takes REPETITIONS + 1 samples"), - ) - }); + )) + }) + .expect("a supplied sample cannot refuse to pin"); // Timed samples are 500, 100, 400, 300, 200 -> sorted 100, 200, 300, 400, // 500 -> median 300. @@ -423,8 +498,9 @@ fn the_warm_up_pass_is_discarded_rather_than_measured() { // An absurd first value makes that visible: it must not move the answer. let mut supplied = [1e12, 100.0, 200.0, 300.0, 400.0, 500.0].into_iter(); let run = super::median("under test", || { - sample_of(supplied.next().expect("six samples")) - }); + Ok(sample_of(supplied.next().expect("six samples"))) + }) + .expect("a supplied sample cannot refuse to pin"); assert!( (run.nanos_per_item - 300.0 / super::ITEMS as f64).abs() < f64::EPSILON, @@ -445,8 +521,9 @@ fn the_two_rates_are_reciprocal_views_of_the_same_sample() { let nanos = 4_000_000.0; let mut supplied = std::iter::repeat_n(nanos, super::REPETITIONS + 1); let run = super::median("under test", || { - sample_of(supplied.next().expect("enough samples")) - }); + Ok(sample_of(supplied.next().expect("enough samples"))) + }) + .expect("a supplied sample cannot refuse to pin"); assert!( (run.nanos_per_item - nanos / super::ITEMS as f64).abs() < f64::EPSILON, diff --git a/crates/windows-platform-probes/CHECKLIST.md b/crates/windows-platform-probes/CHECKLIST.md index 812aa8ae5..2aa865987 100644 --- a/crates/windows-platform-probes/CHECKLIST.md +++ b/crates/windows-platform-probes/CHECKLIST.md @@ -195,34 +195,81 @@ correctness in the archive. **Blocker recorded when queued:** none. The dependency exists next door and is already proven by that crate's own tests. -- [ ] **M4.9** -- Route a placement probe's refusal-to-measure through the report sink instead of a +- [x] **M4.9** -- Route a placement probe's refusal-to-measure through the report sink instead of a panic, so a host that cannot be pinned is a reported observation rather than a crash. - **Gap:** `windows-placement-probe`'s pinning helper asserts on `SetThreadGroupAffinity`, and + **Gap:** `windows-placement-probe`'s pinning helper asserted on `SetThreadGroupAffinity`, and `core_affinity::measure()` reaches it through `time_model_on` / `time_model_placed`. **The - decision to stop is correct and is not what this item changes** -- the assert's own message makes - the argument, that an unpinned thread "would produce a plausible number that answers a different - question, and nothing in the output would say so." That is this repository's position and it - should stand. What is wrong is the *mechanism*: a panic bypasses `emit_report`, so the refusal - lands on stderr while stdout carries a truncated report with no row saying why it stopped. A - fleet survey mining stdout sees a probe that produced a banner and then nothing, which is - indistinguishable from a job that died for an unrelated reason -- the same "cannot tell a - measured absence from a missing run" defect that `report_unmeasured` exists to close for - discovery failure, left open for pinning failure. - - **Reachability changed with the probe binaries.** The assert predates them and `main` carries it - untouched, but `main` has no binary that reaches it; `probe-core-affinity` and - `probe-peer-index-cache` are the first consumers, so this is newly reachable rather than newly - written. - - **Target:** the pinning helper returns a domain error instead of asserting, `measure()` - propagates it, and the binaries render it after the banner the way they already render a - topology-discovery failure -- with an `x-probe-*` row carrying the refusal, so the absence is - mineable. The assert's message text is kept; it is the right message, in the wrong channel. - - **Blocker recorded when queued:** this changes a public signature in `windows-placement-probe`, - which the branch that introduced the probe binaries deliberately does not touch. Raised rather - than taken unilaterally, per the mono-repo bug policy. + decision to stop was correct and did not change** -- the assert's own message makes the argument, + that an unpinned thread "would produce a plausible number that answers a different question, and + nothing in the output would say so." What was wrong is the *mechanism*: a panic bypasses + `emit_report`, so the refusal landed on stderr while stdout carried a truncated report with no + row saying why it stopped. + + **Measured, before and after.** With every pin forced to fail, the old binary wrote three lines + to stdout -- banner, heading, blank -- and exited 101, with the explanation only on stderr. It + now writes the refusal itself to stdout and exits 0, matching how the same binary already reports + a topology-discovery failure. + + **Done:** `pin_current_thread` returns `io::Result`; `time_model`, `time_model_on`, + `time_model_placed`, `median` and `peer_index_cache::measure` propagate it; both probe binaries + handle it. The assert's message text is kept -- it was the right message, in the wrong channel. + `PinSignal` still publishes `PIN_FAILED`, now on the early-return path as well as on an unwind, + so the consumer never waits on a producer that has left. + + **Two things a later review corrected in the first attempt at this item, both the same mistake -- + a claim made about the code rather than from it.** + + `check_group_support` (then `assert_group_support`) tests the *same* predicate as the + `usize::BITS` branch, over every discovered processor, and runs before any pin. So converting + only the branch left this one deciding the outcome: the probe still died with banner, heading and + nothing else. Fixing one site of a rule and leaving its twin is the blast-radius miss this + repository has a rule against, and the before/after measurement recorded here did not catch it + because it forced the pin argument rather than the discovered set. Both sites now refuse. + + `peer_index_cache::measure` **cannot** refuse -- it starts only unpinned runs -- so its `# Errors` + section documented a failure mode no host can produce, and the entry here said "both probe + binaries render it" as though both could decline. Only `probe-core-affinity` pins. The signature + stays `Result` because its helpers are fallible in general; the docs now say that instead of the + opposite. + + **Also corrected:** the refusal text was shared by both causes while being written for one. For + the mask-width cause it claimed the processor "was reported by this machine's own topology, so + this is unexpected rather than a limit of the tool" and invited a bug report -- the exact reverse + of that cause, which *is* a limit of the tool. The explanation is now a parameter, verified in + both directions. + + **One correction to this item as written.** The blocker recorded when queuing was **wrong** -- + `windows-placement-probe` is `publish = false` and absent from + [.release-please-manifest.json](../../.release-please-manifest.json), and + [check-commit-scope.ps1](../../tools/check-commit-scope.ps1) says in terms that such a crate + "cannot be poisoned, because it is never released -- so it is not a finding." The `x-probe-*` row + it called for is deferred to `M4.10`, which turned out to be a larger question than these two + probes. + +- [ ] **M4.10** -- Decide whether a probe's report owes a machine-readable row, and make the answer + uniform. + + **Gap:** three of this crate's sixteen probe binaries emit an `x-probe-*` NDJSON line beside + their prose -- `doorbell_cost`, `request_cost`, and `topology` through `topology_report`. The + other thirteen emit prose only, `probe-core-affinity` and `probe-peer-index-cache` among them, + and so does `queue_contention` despite being the probe whose captures are analysed by committed + scripts. So a survey can mine three probes and must scrape or skip the rest. + + **This item was first written the other way round**, asserting that the two placement probes were + the exception and that `queue_contention` emitted a row. Both halves were false: prose-only is + the majority, by four to one. The correction changes what is being proposed -- not "bring two + stragglers up to the norm" but "the crate has two conventions and no stated rule for which + applies." + + **Target:** state the rule first, in [DESIGN-NOTES.md](DESIGN-NOTES.md), since it decides twelve + more binaries than it decides these two: which probes owe a row, and what a refusal looks like in + one. `report_unmeasured` already draws the "measured and declined" against "never ran" + distinction for the topology probe, and the cost probes' `EVERY_LABEL` arrangement is the worked + example for keeping a row and its prose from drifting. Then apply it. + + **Blocker recorded when queued:** none, but note this is a design decision before it is a code + change, so it wants the engineer's call on scope rather than a unilateral sweep. ## M5 -- Carried over from M2: unblocked hygiene diff --git a/crates/windows-platform-probes/src/bin/core_affinity.rs b/crates/windows-platform-probes/src/bin/core_affinity.rs index ef232349f..ee8c636d2 100644 --- a/crates/windows-platform-probes/src/bin/core_affinity.rs +++ b/crates/windows-platform-probes/src/bin/core_affinity.rs @@ -33,12 +33,24 @@ fn render(out: &mut dyn std::fmt::Write) { // in `main`'s argument list where it used to sit. A measurement called // before the renderer is entered is outside the sink entirely, so a host // where it fails gives a reader no banner and no indication of which probe - // died. `measure` reads the topology and can fail, which is exactly the - // case worth naming. + // died. + // + // **`measure` now has two ways to fail, and this must not name the wrong + // one.** It reads the topology, which can fail; and it pins each side to a + // chosen processor, which a job object or container can refuse. This arm + // used to say "could not read this machine's topology" for whatever came + // back, so a pin refusal would have been reported as a discovery failure -- + // a specific, checkable claim about the machine, made from an error that + // says something else. The refusal's own text explains itself, so the + // wording here stays neutral and lets it. let observation = &match measure() { Ok(observation) => observation, Err(error) => { - let _ = writeln!(out, "could not read this machine's topology: {error}"); + let _ = writeln!( + out, + "this host could not be measured:\n{}", + error.to_string().trim_end() + ); let _ = writeln!( out, "\nNothing below could be measured, so nothing below is reported. This is\n\ diff --git a/crates/windows-platform-probes/src/bin/peer_index_cache.rs b/crates/windows-platform-probes/src/bin/peer_index_cache.rs index 1d91c3446..29e2a3bc2 100644 --- a/crates/windows-platform-probes/src/bin/peer_index_cache.rs +++ b/crates/windows-platform-probes/src/bin/peer_index_cache.rs @@ -31,7 +31,32 @@ fn render(out: &mut dyn std::fmt::Write) { "== what does caching the peer's index buy an SPSC ring? ==\n" ); - let observation = measure(); + // Measured after the banner and heading are already out, so a host that + // refuses still gives a reader the line naming which probe declined and on + // what machine. + // + // **This arm is required by the type, not reachable today.** + // `peer_index_cache::measure` starts only unpinned runs, so nothing it does + // can currently return `Err`; the handler exists because the signature is + // `Result` and deleting it would mean discarding the error instead. Said + // here because an earlier version of this comment implied this binary could + // decline, which it cannot -- `probe-core-affinity` is the one that pins. + let observation = match measure() { + Ok(observation) => observation, + Err(error) => { + let _ = writeln!( + out, + "this host declined to be measured:\n{}", + error.to_string().trim_end() + ); + let _ = writeln!( + out, + "Nothing below could be measured, so nothing below is reported. This is\n\ + a refusal to measure the host, not a finding about it." + ); + return; + } + }; let _ = writeln!( out, From 99b4467409f2db06247514f590a33cf7ab9e2180 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Thu, 17 Sep 2026 17:00:00 -0400 Subject: [PATCH 03/11] docs: push every rule to the earliest rung that can enforce it Seven review rounds on this branch found 4, 4, 4, 5, 1, 1 and 6 defects. Rounds 2 through 5 each found a defect introduced by the immediately preceding round's fix, and round 7 found six in code written an hour earlier. The rounds were not finding new ground; they were finding the corrections. Classified by HOW they escaped rather than by what they were, one mechanism dominates: a predicate implemented at two or three sites and corrected at one. Three predicates, eight findings -- the directed pair against the undirected hop, a processor number against the affinity-mask width, and `Observed::Absent` against `Observed::NotObserved`. Every one was greppable inside a single file, and the file was in the commit each time. **So the rule was not missing. It was mis-filed.** CONTRACT INTEGRITY rule 3 already requires "grep the commit's other files for the same defect", but it opens "Before committing a change to a stated contract rule", sits under a heading about restatement drift, and sweeps a word across documents in its worked example. A `>=` in a function does not present itself as a stated contract rule, so the sweep never fires at the moment of need. A rule filed where it will not be recalled is indistinguishable, in outcome, from one that does not exist -- which is the argument for not relying on recall at all. Hence the ladder, stated in copilot-instructions.md: the build, then unit tests on every developer's machine, then integration tests before a milestone closes, then CI as the last resort. The order is the cost order -- how much a defect costs once it escapes the rung above -- and prose is not a rung, because a rule in a comment is enforced by whoever remembers it. It generalises this file's own "Prefer to discharge the rule in a type, where no caller can see it", which made the same argument for a single rule. Six rules follow from it, each written because it was violated repeatedly in work that had already passed several review rounds: give a two-site rule one site; sabotage where the real condition enters; test that a guard rejects AND accepts; give every new error edge a test that traverses it; re-read the doc comment on the item whose contract you just changed; and produce any claim that counts repository artifacts from a command rather than from memory. **Three of them are encoded here rather than only written down.** `is_nameable_in_a_mask` and `undirected` replace two copies each of an expression that had already been half-converted once. Verified: one edit to the shared mask predicate now reaches `check_group_support` in the other module, which is precisely what was impossible before -- that split is what let a fix be measured as working while the binary still died with a banner and nothing else. `the_probes_that_emit_a_machine_readable_row` runs every registered probe and asserts which ones emit a row. It exists because a checklist item claimed four probes emit one and that two were the exception; three do, one of the four named does not, and prose-only is the majority four to one. A census of our own tree is not a measurement of the world, so recollection must not produce it. **That test was a unit test grepping `src/bin` until a review took it apart, and the correction is the more useful half of this commit.** The grep was unsound twice. The walk was shallow, so it never saw `queue_contention` -- whose source is nested one directory down, and which is precisely the probe whose classification the test existed to pin down. And a bare substring matched `topology.rs`, whose only mention of the tag is a `//!` comment; its row comes from `topology_report` in the library. **The asserted set was correct while neither half of the method was.** No proxy over the source could fix that, because the emission may live in any module the binary calls. Emitting a row is a property of a probe's OUTPUT, so the rung that can enforce it is the one crossing a process boundary: run all sixteen registered binaries and read what they print. That costs about ten seconds, excluding the contention benchmark at roughly sixty-five, which is censused by the weaker source question and labelled as weaker. The binary list comes from `Cargo.toml`, so a probe added tomorrow joins the census without anyone remembering. Sabotage-verified in all three directions the old test got wrong: a comment-only mention no longer flips it, a real new emitter is caught by name, and an emission added to the nested probe -- a file the old walk never opened -- is caught. **A later round found it launching a probe documented as unsafe to launch from a test.** `probe-cancel-io` measures a call that can fail to return, and its module doc draws the conclusion itself: "Binary only, and deliberately not a test ... a wedged `#[test]` would take the whole suite with it." It guards each case with an internal watchdog; `Command::output` waits on the child with none, so running it here reintroduced exactly the hazard that probe is shaped to avoid. It is now excluded and source-censused alongside the benchmark. `NOT_RUN` became a list of (name, reason) pairs in the same change, because the single bare name carried its reason only in a doc comment above it -- and an exclusion whose reason is not attached to it is indistinguishable from an oversight, which is what the second one would have looked like. So the ladder gains a clause it did not have: **take the lowest rung the fact supports, not the lowest rung available.** A rule pushed below that point does not become cheap, it becomes decoration that reads like enforcement, and this commit shipped one until it was reviewed. Also recorded: the hazard is partial conversion, not duplication. Before the change that broke it, the mask-width predicate was implemented twice and both sites panicked -- consistent, no defect, and six review rounds correctly saw nothing. Converting one created the inconsistency. "Duplicated logic" is therefore the wrong thing to hunt; the reviewable event is a change that makes two agreeing sites disagree. **And the note explaining the anti-restatement rule was restating a rule**, which a later round caught and which is the third defect this commit introduced into its own subject. `DESIGN-NOTES.md` opened its ladder section with "it does not restate the rule" and then reproduced all four rungs under it, in near-identical words -- "run on every developer's machine, every time, in under a second", "after a push, on a machine the author is not at, in a log they must go and read". That is the pattern CONTRACT INTEGRITY forbids for an analysis document, and it is the rule's own first hazard set up in the file that explains it. The rungs are now enumerated once, in the rule. The note keeps the argument for the ORDERING, which the list does not carry: the order is how much a defect costs between being written and its author reading about it, which is what puts CI last despite being the most thorough gate. Swept the class rather than the instance -- the normative clause about which rung to take was also stated twice inside the rule itself, and is now stated once. Its per-round tally went too. "Seven review rounds, finding 4, 4, 4, 5, 1, 1 and 6" moved with every round while the note existed, which is precisely the drift rule 4 describes. The shape is the finding and the shape is what it now states: each round's defects were predominantly in the code the previous round had just written to fix its findings. Unrelated to the ladder, found in an earlier review and fixed here because it is one line of the same class: the NUMA table prints a baseline and a cached timing side by side as a comparison at one placement, but computed its redirection marker from the baseline alone. A cached allocation that landed elsewhere, or whose placement could not be determined, printed under an unqualified `node N`. The marker now names which run missed -- `!base`, `!cached` or `!both`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/copilot-instructions.md | 115 ++++++++++ DESIGN-NOTES.md | 104 +++++++++ .../src/core_affinity.rs | 29 +-- .../src/peer_index_cache.rs | 52 ++++- .../src/bin/core_affinity.rs | 86 +++++--- ...probes_that_emit_a_machine_readable_row.rs | 199 ++++++++++++++++++ 6 files changed, 530 insertions(+), 55 deletions(-) create mode 100644 crates/windows-platform-probes/tests/the_probes_that_emit_a_machine_readable_row.rs diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index f35201ad6..072fe08db 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1351,6 +1351,121 @@ crate that happens to publish measurements. Every instance found so far has been existing decision rather than a gap in it. Apply it while writing: no checker can find these, because nothing is inconsistent. +## FAIL FAST — push every rule to the earliest rung that can enforce it + +CONTRACT INTEGRITY above tells you to keep restatements in step. This tells you where to put the +enforcement, and it applies to **code** as much as to prose. The rationale and the evidence are in +[DESIGN-NOTES.md](../DESIGN-NOTES.md) -> [Push every rule down the detection ladder](../DESIGN-NOTES.md#detection-ladder); +what follows is the rule. + +**The ladder, strongest first. Always ask what the next rung down would cost, and take it if you +can afford it:** + +1. **The build.** A defect that cannot compile cannot ship. Prefer making a mistake + *unrepresentable* over making it detectable: one definition rather than two agreeing ones, a + type that excludes the bad state, a `const` assertion. This is the only rung that cannot be + skipped, run stale, or pass on a machine that never exercised the path. +2. **Unit tests.** They run on **every developer's machine, every time**, in under a second. A rule + that lands here is checked by everyone who touches the tree, including the person who has never + read the rule. +3. **Integration tests.** Reserved for what genuinely crosses a process, filesystem, device or OS + boundary (see "Quality" below). These run before a milestone closes, so a defect here is caught + days after it is written rather than minutes. +4. **CI.** The **last** resort, not the first. CI catches a defect after it is pushed, on a machine + the author is not sitting at, in a log they have to go and read. A rule that exists only in CI + has already cost the author a context switch before it says anything. + +**Take the lowest rung the fact supports, not the lowest rung available.** The ladder is about +where a rule *can* be enforced, and a rule pushed below that point does not become cheap -- it +becomes decoration that reads like enforcement. A property of a program's output cannot be +established by a proxy over its source; a property of one run cannot be established by a constant. +When the honest rung is expensive, pay it or say plainly what is left unchecked. + +**Prose is not a rung.** A rule that lives only in a comment, a design note or a checklist is +enforced by whoever happens to remember it, which over a long change is nobody. + +Six specific rules follow. Each is written because it was violated, repeatedly, in work that had +already passed several review rounds; each names the rung it belongs on. + +### 1. Never half-convert a rule that lives at two sites -- give it one site + +**Two consistent copies of a predicate are not a defect. The defect is the next change reaching one +of them.** This is the single most expensive pattern measured here: one predicate at two or three +sites, corrected at one, three separate times over three distinct predicates. + +- **Rung: the build.** Extract the predicate to one function and the split becomes impossible + rather than merely discouraged. `is_nameable_in_a_mask` and `undirected` in + `windows-placement-probe` / `windows-platform-probes` are the worked examples; each replaced two + copies of an expression that had already been half-converted once. +- Before changing a predicate, **grep for the expression, not the identifier** -- a duplicated rule + usually has no shared name, which is exactly why it was duplicated. +- CONTRACT INTEGRITY rule 3 already required this sweep and says so in one clause; it is repeated + here because its section is framed around *stated claims*, so a `>=` in a function does not + announce itself as "a stated contract rule" and the sweep never fires. + +### 2. Sabotage must enter where the real condition enters + +A sabotage that forces an internal variable proves only that the site you already changed responds. +**Inject at the input the real condition would arrive through**, and confirm the failure reaches +your change from there. + +Worked example of getting it wrong: a pinning refusal was verified by forcing the processor +*argument* inside the pinning helper. That bypassed a second check, over the *discovered processor +set*, which ran first and still panicked -- so the change was measured as working while the +binary's behaviour was unchanged. Sabotaging the discovered set would have shown it immediately. + +### 3. A guard is bidirectional; test that it rejects AND that it accepts + +Every guard has two failure modes and testing one is the normal mistake. A census that proves +`A subset B` says nothing about `B subset A`; a recogniser that accepts everything valid may also +accept things the producer cannot emit; a message shared by two causes is wrong for at least one. + +- **Rung: unit tests.** Assert both directions in the same test, or the one-directional half will + be written and the other forgotten. +- This generalises the `expect: "survives"` control the sabotage harness already requires -- a + manifest with no control can only tell you the tests are sensitive, never that they are sensitive + to the right thing. + +### 4. Every new error edge needs a test that traverses it + +A `?`, an `Err` return, or a new match arm is a path. **If no test can reach it, it is not +implemented, it is only written.** When a path genuinely cannot be reached on any host, say so at +the definition and say what would reach it -- do not leave it silently untested. + +Prefer a deterministic reachable condition over a host-dependent one, and be willing to convert an +assert into an error *for the sake of testability* when that is the only condition a developer +machine can produce on demand. + +### 5. When you change an item's contract, re-read that item's own doc comment first + +The nearest restatement of any contract is the `///` block attached to it, and it is the easiest to +skip because you are looking at the body. Measured: every `//` comment in a file was updated and +both `///` headers -- on the two items whose contract the change altered -- were left asserting the +behaviour that had just been removed. + +Do this before the wider sweep, not after. + +### 6. A claim that counts or enumerates repository artifacts must come from a command + +"Every other probe emits a row", "several of its six placement rows", "three probes do X" -- these +are facts about our own tree, not measurements of the world, so **the build can establish them and +recollection must not.** Run the command, paste nothing, and where the claim matters, bind it: + +- **Rung: integration tests**, and the first attempt got that wrong, which is the lesson. + `the_probes_that_emit_a_machine_readable_row` in `windows-platform-probes` runs every registered + probe binary and asserts which ones emit a row. It replaced a *unit* test that walked `src/bin` + and grepped for a substring -- cheaper, and unsound twice over: the walk was shallow, so it never + saw the one probe whose classification the test existed to pin down, and a bare substring matched + a probe whose only mention of the tag is a `//!` comment. **The asserted set was correct while + neither half of the method was.** +- **This is the ladder clause above, in its worked failure.** Emitting a row is a property of a + probe's *output*, so no proxy over its source can establish it -- the emission may live in any + module the binary calls. Reaching for the cheaper rung produced a test that looked like + enforcement and was decoration. +- This is distinct from CONTRACT INTEGRITY rule 4, which governs *measured* numbers. A census of + our own source has no artifact to cite and no measurement error; it is simply either checked or + invented. + ## REVIEW FEEDBACK — answer it where it was raised, not only in the commit **A review round is not finished when the code changes. It is finished when the reviewer has diff --git a/DESIGN-NOTES.md b/DESIGN-NOTES.md index bcb098811..7b285cc8a 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -1908,3 +1908,107 @@ that has never been wrong while leaving the prose that keeps being wrong in prop was taken, which findings it is drawn from, what was rejected on the way, and the one cheap remedy that was costed but not adopted are in [DESIGN-RATIONALE.md](DESIGN-RATIONALE.md#why-restatement-count-is-what-is-watched). + +## Push every rule down the detection ladder + +The normative rule is in +[copilot-instructions.md](.github/copilot-instructions.md) -> "FAIL FAST -- push every rule to the +earliest rung that can enforce it". This records why it exists and what it was measured against; +it does not restate the rule. + +### Why the order is the cost order + +The rungs, and what each one costs, are enumerated once in the rule -- follow the link above rather +than looking for them here. **An earlier revision of this section restated all four under a +sentence saying it would not**, which is the defect the rule's own first hazard describes and which +this note exists to explain, so it is worth recording that it survived a review round in this very +file. + +What belongs here is the argument for the ordering, which is not obvious from the list: the order +is not a preference about tooling, and not a statement about which mechanism is most rigorous. It +is the order of **how much a defect costs once it escapes the rung above** -- measured from the +moment it is written to the moment its author reads about it. That framing is what makes CI last +rather than first despite being the most thorough gate: thoroughness is not the axis. + +This extends +[A failable call has its failure handled, always](#a-failable-call-has-its-failure-handled-always) +-> "Prefer to discharge the rule in a type, where no caller can see it", which made the same +argument for one rule. The ladder is that argument generalised. + +### What it was measured against + +Successive independent review rounds on one branch +(`mikegrier/platform-probes-cost-and-placement`). The per-round counts are not restated here -- +they moved with every round while this note existed, which is exactly the drift CONTRACT INTEGRITY +rule 4 describes, and they are recoverable from the branch's commit messages. + +The shape is the finding, and it held across every round but one: **each round's defects were +predominantly in the code the previous round had just written to fix its findings.** The rounds +were not reaching new ground; they were finding the corrections. One round broke the pattern by +surfacing a pre-existing condition instead, and even that one was reachable only because of code +the branch had added. + +Classifying them by **how they escaped** rather than by what they were gives one dominant +mechanism: a predicate implemented at two or three sites and corrected at one. Three distinct +predicates, eight findings between them. + +| predicate | sites | rounds it recurred in | +|---|---|---| +| directed pair vs undirected hop | 3 | 1, 3, 4 | +| processor number against affinity-mask width | 2 | 6, 7 | +| `Observed::Absent` against `Observed::NotObserved` | 3 | 2, 4, 7 | + +Every one was greppable within a single file. The sweep that would have found them was already +required -- CONTRACT INTEGRITY rule 3, "grep the commit's other files for the same defect" -- and +the file was in the commit each time. So the rule was not missing. **It was mis-filed:** rule 3 +opens "Before committing a change to a stated contract rule", sits under a heading about +restatement drift, and its worked example sweeps a word across documents. A `>=` in a function does +not present itself as a stated contract rule, so the sweep never fires at the moment of need. + +That is the finding worth keeping from the exercise: a rule filed where it will not be recalled is +indistinguishable, in outcome, from a rule that does not exist. Hence the ladder -- the remedy is +not to file it better but to stop relying on recall. + +### The hazard is partial conversion, not duplication + +Worth stating separately because it exonerates six review rounds that looked at the code and +correctly saw nothing. Before the change that broke it, the mask-width predicate was implemented +twice and **both sites panicked** -- consistent, no defect, nothing to find. Converting one to a +returned refusal created the inconsistency, and the probe went on dying at the other with a banner +and nothing else while the change was recorded as verified. + +So "duplicated logic" is the wrong thing to hunt. The reviewable event is a change that makes two +agreeing sites disagree, and the durable remedy is to leave only one site to change. + +### Altitude is the question none of the rules asked + +Across the rounds the reviewers kept describing findings in the same shape, which the rules had no +word for: "still crashes the probe, one frame higher"; "loses the census in the reverse direction"; +"count against *which* disclaimer, not against disclaimed-or-not". Each asks whether a fix sits at +the level the rule lives at, or at the level the author happened to be editing. The six rules the +instructions now carry are that question, made specific enough to act on. + +### Taking a rung below the one the fact supports + +The ladder says take the lowest rung you can afford. It does **not** say take the lowest rung, and +the first attempt at encoding the census rule got that wrong in a way worth recording, because the +result passed and looked like enforcement. + +"Which probes emit a machine-readable row" is a property of a probe's *output*. The first encoding +was a unit test that walked `src/bin` and grepped each file for the tag -- cheaper by a rung, and +unsound twice: the walk was shallow, so it never saw `queue_contention`, whose source is nested one +directory down and which is exactly the probe whose classification the test was written to pin +down; and a bare substring matched `topology.rs`, whose only mention of the tag is a `//!` comment, +its row being emitted from `topology_report` in the library. The asserted set was correct while +neither half of the method was. + +No proxy over the source could have fixed it, because the emission may live in any module the +binary calls. The honest rung is the one that crosses a process boundary: run every registered +probe and read its output, which costs about ten seconds and answers the question asked. A rule +pushed below the rung its fact supports does not become cheap; it becomes decoration. + +**No work is scheduled by this note**, per "design notes are not a work queue". The rules it +explains are binding where they are stated; the encodings taken at the time of writing -- +`is_nameable_in_a_mask` and `undirected` as single definitions, and an integration test asserting +which probes emit a machine-readable row -- are in the commits that added this section and the one +that corrected it. diff --git a/crates/windows-placement-probe/src/core_affinity.rs b/crates/windows-placement-probe/src/core_affinity.rs index 219ca2c90..1d280db12 100644 --- a/crates/windows-placement-probe/src/core_affinity.rs +++ b/crates/windows-placement-probe/src/core_affinity.rs @@ -82,7 +82,9 @@ use std::io::ErrorKind; use windows_topology_sys::MachineMemoryTopology; use crate::fingerprint::{Fingerprint, ProcessorPlace, Slice, places_from_topology}; -use crate::peer_index_cache::{ITEMS, Strategy, time_model_on, time_model_placed}; +use crate::peer_index_cache::{ + ITEMS, Strategy, is_nameable_in_a_mask, mask_width, time_model_on, time_model_placed, +}; /// Repetitions per placement; the median is reported. /// @@ -555,25 +557,24 @@ pub fn representative_pairs( /// /// If the discovered processors cannot be measured as they are. /// -/// **This is the same predicate `pin_current_thread` checks, and converting only -/// that one left this one deciding the outcome.** It runs over every discovered -/// processor *before* any pin is attempted, so a machine with a processor too -/// wide for an affinity mask died here -- with the banner, the heading and -/// nothing else on stdout -- and never reached the branch that had just been -/// made to report it. Fixing one site of a rule and leaving its twin is how the -/// fix came to be measured as working while the binary's behaviour was -/// unchanged. +/// **This asks the same predicate `pin_current_thread` asks, and it now asks it +/// through the same function.** It runs over every discovered processor *before* +/// any pin, so when the two were separate expressions, converting only that one +/// from a panic to a refusal left this one deciding the outcome -- the probe +/// still died with banner, heading and nothing else, and never reached the +/// branch that had just been made to report it. Neither site was wrong; the +/// hazard was changing one of them. fn check_group_support(processors: &[ProcessorPlace]) -> std::io::Result<()> { if processors.is_empty() { return Err(std::io::Error::other( "no processors were discovered, so there is nothing to measure", )); } - // Every discovered processor must be pinnable. A number at or above the - // width of an affinity mask cannot be expressed in one, and measuring the - // rest while dropping it would report a machine smaller than the real one. + // Every discovered processor must be pinnable. A number a mask cannot name + // cannot be pinned to, and measuring the rest while dropping it would + // report a machine smaller than the real one. for place in processors { - if u32::from(place.number) >= usize::BITS { + if !is_nameable_in_a_mask(place.number) { return Err(std::io::Error::other(format!( " This run is stopping, and no measurement was taken. @@ -588,7 +589,7 @@ this build cannot name that processor. Measuring the rest while dropping it would report a machine smaller than the real one, so the run stops instead. ", - width = usize::BITS + width = mask_width() ))); } } diff --git a/crates/windows-placement-probe/src/peer_index_cache.rs b/crates/windows-placement-probe/src/peer_index_cache.rs index 4e1599a4c..58e91f7c2 100644 --- a/crates/windows-placement-probe/src/peer_index_cache.rs +++ b/crates/windows-placement-probe/src/peer_index_cache.rs @@ -808,21 +808,28 @@ fn pin_current_thread(cpu: Option<(u16, u8)>) -> std::io::Result let Some((group, number)) = cpu else { return Ok(AffinityGuard { previous: None }); }; - // **An error rather than an assert, and for the same reason the OS failure - // below is one.** A group affinity mask is a `usize`, so on a 32-bit target - // a processor numbered 32 or above cannot be named at all -- that is a - // limit of the mask, not a caller mistake, and it is the condition the - // pinning tests use because it is the only one reachable deterministically - // on an ordinary host. Leaving it a panic would have left the fallible path - // with no test that could reach it. - if u32::from(number) >= usize::BITS { + // **The predicate lives in `is_nameable_in_a_mask`, not here.** It is also + // asked by `core_affinity::check_group_support` over every discovered + // processor, and when the two were separate expressions, converting this one + // from a panic to a refusal left that one deciding the outcome -- so the fix + // changed nothing a binary could reach. One definition cannot be + // half-converted. + // + // An error rather than an assert, and for the same reason the OS failure + // below is one: a group affinity mask is a `usize`, so on a 32-bit target a + // processor numbered 32 or above cannot be named at all. That is a limit of + // the mask, not a caller mistake, and it is the condition the pinning tests + // use because it is the only one reachable deterministically on an ordinary + // host. Leaving it a panic would have left the fallible path with no test + // that could reach it. + if !is_nameable_in_a_mask(number) { return Err(refusal( group, number, &format!( "a group affinity mask on this target holds {} processors, so \ processor {number} cannot be named in one", - usize::BITS + mask_width() ), MASK_TOO_NARROW, )); @@ -865,6 +872,33 @@ fn pin_current_thread(cpu: Option<(u16, u8)>) -> std::io::Result }) } +/// Whether a processor number can be named in a group affinity mask. +/// +/// **One definition, because the split between two is exactly how a fix came to +/// be measured as working while the binary's behaviour was unchanged.** This +/// predicate lived inline in `pin_current_thread` and again in +/// `core_affinity::check_group_support`, which runs over every discovered +/// processor *before* any pin. Converting only the first from a panic to a +/// refusal changed nothing a binary could reach: the run still died at the +/// second, with the banner, the heading and nothing else. +/// +/// Both sites were consistent before that change and neither was wrong. The +/// hazard is **partial conversion** -- and a shared predicate makes the two +/// impossible to get out of step, which is stronger than remembering to grep. +#[must_use] +pub fn is_nameable_in_a_mask(number: u8) -> bool { + u32::from(number) < usize::BITS +} + +/// How many processors a group affinity mask can name on this target. +/// +/// Reported by both refusals, so the reader is told the actual bound rather +/// than being left to infer it from a pointer width. +#[must_use] +pub fn mask_width() -> u32 { + usize::BITS +} + /// The refusal a failed pin carries, as an error rather than a panic. /// /// **The decision to stop is unchanged; only the channel is.** The message diff --git a/crates/windows-platform-probes/src/bin/core_affinity.rs b/crates/windows-platform-probes/src/bin/core_affinity.rs index ee8c636d2..730151c71 100644 --- a/crates/windows-platform-probes/src/bin/core_affinity.rs +++ b/crates/windows-platform-probes/src/bin/core_affinity.rs @@ -600,6 +600,26 @@ interpretation: } } +/// The undirected hop a directed pair belongs to. +/// +/// **One definition, because two copies is how this went wrong twice.** A hop is +/// measured in both directions, so `(0, 1)` and `(1, 0)` are one hop. Forgetting +/// that made a single-pair guard unreachable in one review round; the fix for +/// that wrote the rule inline, and a later round then attributed a spread to +/// "the hops" when it came entirely from reversing one hop -- because the fix +/// for *that* wrote the same expression a second time, fifty lines below the +/// first, instead of calling it. +/// +/// Two consistent copies are not themselves a defect. The defect is the next +/// change reaching one of them, and a function cannot be half-converted. +fn undirected(pair: (u32, u32)) -> (u32, u32) { + if pair.0 <= pair.1 { + (pair.0, pair.1) + } else { + (pair.1, pair.0) + } +} + /// Print the per-node-pair handoff cost, when the host has nodes to cross. /// /// Silent on a single-node machine: there is nothing to say, and a header over @@ -627,7 +647,8 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio // names. let _ = writeln!( out, - " (`ring on` is the node requested; `!` means it landed elsewhere)" + " (`ring on` is the node requested; `!base`, `!cached` or `!both` names\n \ + any run whose memory did not land there)" ); // Keyed on the ring placement as well as the pair, because the table above @@ -666,13 +687,30 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio // apart. format!("{} -> {}", pair.0, pair.1), // The requested node, matching the key above and the probe - // crate's own report. A trailing `!` marks a row whose memory - // did not land where it was asked to go, so a redirected run - // is not read as a measurement of the placement it names. - match (base.requested_memory_node, base.memory_node) { - (Some(asked), Some(got)) if asked == got => format!("node {asked}"), - (Some(asked), _) => format!("node {asked}!"), - (None, _) => "unspecified".to_owned(), + // crate's own report. A trailing marker names any run whose + // memory did not land where it was asked to go, so a redirected + // run is not read as a measurement of the placement it names. + // + // **Both runs, not just the baseline.** The row prints a + // baseline and a cached timing side by side as a comparison at + // one placement, but the marker was computed from `base` alone + // -- so a cached allocation that was redirected, or whose + // placement could not be determined, printed under an + // unqualified `node N` beside a baseline that did land there. + // The reader was shown a same-placement comparison that was not + // achieved, which is the defect the marker exists to prevent, + // applied to one of the two columns only. + match base.requested_memory_node { + None => "unspecified".to_owned(), + Some(asked) => { + let landed = |node: Option| node == Some(asked); + match (landed(base.memory_node), landed(cached.memory_node)) { + (true, true) => format!("node {asked}"), + (false, true) => format!("node {asked}!base"), + (true, false) => format!("node {asked}!cached"), + (false, false) => format!("node {asked}!both"), + } + } }, format!("g{}/cpu{}", base.producer.group, base.producer.number), format!("g{}/cpu{}", base.consumer.group, base.consumer.number), @@ -696,16 +734,7 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio // host and even and >= 2 otherwise, so `== 1` never held and a two-node // machine fell into the spread analysis below, where it compared a hop // against its own reverse and reported the two as different hops. - let mut hops: Vec<(u32, u32)> = pairs - .iter() - .map(|pair| { - if pair.0 <= pair.1 { - (pair.0, pair.1) - } else { - (pair.1, pair.0) - } - }) - .collect(); + let mut hops: Vec<(u32, u32)> = pairs.iter().copied().map(undirected).collect(); hops.sort_unstable(); hops.dedup(); @@ -750,19 +779,12 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio // What varied between the two extremes decides what the spread is evidence // ABOUT, and it is not always the hop. // - // **Compared as UNDIRECTED hops, because that is the unit the guard above - // just counted.** Directed equality made `0 -> 1` and `1 -> 0` different - // hops, so a spread that came entirely from reversing one hop was reported - // as the hops not being interchangeable -- the same directed/undirected - // confusion the guard above exists to fix, reintroduced in the code that - // fixed it. - let canonical = |pair: (u32, u32)| { - if pair.0 <= pair.1 { - (pair.0, pair.1) - } else { - (pair.1, pair.0) - } - }; + // **Compared as UNDIRECTED hops, through the same `undirected` the guard + // above uses.** Directed equality made `0 -> 1` and `1 -> 0` different hops, + // so a spread that came entirely from reversing one hop was reported as the + // hops not being interchangeable -- the same confusion the guard above + // exists to fix, reintroduced a few dozen lines below it as a second copy of + // the same expression. There is now one. if best_pair == worst_pair && best_ring == worst_ring { // One row was both extremes, so there is no spread to describe: this is // a single measurement, not a comparison. @@ -771,7 +793,7 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio " Both extremes are the same row, so the table holds one\n \ measurement per hop and this restates it rather than comparing." ); - } else if canonical(best_pair) == canonical(worst_pair) { + } else if undirected(best_pair) == undirected(worst_pair) { let _ = writeln!( out, " Both extremes are the same hop, so that spread is within it --\n \ diff --git a/crates/windows-platform-probes/tests/the_probes_that_emit_a_machine_readable_row.rs b/crates/windows-platform-probes/tests/the_probes_that_emit_a_machine_readable_row.rs new file mode 100644 index 000000000..40bc17b00 --- /dev/null +++ b/crates/windows-platform-probes/tests/the_probes_that_emit_a_machine_readable_row.rs @@ -0,0 +1,199 @@ +// Copyright (c) Mike Grier. + +//! Which probes carry a machine-readable row, established by running them. +//! +//! **This replaces a unit test that asked the wrong question and got the right +//! answer.** That test walked `src/bin` and grepped each file for the string +//! `x-probe`. It was wrong twice: the walk was shallow, so it never saw +//! `queue_contention`, whose source is `src/bin/queue_contention/main.rs` -- and +//! `queue_contention` is precisely the probe whose classification the test was +//! written to stop a document getting wrong. And a bare substring matched +//! `topology.rs`, whose only mention of `x-probe` is a `//!` comment; its row is +//! emitted from `topology_report`, in the library. The asserted set happened to +//! be correct while neither half of the method was. +//! +//! Emitting a row is a property of a probe's OUTPUT, so a proxy over its source +//! cannot establish it, and a tighter proxy would not either: the emission may +//! live in any module the binary calls. The rung that can enforce this is the +//! one that crosses a process boundary, which is this one. +//! +//! The binary list comes from `Cargo.toml` rather than from a list kept here, so +//! a probe added tomorrow joins the census without anyone remembering to add it. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +/// The probes whose report carries an `x-probe-*` NDJSON line. +/// +/// Measured, not recalled. A document that describes the split -- `M4.10` in +/// CHECKLIST.md is the current one -- is corrected by this failing rather than +/// by somebody noticing. +const EMIT_A_ROW: [&str; 3] = [ + "probe-doorbell-cost", + "probe-request-cost", + "probe-topology", +]; + +/// The probes this test does not run, and why each one is excluded. +/// +/// A list of pairs rather than a bare name, because an exclusion without its +/// reason is indistinguishable from an oversight -- and the second entry here +/// was exactly that until a review found it. +/// +/// - `probe-queue-contention` is the contention benchmark: a host-dependent run +/// its own module doc puts at about sixty-five seconds, with no short mode. +/// Ten seconds for the rest is worth paying every time; seventy-five is not. +/// - `probe-cancel-io` measures a call that **can fail to return**. Its own +/// module doc says so and draws the conclusion: "Binary only, and deliberately +/// not a test ... a wedged `#[test]` would take the whole suite with it." It +/// guards each case with an internal watchdog, but `Command::output` waits on +/// the child without one, so running it here would reintroduce the hazard that +/// probe is shaped to avoid. +/// +/// Both are still censused, by the one question a source can answer soundly -- +/// whether the emission literal appears anywhere in their sources at all. That +/// is weaker than running them, and it is stated as weaker rather than blended +/// in with the rest. +const NOT_RUN: [(&str, &str); 2] = [ + ("probe-queue-contention", "src/bin/queue_contention"), + ("probe-cancel-io", "src/bin/cancel_io.rs"), +]; + +/// Every `[[bin]]` name the manifest registers. +fn registered_binaries(manifest: &str) -> Vec { + let mut names = Vec::new(); + let mut in_bin = false; + for line in manifest.lines() { + let line = line.trim(); + if line.starts_with('[') { + in_bin = line == "[[bin]]"; + continue; + } + if !in_bin { + continue; + } + if let Some(rest) = line.strip_prefix("name") { + let value = rest.trim_start().trim_start_matches('=').trim(); + names.push(value.trim_matches('"').to_owned()); + } + } + names +} + +fn crate_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) +} + +/// The directory cargo put this run's binaries in. +/// +/// Taken from a binary cargo itself resolved, so it is right under `--release`, +/// under a custom `CARGO_TARGET_DIR`, and inside cargo-mutants' scratch copy. +fn binary_directory() -> PathBuf { + Path::new(env!("CARGO_BIN_EXE_probe-topology")) + .parent() + .expect("a built binary has a parent directory") + .to_path_buf() +} + +/// Every `.rs` file under `root`, at any depth. +fn walk(root: &Path) -> Vec { + let mut found = Vec::new(); + let Ok(entries) = std::fs::read_dir(root) else { + return found; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + found.extend(walk(&path)); + } else if path.extension().is_some_and(|ext| ext == "rs") { + found.push(path); + } + } + found +} + +#[test] +fn the_probes_that_emit_a_machine_readable_row_are_the_ones_we_say_they_are() { + let manifest = std::fs::read_to_string(crate_root().join("Cargo.toml")) + .expect("the crate's manifest is readable"); + let registered = registered_binaries(&manifest); + assert!( + registered.len() > 1, + "no `[[bin]]` targets were read from the manifest, so this test checked \ + nothing -- the manifest's shape has probably changed" + ); + for (name, _) in NOT_RUN { + assert!( + registered.iter().any(|registered| registered == name), + "`{name}` is excluded by name, so it must still be a registered \ + target; if it was renamed, this exclusion is silently covering a \ + probe nobody is censusing" + ); + } + + let directory = binary_directory(); + let mut emitting: Vec = Vec::new(); + for name in ®istered { + if NOT_RUN.iter().any(|(excluded, _)| excluded == name) { + continue; + } + let executable = directory.join(format!("{name}{}", std::env::consts::EXE_SUFFIX)); + let output = Command::new(&executable) + .output() + .unwrap_or_else(|error| panic!("{} could not be run: {error}", executable.display())); + assert!( + output.status.success(), + "{name} exited {:?}; a probe that cannot run cannot be censused", + output.status.code() + ); + // The row as it is actually written, not the bare tag: a probe that only + // mentions `x-probe` in prose is not a probe that emits one, and that + // exact confusion is why this test moved off a source grep. + if String::from_utf8_lossy(&output.stdout).contains(r#""reason":"x-probe"#) { + emitting.push(name.clone()); + } + } + emitting.sort(); + + assert_eq!( + emitting, EMIT_A_ROW, + "the set of probes emitting a machine-readable row has changed; update \ + `EMIT_A_ROW` and every document that describes the split, rather than \ + leaving the two to disagree" + ); + + // The excluded probes, by the weaker question. If an emission literal ever + // appears in their sources this stops being answerable without running + // them, and the assertion says so rather than quietly going stale. + for (name, source_path) in NOT_RUN { + let path = crate_root().join(source_path); + let sources: Vec = if path.is_dir() { + walk(&path) + .iter() + .map(|file| std::fs::read_to_string(file).unwrap_or_default()) + .collect() + } else { + vec![std::fs::read_to_string(&path).unwrap_or_else(|error| { + panic!( + "{}'s source at {} is readable: {error}", + name, + path.display() + ) + })] + }; + assert!( + !sources.is_empty(), + "no sources were found for `{name}` at {}, so its half of the \ + census checked nothing", + path.display() + ); + assert!( + sources + .iter() + .all(|source| !source.contains(r#""reason":"x-probe"#)), + "`{name}` has gained an emission literal, so the source question can \ + no longer stand in for running it -- either run it here and accept \ + the cost, or state plainly that it is uncensused" + ); + } +} From f839d58feee2693c161dbc51f9e09cf1e558d775 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Thu, 17 Sep 2026 18:07:50 -0400 Subject: [PATCH 04/11] fix(platform-probes): disclose when the banner and the rows describe different machines `probe-core-affinity` wrote its banner from one topology discovery and measured its rows from another. `core_affinity::measure` reads the topology itself and returns the shape it used, and nothing compared the two -- so a processor going offline, or moving group or node, between the banner and the end of the measurement produced a report whose header names one machine and whose body describes a different one, with nothing in the output saying so. **`Observation::host` exists for exactly this, and its own rustdoc says so:** it reports "the shape this run actually saw" so that a caller "can compare the two and refuse". `windows-placement-probe`'s own binary does compare, and refuses, calling the alternative "a splice of two machines ... with nothing in the file saying so and a reader interpreting the rows through the wrong machine". The probe added on this branch consumed the same library and did neither. This is the defect class three earlier rounds were spent on in `attribution`, for the topology probe -- which machine does the body describe, and what is the report allowed to claim when the readings disagree. That probe ended up with a three-reading disclaimer mechanism. This one shipped with no check at all, so the lesson had been learned in one renderer and not carried to the new one. It discloses rather than refuses, which is the difference between the two binaries: `placement-probe` writes a corpus record a runner consented to, so a mismatch invalidates it. This is a fleet-survey report, and the rows were still measured -- a reader told which machine they belong to can use them. What must not happen is being told nothing. The banner is still written before the measurement, so a run that dies still names its machine; the reconciliation happens after `measure` returns. Verified both ways on this host: silent when the shapes agree, and naming both shapes when the comparison is forced. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/bin/core_affinity.rs | 39 ++++++++++++++++++- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/crates/windows-platform-probes/src/bin/core_affinity.rs b/crates/windows-platform-probes/src/bin/core_affinity.rs index 730151c71..3be17331b 100644 --- a/crates/windows-platform-probes/src/bin/core_affinity.rs +++ b/crates/windows-platform-probes/src/bin/core_affinity.rs @@ -19,10 +19,16 @@ fn render(out: &mut dyn std::fmt::Write) { // First line of the report, and part of the returned text rather than // written out here: a captured report must carry the line naming the // machine that produced it, and the taint marker with it. + // + // **Kept, rather than taken from the measurement, so a run that dies still + // names its machine** -- and then reconciled below, because the banner and + // the rows would otherwise come from two separate discoveries with nothing + // saying so. + let announced = windows_placement_probe::fingerprint::Fingerprint::discover(); let _ = writeln!( out, "{}", - windows_placement_probe::fingerprint::banner_line() + windows_placement_probe::fingerprint::banner_line_for(&announced) ); let _ = writeln!( out, @@ -60,7 +66,36 @@ fn render(out: &mut dyn std::fmt::Write) { } }; - let _ = writeln!(out, "processors, as discovered:"); + // **The banner and the rows must describe the same machine, or the report is + // a splice of two.** The banner came from the discovery above; every row + // came from the one `measure` took. A processor going offline, or moving + // group or node, between them is enough to produce a report whose header + // names one machine and whose body describes another, with nothing saying + // so -- and `Observation::host` exists precisely so a caller can notice, + // its own rustdoc saying it "lets the caller compare the two and refuse". + // + // `windows-placement-probe`'s own binary does refuse here, because it is + // writing a corpus record a runner consented to. This one is a fleet-survey + // report, so it discloses instead: the rows below were still measured, and + // a reader told which machine they belong to can use them. What must not + // happen is the reader being told nothing. + if let Ok(announced) = &announced + && *announced != observation.host + { + let _ = writeln!( + out, + "\nHOST CHANGED DURING THE RUN: the banner above and the rows below\n\ + describe different machines, so which one the measurements belong to\n\ + was not established.\n \ + banner: {announced}\n \ + measured: {}\n\ + The rows below were measured on the second. Read them through it, or\n\ + run again on a machine that is not changing shape.", + observation.host + ); + } + + let _ = writeln!(out, "\nprocessors, as discovered:"); let _ = writeln!( out, " {:>8} {:>16} {:>13}", From 2ad67cc423eda6e2719229d512feac895a811379 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Thu, 17 Sep 2026 16:06:03 -0700 Subject: [PATCH 05/11] fix(platform-probes): stop two verdicts claiming more than their figures support Two renderers asserted conclusions their own adjacent numbers did not carry. **The warming control decided a throughput question from a read count.** `probe-peer-index-cache` prints the control's throughput ratio and its shared-read ratio on one line, then branched on the read count alone and closed with "A discarded load cannot help" -- a claim about speed, reached without consulting the speed. Measured on this host: one run in twelve reported 1.10x throughput directly under that sentence. What the control establishes is the read count, so that is what it now claims; the throughput is reported beside it and left alone, because a single pair of runs cannot separate a real effect from this probe's own spread and saying which it is would be the same over-claim pointed the other way. **The NUMA spread compared hops without holding the ring placement fixed.** Every hop is measured twice, once with the ring on the producer's node and once on the consumer's, and the extrema were taken globally across both. So the cheapest row could be producer-local on one hop while the dearest was consumer-local on another, and their ratio spanned a change of hop AND a change of locality -- then got reported as evidence about hops. This is the third depth of one error. An earlier round fixed it for direction (`0 -> 1` against `1 -> 0` compared as different hops), and another for the case where both extrema land on the same hop. Neither reaches the case where the hops genuinely differ and the locality differs too, which is the one that matters most because locality dominates the measurement. Each locality is now compared against itself, and only a locality spanning two or more hops can say anything about hops at all; a run where no single placement covers two hops says so instead of inventing a comparison. Verified by driving the renderer with synthetic three-node rows built to defeat the old logic -- every hop within one percent at matching placements, ten-fold between placements. The same rows give 10.1x and "the hops are NOT interchangeable" under a global minimum and maximum, against 1.0x and "close to equidistant at this placement" now. This host has one NUMA node, so that renderer cannot run here otherwise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/bin/core_affinity.rs | 204 ++++++++++++------ .../src/bin/peer_index_cache.rs | 31 ++- 2 files changed, 168 insertions(+), 67 deletions(-) diff --git a/crates/windows-platform-probes/src/bin/core_affinity.rs b/crates/windows-platform-probes/src/bin/core_affinity.rs index 3be17331b..6df8c9175 100644 --- a/crates/windows-platform-probes/src/bin/core_affinity.rs +++ b/crates/windows-platform-probes/src/bin/core_affinity.rs @@ -655,6 +655,51 @@ fn undirected(pair: (u32, u32)) -> (u32, u32) { } } +/// Where a row's ring sat relative to the two ends of its hop. +/// +/// The axis that has to be held fixed before two hops can be compared. A hop +/// measured with the ring on the producer's node and the same hop measured with +/// it on the consumer's are different costs, so a minimum drawn from one and a +/// maximum from the other says nothing about the hops. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Locality { + /// The ring was asked for the producer's node. + Producer, + /// The ring was asked for the consumer's node. + Consumer, + /// Asked for a third node, or not asked for one at all. + Elsewhere, +} + +impl Locality { + fn of(pair: (u32, u32), requested: Option) -> Self { + match requested { + Some(node) if node == pair.0 => Self::Producer, + Some(node) if node == pair.1 => Self::Consumer, + _ => Self::Elsewhere, + } + } + + fn label(self) -> &'static str { + match self { + Self::Producer => "with the ring on the producer's node", + Self::Consumer => "with the ring on the consumer's node", + Self::Elsewhere => "with the ring elsewhere", + } + } +} + +/// One measured node-pair row, reduced to what the spread analysis needs. +struct NodeRow { + /// The undirected hop, which is the unit hops are compared as. + hop: (u32, u32), + /// The directed pair, kept so a row can be named the way the table names it. + directed: (u32, u32), + locality: Locality, + ring: Option, + nanos: f64, +} + /// Print the per-node-pair handoff cost, when the host has nodes to cross. /// /// Silent on a single-node machine: there is nothing to say, and a header over @@ -686,16 +731,16 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio any run whose memory did not land there)" ); - // Keyed on the ring placement as well as the pair, because the table above - // measures every hop TWICE -- once with the ring on the producer's node and - // once on the consumer's -- and the comment there says so. Recording only - // the pair swept the min and the max over both placements while labelling - // each with a hop alone, so the cheapest and dearest rows could be the SAME - // hop at two ring placements, and the spread between them was then reported - // as evidence that hops differ. That is a difference attributed to a factor - // that did not vary. - let mut slowest: Option<(f64, (u32, u32), Option)> = None; - let mut fastest: Option<(f64, (u32, u32), Option)> = None; + // **Every row, not just the extremes, because the extremes alone cannot say + // what varied.** Taking a global minimum and maximum mixes two factors: a + // hop can be measured with the ring on the producer's node or the + // consumer's, and those are different costs. The cheapest row may be + // producer-local on one hop while the dearest is consumer-local on another, + // so their ratio spans a change of hop AND a change of locality -- and + // attributing it to the hops is the same error as reporting a direction + // reversal as a hop difference, one level further out. Holding locality + // fixed is what makes a hop comparison a comparison. + let mut rows: Vec = Vec::new(); for pair in &pairs { for base in observation.node_pair_rows(*pair, Strategy::Baseline) { @@ -753,13 +798,13 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio cached.nanos_per_item, cached.consumer_batch ); - let seen = (base.nanos_per_item, *pair, base.requested_memory_node); - if slowest.is_none_or(|(worst, _, _)| seen.0 > worst) { - slowest = Some(seen); - } - if fastest.is_none_or(|(best, _, _)| seen.0 < best) { - fastest = Some(seen); - } + rows.push(NodeRow { + hop: undirected(*pair), + directed: *pair, + locality: Locality::of(*pair, base.requested_memory_node), + ring: base.requested_memory_node, + nanos: base.nanos_per_item, + }); } } @@ -783,71 +828,110 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio return; } - let (Some((worst, worst_pair, worst_ring)), Some((best, best_pair, best_ring))) = - (slowest, fastest) - else { + // **The comparison is made WITHIN a ring locality, never across them.** A + // global minimum and maximum span whatever varied between those two rows, + // and two things vary here: which hop, and where the ring sat. On a machine + // where locality dominates -- every hop cheap producer-local and dear + // consumer-local -- the cheapest row and the dearest row can be one percent + // apart at matching placements while their ratio is tenfold, and reporting + // that as "the hops are not interchangeable" attributes to the hops a + // difference the hops did not make. + // + // So each locality is compared against itself, and only a locality that + // actually spans two or more hops can say anything about hops at all. + let mut verdict: Option<(Locality, &NodeRow, &NodeRow)> = None; + for locality in [Locality::Producer, Locality::Consumer, Locality::Elsewhere] { + let within: Vec<&NodeRow> = rows.iter().filter(|row| row.locality == locality).collect(); + let mut spanned: Vec<(u32, u32)> = within.iter().map(|row| row.hop).collect(); + spanned.sort_unstable(); + spanned.dedup(); + if spanned.len() < 2 { + continue; + } + let (Some(best), Some(worst)) = ( + within + .iter() + .copied() + .min_by(|a, b| a.nanos.total_cmp(&b.nanos)), + within + .iter() + .copied() + .max_by(|a, b| a.nanos.total_cmp(&b.nanos)), + ) else { + continue; + }; + // The widest within-locality spread is the strongest hop evidence the + // run holds, so that is the one reported. + let wider = verdict.is_none_or(|(_, previous_best, previous_worst)| { + worst.nanos / best.nanos > previous_worst.nanos / previous_best.nanos + }); + if wider { + verdict = Some((locality, best, worst)); + } + } + + // `hops.len()`, not `pairs.len()`. `pairs` is directed, so a three-node host + // has six entries for the three hops the guard above just counted -- and + // printing the directed count here made two adjacent paragraphs describe one + // machine with two different numbers. + let Some((locality, best, worst)) = verdict else { + let _ = writeln!( + out, + "\n {} node hop(s), but no single ring placement covers two of them,\n \ + so this run cannot compare hops: every pair of rows differs in where\n \ + the ring sat as well as which hop it crossed.", + hops.len() + ); + let _ = writeln!( + out, + " This measures the handoff between two nodes; it is not a distance\n \ + matrix read from firmware. Windows exposes no NUMA distance table, so\n \ + these numbers are the observable rather than a restatement of ACPI." + ); return; }; let ring = |node: Option| match node { Some(node) => format!("ring on node {node}"), None => "ring unspecified".to_owned(), }; - // `hops.len()`, not `pairs.len()`. `pairs` is directed, so a three-node host - // has six entries for the three hops the guard above just counted -- and - // printing the directed count here made two adjacent paragraphs describe one - // machine with two different numbers. + let spread = worst.nanos / best.nanos; let _ = writeln!( out, - "\n {} node hop(s). Cheapest {} -> {} ({}) at {:.1} ns/item; dearest\n \ + "\n {} node hop(s). Compared {}, which is the widest spread any single\n \ + placement shows: cheapest {} -> {} ({}) at {:.1} ns/item; dearest\n \ {} -> {} ({}) at {:.1} ns/item -- a spread of {:.1}x.", hops.len(), - best_pair.0, - best_pair.1, - ring(best_ring), - best, - worst_pair.0, - worst_pair.1, - ring(worst_ring), - worst, - worst / best + locality.label(), + best.directed.0, + best.directed.1, + ring(best.ring), + best.nanos, + worst.directed.0, + worst.directed.1, + ring(worst.ring), + worst.nanos, + spread ); - // What varied between the two extremes decides what the spread is evidence - // ABOUT, and it is not always the hop. - // - // **Compared as UNDIRECTED hops, through the same `undirected` the guard - // above uses.** Directed equality made `0 -> 1` and `1 -> 0` different hops, - // so a spread that came entirely from reversing one hop was reported as the - // hops not being interchangeable -- the same confusion the guard above - // exists to fix, reintroduced a few dozen lines below it as a second copy of - // the same expression. There is now one. - if best_pair == worst_pair && best_ring == worst_ring { - // One row was both extremes, so there is no spread to describe: this is - // a single measurement, not a comparison. - let _ = writeln!( - out, - " Both extremes are the same row, so the table holds one\n \ - measurement per hop and this restates it rather than comparing." - ); - } else if undirected(best_pair) == undirected(worst_pair) { + if undirected(best.directed) == undirected(worst.directed) { let _ = writeln!( out, " Both extremes are the same hop, so that spread is within it --\n \ - between its two directions, its two ring placements, or both -- and\n \ - says nothing about how the hops compare." + between its two directions -- and says nothing about how the hops\n \ + compare." ); - } else if worst / best < 1.2 { + } else if spread < 1.2 { let _ = writeln!( out, " That spread is small enough that this host's nodes are close to\n \ - equidistant, so the single `cross NUMA node` row above is a fair\n \ - summary of it." + equidistant at this placement, so the single `cross NUMA node` row\n \ + above is a fair summary of it." ); } else { let _ = writeln!( out, - " The hops are NOT interchangeable, so the single `cross NUMA node`\n \ - row above reports whichever one was enumerated first and should not\n \ - be read as 'the' cost of leaving a node." + " The hops are NOT interchangeable at this placement, so the single\n \ + `cross NUMA node` row above reports whichever one was enumerated\n \ + first and should not be read as 'the' cost of leaving a node." ); } let _ = writeln!( diff --git a/crates/windows-platform-probes/src/bin/peer_index_cache.rs b/crates/windows-platform-probes/src/bin/peer_index_cache.rs index 29e2a3bc2..069f96608 100644 --- a/crates/windows-platform-probes/src/bin/peer_index_cache.rs +++ b/crates/windows-platform-probes/src/bin/peer_index_cache.rs @@ -356,30 +356,47 @@ fn render(out: &mut dyn std::fmt::Write) { }; let warm_reduction = baseline.consumer_refreshes as f64 / warmed.consumer_refreshes.max(1) as f64; + let warm_throughput = baseline.nanos_per_item / warmed.nanos_per_item; let _ = writeln!(out); let _ = writeln!( out, - " control (warming load): {:.2}x throughput, {:.2}x fewer consumer reads.", - baseline.nanos_per_item / warmed.nanos_per_item, - warm_reduction + " control (warming load): {warm_throughput:.2}x throughput, \ + {warm_reduction:.2}x fewer consumer reads." ); if warm_reduction < 1.5 { + // **The read count is what makes this a control; it is not a statement + // about speed.** This arm used to close with "a discarded load cannot + // help", which is a claim about throughput, decided entirely by the read + // count and contradicted by the figure printed one line above it + // whenever the run happened to come out faster. Measured on this host: + // one run in twelve reported 1.10x throughput under that sentence. + // + // What the control establishes is exactly the read count, so that is + // what is claimed. The throughput is reported beside it and left to the + // reader, because a single pair of runs cannot separate a real effect + // from this probe's own spread -- and saying which it is would be the + // same over-claim in the other direction. + let _ = writeln!( + out, + " It removed no shared read, which is what a control should do: the" + ); let _ = writeln!( out, - " It removed no shared read, which is what a control should do. A" + " authoritative load still happens, so the technique's saving has to" ); let _ = writeln!( out, - " discarded load cannot help: the authoritative load still happens," + " come from REMOVING that load rather than from warming it." ); let _ = writeln!( out, - " and in a tight handoff loop the prefetch has no time to land." + " The throughput figure above is not part of that: this run does not" ); let _ = writeln!( out, - " So the technique works by REMOVING the load, not by warming it." + " separate a warming effect from its own run-to-run spread, and one" ); + let _ = writeln!(out, " pair of runs is not enough to try."); } else { let _ = writeln!( out, From 7b54bc50710274316d05da484c546b686dbafe5d Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Thu, 17 Sep 2026 16:33:58 -0700 Subject: [PATCH 06/11] fix(platform-probes): correct four claims their own cited sources contradict An unguided review found no defect in runtime behaviour and four statements that do not survive a read of what they cite. Three are documentation; one is a selection rule whose comment describes a different quantity from the one it computes. **`request_cost.rs` attributed to `D-18` a word that decision retracts, and a claim it never makes.** The module doc said `D-18` "records keeping it over the cheaper lexical alternative, and says plainly that whether it enters the kernel is not established". `D-18` carries a paragraph headed "No cost comparison is claimed, and that is deliberate", whose content is the retraction of exactly that adjective -- "an earlier draft of this decision called the alternatives 'cheaper' ... so the word was a guess wearing the clothes of a measurement". And the string `kernel` does not occur in that section at all; the hedge belongs to this probe's own timing loop, not to the decision. Worse, it was a regression the branch introduced: `main`'s text read "records keeping it over the canonicalizers that do not root. It claims no cost comparison against those alternatives, because nothing here measures them", which is faithful. This branch replaced a correct summary with an incorrect one while its commit message claimed the paragraph had been checked against `D-18`. `main`'s wording is restored. **This crate's `DESIGN-NOTES.md` still said pinning failures panic.** That contract was removed two commits ago; this was the only surviving statement of it in the tree. It is also precisely the failure this branch's own new rule 5 names -- re-read the changed item's own documentation first -- landing in the design note for the probe whose contract changed. **The ladder rule and its design note both said the census runs every registered probe.** It runs fourteen of sixteen. The test itself is careful about this, its exclusion list carrying each reason and saying the source fallback is "weaker than running them, and it is stated as weaker rather than blended in with the rest"; the two restatements dropped the qualification. That is the count-claim rule 6 governs, inside rule 6's own worked example. **The NUMA hop verdict selected on a quantity its comment did not describe.** Within one locality a hop appears twice -- `0 -> 1` with the ring on node 0 and `1 -> 0` with it on node 1 are both producer-local -- so the widest spread could be one hop's two directions, and the renderer would then report the run as unable to compare hops while discarding a comparison another locality held. Restricting candidates to distinct hops exposed a second layer: pairing one hop's fastest row against another's slowest spans a direction difference as well as a hop difference and reports the sum as the hop's. On rows built for it that read 25x where the hops differ by 6x. Each hop is now reduced to its fastest row at that locality and the spread taken across those. Verified against both cases -- the direction-asymmetry one now reads 6.0x, and round 12's locality-dominated one still reads 1.0x and "close to equidistant". One claim is corrected here rather than where it was made, because the commit carrying it is four deep and rewording it needs an interactive rebase this repository forbids. `cb45ed62`'s message says taking `ci.yml` was warranted because "nothing on main ran them". Main's workflow already contained thirteen probe steps; what it lacked was the build gate, and zero steps carried it. The scope decision was right and the reason given for it was not. `M2.7`'s archived entry in COMPLETED-CHECKLIST.md states the position accurately and needed no change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/copilot-instructions.md | 3 +- DESIGN-NOTES.md | 9 ++-- .../windows-platform-probes/DESIGN-NOTES.md | 6 ++- .../src/bin/core_affinity.rs | 51 ++++++++++++++----- .../src/request_cost.rs | 11 ++-- 5 files changed, 59 insertions(+), 21 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 072fe08db..c617efb26 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1453,7 +1453,8 @@ recollection must not.** Run the command, paste nothing, and where the claim mat - **Rung: integration tests**, and the first attempt got that wrong, which is the lesson. `the_probes_that_emit_a_machine_readable_row` in `windows-platform-probes` runs every registered - probe binary and asserts which ones emit a row. It replaced a *unit* test that walked `src/bin` + probe binary except two it names with a reason, and asserts which ones emit a row. It replaced a + *unit* test that walked `src/bin` and grepped for a substring -- cheaper, and unsound twice over: the walk was shallow, so it never saw the one probe whose classification the test existed to pin down, and a bare substring matched a probe whose only mention of the tag is a `//!` comment. **The asserted set was correct while diff --git a/DESIGN-NOTES.md b/DESIGN-NOTES.md index 7b285cc8a..05ce2f797 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -2003,9 +2003,12 @@ its row being emitted from `topology_report` in the library. The asserted set wa neither half of the method was. No proxy over the source could have fixed it, because the emission may live in any module the -binary calls. The honest rung is the one that crosses a process boundary: run every registered -probe and read its output, which costs about ten seconds and answers the question asked. A rule -pushed below the rung its fact supports does not become cheap; it becomes decoration. +binary calls. The honest rung is the one that crosses a process boundary: run the registered probes +and read their output, which costs about ten seconds and answers the question asked. Two are +excluded by name and by reason -- one too slow to run every time, one documented as unsafe to launch +from a test at all -- and censused by the weaker source question instead, which the test states as +weaker rather than blending in with the rest. A rule pushed below the rung its fact supports does +not become cheap; it becomes decoration. **No work is scheduled by this note**, per "design notes are not a work queue". The rules it explains are binding where they are stated; the encodings taken at the time of writing -- diff --git a/crates/windows-platform-probes/DESIGN-NOTES.md b/crates/windows-platform-probes/DESIGN-NOTES.md index 851c48193..104adce84 100644 --- a/crates/windows-platform-probes/DESIGN-NOTES.md +++ b/crates/windows-platform-probes/DESIGN-NOTES.md @@ -812,9 +812,11 @@ the finer conclusion; on such a host several of its placement rows come back `n/ a placement as inexpressible is deliberately not the same as reporting that it made no difference. -Two construction notes. **Pinning failures panic** rather than warn: a silently unpinned thread turns a +Two construction notes. **Pinning failures refuse** rather than warn: a silently unpinned thread turns a placement experiment into a measurement of the scheduler's preferences while still printing a confident -number. And **batch depth is read from the cached runs only** -- the baseline strategy reads the shared +number. The refusal is returned and rendered into the report rather than raised, so a host that cannot +be pinned is a reported observation rather than a probe that died with a banner and nothing under it; +the argument for stopping is unchanged, only the channel it travels on. And **batch depth is read from the cached runs only** -- the baseline strategy reads the shared line on every operation by definition, so its depth is ~1 at every placement and carries no information. An earlier revision compared the baseline depths and duly reported 0.8 against 0.4, which is noise around a constant being read as a finding. diff --git a/crates/windows-platform-probes/src/bin/core_affinity.rs b/crates/windows-platform-probes/src/bin/core_affinity.rs index 6df8c9175..9d8451c16 100644 --- a/crates/windows-platform-probes/src/bin/core_affinity.rs +++ b/crates/windows-platform-probes/src/bin/core_affinity.rs @@ -848,20 +848,49 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio if spanned.len() < 2 { continue; } + // **One representative per hop, so the comparison is like against + // like.** Two corrections are folded in here, and the second only became + // visible once the first was made. + // + // A plain minimum and maximum within a locality can land on the two + // directions of a single hop -- `0 -> 1` with the ring on node 0 and + // `1 -> 0` with it on node 1 are both producer-local -- and that pair + // would then win the "widest" contest, making the run report itself + // unable to compare hops while discarding a comparison it did hold. + // + // Restricting to pairs on different hops fixes that and leaves a subtler + // version: pairing one hop's fastest row against another hop's slowest + // still spans a direction difference as well as a hop difference, and + // reports the sum as the hop's. Measured on synthetic rows built for it, + // that read 25x where the hops differ by 6x. Each hop is therefore + // reduced to its fastest row at this locality -- the measurement least + // perturbed by whatever else the machine was doing -- and the spread is + // taken across those. Direction asymmetry within a hop is two adjacent + // rows of the table above; it is not what this paragraph compares. + let representatives: Vec<&NodeRow> = spanned + .iter() + .filter_map(|hop| { + within + .iter() + .copied() + .filter(|row| row.hop == *hop) + .min_by(|a, b| a.nanos.total_cmp(&b.nanos)) + }) + .collect(); let (Some(best), Some(worst)) = ( - within + representatives .iter() .copied() .min_by(|a, b| a.nanos.total_cmp(&b.nanos)), - within + representatives .iter() .copied() .max_by(|a, b| a.nanos.total_cmp(&b.nanos)), ) else { continue; }; - // The widest within-locality spread is the strongest hop evidence the - // run holds, so that is the one reported. + // The widest cross-hop spread any one placement shows is the strongest + // hop evidence the run holds, so that is the one reported. let wider = verdict.is_none_or(|(_, previous_best, previous_worst)| { worst.nanos / best.nanos > previous_worst.nanos / previous_best.nanos }); @@ -912,14 +941,12 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio worst.nanos, spread ); - if undirected(best.directed) == undirected(worst.directed) { - let _ = writeln!( - out, - " Both extremes are the same hop, so that spread is within it --\n \ - between its two directions -- and says nothing about how the hops\n \ - compare." - ); - } else if spread < 1.2 { + // No same-hop arm: the selection above only considers pairs on different + // hops, so the two extremes cannot be one hop's two directions. A direction + // asymmetry within a hop is still visible -- it is two adjacent rows of the + // table above -- but it is not what this paragraph is comparing, and an arm + // that could never fire would be a claim about a state the code excludes. + if spread < 1.2 { let _ = writeln!( out, " That spread is small enough that this host's nodes are close to\n \ diff --git a/crates/windows-platform-probes/src/request_cost.rs b/crates/windows-platform-probes/src/request_cost.rs index 35c178ab9..28ed13ca8 100644 --- a/crates/windows-platform-probes/src/request_cost.rs +++ b/crates/windows-platform-probes/src/request_cost.rs @@ -99,9 +99,14 @@ //! The owning crate now settles both halves rather than leaving them to be //! re-derived from a probe: see `windows-namespace-request-sys`' //! [DESIGN-NOTES.md](../../windows-namespace-request-sys/DESIGN-NOTES.md) -> -//! `D-18`, which states what the call actually does, records keeping it over -//! the cheaper lexical alternative, and says plainly that whether it enters the -//! kernel is not established. +//! `D-18`, which states what the call actually does and records keeping it over +//! the canonicalizers that do not root. It claims no cost comparison against +//! those alternatives, because nothing here measures them -- but it does settle +//! the mechanism question this module once left open: resolving a +//! drive-relative path for another drive checks that drive's recorded entry +//! against the filesystem, and writes the entry back **when that check rejects +//! it** -- an accepted entry is left alone, so the write is conditional rather +//! than part of every such resolution. //! //! The two schemes that might reduce it recover different halves. **Inline //! storage** removes the allocation and copy, which is what From 8a0b18f8f7f67639f9dfa62da5bb8722623c00f0 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Thu, 17 Sep 2026 17:01:25 -0700 Subject: [PATCH 07/11] fix(platform-probes): bracket the whole placement run, and keep redirected rows out of the verdict Two more cases where a conclusion rested on something narrower than it claimed. Both are in the NUMA path, which this host cannot execute, so both were verified against synthetic rows built for them. **The host-change disclosure watched the wrong window.** It compared the banner reading against `Observation::host` -- but `core_affinity::measure` derives that fingerprint from the topology it reads at the START, before any pair is timed. So the check bracketed the gap before the work and left the work itself, which is by far the longer part, unwatched: a processor going offline mid-measurement left both readings equal and the report unqualified. A third reading is taken after `measure` returns, and all three must agree. This is the same bracket `topology_report::attribution` puts around the topology probe's discovery, applied to the measurement that actually takes time. A bracket read that fails is reported rather than counted as agreement, since a failed read establishes nothing either way. Verified by perturbing the after-reading: it names which of the three differs, and the two-reading check it replaces was silent on that input by construction -- it never read that value. **A row whose memory landed somewhere else still carried a locality verdict.** The table marks such a row `!base`, and the row was then classified by the node it REQUESTED and fed into the locality-controlled hop comparison. So the probe could print "the hops are NOT interchangeable at the producer's placement" from a timing that was not taken at that placement -- the exact confound the locality split exists to remove, re-entering through a row the renderer had already identified as redirected. Rows that did not land are now excluded from the comparison and counted, and the report says how many and why, so a reader tallying the table against the paragraph is not left to wonder. Measured on rows built for it: a redirected 900 ns row against two that landed 1% apart gave 90.0x and "NOT interchangeable"; it now gives 1.0x, "close to equidistant", and a line saying one row was set aside. Verified no false alarm on this host: three runs, no spurious disclosure from either guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/bin/core_affinity.rs | 103 ++++++++++++++---- 1 file changed, 79 insertions(+), 24 deletions(-) diff --git a/crates/windows-platform-probes/src/bin/core_affinity.rs b/crates/windows-platform-probes/src/bin/core_affinity.rs index 9d8451c16..9202c0e35 100644 --- a/crates/windows-platform-probes/src/bin/core_affinity.rs +++ b/crates/windows-platform-probes/src/bin/core_affinity.rs @@ -74,24 +74,49 @@ fn render(out: &mut dyn std::fmt::Write) { // so -- and `Observation::host` exists precisely so a caller can notice, // its own rustdoc saying it "lets the caller compare the two and refuse". // - // `windows-placement-probe`'s own binary does refuse here, because it is - // writing a corpus record a runner consented to. This one is a fleet-survey - // report, so it discloses instead: the rows below were still measured, and - // a reader told which machine they belong to can use them. What must not - // happen is the reader being told nothing. - if let Ok(announced) = &announced - && *announced != observation.host - { - let _ = writeln!( - out, - "\nHOST CHANGED DURING THE RUN: the banner above and the rows below\n\ - describe different machines, so which one the measurements belong to\n\ + // **Three readings, not two, because `Observation::host` is taken at the + // START of the measurement.** `core_affinity::measure` discovers the + // topology, derives `host` from it, and only then runs every timed pair -- + // so comparing the banner against `host` brackets the gap before the work + // and leaves the work itself, which is the long part, unwatched. A reading + // taken after `measure` returns closes that window. This is the same + // bracket `topology_report::attribution` puts around the topology probe's + // measurement, applied to the one that takes far longer. + // + // `windows-placement-probe`'s own binary refuses on a mismatch, because it + // is writing a corpus record a runner consented to. This one is a + // fleet-survey report, so it discloses instead: the rows below were still + // measured, and a reader told which machine they belong to can use them. + // What must not happen is the reader being told nothing. + let settled = windows_placement_probe::fingerprint::Fingerprint::discover(); + let disagreement = match (&announced, &settled) { + (Ok(announced), Ok(settled)) => { + *announced != observation.host || *settled != observation.host + } + // A bracket read that failed establishes nothing either way, so it is + // reported rather than treated as agreement. + _ => true, + }; + if disagreement { + let describe = |reading: &std::io::Result< + windows_placement_probe::fingerprint::Fingerprint, + >| match reading { + Ok(fingerprint) => fingerprint.to_string(), + Err(error) => format!("UNKNOWN -- topology discovery failed: {error}"), + }; + let _ = writeln!( + out, + "\nHOST NOT HELD STILL: the three readings that bracket this run do not\n\ + all describe the same machine, so which one the measurements belong to\n\ was not established.\n \ - banner: {announced}\n \ - measured: {}\n\ - The rows below were measured on the second. Read them through it, or\n\ - run again on a machine that is not changing shape.", - observation.host + before: {}\n \ + measured: {}\n \ + after: {}\n\ + The rows below were measured on the middle one. Read them through it,\n\ + or run again on a machine that is not changing shape.", + describe(&announced), + observation.host, + describe(&settled) ); } @@ -741,6 +766,10 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio // reversal as a hop difference, one level further out. Holding locality // fixed is what makes a hop comparison a comparison. let mut rows: Vec = Vec::new(); + // Rows the table shows but the verdict must not use: their allocation did + // not land where it was asked, so they belong to no locality. Counted so the + // report can say the comparison is thinner than the table looks. + let mut redirected = 0_usize; for pair in &pairs { for base in observation.node_pair_rows(*pair, Strategy::Baseline) { @@ -798,13 +827,24 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio cached.nanos_per_item, cached.consumer_batch ); - rows.push(NodeRow { - hop: undirected(*pair), - directed: *pair, - locality: Locality::of(*pair, base.requested_memory_node), - ring: base.requested_memory_node, - nanos: base.nanos_per_item, - }); + // **Only a row that landed where it was asked can carry a + // locality-controlled conclusion.** The marker above already + // recognises a redirected or undeterminable allocation, and the row + // stays in the table because it was measured -- but classifying it + // by the node it *requested* would let the verdict below say "at the + // producer's placement" about a timing taken somewhere else. The + // whole point of holding locality fixed is lost if a row can be + // filed under a locality it did not achieve. + match (base.requested_memory_node, base.memory_node) { + (Some(asked), Some(landed)) if asked == landed => rows.push(NodeRow { + hop: undirected(*pair), + directed: *pair, + locality: Locality::of(*pair, base.requested_memory_node), + ring: base.requested_memory_node, + nanos: base.nanos_per_item, + }), + _ => redirected += 1, + } } } @@ -899,6 +939,19 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio } } + // Said wherever a verdict is reached or declined, because a reader comparing + // the table to the paragraph would otherwise count more rows than the + // paragraph used and have no way to learn why. + let excluded = |out: &mut dyn std::fmt::Write| { + if redirected > 0 { + let _ = writeln!( + out, + " {redirected} row(s) above are not in that comparison: their memory did not\n \ + land on the node they asked for, so they belong to no placement." + ); + } + }; + // `hops.len()`, not `pairs.len()`. `pairs` is directed, so a three-node host // has six entries for the three hops the guard above just counted -- and // printing the directed count here made two adjacent paragraphs describe one @@ -911,6 +964,7 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio the ring sat as well as which hop it crossed.", hops.len() ); + excluded(out); let _ = writeln!( out, " This measures the handoff between two nodes; it is not a distance\n \ @@ -941,6 +995,7 @@ fn render_node_distances(out: &mut dyn std::fmt::Write, observation: &Observatio worst.nanos, spread ); + excluded(out); // No same-hop arm: the selection above only considers pairs on different // hops, so the two extremes cannot be one hop's two directions. A direction // asymmetry within a hop is still visible -- it is two adjacent rows of the From 8dcaace7e22af26e1a08830ac3b952577bfff85b Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Thu, 17 Sep 2026 17:14:30 -0700 Subject: [PATCH 08/11] fix(tools): read the release package list from the config, not the manifest `check-commit-scope.ps1` decides which crates release-please versions, and it read that from `.release-please-manifest.json`. That file records the current version of each managed package; nothing prunes an entry when a package leaves `release-please-config.json`, which is the actual authority for what is managed. This repository has exactly that drift. The manifest carries `crates/windows-platform-probes`, the config does not manage it, and its Cargo.toml says `publish = false`. So the script treated an unreleased crate as released and flagged a commit for mislabelling a changelog entry that could never be written -- and the flag was acted on, renaming a commit's scope on the strength of it. The rename was harmless; acting on a false positive was not. The list now comes from the config's `packages` map. The drift itself is reported rather than silently resolved, because a stale manifest entry is harmless to releases and misleading to anyone who reads it as the package list; saying so once per run is cheaper than the next person rediscovering it. An empty package list is refused outright: a misread config that passes every commit is the one outcome a guard must not have. **The guard had no test, which is why being wrong cost nothing to be wrong.** `test-check-commit-scope.ps1` follows the convention `test-run-sabotage.ps1` established -- throwaway two-crate repositories, no Pester, both shells -- and covers both directions, because a guard that has stopped firing passes a one-directional test as happily as a correct one. Six cases: the manifest-only crate, a genuine two-crate span, a single-crate commit, a `chore` span, the drift note, and the empty-config refusal. Sabotage-verified by restoring the original defect: three cases fail, including the manifest-only crate flagging where it should pass. Worth recording that the first sabotage attempt reported all six still passing -- a multi-line `.Replace` needle with CRLF against a file git keeps as LF matched nothing, so the patch never applied. It briefly looked like evidence the tests were worthless. The harness guards that exact mistake with a match-count check; done by hand, nothing does. `-RepoRoot` is added so the tests can drive a fixture instead of this repository, and the script now restores the caller's location on every exit path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/check-commit-scope.ps1 | 60 +++++++-- tools/test-check-commit-scope.ps1 | 197 ++++++++++++++++++++++++++++++ 2 files changed, 249 insertions(+), 8 deletions(-) create mode 100644 tools/test-check-commit-scope.ps1 diff --git a/tools/check-commit-scope.ps1 b/tools/check-commit-scope.ps1 index 5eb8482cb..34f0855ee 100644 --- a/tools/check-commit-scope.ps1 +++ b/tools/check-commit-scope.ps1 @@ -32,6 +32,13 @@ The Conventional Commits type you are about to use, e.g. 'feat', 'fix!', 'chore'. Only meaningful with -Staged. +.PARAMETER RepoRoot + The repository to read `release-please-config.json` from, and to run git in. + Defaults to this script's parent directory, which is the normal case. Exists + so the tests can drive the script against a throwaway repository rather than + this one -- a guard with no test is enforced by whoever remembers it, and + this one was silently reading the wrong file until a review found it. + .EXAMPLE .\tools\check-commit-scope.ps1 .\tools\check-commit-scope.ps1 -Range 'origin/main..HEAD' @@ -41,18 +48,52 @@ param( [string] $Range, [switch] $Staged, - [string] $Type + [string] $Type, + [string] $RepoRoot ) $ErrorActionPreference = 'Stop' -$repo = Split-Path $PSScriptRoot -Parent +$repo = if ($RepoRoot) { (Resolve-Path $RepoRoot).Path } else { Split-Path $PSScriptRoot -Parent } +Push-Location $repo +try { + +# The crates release-please actually versions -- read from the CONFIG, which is +# the authority for which packages it manages. A `publish = false` crate cannot +# be poisoned, because it is never released, and a crate release-please is not +# configured for is not released whatever its manifest says. +# +# **The manifest is the wrong source, and reading it made this script wrong.** +# `.release-please-manifest.json` records the current version of each managed +# package, but nothing prunes an entry when a package leaves the config: this +# repository's manifest still carries `crates/windows-platform-probes`, which +# `release-please-config.json` does not manage and whose `Cargo.toml` says +# `publish = false`. Reading the manifest therefore treated that crate as +# released and flagged a commit for mislabelling a changelog entry that could +# never be written. +$configPath = Join-Path $repo 'release-please-config.json' +if (-not (Test-Path $configPath)) { throw "No release-please-config.json at $configPath" } +$config = Get-Content $configPath -Raw | ConvertFrom-Json +$released = @() +if ($config.packages) { + $released = @($config.packages.PSObject.Properties | ForEach-Object { Split-Path $_.Name -Leaf }) +} +# An empty package list is a misread file, not an empty release set. Passing +# every commit silently is the one outcome a guard must not have. +if ($released.Count -eq 0) { throw "release-please-config.json names no packages" } -# The crates release-please actually versions. A `publish = false` crate cannot -# be poisoned, because it is never released -- so it is not a finding. +# Drift between the two files is what produced the defect above, so it is +# reported rather than silently resolved in the config's favour. Not a failure: +# a stale manifest entry is harmless to releases, and only misleads a reader who +# takes it for the package list. $manifestPath = Join-Path $repo '.release-please-manifest.json' -if (-not (Test-Path $manifestPath)) { throw "No .release-please-manifest.json at $manifestPath" } -$released = (Get-Content $manifestPath -Raw | ConvertFrom-Json).PSObject.Properties.Name | - ForEach-Object { Split-Path $_ -Leaf } +if (Test-Path $manifestPath) { + $inManifest = (Get-Content $manifestPath -Raw | ConvertFrom-Json).PSObject.Properties.Name | + ForEach-Object { Split-Path $_ -Leaf } + $orphans = $inManifest | Where-Object { $_ -notin $released } + if ($orphans) { + Write-Host ("note: {0} in .release-please-manifest.json but not managed by release-please-config.json; not treated as released." -f ($orphans -join ', ')) -ForegroundColor DarkGray + } +} function Get-ReleasedCrates([string[]] $paths) { $paths | @@ -215,4 +256,7 @@ Write-Host ' consumer (chore), then delete the alias (feat!, owning crate only) Write-Host '' Write-Host ' Already committed and not worth rewriting? Correct it at release time with a' Write-Host ' `Release-As: x.y.z` footer on a commit touching only that crate.' -exit 1 \ No newline at end of file +exit 1} +finally { + Pop-Location +} diff --git a/tools/test-check-commit-scope.ps1 b/tools/test-check-commit-scope.ps1 new file mode 100644 index 000000000..9312b8362 --- /dev/null +++ b/tools/test-check-commit-scope.ps1 @@ -0,0 +1,197 @@ +# Copyright (c) 2026 Mike Grier. All rights reserved. +<# +.SYNOPSIS + Tests for check-commit-scope.ps1. Exits 0 only if every case passes. + +.DESCRIPTION + WHY THIS EXISTS. The script decides which crates release-please versions, + and it read that from the wrong file: `.release-please-manifest.json` + records the current version of each managed package, but nothing prunes an + entry when a package leaves `release-please-config.json`. This repository's + manifest still carries `windows-platform-probes`, which the config does not + manage and whose Cargo.toml says `publish = false`. + + So the script treated an unreleased crate as released and flagged a commit + for mislabelling a changelog entry that could never be written. The flag was + acted on. Nothing caught it, because a guard with no test is enforced by + whoever remembers it. + + Both directions are checked here, because a guard that has stopped firing + passes a one-directional test as happily as a correct one. + + WHY A THROWAWAY REPOSITORY. Asserting against this repository's own history + would tie the tests to specific commits and to a package list that is + expected to change. Each case builds a two-crate repository in a temporary + directory, writes whatever config and manifest the case is about, and makes + commits touching whichever crates it needs. + + WHY NOT PESTER. Same reason as test-run-sabotage.ps1: Windows PowerShell + 5.1 ships Pester 3, whose syntax differs incompatibly from Pester 5. +#> +[CmdletBinding()] +param() + +$ErrorActionPreference = 'Stop' +. (Join-Path $PSScriptRoot 'test-common.ps1') + +$script:Script = Join-Path $PSScriptRoot 'check-commit-scope.ps1' + +<# +.SYNOPSIS + A git repository with two crates, and whatever release configuration the + caller asks for. +#> +function New-Fixture { + param( + [string[]] $ManagedCrates, + [string[]] $ManifestCrates + ) + $root = Join-Path ([System.IO.Path]::GetTempPath()) ("ccs-" + [guid]::NewGuid().ToString('N')) + New-Item -ItemType Directory -Path $root | Out-Null + + $packages = @{} + foreach ($crate in $ManagedCrates) { + $packages["crates/$crate"] = @{ 'package-name' = $crate; component = $crate } + } + @{ packages = $packages } | ConvertTo-Json -Depth 5 | + Set-Content -Path (Join-Path $root 'release-please-config.json') -Encoding utf8 + + $manifest = @{} + foreach ($crate in $ManifestCrates) { $manifest["crates/$crate"] = '0.1.0' } + $manifest | ConvertTo-Json -Depth 5 | + Set-Content -Path (Join-Path $root '.release-please-manifest.json') -Encoding utf8 + + Push-Location $root + try { + git init --quiet 2>&1 | Out-Null + git config user.email 'test@example.invalid' 2>&1 | Out-Null + git config user.name 'Test' 2>&1 | Out-Null + git add -A 2>&1 | Out-Null + git commit --quiet -m 'chore: fixture' 2>&1 | Out-Null + } + finally { Pop-Location } + return $root +} + +<# +.SYNOPSIS + Commit an edit to each named crate's `src/lib.rs`, with the given subject. +#> +function Add-CrateCommit { + param([string] $Root, [string] $Subject, [string[]] $Crates) + Push-Location $Root + try { + foreach ($crate in $Crates) { + $dir = Join-Path $Root "crates/$crate/src" + New-Item -ItemType Directory -Path $dir -Force | Out-Null + Add-Content -Path (Join-Path $dir 'lib.rs') -Value "// $([guid]::NewGuid())" + } + git add -A 2>&1 | Out-Null + git commit --quiet -m $Subject 2>&1 | Out-Null + return (git rev-parse HEAD).Trim() + } + finally { Pop-Location } +} + +function Invoke-Check { + param([string] $Root, [string] $Range) + # `*>&1`, not `2>&1`: the script reports through `Write-Host`, which writes + # to the host rather than the output stream, so a plain merge captures + # nothing and every assertion here would pass against silence. + $output = & $script:Script -RepoRoot $Root -Range $Range *>&1 | Out-String + return [pscustomobject]@{ ExitCode = $LASTEXITCODE; Output = $output } +} + +Write-Report "check-commit-scope.ps1 tests on $($PSVersionTable.PSVersion)" -Level heading + +# **The defect.** A crate present in the manifest but absent from the config is +# not released, so a commit touching it alongside a released crate spans one +# released crate, not two. +Test-Case 'a crate in the manifest but not the config is not treated as released' { + $root = New-Fixture -ManagedCrates @('alpha') -ManifestCrates @('alpha', 'orphan') + try { + Add-CrateCommit -Root $root -Subject 'fix(alpha): touch both' -Crates @('alpha', 'orphan') | Out-Null + $result = Invoke-Check -Root $root -Range 'HEAD~1..HEAD' + Assert-Equal 0 $result.ExitCode 'exit code' + if ($result.Output -notmatch 'No release-triggering commit spans more than one') { + throw "expected a clean result, got: $($result.Output)" + } + } + finally { Remove-Item $root -Recurse -Force -ErrorAction SilentlyContinue } +} + +# The other direction. A guard that stopped firing would pass the case above. +Test-Case 'a release commit spanning two managed crates is still flagged' { + $root = New-Fixture -ManagedCrates @('alpha', 'beta') -ManifestCrates @('alpha', 'beta') + try { + Add-CrateCommit -Root $root -Subject 'fix(alpha): touch both' -Crates @('alpha', 'beta') | Out-Null + $result = Invoke-Check -Root $root -Range 'HEAD~1..HEAD' + Assert-Equal 1 $result.ExitCode 'exit code' + if ($result.Output -notmatch 'span more than one released crate') { + throw "expected the span warning, got: $($result.Output)" + } + } + finally { Remove-Item $root -Recurse -Force -ErrorAction SilentlyContinue } +} + +Test-Case 'a release commit touching one managed crate is not flagged' { + $root = New-Fixture -ManagedCrates @('alpha', 'beta') -ManifestCrates @('alpha', 'beta') + try { + Add-CrateCommit -Root $root -Subject 'fix(alpha): touch one' -Crates @('alpha') | Out-Null + $result = Invoke-Check -Root $root -Range 'HEAD~1..HEAD' + Assert-Equal 0 $result.ExitCode 'exit code' + } + finally { Remove-Item $root -Recurse -Force -ErrorAction SilentlyContinue } +} + +# `chore` triggers no release, so it cannot poison a sibling whatever it touches. +Test-Case 'a chore spanning two managed crates is not flagged' { + $root = New-Fixture -ManagedCrates @('alpha', 'beta') -ManifestCrates @('alpha', 'beta') + try { + Add-CrateCommit -Root $root -Subject 'chore(alpha): touch both' -Crates @('alpha', 'beta') | Out-Null + $result = Invoke-Check -Root $root -Range 'HEAD~1..HEAD' + Assert-Equal 0 $result.ExitCode 'exit code' + } + finally { Remove-Item $root -Recurse -Force -ErrorAction SilentlyContinue } +} + +# The drift that produced the defect is reported, so the next reader does not +# have to rediscover which file is authoritative. +Test-Case 'manifest entries the config does not manage are reported' { + $root = New-Fixture -ManagedCrates @('alpha') -ManifestCrates @('alpha', 'orphan') + try { + Add-CrateCommit -Root $root -Subject 'fix(alpha): touch one' -Crates @('alpha') | Out-Null + $result = Invoke-Check -Root $root -Range 'HEAD~1..HEAD' + if ($result.Output -notmatch 'orphan') { + throw "expected the orphan note, got: $($result.Output)" + } + } + finally { Remove-Item $root -Recurse -Force -ErrorAction SilentlyContinue } +} + +# A config with no packages is a misread file, not an empty release set: every +# commit would silently pass. Better to stop. +Test-Case 'a config naming no packages is refused rather than passing everything' { + $root = New-Fixture -ManagedCrates @() -ManifestCrates @('alpha') + try { + Add-CrateCommit -Root $root -Subject 'fix(alpha): touch one' -Crates @('alpha') | Out-Null + # A terminating error, so it is caught here rather than read off the + # output: refusing loudly is the behaviour, and a test that let the + # throw escape would report the right outcome as a failure. + $refused = $false + try { Invoke-Check -Root $root -Range 'HEAD~1..HEAD' | Out-Null } + catch { $refused = "$($_.Exception.Message)" -match 'names no packages' } + if (-not $refused) { + throw 'expected the script to refuse a config with no packages' + } + } + finally { Remove-Item $root -Recurse -Force -ErrorAction SilentlyContinue } +} + +Write-Report '' +if ($script:Failures -gt 0) { + Write-Report "$($script:Failures) failure(s)" -Level bad + exit 1 +} +Write-Report 'all cases passed' -Level good +exit 0 From 65ae98e4d9d281c490a54bf83ff4533dbea600ef Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Thu, 17 Sep 2026 17:26:53 -0700 Subject: [PATCH 09/11] style(tools): put the try block's closing brace on its own line `exit 1}` welded a two-hundred-line `try`'s closing brace onto an exit statement. It parses -- PowerShell accepts the brace there, the script runs, and its six tests pass -- but nothing about reading it says so, and a reviewer called it a parse error that prevents the script running. That is the wrong conclusion from the right observation: the line is genuinely unreadable. Swept the two tools files for other statements welded to a brace; this was the only one. It came from appending the `finally` block without a leading newline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/check-commit-scope.ps1 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/check-commit-scope.ps1 b/tools/check-commit-scope.ps1 index 34f0855ee..1f6f853b5 100644 --- a/tools/check-commit-scope.ps1 +++ b/tools/check-commit-scope.ps1 @@ -256,7 +256,8 @@ Write-Host ' consumer (chore), then delete the alias (feat!, owning crate only) Write-Host '' Write-Host ' Already committed and not worth rewriting? Correct it at release time with a' Write-Host ' `Release-As: x.y.z` footer on a commit touching only that crate.' -exit 1} +exit 1 +} finally { Pop-Location } From c2eebf8ea61fd218402ee9b6d7975dc8a6c945ef Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Thu, 17 Sep 2026 17:47:04 -0700 Subject: [PATCH 10/11] fix(platform-probes): fail the source census loudly when it cannot read a source The census that stands in for running the two excluded probes read their sources two different ways. The single-file branch panicked when the file could not be read; the directory branch mapped the same failure to an empty string through `unwrap_or_default`. An empty string contains no emission literal, so a source this census could not read passed it -- and the non-empty guard passed too, because the vector still had an entry for every file it failed to read. The weaker half of the census could therefore report a clean answer having read nothing, which is the one outcome a fallback must not have. It is also the defect this branch has spent fourteen review rounds on, appearing in the guard added to close an instance of it: a check that looks like it establishes something and establishes nothing. One read for both shapes now, failing loudly and naming the file and the reason. Verified by holding a censused source open with a deny-all share mode, which makes `read_to_string` fail without altering a byte: the test now fails by name with the OS error. Under `unwrap_or_default` the identical sabotage passed. Found by Copilot review on PR #94, as a suppressed comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...probes_that_emit_a_machine_readable_row.rs | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/crates/windows-platform-probes/tests/the_probes_that_emit_a_machine_readable_row.rs b/crates/windows-platform-probes/tests/the_probes_that_emit_a_machine_readable_row.rs index 40bc17b00..b8652eb3d 100644 --- a/crates/windows-platform-probes/tests/the_probes_that_emit_a_machine_readable_row.rs +++ b/crates/windows-platform-probes/tests/the_probes_that_emit_a_machine_readable_row.rs @@ -167,19 +167,28 @@ fn the_probes_that_emit_a_machine_readable_row_are_the_ones_we_say_they_are() { // them, and the assertion says so rather than quietly going stale. for (name, source_path) in NOT_RUN { let path = crate_root().join(source_path); - let sources: Vec = if path.is_dir() { - walk(&path) - .iter() - .map(|file| std::fs::read_to_string(file).unwrap_or_default()) - .collect() - } else { - vec![std::fs::read_to_string(&path).unwrap_or_else(|error| { + // **One read for both shapes, and it fails loudly.** These were two + // reads with opposite failure behaviour: the file branch panicked on an + // unreadable source, the directory branch mapped it to an empty string + // through `unwrap_or_default`. An empty string contains no emission + // literal, so a source this census could not read passed it -- and the + // non-empty guard below passed too, because the vector still had an + // entry. The weaker half of the census could therefore report a clean + // answer having read nothing, which is the one outcome it must not have. + let read = |file: &Path| -> String { + std::fs::read_to_string(file).unwrap_or_else(|error| { panic!( - "{}'s source at {} is readable: {error}", - name, - path.display() + "{name}'s source at {} could not be read: {error} -- the \ + source census cannot stand in for running it if it cannot \ + read it", + file.display() ) - })] + }) + }; + let sources: Vec = if path.is_dir() { + walk(&path).iter().map(|file| read(file)).collect() + } else { + vec![read(&path)] }; assert!( !sources.is_empty(), From 143c72141bc4fba38b54081b12287e8c41a2602d Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Thu, 17 Sep 2026 17:58:57 -0700 Subject: [PATCH 11/11] fix(platform-probes): stop the disagree disclaimer pointing at a body that may not exist `READINGS_DISAGREE` said the differing brackets left it unestablished "which of them names the machine the body below describes". That sentence was written when `attribution` took only the two endpoint readings, and it is wrong in both directions now that a measured read can be passed: - **With a measured read, the body's machine IS known.** The measured reading names it by construction -- that is what the parameter was added for. What differing brackets leave unestablished is whether the machine held still, not which reading the body belongs to. - **On the `report_unmeasured` path there is no body.** `bin/topology.rs` builds the banner with `attribution(None, ...)` and hands it to `report_unmeasured`, which renders a discovery failure and no topology at all, so "the body below" referred to nothing. `NOT_ESTABLISHED` already had the right shape -- state what the readings did and what that leaves unconfirmed, without reaching forward to a body -- and this now matches it. Verified on the path the finding named: brackets differing, no measured read, rendered through `report_unmeasured`. `MEASURED_UNCONFIRMED` keeps its reference to the body, and the reason is recorded beside it: that arm requires a measured read, and the unmeasured path has none by definition, so wherever it is emitted a body exists. That holds by what the arm requires rather than by a caller remembering. **An assert enforcing the same thing was written and withdrawn**, which is worth recording because the withdrawal is the interesting half. Asserting in `report_unmeasured` that its banner carries no body-referencing disclaimer broke `a_banner_shape_the_renderer_cannot_produce_is_contained`, which deliberately feeds it such a banner to exercise containment. The assert was over-reach: it constrained a public function to forbid something no shipping path does, and the test that caught it was right to. Found by Copilot review on PR #94, as a suppressed comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/topology_report.rs | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/crates/windows-platform-probes/src/topology_report.rs b/crates/windows-platform-probes/src/topology_report.rs index 67e9b8a36..6c8d79f46 100644 --- a/crates/windows-platform-probes/src/topology_report.rs +++ b/crates/windows-platform-probes/src/topology_report.rs @@ -142,9 +142,21 @@ use crate::topology::{ /// [`preamble`], which has to recognise an attribution-shaped banner to pass it /// through. A second copy of this sentence in the recogniser would be a /// restatement that could drift out of step with the one that writes it. +/// +/// **It does not mention the body, and that is a correction.** It used to say +/// which reading "names the machine the body below describes was not +/// established", which was wrong in both directions once `measured` existed. +/// When a measured read is passed, the body's machine IS known -- the measured +/// reading names it by construction, and what the differing brackets leave +/// unestablished is whether the machine held still, not which reading the body +/// belongs to. And `report_unmeasured` renders this banner over a report with +/// no topology body at all, so "the body below" referred to nothing there. +/// +/// [`NOT_ESTABLISHED`] already had the right shape and this now matches it: +/// state what the readings did, and what that leaves unconfirmed, without +/// reaching forward to a body that may not exist. const READINGS_DISAGREE: &str = "HOST READINGS DISAGREE: the two readings that bracket the measurement\n\ - differ, so which of them names the machine the body below describes\n\ - was not established."; + differ, so nothing confirmed the machine held still under it."; /// The disclaimer `attribution` renders when at least one bracket read failed. const NOT_ESTABLISHED: &str = "HOST NOT ESTABLISHED: at least one of the two readings that bracket the measurement\n\ @@ -162,6 +174,13 @@ const NOT_ESTABLISHED: &str = "HOST NOT ESTABLISHED: at least one of the two rea /// record, so three reads of one unchanged machine can disagree. With no /// disclaimer for it, that host printed an unqualified banner while the run /// held the evidence against it. +/// +/// **This one keeps its reference to the body, and the reason is reachability.** +/// [`READINGS_DISAGREE`] lost its equivalent phrase because it renders on the +/// `report_unmeasured` path, where there is no body to point at. This arm needs +/// a measured read, and the unmeasured path has none by definition -- so +/// wherever it is emitted, a body exists and the measured reading describes it. +/// That holds by what the arm requires, not by a caller remembering. const MEASURED_UNCONFIRMED: &str = "HOST READING UNCONFIRMED: the two readings that bracket the measurement agree\n\ with each other but not with the read the body below describes, so one of\n\ the three discoveries did not see what the others saw.";