Skip to content

Add waitable queues and the placement probe, and reshape the topology model - #56

Open
MikeGrier wants to merge 375 commits into
mainfrom
mikegrier/deferred-namespace-ops
Open

Add waitable queues and the placement probe, and reshape the topology model#56
MikeGrier wants to merge 375 commits into
mainfrom
mikegrier/deferred-namespace-ops

Conversation

@MikeGrier

@MikeGrier MikeGrier commented Aug 31, 2026

Copy link
Copy Markdown
Owner

Adds two Windows-only crates, replaces windows-topology-sys' model, and puts the release machinery in place to ship them.

Crate State after merge
windows-waitable-queues new, publishable -- first release
windows-topology-sys reshaped -- new model replaces 0.1.0's
windows-placement-probe new, publish = false -- shipped as a prebuilt binary
topology-planner new -- planning documents only, no code

windows-waitable-queues

Bounded producer/consumer queues whose readiness is a waitable Windows HANDLE.

That is the crate's reason to exist. crossbeam-channel blocks on its own internal primitive and exposes no HANDLE, and its Select accepts only channel operations, so a thread that must wake on "a message arrived or my I/O completed or shutdown was signalled" cannot express that wait. A queue whose readiness is a HANDLE composes with WaitForMultipleObjects, MsgWaitForMultipleObjects, thread-pool waits, and alertable waits.

Every item is behind cfg(windows); the crate builds to an empty shell elsewhere.

Three shapes, none canonical -- there is no type named Queue, so a caller names the shape it wants:

Module Producers Storage Full behaviour
spsc one (!Clone producer) bounded ring refuses
slotwise_mpsc many (Clone producer) bounded array, per-slot sequence refuses
reserving_mpsc many bounded array, packed claim word refuses, honours reservations

reserving_mpsc adds reservations: reserve() claims capacity up front and returns a Reservation redeemed by send or released on drop. A granted reservation is always redeemable, so a caller can establish it has room before doing work whose result must not be dropped. The queue stays connected while a reservation is outstanding.

The doorbell is a lazily created manual-reset event per queue. A consumer that only polls never allocates a kernel object; one that waits calls doorbell() for a borrowed HANDLE (or doorbell_owned() for an OwnedHandle). arm() clears the doorbell and then asks whether an item is takeable, so a push landing in that window cannot have its signal erased.

windows-topology-sys

The published 0.1.0 modelled a ladder of levels with optional rungs. This replaces it with a model of observed connectivity, which is the shape of question consumers actually ask.

  • Topology is now MachineMemoryTopology -- the old name claimed more than the type delivers.
  • Relations are a set with per-relation provenance, not reduced on insert. Observation and Source record which observer saw a relation, so CPU Sets and the relationship walk sit side by side instead of merging into one lossy answer.
  • Observed<T> replaces Option<T> where absence was ambiguous. Known / Absent / NotObserved are three distinct facts, carried by serde as three distinct encodings (number, explicit null, omission).
  • Granularity orders what a set of processors shares by observed set inclusion, with minimal_shared as the meet and the whole machine as the top. "How close are these two processors" is therefore answered from observed membership across every domain kind at any depth, rather than from a firmware-asserted cache level.
  • proximity reads an inclusion-ordered partitioning rather than re-implementing the partitioning rule.
  • distances and Distances are deleted. The crate does not go below the Win32 topology APIs, so a fact Win32 does not report is not a fact the crate has. The field was never populated.
  • Domain::id is removed in favour of per-observation labels: one id cannot name a domain two observers labelled differently.
  • discover retries until the two sources agree, up to three passes, and states the outcome as a new public Coherence on the topology. Windows offers no transactional way to read the relationship walk and the CPU-set enumeration together, so a processor hot-added between the calls appears in one and not the other; a second pass settles that, and what survives the bound is reported as a genuine disagreement rather than hidden. Grouping disagreements are deliberately not retried -- they are persistent in the field, so re-reading cannot settle them.

windows-placement-probe

A measurement tool that reports where the OS actually places threads and memory, released as a prebuilt binary rather than a crate. Date-based version (2026.902.0) because its output is dated evidence, not an API contract.

It places every online processor, refuses to invent NUMA membership, reports a cache relationship the topology never established as unknown rather than as same-or-cross, and publishes its record backup by rename so a failed write cannot leave a truncated file under a complete record's name.

topology-planner

Planning and design documents for a future planner consuming an abstracted topology description. No code ships: the topology crate is justified as the refined view of what the platform publishes, with an adapter absorbing the planner's needs.


Breaking changes for consumers

  • windows-topology-sys -- Topology renamed; Domain::id, distances and Distances removed; Option<T> becomes Observed<T> on Memory::memory_bytes; usable() removed (it consulted a CPU-set byte this build never populates and returned false for every processor, and it encoded policy in a facts crate).
  • windows-file-watcher -- reopen-by-id removed.
  • windows-waitable-queues -- first release, so nothing to migrate.

Versions this merge proposes

Crate From To
windows-topology-sys 0.1.0 0.2.0
windows-waitable-queues 0.0.1 0.1.0
windows-file-watcher 0.1.3 0.2.0
windows-ioring-sys 0.2.0 0.2.1 (pinned)
windows-thread-ambient-sys 0.2.0 0.2.1
windows-file-watcher-example-test-harness 0.1.2 0.1.3

The other six released crates get no release: they carry only test:, docs: and refactor: commits since their tags.

windows-ioring-sys is pinned to 0.2.1 because no public item in it changed on this branch -- release-please attributes commits by the paths they touch, and two topology-scoped breaking commits reached it through an example and one doc-comment heading. A 0.3.0 would send consumers looking for a migration that does not exist. tools/check-commit-scope.ps1 is wired into the pre-commit gate and flags this class of cross-crate attribution.


Known limitations, disclosed deliberately

  • reserving_mpsc can lose an item after 2^32 pushes, on every target -- not only 32-bit ones. Its claim position is a 32-bit half of a packed word by construction. A producer that checks for room, is descheduled, and resumes after a complete wrap can write into a slot whose emptiness was decided a generation earlier, silently overwriting an item the consumer had not taken. Measured exposure: 37 seconds to ~4 minutes of sustained pushing. The crate docs lead with this, and slotwise_mpsc does not have the hazard (64-bit positions on every target).
  • permit_mpsc is experimental and exempt from the crate's semver promise. Behind the non-default experimental-permit-claim feature; it exists to be measured against the shipping claim protocol, and will be merged into reserving_mpsc or deleted.
  • CPU-set flag bit positions are unverified. SYSTEM_CPU_SET_INFORMATION::AllFlags reads constant zero on this build even after SetProcessDefaultCpuSets succeeds, so the bit meanings are neither confirmed nor falsifiable here.
  • Producer-side backpressure beyond a Full return is out of scope for the queue crate's first release. PushError is #[non_exhaustive], so adding to it later is not breaking.

Dependencies

No third-party dependency is added and no existing external dependency changes version. The only Cargo.lock additions are the two new workspace-local crates.

What a consumer pulls in on a default build:

Crate External tree
wtf-string nothing
windows-waitable-queues windows-sys -> windows-link
windows-topology-sys windows-sys -> windows-link
windows-file-watcher log, windows-sys -> windows-link

Both new publishable crates cost exactly one external dependency, and it is the one every other crate here already uses. serde (topology), serde/serde_json (file watcher) and windows-core (wtf-string) are optional and off by default. Across the whole workspace the external set is 14 crates, with no duplicate versions.

cargo publish --dry-run succeeds for windows-waitable-queues, windows-topology-sys, windows-file-watcher and windows-ioring-sys. Nothing pins windows-topology-sys or windows-waitable-queues to a version, so the bumps above cannot break resolution inside the workspace.

CI and release automation

  • Probe jobs measuring topology, doorbell cost, and request cost.
  • A numa-spikes job running the standalone NUMA spikes through the scratch-crate procedure their README documents, so that instruction cannot rot silently. Observational, but it fails when a spike fails to build or run.
  • release-placement-probe.yml builds, verifies and attaches windows-placement-probe binaries for x86_64 and ARM64, checking that artifacts carry a build-identity stamp and that unofficial builds cannot be released by accident. Released binaries carry a GitHub artifact attestation -- a signed statement binding the exact bytes to this repository, workflow and commit, verifiable with gh attestation verify. They are not Authenticode-signed, and the "official" stamp in --version is a self-reported build-identity marker, not a signature: build.rs reads it from environment variables, so it catches an accidental local build rather than a forgery.
  • A CI check asserts every release-managed crate actually has a publish trigger.

Copilot AI lite review requested due to automatic review settings August 31, 2026 21:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Confirmed correctness issues in new code paths (notably recv_timeout potentially waiting forever and test hook state leaking on panic) should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This pull request expands the workspace’s Windows observability and measurement tooling (new probes + CI jobs), introduces new crates (windows-waitable-queues, windows-placement-probe), and hardens topology trust semantics by adding an explicit provenance marker to windows-topology-sys.

Changes:

  • Add windows-waitable-queues (bounded, waitable HANDLE-backed queues) and wire it into workspace + release tracking.
  • Add windows-placement-probe and expand windows-platform-probes (new probe binaries + tests), then run probes/spikes in CI for ongoing fleet measurement.
  • Add Provenance to windows-topology-sys so serialized / hand-built topologies cannot silently pass as “measured”.
