Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,29 @@ jobs:
run: cargo run -p windows-platform-probes --bin probe-ioring --locked
- name: probe magnitudes (completion port)
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
# survey of what real machines look like -- and the negative result, that
# cloud runners are consistently single-node, is itself evidence for how
# the execution-domain design should size itself by default. The probe
# 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
# 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.
#
# 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()'
run: cargo run -p windows-platform-probes --bin probe-topology --locked
# Both halves of the long-path pair, deliberately. Either alone says
# nothing: the finding is the *difference* between two executables that
# differ only in whether `build.rs` embedded the `longPathAware` manifest,
Expand Down
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 26 additions & 1 deletion crates/windows-placement-probe/src/fingerprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -868,7 +868,32 @@ pub fn places_from_topology(
/// to rest on a human having read the format string.
#[must_use]
pub fn banner_line() -> String {
match Fingerprint::discover() {
banner_line_for(&Fingerprint::discover())
}

/// The banner line for a fingerprint that has already been discovered.
///
/// Split out of [`banner_line`] for a caller that must BRACKET the discovery --
/// reading the host before and after a measurement, so two readings that differ
/// are visible. Such a caller holds the readings already, and formatting
/// `host: {fingerprint}` itself would put a second copy of this line's shape
/// in another crate; a probe's banner is comparable with every other probe's
/// only while exactly one place produces it.
///
/// Bracketing establishes that the two readings DIFFER, not that the host
/// changed: [`Fingerprint::discover`] returns `Ok` on a parse that dropped a
/// record or whose sources disagreed, so consecutive fingerprints can differ
/// because the enumeration was flaky. Naming a cause is the caller's business
/// and no caller can name that one.
///
/// **Takes the `Result` rather than the `Fingerprint`, which is the point.**
/// [`banner_line`] renders both outcomes into one string, so a caller comparing
/// two of those strings cannot tell two differing readings from a discovery
/// that failed -- two failures carrying different `io::Error` text differ as
/// strings while establishing nothing about the machine at all.
#[must_use]
pub fn banner_line_for(discovered: &std::io::Result<Fingerprint>) -> String {
match discovered {
Ok(fingerprint) => format!("host: {fingerprint}"),
Err(error) => format!("host: UNKNOWN -- topology discovery failed: {error}"),
}
Expand Down
41 changes: 41 additions & 0 deletions crates/windows-placement-probe/src/fingerprint/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1341,3 +1341,44 @@ mod multi_group_conversion {
}
}
}

#[test]
fn a_banner_for_an_already_discovered_fingerprint_renders_both_outcomes() {
// `banner_line_for` exists so a caller that BRACKETS the discovery -- reading
// the host before and after a measurement, to detect one that changed under
// it -- can render the readings it already holds, rather than formatting this
// line a second time somewhere else. A failed read stays distinguishable,
// which is the whole reason it takes the `Result`: a caller comparing two
// rendered lines cannot otherwise tell a host that moved from one that could
// not be read.
// The two are compared on ONE reading, not on two. Asserting
// `banner_line_for(&Ok(discovered)) == banner_line()` re-reads the host
// inside the assertion, so a machine that changed between them fails a test
// about string formatting -- in the crate whose `attribution` exists
// precisely because two consecutive discoveries can differ. The property
// under test is that one place owns the format, and that is shown by
// rendering the same value twice.
let fingerprint = Fingerprint::discover().expect("this machine must be discoverable");
let rendered = super::banner_line_for(&Ok(fingerprint.clone()));

assert_eq!(
rendered,
super::banner_line_for(&Ok(fingerprint.clone())),
"the rendering is a function of the reading alone"
);
assert!(
rendered.starts_with("host: "),
"and it is the shape `banner_line` has always produced: {rendered}"
);
assert!(
rendered.contains(&fingerprint.to_string()),
"got {rendered}"
);

let failed = super::banner_line_for(&Err(std::io::Error::other("no topology")));
assert!(failed.starts_with("host: "), "got {failed}");
assert!(
failed.contains("UNKNOWN") && failed.contains("no topology"),
"a failed read says so, and says why: {failed}"
);
}
84 changes: 84 additions & 0 deletions crates/windows-platform-probes/CHECKLIST.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,87 @@ piece of work rather than a correction to that one.
and record the Ctrl-C observation in [DESIGN-NOTES.md](DESIGN-NOTES.md) -- an interactive signal is not
something to assert in CI, but it is the property the milestone exists for, so it must be measured
once rather than assumed.


## 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.

- [ ] **M2.1** -- Add a report oracle to this crate: one shared executable definition of the
correlations that must hold between the parts of a rendered report, checked against the rendered
artifact rather than against internal state. Seed it with the three known invariants: an alarm in
the prose implies the verdict is not `agree`; a fact rendered in both prose and NDJSON agrees across
the two; an uncaveated hardware claim implies `!parse_in_doubt`. Model it on
[../windows-file-watcher/src/contract.rs](../windows-file-watcher/src/contract.rs)'s
`ContractChecker`, which is this repository's worked example and which existed unused while this
probe was being written.

- [ ] **M2.2** -- Route every test that renders a report through the oracle, so the roughly
twenty-five existing `report()` call sites inherit the checks and every future one does too. This is
the step that makes it an oracle rather than three more tests: a test added beside the others checks
one case, whereas binding the call sites checks every case anyone writes later. Verify the binding by
sabotage -- change an invariant and confirm existing tests go red -- because a binding that only moves
when its own test moves is cosmetic.

- [ ] **M2.3** -- Add the missing integration test: run `measure()` against the real host, render the
report, and apply the oracle. At the time of M2 the crate had one integration test, asserting only
that a probe writes to stdout, and none of the twenty-five `report()` calls rendered from a real
measurement -- every one used a hand-built `Observation`, which can only contain states its author
already imagined. On CI this runs across the whole hosted-runner fleet, which is where states no
fixture anticipates will actually appear.

- [ ] **M2.4** -- Explore, with the sparse matrix as the instrument, whether the same correspondence
failures exist for `Coherence`, `BracketOutcome` and `Verdict`, and in the sibling probes' renderers.
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
correspond" 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 oracle from M2.1.

> **-> 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.

- [ ] **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.
8 changes: 8 additions & 0 deletions crates/windows-platform-probes/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ path = "src/bin/ioring.rs"
name = "probe-pool-growth"
path = "src/bin/pool_growth.rs"

[[bin]]
name = "probe-topology"
path = "src/bin/topology.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.
Expand Down Expand Up @@ -74,6 +78,10 @@ path = "src/bin/long_path_unaware.rs"
# reimplementation of the SDK's inline environment helpers, so it depends on the
# real crate.
windows-threadpool-sys = { path = "../windows-threadpool-sys" }
# Same reason: the topology probe measures what the shipping parse produces, not
# 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
Expand Down
Loading