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