File summaries
File Description
tools/run-numa-spikes.ps1 Builds/runs standalone NUMA “spike” instruments in a scratch crate and captures transcripts.
.github/workflows/ci.yml Runs additional probes (topology, doorbell cost, request cost) and adds a “numa-spikes” observational job with artifact upload.
release-please-config.json Adds windows-waitable-queues to release-please configuration.
.release-please-manifest.json Adds crates/windows-waitable-queues version for release tracking.
Cargo.toml Adds windows-placement-probe and windows-waitable-queues to workspace members.
Cargo.lock Records new workspace crates and their dependency edges.
.gitignore Ignores placement-probe-v*.json backup records to avoid accidental commits.
PLANS.md Adds planning rows / trackers for the new crate and related checklists.
crates/windows-waitable-queues/PLANS.md Adds per-component plans pointer back to the root checklist.
crates/windows-waitable-queues/Cargo.toml Declares the new publishable windows-waitable-queues crate and its windows-sys feature set.
crates/windows-waitable-queues/src/race_hooks.rs Adds deterministic race-window hooks for tests to drive correctness-critical interleavings.
crates/windows-waitable-queues/src/options.rs Adds an Options builder for construction-time queue switches (disposal policy, high-water tracking).
crates/windows-waitable-queues/src/metrics.rs Adds queue metrics (refusals + optional high-water).
crates/windows-waitable-queues/src/metrics/tests.rs Tests metric arithmetic and concurrency behavior in isolation.
crates/windows-waitable-queues/src/disposal.rs Adds teardown policy plumbing for handing undrained items to a caller-provided sink.
crates/windows-waitable-queues/src/disposal/tests.rs Tests disposal/teardown behavior including panicking sinks.
crates/windows-waitable-queues/src/capacity.rs Centralizes bounded-capacity validation shared across queue shapes.
crates/windows-waitable-queues/src/blocking.rs Adds shared blocking recv / recv_timeout loop that parks on a Win32 event handle.
crates/windows-topology-sys/src/topology.rs Adds Topology::provenance and stamps Measured only in discover(), with serde downgrade-on-load.
crates/windows-topology-sys/src/topology/tests.rs Updates/extends tests to pin provenance downgrade semantics and safe defaults.
crates/windows-topology-sys/src/provenance.rs Introduces Provenance type plus serde downgrade helper.
crates/windows-topology-sys/src/provenance/tests.rs Tests provenance ordering, defaults, downgrade rules, and rendering.
crates/windows-topology-sys/src/lib.rs Exposes Provenance from the crate.
crates/windows-topology-sys/DESIGN-NOTES.md Documents the provenance decision (D-12) and its rationale/semantics.
crates/windows-ioring-sys/design-sessions/spikes/README.md Documents why certain spikes are checked in “ready but unrun” and how to interpret/run them.
crates/windows-ioring-sys/CHECKLIST.md Adds/updates queued repairs from the NUMA-sharding measurement (M20).
crates/windows-platform-probes/Cargo.toml Adds new probe binaries and depends on shipping crates for measurement fidelity.
crates/windows-platform-probes/src/lib.rs Exposes new probe modules.
crates/windows-platform-probes/src/tests.rs Adds topology probe consistency tests against real host counters.
crates/windows-platform-probes/src/request_cost.rs Adds request construction timing probe logic.
crates/windows-platform-probes/src/bin/topology.rs New probe-topology binary emitting both human output and a single JSON line for mining.
crates/windows-platform-probes/src/bin/doorbell_cost.rs New probe-doorbell-cost binary measuring doorbell relative costs.
crates/windows-platform-probes/src/bin/request_cost.rs New probe-request-cost binary reporting request construction costs and ratios.
crates/windows-platform-probes/src/bin/queue_contention.rs New probe-queue-contention binary exploring queue tail-claim contention scaling.
crates/windows-placement-probe/Cargo.toml Introduces windows-placement-probe crate and build identity stamping (publish=false for now).
crates/windows-placement-probe/README.md Documents purpose, privacy posture, usage, and provenance expectations for runners.
crates/windows-placement-probe/DESIGN-NOTES.md Records publication/provenance constraints and why crates.io must not become the primary path yet.
crates/windows-placement-probe/build.rs Stamps commit/dirty/source into the binary (CI vs local vs unknown).
crates/windows-placement-probe/src/lib.rs Defines the crate’s module surface and documentation for the measurement tool.
crates/windows-placement-probe/src/build_identity.rs Implements build identity model + trust ordering and “official build” predicate.
crates/windows-placement-probe/src/build_identity/tests.rs Tests build identity trust semantics and build-script stamping shape.
crates/windows-placement-probe/src/submission.rs Renders the “paste-able” submission payload and includes a truncation-detecting checksum.
crates/windows-placement-probe/src/peer_index_cache/tests.rs Tests correctness/honesty of memory placement reporting (esp. on single-node hosts).
crates/windows-placement-probe/src/machine/tests.rs Tests registry-based machine description collection policy and safety invariants.
crates/windows-placement-probe/schema/v1.txt Schema golden for v1 submission records.
crates/windows-placement-probe/schema/v2.txt Schema golden for v2 submission records (adds memory_node).
CHECKLIST.md Records completed tooling item(s) related to sabotage harness promotion.
COMPLETED-PLANS.md Archives the completed topology provenance checklist entry.
COMPLETED-CHECKLIST.md Archives the completed topology provenance checklist in detail.
Review details
  • Files reviewed: 81/95 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/windows-waitable-queues/src/blocking.rs Outdated
Comment thread crates/windows-waitable-queues/src/race_hooks.rs
Copilot AI review requested due to automatic review settings August 31, 2026 22:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

There are a few correctness/documentation issues in the new queue crate (broken rustdoc links and a test hook that can leak state on panic) that should be fixed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/windows-waitable-queues/src/race_hooks.rs:78

  • Hook::with does not clear/restore the thread-local hook if body panics (e.g., a failing assertion inside the test closure). That can leak the hook into later tests on the same thread and cause cascading/irreproducible failures. Use a drop guard to restore the previous hook even during unwinding.
  • Files reviewed: 81/95 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread crates/windows-waitable-queues/src/blocking.rs
Comment thread crates/windows-waitable-queues/src/metrics.rs Outdated
Copilot AI review requested due to automatic review settings August 31, 2026 22:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

There are a few correctness/behavior issues in the new queue code/docs (panic cleanup in race_hooks::Hook::with, potential tight-loop at sub-millisecond timeouts in recv_timeout, and Metrics rustdoc inconsistencies) that should be addressed before approval.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

crates/windows-waitable-queues/src/metrics.rs:13

  • The module docs say Metrics holds three counters including "Doorbell rings", but this type currently only stores refused and high_water (doorbell rings are tracked in doorbell::Doorbell). This makes the rustdoc misleading for readers looking for where to get the ring count.

This issue also appears on line 20 of the same file.
crates/windows-waitable-queues/src/blocking.rs:140

  • When remaining is non-zero but < 1ms, remaining.as_millis() truncates to 0, so this computes millis = 0 and can spin in a tight loop until the deadline. Clamping to at least 1ms avoids the busy-wait at the end of the timeout budget.

crates/windows-waitable-queues/src/metrics.rs:20

  • The docs below still describe a 3-counter design (including a "Rings" counter) and explain why "two of the three are free". Since ring counting lives in doorbell::Doorbell rather than Metrics, this section should be updated to describe only the counters actually accumulated here.
    crates/windows-waitable-queues/src/race_hooks.rs:77
  • Hook::with claims to install a hook only for the duration of body, but if body panics (or unwinds via ? through a panic), the hook is never cleared because the cleanup happens after body() returns. This can leak the hook into subsequent operations on the same thread and make later tests flaky or misleading.
  • Files reviewed: 81/95 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

MikeGrier pushed a commit that referenced this pull request Aug 31, 2026
CI's rustdoc job has failed on all three runs of pull request #56. `main` is
green, so the branch broke it -- most likely the `mpsc` -> `slotwise_mpsc`
rename, which moved every item these links named.

Twelve sites across three crates. The job denies `broken_intra_doc_links`,
`private_intra_doc_links` and `invalid_rust_codeblocks`, so eleven of these were
errors rather than warnings.

`MIN_CAPACITY` did not exist anywhere in the workspace: the public docs for
`slotwise_mpsc::bounded` referred readers to a constant that was never written.
The rule it was supposed to explain lives in the private `BOUNDS`, so the
sentence now states the rule itself -- a capacity of one cannot distinguish
"published" from "free" under the sequence protocol.

The links into private items are delinked rather than repointed, because a
public page cannot link to a page rustdoc does not generate. Where the private
name is still useful to a maintainer it stays as inline code.

Also corrects SH-2.4, which described these as pre-existing warnings to clear
before publication. They were neither pre-existing nor warnings, and nothing was
waiting on the release -- the release was waiting on them.

Completed item: SH-2.4: Clear the eight rustdoc warnings in windows-waitable-queues before it is published

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 31, 2026 22:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

There are correctness issues in new test infrastructure and CI tooling (panic-unsafe hook scoping and missing runtime failure handling in the NUMA spike runner) that should be fixed before merging.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

crates/windows-waitable-queues/src/options.rs:49

  • The rustdoc example comment says “The peak, not the depth right now.”, but the next line asserts rx.len() == 1 (current depth). This reads as a contradiction for users learning the API. Adjust the comment so it matches what’s being asserted.

crates/windows-waitable-queues/src/race_hooks.rs:78

  • Hook::with does not clear the thread-local hook if body panics, which can leak the hook into later code on the same thread (e.g., if a test uses catch_unwind / asserts a panic) and make failures non-deterministic. Make the hook clearing panic-safe via an RAII guard that resets the slot in Drop.
  • Files reviewed: 82/96 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

