Survey the machine topology and cross-check the shipping parse - #82
Conversation
…covered
`banner_line` discovers the host and renders it in one step, so a caller that
must BRACKET the discovery -- reading the host before and after a measurement
to detect one that changed under it -- had nowhere to put readings it already
held. Formatting `host: {fingerprint}` at the call site would put a second
copy of this line's shape in another crate, and a probe's banner is comparable
with every other probe's only while exactly one place produces it.
`banner_line_for` is that one place, and `banner_line` is now expressed in
terms of it.
It takes the `std::io::Result` rather than the `Fingerprint`, which is the
point rather than a convenience: `banner_line` renders both outcomes into one
string, so a caller comparing two of those strings cannot tell a host that
changed from a discovery that failed -- and two failures whose `io::Error`
text differs compare unequal while establishing nothing about the machine at
all.
…e shipping parse Adds `probe-topology`: a binary that prints the machine's processor, NUMA and cache shape and what each candidate partitioning policy would yield, plus one `x-probe-topology` NDJSON line so accumulated CI output can be mined without parsing the prose. CI already runs it on every build, which turns the hosted-runner fleet into a slow survey of what real machines look like -- including the negative result that cloud runners are consistently single-node. The parse comes from `windows_topology_sys::MachineMemoryTopology::discover` rather than a reimplementation here, for the same reason the pool-growth probe uses the real thread-pool crate: a reimplementation would measure the reimplementation. Three Win32 counters are then read INDEPENDENTLY -- GetActiveProcessorCount, GetActiveProcessorGroupCount and GetNumaHighestNodeNumber -- so the probe doubles as a cross-check on that crate's parsing across every machine CI ever runs on. `measure` reads the counters and `observe` reduces a topology plus those counters to an `Observation`, split so the reduction is testable without a host. The design problem this probe is mostly made of is saying only what a run established. `cross_check` returns three lists, deliberately not merged, because they are claims about different things with different owners: `disagreements` (a counter was compared and did not match -- a finding about the shipping crate), `not_compared` (a counter could not be read -- a gap in this measurement), and `parse_incomplete` (the parse is short or disputed between its two sources). `Agree` requires all three empty; only `disagreements` yields `Disagree`, because an incomplete parse is not a wrong one and reporting it as a divergence would send a reader to audit a mismatch that does not exist. `Verdict` is an enum rather than a bool, and the NDJSON carries a tri-state `"cross_check"`, so neither a renderer nor a mining pass can treat "two things checked out and the third was never established" as agreement. The same discipline shapes the rest. Zero from either processor counter is a failed read rather than a count. Node numbers are compared highest-against-highest and taken from whichever source reported them, since `NumaNodeIndex` is machine-wide from both -- a count against `highest + 1` would call a sparse-but-correct machine a regression. Every domain-policy count is clamped, and the clamp is the contract rather than defensiveness: a fleet sized at zero domains performs no I/O. The outermost partitioning cache is asked of the crate, never re-derived, because a higher cache level is not always the coarser one. `PartitioningCache` gives the absent answer four distinct forms so neither the prose nor the JSON can report "no cache level partitions this machine" -- a claim about hardware -- when the truth is that no level was reported, or that a level decoded to nothing, or that the survey lost a summary. `CrossCheck::parse_in_doubt` is the single gate on every hardware conclusion the renderer draws, and it deliberately excludes `not_compared`, because a counter this probe could not read says nothing about the parse. The host banner is bracketed the same way, and for the same reason: the fingerprint is a topology rendering rather than a name, so a machine that changed across the run would print one shape above a body describing another. It is read before and after through `Fingerprint::discover` rather than `banner_line`, so a failed read stays distinguishable from a host that moved. Tests pin the invariants that must hold on any machine, since almost every number here is host-specific and there is nothing to assert about its value. The three DESIGN-NOTES decisions record the three-list taxonomy, why the banner is bracketed, and why a verdict other than `Agree` fails CI even though a flaky hypervisor can produce one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are well-contained, tested, and consistent with existing probe/report patterns; only minor clarity nits were identified in the new report module.
Pull request overview
Adds a new windows-platform-probes topology probe (probe-topology) that (1) prints the machine’s processor/NUMA/cache shape for human inspection and (2) emits one x-probe-topology NDJSON row per run for log mining, while cross-checking windows-topology-sys’s shipping parse against independently-read Win32 counters. This fits the existing “platform probes” pattern: binaries perform host-specific measurement; shared library code makes the report/testable invariants easy to assert.
Changes:
- Add topology measurement + cross-check logic (
measure/observe/cross_check) and a text+NDJSON report renderer. - Add
probe-topologybinary and wire it into CI (ensuring it still runs even when earlier steps fail). - Add
banner_line_fortowindows-placement-probeso callers can bracket host discovery and render already-held fingerprint readings without duplicating banner formatting.
File summaries
| File | Description |
|---|---|
| crates/windows-platform-probes/src/topology.rs | Implements the topology observation model, counter bracketing, and cross-check logic against Win32 counters. |
| crates/windows-platform-probes/src/topology_report.rs | Renders the topology probe report (prose + x-probe-topology NDJSON line) from an Observation. |
| crates/windows-platform-probes/src/lib.rs | Exposes the new topology modules and documents the new asserted probe entry. |
| crates/windows-platform-probes/src/bin/topology.rs | Adds the probe-topology binary that brackets fingerprint discovery and emits the report via the shared sink. |
| crates/windows-platform-probes/DESIGN-NOTES.md | Records the topology probe’s “three lists / three owners” cross-check decision and CI strictness rationale. |
| crates/windows-platform-probes/Cargo.toml | Registers the new binary and adds the path dependency on windows-topology-sys. |
| crates/windows-placement-probe/src/fingerprint.rs | Refactors banner rendering to a reusable banner_line_for(Result<Fingerprint>). |
| crates/windows-placement-probe/src/fingerprint/tests.rs | Adds coverage for banner_line_for formatting of both Ok/Err cases. |
| .github/workflows/ci.yml | Runs probe-topology on every CI build (with if: '!cancelled()') for fleet surveying and failure forensics. |
| Cargo.lock | Includes the new windows-topology-sys dependency for windows-platform-probes. |
Review details
- Files reviewed: 9/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… are `"efficiency_classes"` was a count under a plural name, which is ambiguous in the one way that matters for a field whose whole purpose is log mining. On a single-class host the row read `"efficiency_classes":1` while the prose two lines above it printed `efficiency classes: [0]` -- the same fact, in the same report, rendered so the JSON value coincides with a plausible class *label*. A consumer reading the row alone cannot tell "one class" from "class 1", and the two renderings cannot be reconciled. It now emits the list the name promises. That agrees with the prose, removes the count/label collision, and carries strictly more: the count is still available as the array's length, and a fleet survey can additionally see WHICH classes a host reported. Safe to change now and never again -- the probe has not merged, so no accumulated CI output depends on the old shape. Also drops `topology_report.rs`'s provenance header. It named commit `c335ae5`, which was squashed away and is not an ancestor of this branch, so the reference resolves to nothing on any other clone. The header exists so `git blame -C` can trace lines through a split commit; this branch contains no such commit -- both files arrive together -- so there is nothing for a reader to follow and the module doc already records why the renderer is its own module. Raised by review on #82.
There was a problem hiding this comment.
🔵 Needs a closer look
A couple of small but concrete robustness/maintainability issues were found in the new topology module (stored as review comments with suggested fixes).
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
crates/windows-platform-probes/src/topology.rs:441
domain_counts()currently derives the "by-outermost-partitioning-cache" count viaoutermost_partitioning_cache(), which collapses thePartitioningCache::SummaryMissing(_)case into the same output as "no level was chosen" (it becomesmap_or(1, ...)). SinceSummaryMissingis explicitly modeled to prevent this kind of silent fallback in renderers/serializers, it’s better fordomain_counts()to key offpartitioning_cache()so the fallback is explicit and future-safe.
crates/windows-platform-probes/src/topology.rs:1369overlapping_walk_relationsbuilds an array of&dyn Fnfrom closure literals (including an explicit cast on only the first element). This is harder to read and relies on trait-object coercions/temporary lifetimes that are easy to break with small edits. Prefer plainfnpointers (or named locals) for each kind predicate and iterate over those; it’s simpler and avoids borrowing closure temporaries.
- Files reviewed: 9/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
…summary `PartitioningCache::SummaryMissing` is the state where the topology crate named a level as the outermost partitioning cache and this survey carries no summary for it. The renderer already printed it as "BUG IN THIS PROBE ... Nothing below about cache partitioning can be trusted" -- and `cross_check` said nothing about it, so `verdict()` could certify the same run as `agree`. Two paragraphs of one report, contradicting each other, which is the exact failure this probe exists to prevent. `domain_counts` read the same state through `outermost_partitioning_cache`, whose `None` folds four distinct answers together, so a sizing policy silently got "one domain" from a state the report calls a bug. It now matches on `partitioning_cache()` per variant: every absent case still sizes to one, but they are spelled out, so a variant added later is a compile error here instead of joining the fallback unnoticed. That is the whole reason `PartitioningCache` exists rather than an `Option`. Unreachable through `observe`, which builds the level and the summaries from one survey; reachable through the public fields and `partitioning_cache`, which is the same reason `domain_counts` clamps at all. Also replaces the `&dyn Fn` array behind `overlapping_walk_relations` with plain `fn` pointers. The predicates capture nothing, so the trait objects bought nothing and cost a cast on the first element plus a reliance on temporary lifetimes that a small edit could break. Raised by review on #82.
|
Both fixed in 033d789 -- and the first one was hiding a contradiction rather than just a style problem, so thank you for it.
Pinned by The Verified: 147 tests pass, clippy clean, |
There was a problem hiding this comment.
🟡 Changes recommended
The topology report and binary have a couple of concrete output/convention issues (stored inline) that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
crates/windows-platform-probes/src/bin/topology.rs:17
- All other probe binaries route output via
report::emit_report(andreport.rsdocuments this as the invariant for panic-safe partial output). This binary imports and callsemitdirectly instead, which breaks that crate-level convention and can drop any already-rendered output if something panics before the finalemitcall.
This issue also appears on line 49 of the same file.
crates/windows-platform-probes/src/bin/topology.rs:49
- Use
emit_reporthere (consistent with the other probe mains) so a panic during or after rendering still flushes whatever was already written to the buffer.
emit(&mut Stdout, &text);
- Files reviewed: 9/11 changed files
- Comments generated: 1
- Review effort level: Lite
…ery instrument Twenty-eight review rounds, two independent readers each, and zero surviving mutants on both modules did not find a state where the renderer printed "BUG IN THIS PROBE ... nothing below about cache partitioning can be trusted" while `cross_check` had no branch for it and the verdict could print `agree` two paragraphs below. A pull-request review found it in code none of that had touched. The decision records why each instrument was structurally incapable rather than unlucky: mutation testing perturbs code that exists, so a missing branch has no mutants and the 180/0 score was unaffected by the gap; exhaustiveness checking compels a `match`, not a concept, so it bought nothing in the two consumers that reached the same state through an `Option`-returning accessor or never asked; and per-artifact review confirms both sides of a contradiction, because each side is locally true. It also records that the reviewers' shared prompt -- which had accumulated focus areas and "already verified" facts -- correlated them far more than their differing models decorrelated them. It records the split that follows: a sparse matrix as a transient exploratory instrument for finding which correlations exist, and an oracle as the durable home for the few that turn out to mean something. Committing the matrix as a maintained table is rejected explicitly -- it would be a second copy of the code's structure that nothing verifies, and a stale "all cells covered" table is worse than none. The work is queued as M2 in the crate's CHECKLIST rather than left in the decision, per the rule that design notes are not a work queue. Whether this generalises past this crate is left open for the engineer.
…top laundering it to null Both defects are consequences of 033d789, which made `SummaryMissing` block agreement. Before that commit `SummaryMissing` did not put the parse in doubt on its own, so the missing-level caveat -- whose gate excludes `Level(_)` alone -- could not reach it. Once it did, the report printed BUG IN THIS PROBE: the topology crate named L3 as the outermost partitioning cache and this survey carries no summary for it. Or the parse is not whole and the level that would have partitioned this machine is missing -- see the cross-check below for why. A level WAS named; its summary is what is absent, and the arm above already says so. `the_missing_level_caveat_needs_both_doubt_and_an_absent_level` states that invariant for `Level` -- "a named level is not missing, whatever else is in doubt" -- and `SummaryMissing` names a level too. Both variants that name one are now excluded from the gate. The same commit took `domain_counts` off `outermost_partitioning_cache`, whose `None` folds the summary-missing case in with the three that name no level, and did not sweep the NDJSON serialiser reading through the same accessor. So the row carried `"outermost_partitioning_cache_level":null` beside `"outermost_partitioning_cache":"summary_missing"` while the prose printed the number: one fact, two renderings, and a mining pass counting nulls as "no level was named" folding in a host where one was. The level is now read per variant. Found by a review round aimed specifically at correspondence between the report's parts rather than at either part alone -- the class recorded in DESIGN-NOTES.md under `#d-correspondence-failures`, whose worked example is the contradiction 033d789 fixed. That the fix for one instance produced two more is the strongest argument for the oracle queued as M2: these were caught by a reviewer looking for the shape, and nothing in the suite would have caught them on the next change.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are internally consistent, add targeted test coverage for the new banner formatting path, and the CI wiring matches the stated intent without introducing any obvious correctness or policy issues.
Review details
- Files reviewed: 11/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
A probe run performs three independent topology discoveries -- one for the banner, one inside `measure()` for the body, one for the banner again -- and `attribution` compares only the two endpoints. Equal endpoints therefore print an unqualified banner without establishing that the read the body describes agreed with them. Recorded rather than fixed in this pull request, with the reachability stated precisely in both directions. `measure()` already brackets its counters 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 therefore narrow -- cache structure differing between reads while processor, group and NUMA counts hold -- which real hardware essentially cannot do and a misbehaving hypervisor can, which is the population the probe exists to survey. Filed under M2 because "the banner describes the measured read" is a correspondence invariant and belongs in that milestone's oracle, checked on every rendered report, rather than asserted once beside it. The reviewer disagreement is recorded with it: across two rounds one reader raised this twice while two others cleared it, one explicitly after being asked the question directly. Nothing in the test suite decides it either way, and that is the argument for the oracle rather than a fact about who was right.
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large new probe surface plus a CI behavior change that can affect build reliability, which warrants final human review despite no specific blocking defects found.
Review details
- Files reviewed: 11/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
…nose loudly Findings from two Copilot reviews on #83. Its `GetLogicalDrives` and `measure_park_and_wake` findings were already fixed in `c0a0021` and `cc87153`; these five are live. Route every probe through `report::emit_report`. `report.rs` states that every probe in this crate routes through it "with no exceptions", and three did not: both new ones here and `topology.rs`, which shipped in #82. All three were mine, so the claim has been false for two PRs. `emit_report` catches a panic during composition, prints what was already established, and resumes -- which is the difference between a CI log showing which probe died and how far it got, and one showing nothing at all. This interacts directly with the rest of this branch. The last several commits deliberately added panics to these probes -- every status now asserted, every required NDJSON label now `expect` -- so the panic surface grew substantially in exactly the two probes sitting outside the mechanism that reports panics. Adding loud failures while bypassing the thing that makes them diagnosable is half a change. `doorbell_cost` also measured in `main`'s argument list, so its measurement ran before the renderer was entered and outside the protection entirely. It now measures inside `render`, after the banner is in the buffer. Verified by sabotage: a panic at the measurement leaves the banner and heading on stdout and still exits 101. Diagnose loudly. Ten assertions said "SetEvent failed" and dropped the Win32 error, which is what a reader has to work from in CI where a rerun under a debugger is not available; the code distinguishes an invalid handle from a resource limit. They now carry `last_os_error()`, evaluated only on failure, so the timed loops are unaffected. `handle_state.rs` already did this -- the idiom existed and had not reached the new code. `request_cost`'s NDJSON emitted `null` for six timings `measure` always records, the same defect as `c0a0021`'s `NaN` and missed in the same sweep. `null` is right for a value that can legitimately be absent, and these cannot be: it would produce a record that parses cleanly and reads, to a mining pass, as a host where the measurement did not apply. Replace the `usize` round-trip carrying handles into the handshake thread with a named `Send` carrier. The review suggested capturing `HANDLE` directly; that does not compile -- `HANDLE` is `*mut c_void` and therefore `!Send` -- but the complaint under it is right, because the cast asserts "safe to move across threads" with no `unsafe` anywhere near the assertion. The carrier puts the `unsafe impl` and its justification where the claim is made. Worth knowing for anyone repeating this: a newtype alone is not enough. Under edition 2021 precise capture, mentioning `carrier.0` inside the closure captures the `!Send` *field* and the wrapper's impl never applies -- the first attempt failed with the identical error it was meant to fix. The carrier exposes a `take(self)` method so the capture is the whole value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Brings back the topology probe (PR #82), which was peeled out of this branch, reviewed across twenty-eight rounds plus three pull-request reviews, and merged to main in a substantially different form. Eight files conflicted. Resolutions, and why: - `src/topology.rs`, `src/bin/topology.rs` (both added): took main's. Main's is this branch's code after the peel, so it supersedes rather than competes. Verified rather than assumed: the only public items on this branch's side that main lacks are `CacheLevel::domains` (now a derived method), the `memoryless_numa_domains` field (renamed, because the old name said the opposite of what it measured), and `cross_check` returning `Vec<String>` (now the three-list `CrossCheck`). Each successor was confirmed present in main's version before the branch's was discarded. `src/topology_report.rs` arrives new; it did not exist when the peel was cut. - `src/tests.rs`: took main's, then re-appended the two doorbell handshake tests this branch owns. Of the fourteen tests this branch added since the merge base, twelve are already in main's version -- they went out with the peel and came back evolved -- so only those two needed carrying. - `.github/workflows/ci.yml`: kept both. Main's topology step brings `if: '!cancelled()'`, which is what makes the report print on the host whose cross-check failed; this branch's doorbell-cost and request-cost steps have no counterpart on main. The merged file parses as YAML and the topology step keeps its condition. - `Cargo.toml`: kept this branch's binary list, which already contains `probe-topology` alongside four probes main has never seen, and this branch's longer dependency comment, which is a superset of main's. - `src/lib.rs`: union of both module lists. - `DESIGN-NOTES.md`: both sides added different sections at the same point. Main's paragraph continues the section above it and now does so again; this branch's new section follows it. - `Cargo.lock`: regenerated against this branch's manifests, since main's predates the crates this branch adds. Verified on the merged tree: 150 platform-probe tests and the full workspace suite pass, `cargo fmt --check` and clippy are clean, the encoding and workflow-reference checks pass, and `probe-topology` runs and reports `agree` on this host.
Fourth slice peeled off
mikegrier/deferred-namespace-ops(#56), after #79 (probe report sink), #80 (long-path probe pair) and #81 (shared PowerShell native-command guard).Adds
probe-topology: a binary that prints the machine's processor, NUMA and cache shape and what each candidate partitioning policy would yield, plus onex-probe-topologyNDJSON line so accumulated CI output can be mined without parsing the prose. CI runs it on every build, which turns the hosted-runner fleet into a slow survey of what real machines look like -- including the negative result that cloud runners are consistently single-node.It measures the shipping crate, deliberately
The parse comes from
windows_topology_sys::MachineMemoryTopology::discoverrather than a reimplementation here, for the same reason the pool-growth probe uses the real thread-pool crate: a reimplementation would measure the reimplementation. Three Win32 counters are then read independently --GetActiveProcessorCount,GetActiveProcessorGroupCountandGetNumaHighestNodeNumber-- so the probe doubles as a cross-check on that crate's parsing across every machine CI ever runs on.measurereads the counters and brackets the parse between two readings;observereduces a topology plus those counters to anObservation, split out so the reduction is testable without a host.Saying only what a run established
That is the design problem this probe is mostly made of, and it is why
cross_checkreturns three lists that are deliberately not merged -- they are claims about different things with different owners:disagreementsnot_comparedparse_incompleteAgreerequires all three empty. OnlydisagreementsyieldsDisagree, because an incomplete parse is not a wrong one and reporting it as a divergence would send a reader to audit a mismatch that does not exist.Verdictis an enum rather than a bool and the NDJSON carries a tri-state"cross_check", so neither a renderer nor a mining pass can read "two things checked out and the third was never established" as agreement.The same discipline shapes the rest: zero from either processor counter is a failed read rather than a count; NUMA nodes are compared highest-against-highest from platform-sourced labels only, since a count against
highest + 1would call a sparse-but-correct machine a regression; the outermost partitioning cache is asked of the owning crate and never re-derived;PartitioningCachegives the absent answer four distinct forms so neither the prose nor the JSON can report "no cache level partitions this machine" when the truth is that no level was reported.CrossCheck::parse_in_doubtis the single gate on every hardware conclusion the renderer draws.The host banner is bracketed the same way. The fingerprint is a topology rendering rather than a name, so it is read before and after through
Fingerprint::discover-- notbanner_line-- which keeps a failed read distinguishable from two readings that differ.Known and deliberate gaps
Nothing independently measures packages, cores or caches, so a record that decodes cleanly while describing less of the machine than exists raises no anomaly and reaches
agree. That gap is left open on purpose: a coverage check would have to hold on every machine in the runner fleet, and since any verdict other thanAgreefails the build, a check that cannot be validated beyond one host would fail builds for hosts reporting themselves correctly. The code, comments and DESIGN-NOTES were narrowed so nothing claims otherwise.Commits
Two, so each Conventional Commit scope is accurate for release-please:
feat(placement-probe)-- addsbanner_line_for, so a caller that brackets the discovery can render readings it already holds without putting a second copy of the banner's shape in another crate. Verified to compile standalone.feat(platform-probes)-- the probe itself.Verification
cargo check --all-targetsclean across the workspace; clippy--all-targets --all-featurescleantopology.rs180 caught / 6 unviable,topology_report.rs22 caught)agreecheck-encoding.ps1(618 files),check-workflow-refs.ps1(58 refs),check-commit-scope.ps1all cleanReviewed to convergence: the final round ran two independent readers on a neutral prompt -- no focus areas, no exclusions -- both free to cross-reference the sibling crates, and both returned empty.