Peel the platform probes: placement pair, one-source cost rendering, and a detection ladder - #94
Merged
Merged
Conversation
… 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>
… 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>
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>
…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>
…res 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>
…tradict 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>
…ected 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>
…nifest `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>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
tools/check-commit-scope.ps1 currently contains a PowerShell syntax error (exit 1}) that prevents the script from running.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR “peels” updated windows-platform-probes work from a long-lived branch onto main, adding new placement probes and tightening probe/report correctness by deriving multiple outputs from single sources and enforcing “fail fast” validation earlier in the detection ladder (build/tests/CI).
Changes:
- Add two new placement probes (
probe-core-affinity,probe-peer-index-cache) and wire them into the probes crate as first-class[[bin]]targets. - Make cost-probe prose tables and NDJSON derive from a single source of truth (label list +
json_key) and add integration tests that assert the two readers see the same figures. - Improve operational rigor: gate probe steps in CI on an explicit build step; add the “FAIL FAST” rule to Copilot instructions and record its rationale in root
DESIGN-NOTES.md; fixcheck-commit-scope.ps1to read managed crates fromrelease-please-config.jsonand add a standalone PowerShell test harness.
File summaries
| File | Description |
|---|---|
| tools/test-check-commit-scope.ps1 | Adds a throwaway-repo test harness to validate check-commit-scope.ps1 behavior in both directions. |
| tools/check-commit-scope.ps1 | Reads released crates from release-please-config.json, reports manifest/config drift, and adds -RepoRoot for testing. |
| DESIGN-NOTES.md | Records rationale for the “detection ladder” / fail-fast ordering without restating the normative rule text. |
| crates/windows-platform-probes/tests/the_probes_that_emit_a_machine_readable_row.rs | Integration test that runs probes to establish which emit machine-readable NDJSON rows. |
| crates/windows-platform-probes/tests/a_cost_probe_tells_both_readers_the_same_thing.rs | Integration test ensuring cost-probe prose tables and NDJSON agree on values end-to-end. |
| crates/windows-platform-probes/tests/a_real_report_agrees_with_itself.rs | Updates topology report construction to use the measured fingerprint path. |
| crates/windows-platform-probes/tests/a_probe_writes_its_report_to_stdout.rs | Existing integration coverage for stdout emission (unchanged, but part of the probe test suite context). |
| crates/windows-platform-probes/src/topology.rs | Adds measure_observed() returning (Observation, Fingerprint) while preserving measure() for existing callers. |
| crates/windows-platform-probes/src/topology_report.rs | Extends attribution logic to name (and optionally disclaim) the measured read vs endpoint reads. |
| crates/windows-platform-probes/src/tests.rs | Updates unit tests to match new attribution signature and adds new shape/containment assertions. |
| crates/windows-platform-probes/src/doorbell_cost.rs | Introduces json_key + EVERY_LABEL (with presence) to keep prose/NDJSON schema aligned. |
| crates/windows-platform-probes/src/request_cost.rs | Introduces json_key + EVERY_LABEL to keep prose/NDJSON schema aligned. |
| crates/windows-platform-probes/src/bin/doorbell_cost.rs | Reworks NDJSON emission to derive fields from the same source as prose output and enforce label/schema coverage. |
| crates/windows-platform-probes/src/bin/request_cost.rs | Reworks NDJSON emission to derive fields from the same source as prose output and enforce label/schema coverage. |
| crates/windows-platform-probes/src/bin/topology.rs | Switches to measure_observed() and passes the measured fingerprint into attribution. |
| crates/windows-platform-probes/src/bin/peer_index_cache.rs | Adds new probe binary that renders peer-index-cache measurement results via the report sink. |
| crates/windows-platform-probes/src/pool_growth.rs | Improves Win32 failure assertions by including last_os_error() in the message. |
| crates/windows-platform-probes/src/ioring.rs | Improves Win32 failure assertions by including last_os_error() in the message. |
| crates/windows-platform-probes/src/completion_port.rs | Improves Win32 failure assertions by including last_os_error() in the message. |
| crates/windows-platform-probes/README.md | Documents additional probe coverage areas with links back to design notes. |
| crates/windows-platform-probes/CHECKLIST.md | Updates/archives milestone items and adds/refines M4.9/M4.10 notes to reflect corrected scope. |
| crates/windows-platform-probes/Cargo.toml | Registers new probe binaries and updates dependency commentary. |
| crates/windows-placement-probe/src/peer_index_cache.rs | Converts pin failures from panics to io::Result refusals and centralizes shared predicates. |
| crates/windows-placement-probe/src/peer_index_cache/tests.rs | Updates tests to assert refusal-as-value behavior and adds propagation coverage for new error edges. |
| crates/windows-placement-probe/src/core_affinity.rs | Converts pre-pin checks to refusals and propagates new Result-based timing paths. |
| .github/workflows/ci.yml | Adds an explicit build step for probes and gates probe runs on build success while still running after test failures. |
| .github/copilot-instructions.md | Adds the “FAIL FAST — push every rule down the detection ladder” guidance and links to rationale. |
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`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>
Owner
Author
Review round 1 -- responseOne finding, raised both inline and in the review summary. It is the same finding in both places, so this covers the summary and the inline thread carries the detail.
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
One new integration test currently masks source read failures via unwrap_or_default(), which can allow a false-negative pass instead of failing loudly when the “weaker census” can’t actually read its inputs.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/windows-platform-probes/tests/the_probes_that_emit_a_machine_readable_row.rs:174
- In the NOT_RUN source census,
read_to_string(...).unwrap_or_default()will silently treat unreadable files as empty strings. That can let this test pass even when sources weren’t actually read (e.g., permission/encoding/path issues), which defeats the purpose of using this as the “weaker but still sound” fallback. Prefer failing loudly on read errors so the test can’t degrade into a false negative.
- Files reviewed: 29/29 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ad 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>
Owner
Author
Review round 2 -- responseOne finding, raised as a suppressed comment (no inline thread), so it is answered here.
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The new/updated topology banner disclaimer text (READINGS_DISAGREE) is now misleading relative to the measured-fingerprint attribution behavior and should be corrected before approval.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/windows-platform-probes/src/topology_report.rs:147
READINGS_DISAGREEnow claims it was not established which bracket reading names the machine the body describes, but the banner can be built from the measured fingerprint (viameasured) so the body’s host description is known by construction. This message can also appear inreport_unmeasured, where there is no measured topology body at all, making the wording misleading.
- Files reviewed: 29/29 changed files
- Comments generated: 0 new
- Review effort level: Lite
… 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>
Owner
Author
Review round 3 -- responseOne finding, raised as a suppressed comment (no inline thread), so it is answered here.
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
I did not find any concrete correctness, safety, or CI-gating issues in the changed regions, and the added tests/CI changes materially strengthen drift detection for the new probe behaviors.
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 0 new
- Review effort level: Lite
Owner
Author
Review round 4 -- responseNo findings, and nothing to apply. 29/29 files reviewed, zero comments, zero suppressed comments. Recorded here so the round is closed rather than left to be inferred from silence. For the record across the four rounds:
Three of the four were real, and all three were the same class: a claim outrunning what establishes it. That is the class this PR is largely about, so the reviewer finding it three times in the change that addresses it is a fair result rather than an ironic one. CIAll 29 checks pass, including the ones that matter most here:
What remains uncertain, stated plainlyCI passing does not close the thing I called weakest. That is not a blocker and I am not asking for it to be one. It is the part of this change a reviewer with a multi-node machine could settle and nobody else can, and it should be read as unverified rather than as passing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Peels the
windows-platform-probescrate offmikegrier/deferred-namespace-ops, which carries several hundred commits main does not. This is the whole of what that branch still changes in the crate, taken as current state rather than replayed: its history is tangled with workspace-wide refactors that already landed, so the commits do not separate.What it adds
Two placement probes, both thin renderers over library code already on main.
probe-core-affinitymeasures what thread placement costs and refuted the hypothesis it was written to test;probe-peer-index-cacherecords a result that inverts by host, which is why it is kept rather than discarded. The measurement itself lives inwindows-placement-probe, unchanged between the branches.Both cost probes now derive their prose and their NDJSON from one source. A label-to-field relation 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":1came to sit beside a prose[0]..github/workflows/ci.ymlgains a build step and gates twelve probe steps on it. Main ran the probes; nothing gated them.A repository-wide rule — FAIL FAST, in
.github/copilot-instructions.md, with its rationale in the rootDESIGN-NOTES.md— plus a fix tocheck-commit-scope.ps1and its first tests.Scope decisions, each checked rather than preferred
origin/mainwith only this crate applied, before any of it was committed.windows-placement-probeis touched by one commit, which changes a pinning failure from a panic to a returned refusal. Both crates arepublish = falseand neither is a release-please package, so nothing here can produce a version bump or changelog entry.COMPLETED-PLANS.mdand a cache-locality design session are not taken: the first declares this crate'sCHECKLIST.mdcomplete when main's has open items, the second references atopology-plannerthat is not a workspace member.How it was reviewed, and what that found
Fourteen independent review rounds, the last four with no guidance beyond "review all the changes". They found 4, 4, 4, 5, 1, 1, 6, 2, 1, 2, 0, 2, 5, 2 defects.
Almost every round found defects in the code the previous round had just written to fix its findings. That is the most useful thing this branch produced, and the rule it led to is in the diff.
The recurring shape was one error at successive depths. The NUMA hop comparison was corrected five times, each fix exposing the next:
Each correction was right and each revealed a further confound. I do not expect that sequence to terminate by inspection.
The single most expensive mechanism was partial conversion: one predicate at two or three sites, corrected at one. Three predicates, eight findings. Two are now single definitions (
is_nameable_in_a_mask,undirected) so the split cannot recur.What is weakest here
The NUMA renderer has never executed. This host has one node, so every fix to it was verified against synthetic rows I constructed — which validates my model of the data, not the data. A real multi-node host is the one reviewer left that can test it, and that is the main thing CI and any reviewer with such hardware should look at.
Two further items are queued rather than fixed, with reasons recorded in
CHECKLIST.md:M4.9is complete,M4.10asks whether a probe's report owes a machine-readable row at all (three of sixteen currently do).Gate
cargo clippy --all-targets --all-features,cargo fmt, both crates' test suites,cargo doc --workspace --all-featuresunder CI's deny flags, encoding, 7-bit ASCII on the governed markdown, andcheck-commit-scope.ps1— all green locally. CI has not run on this branch before now.