MikeGrier pushed a commit that referenced this pull request Aug 31, 2026
Three findings from the Copilot review on pull request #56.

**`recv_timeout` could become unbounded.** The blocking loop clamped an
oversized remaining duration to `u32::MAX`, which is the same value as
`INFINITE` -- imported three lines above the clamp in the same file. A timeout
longer than about 49.7 days therefore waited forever instead of timing out, and
the loop that was supposed to re-check the deadline never regained control to do
it. The existing comment reasoned carefully about clamping versus truncating and
was right about everything except the one value it chose.

The reviewer's suggested fix, clamping to `u32::MAX - 1`, is incomplete, and a
boundary test written for it is what showed that: a duration of exactly
`u32::MAX` milliseconds *converts* successfully, so the fallback never fires and
`INFINITE` is returned by the conversion rather than by the clamp. Both guards
are needed, and they cover different inputs. The clamp is now derived from
`INFINITE` rather than written as a number, and the arithmetic is extracted so
it can be tested -- the failing case takes 49 days to observe through the public
API and so could never be a test of the loop.

**A hook survived a panic.** `Hook::with` removed the installed hook with a
statement after the body, which an unwind skips. A test that installs a hook and
then fails an assertion -- the ordinary way for a test to fail -- left it
installed to fire inside whatever ran next on that thread, in the facility this
crate's central correctness argument rests on. Now removed by a guard, using
`try_with` so teardown cannot replace an unwind already in progress.

**Eighteen broken documentation links, from a report of two.** `../../` from
`src/*.rs` resolves to `crates/`, which holds no `DESIGN-NOTES.md`; the crate's
own notes are one level up. Sweeping the workspace found sixteen more of the
same, in `spsc`, `traits`, `reserving_mpsc` and `slotwise_mpsc`.

The sweep also caught its own repair damaging a correct link:
`windows-file-watcher/src/contract.rs` used `../../../DESIGN-NOTES.md` to reach
the *workspace* notes deliberately, and a blanket prefix replacement shortened
it. Reverted, and every relative markdown link in every Rust source is now
checked to resolve -- 36 of them, none broken.

The review's fourth point, that `race_hooks` puts a thread-local lookup on
production hot paths, does not hold: `mod race_hooks` and all four call sites
are already `#[cfg(test)]`.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 31, 2026 23:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

There are a few correctness/docs issues (notably build-stamp invalidation and misleading docs) that should be fixed before merging.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

crates/windows-waitable-queues/src/options.rs:49

  • The doc example comment says rx.len() is "The peak", but len() reports the current depth (peak is high_water()). This is misleading for API consumers reading the docs.
    crates/windows-placement-probe/build.rs:33
  • Watching only .git/HEAD is usually insufficient to keep the stamped commit fresh: on most repos HEAD contains a stable ref: refs/heads/<branch> line and does not change when new commits are made. This can leave PLACEMENT_PROBE_COMMIT_OUT stale after committing without editing files.
    crates/windows-platform-probes/src/request_cost.rs:157
  • These Win32 parameters are currently magic numbers, which makes it hard to tell what kind of open is being timed (and easy to accidentally change semantics later). Prefer the named windows-sys constants so the request is self-documenting.
  • Files reviewed: 84/98 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings August 31, 2026 23:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Release tracking for windows-waitable-queues is added, but the publish automation/tag triggers are not updated to actually publish it, and there are also concrete doc/identity-stamping issues identified in the new code.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

crates/windows-waitable-queues/src/metrics.rs:14

  • The module docs say this type tracks three counters (refusals, doorbell rings, peak depth), but Metrics only stores refused and high_water. Either the docs are stale or a counter is missing; as written it misleads readers about where doorbell ring counts live.

This issue also appears on line 20 of the same file.
crates/windows-placement-probe/build.rs:33

  • build.rs prints rerun-if-* directives, which disables Cargo's default "rerun the build script when any package file changes" behavior. As a result, local edits to src/ (which make the tree dirty) will not cause the stamp to be recomputed, so dirty/commit can go stale across rebuilds.

crates/windows-waitable-queues/src/metrics.rs:24

  • Follow-on to the docs above: the rest of this section describes rings as a counter maintained here ("two of the three are free..."). With doorbell ring counts not stored in Metrics, this section should be updated so it only explains the two counters Metrics actually owns.
  • Files reviewed: 84/98 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

MikeGrier pushed a commit that referenced this pull request Aug 31, 2026
Five findings from the Copilot reviews on pull request #56. These arrived only
in review *bodies* rather than as inline threads, so the thread query used last
time could not see them.

**The build stamp did not update on commit.** `build.rs` watched
`../../.git/HEAD`, and on a branch that file holds `ref: refs/heads/<branch>` --
a line that does not change when you commit. Since a `rerun-if-changed`
directive replaces cargo's default of watching the whole package, the script
never re-ran and the binary kept whatever commit it was first built with.

Measured rather than reasoned about: `.git/HEAD` had not been touched in 21
hours while fourteen commits landed, and a freshly rebuilt binary reported
`b6f23ec9f3bc` against a `HEAD` of `0b96b14746bb` -- six behind. CI hides this
completely, because there the commit arrives through `PLACEMENT_PROBE_COMMIT`,
so the stamp was wrong only on local builds, which are exactly the ones whose
commit is their only traceability. The whole submission record rests on this
field.

Now resolves what `HEAD` points at and watches that too, handling a detached
`HEAD` (which changes on its own), a packed ref (where `packed-refs` changes
instead), and a `.git` file redirecting to a worktree. Paths are emitted only
when they exist, since a missing path makes cargo re-run the script on every
build.

**`recv_timeout` busy-waited below a millisecond.** A remainder under 1 ms
truncates to zero, and a zero wait returns at once, so the loop re-armed and
re-waited without sleeping. Arming clears the doorbell, which is a `ResetEvent`
syscall, so this was a syscall storm rather than merely a hot loop. Clamped to a
millisecond: overshooting a blocking deadline by less than a timer tick is the
right trade, and sub-millisecond precision is not available from a blocking wait
at any price.

Also: the `Options` doctest labelled `rx.len()` as "the peak" when it is the
current depth; the `metrics` module described a ring counter that lives on
`Doorbell` without saying so; and `request_cost` passed `0x8000_0000`, `1` and
`3` to an open request instead of `GENERIC_READ`, `FILE_SHARE_READ` and
`OPEN_EXISTING`, which this repository's conventions forbid. The three constants
were checked against `windows-sys` rather than assumed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 31, 2026 23:40
@MikeGrier

Copy link
Copy Markdown
Owner Author

Working through the five findings raised in the last four review bodies. These arrived as suppressed comments in the review summaries rather than as inline threads, so they were invisible to a reviewThreads query -- worth noting for anyone else automating this.

All five were checked against the code before acting, and all five were real. Fixed in dd2e9c5 and 81cff89.

1. Build stamp invalidation (build.rs:33) -- the most serious of these

Correct, and worse in practice than the report suggests. Since a rerun-if-changed directive replaces cargo's default of watching the whole package, watching only .git/HEAD meant the script re-ran essentially never.

Measured rather than argued:

.git/HEAD mtime:   2026-08-30 22:12:39   <- 21 hours and ~14 commits ago
ref file mtime:    2026-08-31 19:24:02   <- matches the last commit exactly

and the consequence, on a freshly rebuilt binary:

actual HEAD:  0b96b14746bb
binary says:  !!UNOFFICIAL!! v0.1.0 b6f23ec9f3bc DIRTY [LOCAL]   <- six commits behind

CI hides this completely, because there the commit arrives through PLACEMENT_PROBE_COMMIT rather than from git. So the stamp was wrong only on local builds -- which are precisely the builds marked [LOCAL], whose commit is the only traceability they have, and the whole submission record rests on that field.

Now resolves what HEAD points at and watches that too, covering a detached HEAD (changes on its own), a packed ref (packed-refs changes instead), and a .git file redirecting to a worktree. Paths are emitted only when they exist, because a missing path makes cargo re-run the script on every build.

Verified end to end: committed, rebuilt without touching any source file, and the stamp followed 0b96b14 -> dd2e9c5.

2. Sub-millisecond tight loop (blocking.rs:140)

Correct, and the cost is higher than a spin. The loop re-arms each turn, and arming clears the doorbell -- so a zero-millisecond wait made it a ResetEvent syscall storm for the last fraction of the timeout budget, not just a hot loop.

Clamped to one millisecond. Overshooting a blocking deadline by less than a timer tick is the right trade: the granularity is coarser than a millisecond anyway, so sub-millisecond precision is not purchasable from a blocking wait at any price -- what a caller would get instead is a burning core. Two tests cover it, and both fail if the lower bound is removed.

3. Metrics rustdoc describes a counter it does not hold (metrics.rs:13, :20)

Correct. Metrics holds refused and high_water only; rings are counted on Doorbell::rings. The design of placing each counter where its cost is already paid was described, but never actually said where rings ended up, so a reader looking for the ring count on Metrics was left without a pointer. Both places now name Doorbell and link to it.

4. Options doctest contradicts itself (options.rs:49)

Correct, and it is a rendered doctest, so it was teaching the wrong thing on the API page. The comment "The peak, not the depth right now." sat directly above assert_eq!(rx.len(), 1), which is the depth right now. Reworded to describe both lines: len is the current depth, high_water is the peak.

5. Magic Win32 parameters (request_cost.rs:157)

Correct, and this repository's conventions forbid bare numeric literals of this kind outright. Now GENERIC_READ, FILE_SHARE_READ and OPEN_EXISTING. The three values were checked against windows-sys rather than assumed: 2147483648, 1, 3 -- matching the literals they replaced.


Unrelated, but found by CI on the same push and worth flagging here: slotwise_mpsc computed its high-water depth after the release store that publishes a slot, so the consumer could drain past that position and the subtraction wrapped. The peak was recorded through fetch_max, so one race poisoned the metric permanently. Fixed in 0b96b14 by moving the read above the publication, which makes the subtraction unable to go negative rather than clamping it after the fact.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

tools/run-numa-spikes.ps1 currently builds scratch spike crates without a lockfile and does not surface non-zero cargo run exits, which can cause silent probe failures and CI flakiness over time.

Review details
  • Files reviewed: 83/98 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Processor-group handling, provenance stamping, probe reporting, and release plumbing contain correctness and reliability gaps.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 83/98 changed files
  • Comments generated: 18
  • Review effort level: Balanced

Comment thread crates/windows-placement-probe/src/core_affinity.rs Outdated
Comment thread crates/windows-placement-probe/build.rs
Comment thread .github/workflows/release-placement-probe.yml Outdated
Comment thread crates/windows-waitable-queues/src/lib.rs Outdated
Comment thread crates/windows-waitable-queues/README.md Outdated
Comment thread crates/windows-platform-probes/src/bin/topology.rs Outdated
Comment thread crates/windows-platform-probes/src/bin/core_affinity.rs Outdated
Comment thread crates/windows-platform-probes/src/bin/peer_index_cache.rs Outdated
Comment thread crates/windows-platform-probes/src/bin/request_cost.rs Outdated
Comment thread crates/windows-platform-probes/src/bin/core_affinity.rs Outdated
Copilot AI review requested due to automatic review settings September 1, 2026 00:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The long-path probe can misread registry state and issue invalid conclusions when opt-in prerequisites or short-path controls fail.

Review details

Suppressed comments (3)

crates/windows-platform-probes/src/long_path.rs:172

  • This substring check also treats values such as 0x10 or 0x100 as enabled. Since the probe uses this flag to decide whether the machine half of the opt-in is present, such a registry value produces a misleading interpretation. Parse the queried value as an exact token (or DWORD) and require 1.
    crates/windows-platform-probes/src/long_path_report.rs:118
  • The sharp-edge classification only examines the long attempts. If DotDot or ForwardSlash also failed below MAX_PATH, this still reports a length-triggered parsing discontinuity even though its control failed. Refuse to draw a verdict unless every short counterpart opened.
    crates/windows-platform-probes/src/long_path_report.rs:124
  • probe-long-path-unaware is expected to refuse the long plain path, and the aware binary is also expected to refuse it when LongPathsEnabled is off. This unconditional branch labels those control outcomes as evidence that the documented opt-in behavior is wrong. Only make that claim when both opt-in halves are present; otherwise report the refusal as expected.
  • Files reviewed: 69/72 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

D-46 pinned `windows-ioring-sys`' next release to 0.2.1, on the ground
that two `topology`-scoped breaking commits had edited this crate's
paths and release-please attributes by path rather than by scope. The
reasoning was sound; the pin never happened.

The `Release-As: 0.2.1` footer sat on `cdce13b`, which stayed on this
branch. The release ran from `main` without it, so tag
`windows-ioring-sys-v0.3.0` exists and the crate's CHANGELOG carries
exactly the entry the pin was written to avoid -- a BREAKING CHANGES
heading citing `**topology:** reshape the topology model around observed
domains`.

So the pin is removed rather than left standing as an intention: a
`Release-As: 0.2.1` carried forward now would ask release-please to
regress a crate already at 0.3.0, and a decision note that says a
release is pinned when it has already shipped at a different version is
worse than no note.

Swept for every statement of it before removing, per the blast-radius
rule. Four sites, all updated together:

- D-46's index row and its section in the crate's DESIGN-NOTES, deleted.
- SH-3.4.2's decision paragraph, which now records the outcome instead
  of the intention.
- The bump table's ioring row, now 0.3.0 shipped rather than 0.2.1
  pinned.
- SH-3.4.2's "cheap correct fix" paragraph, which still read as a live
  proposal and now says the option was not taken.

What survives is the general lesson, and it is now enforced rather than
remembered: tools/check-commit-scope.ps1 is on `main` and flags a
release-triggering commit spanning more than one released crate, which
is the mechanism that produced this bump. The reasoning about path
attribution, the three-commit deprecation dance for a cross-crate
rename, and the measured cost of a blanket one-crate-per-commit rule is
untouched.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Topology and long-path probes contain boundary and interpretation errors that can produce incorrect results or reject valid hardware.

Review details

Suppressed comments (7)

Previously missed (5) — in code that hasn't changed since the last review.

crates/windows-platform-probes/src/bin/topology.rs:165

  • GetNumaHighestNodeNumber returns the largest node number, not a node count; node numbers may be sparse. Printing highest + 1 as “nodes” contradicts the new sparse-NUMA handling in topology::Observation::cross_check and reports a false count for nodes such as 0 and 2.
    crates/windows-platform-probes/src/long_path.rs:295
  • resolved_len excludes the terminating NUL, so 259 UTF-16 units is the longest ordinary path content and 260 is already over MAX_PATH. The current > comparison misclassifies the exact boundary as within the limit (the same boundary is asserted in windows-namespace-request-sys/src/path/tests.rs:261-289).
    crates/windows-platform-probes/src/tests.rs:860
  • This assertion reintroduces the level-number ordering that MachineMemoryTopology::outermost_partitioning_cache deliberately replaced with set inclusion. A valid topology can select a lower-numbered coarser partition while a higher level also partitions (the topology crate tests exactly that at src/topology/tests.rs:1600-1628), so this host-dependent test can reject a correct result. Validate membership inclusion using the owning helper's data, or remove this level-based assertion.
    CHECKLIST.md:187
  • This completed item is much longer than 100 characters but remains in the active checklist with its full historical write-up. The repository's completed-item rule is stated just above at lines 117-124: move the body to COMPLETED-CHECKLIST.md and leave a one-line anchored stub here, as M34.1 demonstrates.
    crates/windows-topology-sys/CHECKLIST.md:22
  • This now says the traits belong to topology-planner, but the newly added component architecture says they live in the separate topology-model crate and that nothing depends on the planner (crates/topology-planner/COMPONENT.md:39-45). Keep the link to the planning checklist, but describe the adapter as implementing the model's traits so the deferred-work note agrees with EP-D-5.

crates/windows-platform-probes/src/long_path.rs:172

  • This substring check also accepts values such as 0x10 and 0x100, even though the probe documents that only DWORD value 1 enables the machine half of long-path support. That makes the report claim the opt-in is active when it is not; parse the value token and compare it exactly.
    crates/windows-platform-probes/src/long_path_report.rs:116
  • The verdict treats every failed non-plain long attempt as evidence of prefix-style reparsing, without checking either that the corresponding short control opened or that the failure was the expected not-found refusal. An access-denied or other unrelated failure therefore produces the confident “SHARP EDGE” conclusion and claims the shape worked below the ceiling even when it did not. Gate this conclusion on both controls and report other failures as inconclusive.
  • Files reviewed: 68/71 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Brings in PR #68, the ioring peel. Six conflicts, all files that peel
touched, all resolved to main's copy: those carry four review rounds
this branch's originals predate -- a bounded `pop_within` replacing an
unbounded completion loop, an HRESULT assertion replacing a Display
string match, the dropped citation of a checklist that is not on main,
and the design session's stale API names annotated rather than rewritten.
Confirmed the branch had made no independent change to any of the six
since the peel was taken, so taking main's loses nothing.

The merged tree then failed one test:
`flush_barrier::a_covering_flush_waits_for_preceding_writes_and_an_unordered_one_does_not`,
with `left: 1, right: 0` -- one of 32 writes completed ahead of the
covering flush.

That is the flake this branch already documented, not a merge defect,
and it was verified rather than assumed:

- The merge touches neither `tests/flush_barrier.rs` nor `src/batch.rs`.
- Ten consecutive isolated runs pass.
- The full suite passes on immediate re-run: 2913 of 2913.

Two records updated as a result.

UNRESOLVED-TEST-FAILURES.md said "observed once"; it is now twice in
three days, so it carries both sightings, the measurements taken after
the second, and the observation that two in three days is a rate rather
than an anomaly -- both were local full-workspace runs, and if the rate
holds it will eventually land in CI, where it would read as a real
ordering defect to whoever sees it first.

CHECKLIST.md gains M20.5, because the decision that record has been
describing since the first sighting was scheduled nowhere. A note that
says work is needed, with no checklist item, is the orphaned-work shape
this repository's rules forbid; the second failure is what made it
visible. The item states why the two candidate answers are not
equivalent -- making the assertion load-independent keeps coverage under
contention but may weaken what it proves, while marking the test serial
preserves the assertion and gives up the contended case, which is the
case a real consumer runs in -- and says not to close it by loosening
the assertion without naming the guarantee surrendered.

Verified: cargo check --all-targets clean, 2913 tests pass across 105
suites, encoding clean over 625 files.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Topology reporting and tests misinterpret valid cache and NUMA states, while the handle benchmark measures more than its documented operation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 59/62 changed files
  • Comments generated: 8
  • Review effort level: Balanced

Comment thread crates/windows-platform-probes/src/bin/topology.rs
Comment thread crates/windows-platform-probes/src/bin/topology.rs
Comment thread crates/windows-platform-probes/src/request_cost.rs Outdated
Comment thread crates/windows-platform-probes/src/tests.rs Outdated
Comment thread crates/windows-platform-probes/src/tests.rs Outdated
Comment thread CHECKLIST.md Outdated
Comment thread CHECKLIST.md Outdated
Comment thread crates/windows-platform-probes/build.rs Outdated
Comment on lines +13 to +15
//! `rustc-link-arg-bin` rather than `rustc-link-arg-bins`: the latter would
//! opt every probe in this crate into long paths, silently changing what the
//! other thirteen measure.
Brings in PR #69 (the flush-barrier contract correction), release-please's
windows-ioring-sys 0.3.1, and three dependabot bumps -- windows-core 0.62.2 to
0.100.0, actions/upload-artifact 4 to 7, actions/download-artifact 4 to 8.

Two conflicts, both in files this branch and main had each edited for the same
underlying reason, and neither resolvable by taking a side wholesale.

UNRESOLVED-TEST-FAILURES.md -- took main's "None currently". This branch still
carried the flush_barrier entry describing the failure as load-sensitive and
following contention. D-47 disproved exactly that: an idle ring failed too, and
the assertion was measuring a claim the platform does not honour. Keeping this
branch's text would have reintroduced a hypothesis the merge itself brings in
the refutation of. The entry now lives in RESOLVED-TEST-FAILURES.md.

CHECKLIST.md -- kept both items, but M20.5 arrives dissolved rather than
pending. It asked whether to make that assertion load-independent or mark the
test serial; the answer is neither, because the premise was wrong. Recorded as a
checked item saying so and pointing at the resolution, rather than deleted --
the question was real and a future reader should find why it went away. M20.6
(from main) stays live, and its parenthetical about reserving the M20.5 number
is updated now that the two have met.

Verified beyond the conflicts, because a clean auto-merge is not the same as a
correct one:

- ci.yml auto-merged with no conflict. Enumerated its job keys: 20, no
  duplicates. An earlier merge on this branch produced a duplicate job this way,
  visible only by enumeration -- conflict markers never appear for it.
- tools/check-workflow-refs.ps1: 56 references across 5 workflow files resolve.
- cargo check --all-targets clean in debug and release against windows-core
  0.100.0, which is the bump most likely to have broken something.
- cargo test --workspace --all-features: 2999 passed, 0 failed, 24 ignored,
  doctests included.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Long-path verdicts can misclassify controls and topology reporting/tests contain invalid assumptions about NUMA numbering and cache-level ordering.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

crates/windows-platform-probes/src/tests.rs:850

  • This test reintroduces the level-number ordering that the new topology model explicitly rejects. A valid result can choose L2 while a finer L3 also partitions (the synthetic test at lines 893-920 demonstrates exactly that), and None can also mean multiple incomparable partitions rather than “no level partitions.” Either case makes this host-dependent test fail against a correct topology; remove it or rewrite it without re-deriving the partitioning rule from level/domain counts.
    crates/windows-platform-probes/src/bin/topology.rs:165
  • GetNumaHighestNodeNumber returns the largest node number, not a node count; node numbers may be sparse (as the new cross-check tests cover with nodes 0 and 2). Reporting highest + 1 as the number of nodes therefore prints false topology information on such machines. Label this as the highest node number and use observation.numa_domains when a count is needed.
    crates/windows-platform-probes/src/long_path_report.rs:118
  • This list does not establish the condition the verdict later claims (“working below it” and then stopping past the ceiling). It selects any unopened long attempt, even when the matching short control also failed or the error was not the is_refusal not-found code. In either case lines 126-138 can report a parsing sharp edge from an invalid control or an unrelated I/O failure. Validate that each short counterpart opened and classify long failures with is_refusal; otherwise report the run as inconclusive.
  • Files reviewed: 58/61 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment on lines +120 to +125
if !plain_long_opened {
return "-- MAX_PATH was NOT lifted for a relative path: even the plain shape was\n\
refused past the ceiling. The documented reading is wrong for this\n\
configuration."
.to_string();
}
Brings in everything peeled and reviewed since the last merge: the
namespace-request tests (#74), the Unicode terminology sweep (#75), the
security last-error correction (#76), the flush-barrier instruments (#77), and
the dangling-anchor repairs (#78).

Two conflicts, both in files this branch originated and main has since improved
through review. Took main's side in every hunk, which is the whole point of the
peel-and-review cycle -- the branch holds the drafts, main holds what survived:

  path/tests.rs      the branch still measured path lengths in bytes and
                     scalars; main's version measures UTF-16 units, guards the
                     helper against an underflowing length, and adds the case
                     that separates the units
  security/tests.rs  the branch still asserted a non-zero last-error from
                     `IsValidSecurityDescriptor`, which Windows does not
                     document as setting it; main's version compares the typed
                     accessor against the source chain instead

Verified beyond the conflicts:

- ci.yml auto-merged with no conflict; enumerated its job keys -- 20, no
  duplicates. An earlier merge on this branch produced a duplicate job exactly
  this way, and it is invisible to conflict markers.
- No conflict markers anywhere in the tree.
- cargo check --all-targets clean.
- cargo test --workspace --all-features: 3000 passed, 0 failed, 24 ignored.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Several probes currently produce misleading verdicts or timings, and completed checklist items are not archived according to repository conventions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

crates/windows-platform-probes/src/long_path_report.rs:124

  • The shared renderer treats the expected result of probe-long-path-unaware as a contradiction: measure(false) normally refuses the over-MAX_PATH plain path, so this branch prints “the documented reading is wrong” even though the control just confirmed that an executable without longPathAware remains limited. It is also conclusive when LongPathsEnabled is absent, despite the warning above saying that run tests a different question. Branch on both manifest_aware and registry_enabled, and reserve this verdict for the aware+enabled case.
    crates/windows-platform-probes/src/bin/topology.rs:164
  • GetNumaHighestNodeNumber returns the largest node identifier, not a node count. NUMA identifiers may be sparse (as the new test at src/tests.rs:763-779 explicitly verifies), so a highest identifier of 2 does not imply three nodes; this report contradicts the probe’s corrected cross-check and can publish false machine data.
    crates/windows-platform-probes/src/request_cost.rs:186
  • This timing includes destruction of every returned CapturedHandle: the generic loop drops black_box(body()) at each semicolon, and CapturedHandle owns an OwnedHandle, whose drop calls CloseHandle. The reported capture_handle value therefore measures DuplicateHandle plus CloseHandle, but the report interprets it as the cost of duplication alone. Either retain/drop results outside the timed region using a bounded batching strategy, or rename and interpret this as a capture-and-release lifecycle cost.
    .github/workflows/ci.yml:249
  • The request-cost probe is also run with the unoptimized dev profile, so its nanosecond timings and cross-operation ratios include debug-only Rust overhead and are not comparable to production request construction. Run this timing probe in release mode as well.
        run: cargo run -p windows-platform-probes --bin probe-request-cost --locked

CHECKLIST.md:187

  • This completed item is longer than 100 characters, so the repository’s completed-item convention (restated in this same file at lines 117-124) requires moving its body to COMPLETED-CHECKLIST.md immediately and leaving a one-line anchored stub here. Keeping the full completed M35 milestone in the active checklist also leaves historical prose where only actionable work should remain.
- [x] **M35.1** -- **Measure whether the long-path opt-in lifts `MAX_PATH` for a *relative* path, and
  whether it does so without changing how the path is parsed.**
  • Files reviewed: 52/55 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread .github/workflows/ci.yml Outdated
Comment thread crates/windows-ioring-sys/CHECKLIST.md Outdated
Mike Grier and others added 2 commits September 7, 2026 20:48
Brings back stages 1 and 2 of peeling this branch apart: the probe output
sink (#79) and the long-path probe pair (#80). Both were taken from this
branch, reviewed, and corrected on the way through, so main holds the
successor of every file they touch.

Twenty-two conflicts, resolved on that basis:

- The sixteen peeled sources -- `report.rs`, the long-path family, the eight
  converted probe binaries, and `build.rs` -- take main's version. Checked
  rather than assumed: the only content on this branch and not on main is
  what main replaced (the pre-sink `emit(&mut Stdout, &render())` shape,
  `registry_enabled: bool`, the `0x1` substring registry read, a bare `183`
  for ERROR_ALREADY_EXISTS, and the dead `let _ = deep_relative`).

- `report.rs` drops this branch's `writeln_to`, which no probe called. Main
  keeps `emit` and `Stdout`, so the six probes still on this branch and not
  yet peeled continue to compile against it unchanged.

- `lib.rs`, `Cargo.toml`, `ci.yml` and this crate's DESIGN-NOTES take the
  union: this branch's four probe modules and six binaries alongside the
  long-path pair, and both sides' design sections.

- Both sides had `[[bin]]` entries for the long-path pair. Kept main's, which
  carries the comment explaining why they are two binaries rather than one
  with a flag, and dropped this branch's uncommented duplicate -- cargo
  rejects the manifest outright with both.

- Root PLANS.md keeps this branch's newer `windows-ioring-sys` row (M1-M19,
  not M1-M7) and takes main's new `windows-platform-probes/CHECKLIST.md` row.
  Main's mutation-survivors row was already here verbatim, so it is not
  duplicated.

One change beyond the resolution: the `GetSystemDirectoryW` comment in
Cargo.toml sat above `Win32_System_Environment`, which is not the feature it
documents. Moved beside `Win32_System_SystemInformation`, which is.

Verified: `check --all-targets` clean with no warnings, `clippy
--all-targets --all-features` clean, `fmt --check` clean, 73 package tests
pass, encoding check 643 files clean, workflow references resolve, and all
seventeen probe binaries build and run to exit 0.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…g stderr

Under Windows PowerShell 5.1 a native command that writes to stderr while
`$ErrorActionPreference` is `Stop` raises a TERMINATING error when its stderr
is redirected with `2>&1`. PowerShell 7 does not, which is why this survived
review. `run-numa-spikes.ps1` had two such captures and no guard.

The shape of the failure is what makes it dangerous. `cargo --quiet` writes
nothing to stderr on a clean build, so under 5.1 this script ran to completion
for as long as every spike was healthy. It threw only when cargo did write
there -- a warning, or a failed compile -- which is exactly the case the script
exists to report. The throw landed before `$buildExit` was assigned, so the
broken-instrument branch never ran: no transcript, no summary, and the one
artifact somebody downloads to diagnose a rotted spike was the one case that
never produced it.

Measured rather than argued, in both directions. Against a deliberately
uncompilable crate on 5.1: unguarded, it threw `RemoteException` and captured
nothing; guarded, it returned exit 101 with all seven diagnostic lines. On 7
both forms are fine. The healthy path was checked too -- removing the guard and
re-running still passed, which is why the earlier reading that this script was
simply broken under 5.1 was wrong.

Both captures now go through an `Invoke-Native` helper matching the ones in
`soak-flush-barrier.ps1` and `test-run-sabotage.ps1` line for line, and its
`ConvertTo-OutputLines` also flattens the ErrorRecords `2>&1` produces. That
removes the stray `System.Management.Automation.RemoteException` the old
comment described working around, so the comment now points at the helper
instead of at a workaround that is gone.

`run-mutants.ps1` was inspected for the same defect and needs no change: it
redirects nothing, and a bare native call raises no error record on 5.1
regardless of the preference. Confirmed by experiment on both hosts, and it
parses clean under 5.1.

Sweeping the class found six more captures of the same shape -- one
`git check-ignore` in `run-sabotage.ps1` and five `git init` / `git add -A`
calls in `test-run-sabotage.ps1`. They are latent rather than firing: the
sabotage suite passes under 5.1 today because git stays silent on those
operations. Queued as M34.4 rather than fixed here, because the fix would be a
third and fourth copy of one guard and `tools/` has no module convention to
share it -- a decision worth making before duplicating further.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Several probes can report incorrect measurements, and completed checklist items remain improperly archived.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

crates/windows-platform-probes/src/tests.rs:860

  • This test reintroduces the level-number ordering that the production API deliberately rejects. A valid topology where a lower-numbered level is the coarser partition fails the Some arm, while incomparable partitionings legitimately return None and fail the other arm. Since this summary has discarded processor membership, it cannot independently verify inclusion; assert that the selected level is preserved and that a selected summary has multiple domains instead.
    crates/windows-platform-probes/src/bin/topology.rs:164
  • GetNumaHighestNodeNumber reports the largest node number, not the number of nodes. Node IDs may be sparse (as the new test at src/tests.rs:763-779 establishes), so rendering highest + 1 as a node count gives false output—for nodes 0 and 2 it prints three nodes. Report only the highest node number here.
                "  GetNumaHighestNodeNumber    : {highest} (so {} nodes)",
                highest + 1

crates/windows-platform-probes/src/request_cost.rs:186

  • This timing includes CloseHandle, not just capture/duplication: time_loop drops each returned CapturedHandle at the end of the statement, and CapturedHandle owns an OwnedHandle. The report repeatedly interprets this as the cost of DuplicateHandle, so it can materially overstate that operation. Either retain the captured handles until after timing or rename the metric and all interpretations to explicitly measure duplicate-plus-close.
    timings.push(time_loop("capture_handle", HANDLE_ITERATIONS, || {
        CapturedHandle::capture(borrowed).expect("duplicating an owned handle")
    }));

crates/windows-ioring-sys/CHECKLIST.md:135

  • This completed item is longer than 100 characters but remains in the active checklist. The repository's completed-item rule requires moving its body to COMPLETED-CHECKLIST.md immediately and leaving a one-line anchored completion stub here.
- [x] **M20.5** -- **Dissolved by [D-47](DESIGN-NOTES.md#d-47-detail), not decided.** This asked whether to
  make the load-sensitive `flush_barrier` assertion load-independent or mark the test serial. Neither: the
  assertion was measuring a claim the platform does not honour, so it was not a flaky test at all. Its own
  contention hypothesis was disproved in the same measurement -- an idle ring failed too. Recorded in
  [RESOLVED-TEST-FAILURES.md](RESOLVED-TEST-FAILURES.md).

CHECKLIST.md:212

  • M35.1 is a large completed item, but its full body remains in the active checklist. Per the completed-item move-with-link rule, archive the body in COMPLETED-CHECKLIST.md and replace it here with a one-line anchored stub so open work remains navigable.
- [x] **M35.1** -- **Measure whether the long-path opt-in lifts `MAX_PATH` for a *relative* path, and
  whether it does so without changing how the path is parsed.**
  **Done 2026-09-04, and it settles a question that had produced three wrong answers from reading.**
  `probe-long-path-aware` and `probe-long-path-unaware` in
  [windows-platform-probes](crates/windows-platform-probes/src/long_path.rs) are the same code
  • Files reviewed: 36/39 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread crates/windows-platform-probes/src/doorbell_cost.rs
Comment thread crates/windows-platform-probes/src/doorbell_cost.rs
…on.ps1

M34.4 queued this as a decision rather than a fix: the remaining sites needed
a third and fourth copy of one guard, and `tools/` had no sharing convention.
Adopting one, and finding out which one, is the substance of this change.

The guard is against Windows PowerShell 5.1's rule that a native command
writing to stderr under `$ErrorActionPreference = 'Stop'` raises a TERMINATING
error when its stderr is redirected with `2>&1`. PowerShell 7 does not, which
is why this class survives review and CI.

The obvious answer -- a .psm1 -- is wrong, and wrong invisibly. A scriptblock
carries the session state it was created in, so `Invoke-Native { cargo build }`
runs in the CALLER's scope while a module copy flips the preference in the
MODULE's scope; the flip never reaches the call. Measured with the identical
body in a module: under 5.1 seven of eight cases failed while all eight passed
under PowerShell 7. Dot-sourcing puts the function in the caller's own scope,
where the plain assignment does reach the call.

A module can be made to work through `$PSCmdlet.SessionState.PSVariable.Set`,
and that was measured working on both hosts. Rejected: the guard would then
rest on a subtlety that looks removable, and simplifying it back reintroduces
a defect that still passes on PowerShell 7 and in CI. Dot-sourcing makes the
property hold by construction rather than by counter-measure.

- tools/common.ps1 -- `Invoke-Native` and `ConvertTo-OutputLines`, with the
  argument and its measurement recorded at the definition site.
- tools/test-common.ps1 -- eight cases: capture, stream merging, exit-code
  survival, diagnostic text, record flattening, preference restoration
  (including when the command throws), and that `Stop` stays armed for
  non-native errors. It runs in the invoking host, then RE-INVOKES ITSELF in
  the other one, and treats a missing host as a failure rather than a skip --
  a single-host pass is not the claim the file exists to make.
- Every capture site routed: `run-numa-spikes.ps1` and `soak-flush-barrier.ps1`
  lose their local copies; `run-sabotage.ps1` (check-ignore) and
  `test-run-sabotage.ps1` (init, four `add -A`, and its child-process harness
  invocation) now go through the shared guard. `run-mutants.ps1` needs none --
  it redirects nothing, confirmed by experiment on both hosts.
- CI runs both shells. The sabotage job ran `shell: pwsh` only, which is
  precisely why the defect was invisible; it now runs test-common.ps1 plus
  test-run-sabotage.ps1 under both `pwsh` and `powershell`.

Verified by sabotage: delivering the identical guard as a module turns the new
suite red on 5.1 (7 of 8) while staying green on 7, so the suite detects the
regression it was written for. Both consumer scripts and the full sabotage
suite pass on both hosts; encoding and workflow-reference checks pass.

Deliberately not shared: `Write-Report`. Six scripts define one and they are
not duplicates -- they differ in level vocabulary and in rendering, with two
emitting GitHub Actions annotations and four emitting console colours. Merging
them would change six tools' output to remove a duplication that is only
apparent. Recorded in DESIGN-NOTES so it is a decision, not an oversight.

Completed item: M34.4: Guard the remaining native-command captures against
Windows PowerShell 5.1's terminating-stderr rule, by deciding and adopting a
sharing convention for tools/.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Several topology and timing probes currently report or enforce conclusions that do not follow from the measured data.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (10)

Previously missed (5) — in code that hasn't changed since the last review.

crates/windows-platform-probes/src/bin/request_cost.rs:225

  • The subtraction is assigned to the wrong component. clone_prepared_units approximates the allocation/copy/drop cost an allocation-oriented change could remove; build - clone estimates the remaining path-resolution and builder work. Printing the remainder as the recoverable amount reverses the probe’s conclusion.
    crates/windows-platform-probes/README.md:66
  • The baseline is deliberately contended: every producer increments the same AtomicU64 (queue_contention.rs:262-278). Calling it uncontended describes a different benchmark and misstates what the comparison controls for.
    crates/windows-platform-probes/src/bin/queue_contention.rs:40
  • available_parallelism() is a per-process estimate and can reflect affinity or job limits; it is not the host’s logical-processor count. On a constrained runner this line mislabels the context used to interpret the producer-count curve. Either populate the field from a machine-wide topology query or label it as process-available parallelism.
    crates/windows-platform-probes/src/topology.rs:111
  • MachineMemoryTopology::outermost_partitioning_cache() also returns None when two valid candidate partitions are incomparable, not only when every level is machine-wide (topology.rs:839-872). Documenting None as “no level divides” loses that case and leads callers to make a false claim about the machine.
    crates/windows-topology-sys/CHECKLIST.md:23
  • This now contradicts the architecture added in topology-planner/COMPONENT.md:30-50: the traits belong to topology-model, and the inward Windows adapter is a separate component; nothing should depend on topology-planner. Leaving this wording directs future implementation across the dependency boundary the component document explicitly forbids.

crates/windows-platform-probes/src/tests.rs:860

  • This test re-derives “outermost” from numeric cache levels, contradicting the inclusion ordering it is meant to preserve. A lower-numbered cache can be the coarser partition, and None can mean incomparable partitions even when several levels split the machine, so either branch can reject a valid observation.
    crates/windows-platform-probes/src/bin/topology.rs:121
  • None can also mean that multiple partitioning levels are incomparable, so this output can claim every level is machine-wide when several levels actually partition the host. Report only that no unique outermost partition was identified.
                "\nno cache level partitions this machine: every level is machine-wide"

crates/windows-platform-probes/src/bin/topology.rs:164

  • NUMA node numbers may be sparse, so highest + 1 is not the node count—the new cross_check and sparse-node test explicitly rely on that distinction. This line would print three nodes for a valid machine containing only nodes 0 and 2.
                "  GetNumaHighestNodeNumber    : {highest} (so {} nodes)",
                highest + 1

crates/windows-platform-probes/src/request_cost.rs:116

  • The value returned by body() is discarded here, so its destructor runs inside the measured interval. Consequently every advertised construction timing includes teardown; notably capture_handle measures both DuplicateHandle and the CloseHandle performed by CapturedHandle’s OwnedHandle, despite the report describing one duplication transition. Either move destruction outside the timed interval or rename and interpret these as construct-and-drop costs.
        std::hint::black_box(body());

crates/windows-platform-probes/src/doorbell_cost.rs:184

  • The probe discards SubmitIoRing’s HRESULT, so an error-path return is recorded and interpreted as the cost of a successful empty submit. Validate at least one empty submission before timing; otherwise the reported “user-mode short circuit” can just be a fast failure.
            let _ = ring.submit_and_wait(0);
  • Files reviewed: 41/44 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread tools/test-common.ps1
. (Join-Path $PSScriptRoot 'common.ps1')

$script:Failures = 0
$script:Host51 = 'C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe'
…mark the ones that are not

PR #56 has accumulated 196 Copilot reviews and nothing said which had been
dealt with. Judging by eye does not scale, and judging by "did a commit
follow it" is guesswork. tools/scan-pr-reviews.ps1 reports the two signals
that are real and adds the one GitHub does not have.

Findings arrive in two shapes and only one of them has state:

- Inline comments become review threads, which can be RESOLVED. That flag is
  durable, visible and queryable, so it is the tag -- nothing to invent.
- Suppressed comments exist only as prose in the review body's <details>
  block. They create NO thread, so there is nothing to resolve, and a review
  is not a reactable object either: POST /pulls/{n}/reviews/{id}/reactions
  returns 404, while the same call against an inline comment succeeds
  (verified both ways). Nothing anywhere records that a suppressed finding
  was read, which is the gap this closes.

For those, -MarkProcessed posts a marker comment on the pull request:

    <!-- copilot-review-processed: 5136043258 -->

An HTML comment, so it does not render; on the pull request rather than in a
file or a session, so it survives a new machine, contributor, or agent
session; and read back by the script, so a handled review stops being
reported. A -Summary is required alongside it, because a marker with no
account of what was done is a claim with no evidence.

The report separates unresolved threads into current and outdated. Outdated
means the anchored line has since changed, which usually means the finding
was fixed and the thread never resolved, so those are cheap to clear and are
listed only under -IncludeOutdated.

Exercised end to end on PR #80, which had four reviews all genuinely handled
in this session: its six threads are now resolved, its one suppressed-only
review (5136043258, the /MANIFESTINPUT: suggestion refuted by measurement) is
marked, and a re-scan reads back clean at exit 0.

Deliberately NOT done: back-filling PR #56's 131 suppressed-carrying reviews.
A marker asserts the review was read. Almost all were addressed in the rounds
that followed them, but "almost all" is not evidence, and marking them
wholesale would convert an honest absence of information into a false record.

Runs on both PowerShell hosts, through the shared Invoke-Native guard.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Probe measurements and topology reporting contain incorrect interpretations, while review pagination and marker trust can omit outstanding findings.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (7)

Previously missed (1) — in code that hasn't changed since the last review.

crates/windows-platform-probes/src/bin/request_cost.rs:225

  • The recoverable-cost arithmetic is reversed. clone_prepared_units is the allocation/copy proxy that inline storage could avoid; build - clone is the remaining path-resolution/builder work. As written, the report can overstate the allocator opportunity by nearly the entire syscall cost.

crates/windows-platform-probes/src/request_cost.rs:118

  • time_loop drops each returned value while the clock is still running. In particular, capture_handle therefore measures both DuplicateHandle and CapturedHandle's OwnedHandle drop (CloseHandle), although the report attributes the number solely to duplication; the other construction rows also include destruction. Retain results until after the elapsed time is sampled, or measure and label destruction separately, so the probe does not draw conclusions from conflated costs.
    let start = Instant::now();
    for _ in 0..iterations {
        std::hint::black_box(body());
    }
    let elapsed = start.elapsed();

crates/windows-platform-probes/src/tests.rs:850

  • This test re-derives the old level-number rule that the new topology API explicitly replaced. A lower-numbered cache can be the coarser partition, and multiple incomparable partitioning levels correctly produce None; either case makes this test fail despite MachineMemoryTopology::outermost_partitioning_cache returning the specified answer. Assert only that a returned summary actually partitions, and leave ordering to the topology crate's inclusion-based tests.
    crates/windows-platform-probes/src/bin/topology.rs:123
  • None does not imply that every cache level is machine-wide. The topology API also returns None when multiple valid partitionings are incomparable, so this message can misreport a real machine. Describe the absence of a unique outermost partition instead.
        None => {
            let _ = writeln!(
                out,
                "\nno cache level partitions this machine: every level is machine-wide"
            );
        }

crates/windows-platform-probes/src/bin/topology.rs:165

  • GetNumaHighestNodeNumber returns a node identifier, not a count; node numbers may be sparse (as the new test in src/tests.rs demonstrates). Printing highest + 1 as the number of nodes makes the probe contradict its own parsed numa_domains value on valid sparse topologies.
    match observation.raw_highest_numa_node {
        Some(highest) => {
            let _ = writeln!(
                out,
                "  GetNumaHighestNodeNumber    : {highest} (so {} nodes)",
                highest + 1
            );

CHECKLIST.md:194

  • This newly completed item is far over the repository's 100-character threshold but remains in the active checklist. Per the completed-item convention documented at .github/copilot-instructions.md:1387-1421 (and demonstrated by M34.1 above), move its body to COMPLETED-CHECKLIST.md and leave a one-line anchored stub here.
- [x] **M35.1** -- **Measure whether the long-path opt-in lifts `MAX_PATH` for a *relative* path, and
  whether it does so without changing how the path is parsed.**
  **Done 2026-09-04, and it settles a question that had produced three wrong answers from reading.**
  `probe-long-path-aware` and `probe-long-path-unaware` in
  [windows-platform-probes](crates/windows-platform-probes/src/long_path.rs) are the same code

crates/windows-ioring-sys/CHECKLIST.md:135

  • This completed multi-line item should not remain expanded in the active checklist. The repository convention at .github/copilot-instructions.md:1387-1421 requires completed items over 100 characters to move immediately to COMPLETED-CHECKLIST.md, leaving an anchored one-line stub.
- [x] **M20.5** -- **Dissolved by [D-47](DESIGN-NOTES.md#d-47-detail), not decided.** This asked whether to
  make the load-sensitive `flush_barrier` assertion load-independent or mark the test serial. Neither: the
  assertion was measuring a claim the platform does not honour, so it was not a flaky test at all. Its own
  contention hypothesis was disproved in the same measurement -- an idle ring failed too. Recorded in
  [RESOLVED-TEST-FAILURES.md](RESOLVED-TEST-FAILURES.md).
  • Files reviewed: 42/45 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread tools/scan-pr-reviews.ps1
Comment on lines +133 to +139
# Reviews already recorded as processed, by marker.
$processed = @{}
foreach ($c in $issueComments) {
foreach ($m in [regex]::Matches((Get-Text $c.body), '<!--\s*copilot-review-processed:\s*(\d+)\s*-->')) {
$processed[[long]$m.Groups[1].Value] = $true
}
}
Comment thread tools/scan-pr-reviews.ps1
Comment on lines +130 to +131
$reviews = Invoke-GitHubJson @('api', "repos/$script:Owner/$script:Name/pulls/$Pr/reviews?per_page=100", '--paginate')
$issueComments = Invoke-GitHubJson @('api', "repos/$script:Owner/$script:Name/issues/$Pr/comments?per_page=100", '--paginate')
…ee checklist items

The eleven open Copilot findings on PR #56, worked through together. Four were
probes reporting a number that was not what its label said, which is the
failure this crate exists to prevent.

**request_cost measured duplicate-plus-close and called it duplication.**
`time_loop` black-boxes its closure's return and drops it at the end of the
statement, so the returned `CapturedHandle` -- which owns an `OwnedHandle` --
called `CloseHandle` inside the timed region. Capture and close are now timed
separately, retaining the duplicates in a pre-sized Vec and timing the drop of
that same Vec so the close is recovered as its own figure rather than
discarded. Measured on this host in release: capture 322 ns, close 272 ns. The
old combined figure was therefore reporting duplication as roughly 594 ns,
nearly double, and the report and design notes read it as duplication alone.
Both numbers are now printed and both appear in the JSON line.

**topology derived a node count from GetNumaHighestNodeNumber.** That reports
the largest node NUMBER and numbers may be sparse, so `highest + 1` prints
three nodes for a machine with nodes 0 and 2 -- the same mistake
`Observation::cross_check` was corrected to stop making, reintroduced in the
output printed beside it. It now prints the identifier and says what it is.

**topology read `None` from outermost_partitioning_cache as "no level
partitions this machine".** It also means two levels partition incomparably,
so the renderer turned a deliberate ambiguity result into a false claim about
the hardware. It now reports the absence and names both possibilities.

**doorbell_cost discarded SubmitIoRing's HRESULT.** A host where the call
fails still produces a plausible timing -- a failing call costs a measurable
transition -- which the report read as a successful empty submission. Both the
status and the expected zero submitted entries are now asserted inside the
measured operation, so a failure fails the probe instead of corrupting its
evidence.

**The outermost-cache test re-derived the selection from level numbers**,
which is the rule the topology crate abandoned and which this module's own doc
comment records abandoning; the synthetic test below it builds the
counterexample. Its `None` arm made the same wrong inference as the renderer.
Since these summaries discard processor membership, inclusion cannot be
re-derived here at all, so it now asserts what survives the summarising: a
selection partitions, is one of the surveyed levels, and agrees with the level
the survey captured.

**The park-and-wake handshake had no regression test** even though its own
documentation records that the first implementation deadlocked. Two bounded
tests: the deterministic `rounds == 0` contract, and a small round trip that
must complete and report a positive finite average.

**The two timing probes now run --release in CI**, and only those two. They
compare operations tens of nanoseconds apart and the design reads their
RATIOS; in an unoptimized build the loop and closure overhead around each
measured call does not inflate them uniformly. Every other probe measures
behaviour, which does not change with the optimisation level, so they keep the
dev profile. The DOORBELL_NS_REFERENCE constant is NOT re-baselined: it is the
figure the 2026-08-30 design session recorded, and replacing it would leave
that session describing a number that exists nowhere. Its unrecorded build
profile is now stated instead, with a pointer to the like-for-like comparison
that does exist -- both probes' own output from the same release CI job.

**Three checklist items archived** per the move-with-link rule: M35.1 here and
M20.5 in windows-ioring-sys, each now a one-line stub pointing at an anchored
archive entry. M35.1's body claimed the manifest embed leaves "the other
thirteen probes" unaffected; there are sixteen, so the count was already stale
and would have been written into the record as a false number. The archived
text uses a count-free phrasing and says why.

Verified: fmt clean, workspace clippy --all-targets --all-features clean, 75
package tests pass, encoding check 634 files clean, and probe-request-cost and
probe-topology were run in release to confirm the corrected output rather than
read from the diff.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@MikeGrier

Copy link
Copy Markdown
Owner Author

Processed in e1501f2, together with the other ten open findings on this pull request.

All five suppressed comments in this review were acted on. Three restated inline findings
that are fixed and resolved (the tests.rs:860 outermost assertion, topology.rs:164
deriving a node count from GetNumaHighestNodeNumber, and request_cost.rs:186 timing
CloseHandle inside the capture loop). The other two were the checklist-archiving rule:
M20.5 in windows-ioring-sys and M35.1 here are now one-line stubs pointing at anchored
entries in their COMPLETED-CHECKLIST files.

The request_cost one turned out to be the costly one, and measuring it said so: on this
host in release, capture is 322 ns and close is 272 ns, so the old combined figure reported
duplication as roughly 594 ns -- nearly double -- and the report and design notes read it as
duplication alone. Both are now measured and printed separately.

While archiving M35.1 its body was found to claim the manifest embed leaves "the other
thirteen probes" unaffected; there are sixteen. The archived text uses a count-free phrasing
so the record cannot go stale again, and says why it was changed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The review scanner currently breaks on multi-page review data and trusts processed markers from untrusted commenters.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (5) — in code that hasn't changed since the last review.

tools/scan-pr-reviews.ps1:131

  • gh api --paginate emits one JSON document per page, but Invoke-GitHubJson concatenates all output and calls ConvertFrom-Json once. As soon as a PR exceeds 100 reviews, this becomes adjacent JSON arrays and parsing fails; PR #56 already has more than 100 reviews. Request slurped output and flatten its pages, or parse each page separately.
    tools/test-common.ps1:36
  • This hardcodes Windows PowerShell to the C: volume, so the mandatory cross-host test falsely reports 5.1 missing when Windows is installed elsewhere. Resolve the path from SystemRoot, matching the repository's handling of non-C: Windows installations.
    crates/windows-platform-probes/src/bin/queue_contention.rs:11
  • mpsc is no longer a queue shape in this crate; the implementation and shared shape constant use slotwise_mpsc. Keeping the removed identifier in the binary's module documentation makes the decision being measured ambiguous.
    crates/windows-platform-probes/src/bin/queue_contention.rs:137
  • The rendered report still names the removed mpsc shape even though this row measures slotwise_mpsc. Use the public shape name so readers can map the comparison to the queue they can actually select.
    crates/windows-topology-sys/CHECKLIST.md:23
  • This statement is stale after the four-part architecture introduced in this PR: topology-planner/COMPONENT.md:39-45 and EP-D-5 place the queried traits in topology-model and explicitly require the inward adapter not to depend on topology-planner. As written, this checklist still documents the dependency edge that EP-D-5 rejects.
  • Files reviewed: 43/46 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread tools/scan-pr-reviews.ps1
Comment on lines +135 to +139
foreach ($c in $issueComments) {
foreach ($m in [regex]::Matches((Get-Text $c.body), '<!--\s*copilot-review-processed:\s*(\d+)\s*-->')) {
$processed[[long]$m.Groups[1].Value] = $true
}
}
pub mod cancel_io;
pub mod completion_port;
pub mod device_map;
pub mod doorbell_cost;
Brings back stage 3 of peeling this branch apart: the shared native-command
guard and the Copilot review scanner (#81). Taken from this branch, then
corrected over ten review rounds on the way through, so main holds the
successor of every file it touches.

Four conflicts, all resolved to main on that basis. Checked rather than
assumed: every line unique to this branch is one those rounds replaced --

- common.ps1: the `try`/`finally` restoration that was dead code, because the
  assignment is function-local and the caller's value was never modified.
- scan-pr-reviews.ps1: `throw`-based error paths that exited 1 and collided
  with the findings code, and the bare marker format that could be retired by
  quoting it in ordinary discussion.
- test-common.ps1: the three cases that asserted the dead restoration and
  could not fail.
- DESIGN-NOTES: the same claims in prose, plus the marker documentation, which
  main now states with the sentinel format and the write-access rule.

Verified on both PowerShell hosts, since that is the whole subject of what
merged: test-common and the sabotage suite pass under 7 and 5.1, the review
scanner runs, encoding checks 636 files clean, workflow references resolve, and
`cargo check --all-targets` is clean with no warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Several probes lose diagnostics on panic, while topology and request-cost reporting can produce misleading results.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 36/39 changed files
  • Comments generated: 7
  • Review effort level: Balanced

Comment on lines +23 to +25
emit(
&mut Stdout,
&render(&measure(), measure_park_and_wake(20_000)),
fn main() {
// The only place that names the real stream. Everything below composes
// text; nothing below knows where it goes.
emit(&mut Stdout, &render());
fn main() {
// The only place that names the real stream. Everything below composes
// text; nothing below knows where it goes.
emit(&mut Stdout, &render());
fn main() {
// The only place that names the real stream. Everything below composes
// text; nothing below knows where it goes.
emit(&mut Stdout, &render());
Comment on lines +240 to +252
let _ = writeln!(
out,
" Cloning already-prepared units is {clone:.0} ns, which bounds what an"
);
let _ = writeln!(
out,
" inline-storage or recycling scheme could recover at {:.0} ns per request",
build - clone
);
let _ = writeln!(
out,
" AT MOST -- and only for a caller that can reuse a resolved path."
);
Comment on lines +291 to +299
// SAFETY: both take no pointer arguments and cannot fail in a way that
// matters here; `ALL_PROCESSOR_GROUPS` is the documented way to ask for the
// machine-wide count.
let raw_active_processors = unsafe { GetActiveProcessorCount(ALL_PROCESSOR_GROUPS) };
let raw_group_count = unsafe { GetActiveProcessorGroupCount() };

let mut highest = 0u32;
// SAFETY: `highest` is a live local for the duration of the call.
let raw_highest_numa_node = if unsafe { GetNumaHighestNodeNumber(&raw mut highest) } != 0 {
Comment on lines +60 to +63
//! That means the measured cost is a *syscall* cost and cannot be tuned away by
//! an allocator. An inline-storage or recycling scheme would only recover the
//! allocation part, which `clone_prepared_units` bounds from below. Knowing
//! which half is which is the point of measuring both.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants