Measure what a doorbell and a namespace request cost

[MikeGrier]

Fifth slice peeled off mikegrier/deferred-namespace-ops (#56), after #79, #80, #81 and #82.

Two probes, peeled together because neither answers its question alone.

The decision they inform

The two-layer ring has a client thread push a descriptor onto a bounded MPSC queue and then, sometimes, signal an event so the domain thread wakes. The design assumes that signal is expensive enough to be worth avoiding, and proposes an eventcount -- publish intent to park, re-check the queue, then wait -- so a producer rings the doorbell only on the empty-to-non-empty edge. Publish-recheck-park is exactly where lost wakeups live, so building it because the cost was assumed would be taking on the highest-risk part of the design without evidence.

probe-doorbell-cost supplies the evidence: SetEvent against an already-signalled event, a full set/reset cycle, a satisfied wait, and an empty SubmitIoRing, each against an uncontended fetch_add floor, plus a real park-and-wake round trip.

probe-request-cost asks the other half. The submission queue was specified to carry POD descriptors with no allocation on push; a deferred CreateFileW carries a path, which is neither. windows-namespace-request-sys makes an OpenFile an owned, Send parameter set, so the queue can carry a request by value -- what remains is what building one costs against the doorbell that would carry it.

Both run --release in CI, and only these two do. Every other probe measures behaviour, which does not change with the optimisation level. These compare operations tens of nanoseconds apart, where the loop and closure indirection carry overhead that does not shrink uniformly.

What it found

On an x86_64 16p/8c host: a doorbell cycle costs 255x an uncontended atomic, a real park-and-wake round trip 43x that again, and building a pathed request ~210 ns.

The batching arithmetic is the finding the design can actually use: at a batch of about 216, the doorbell costs less per operation than the atomic push it accompanies. The skip-when-busy rule is a refinement, not a prerequisite -- batching alone drives the doorbell below the push, so a first implementation can always-signal and stay honest, and the eventcount's lost-wakeup risk can wait for a measurement against real work.

Reviewed before pushing, and it mattered

Two independent reviews found six defects, five of the class this component keeps hitting: a statement true of the machine it was written on, printed as fact everywhere else. All are fixed in fabda22; the notable ones:

• A design conclusion that inverts on this host. The report stated that doorbell tuning would be optimizing "the small half". True on the development machine (165 ns doorbell against a 453 ns build); false here, where the doorbell is ~531 ns against a ~210 ns build. Both probes run in the same CI job, so it was contradicted a few lines down the same log. It now states the comparison, names the figure to read from the sibling probe, and declines the verdict -- which this probe cannot reach, measuring only one side of it.
• The NDJSON handed a machine the ratio the prose forbids. The report tells a human an empty SubmitIoRing is not a fair denominator and that any figure from it is a confident wrong answer -- then emitted "doorbell_share_of_submit" unqualified. Renamed doorbell_over_empty_submit, so the field states its own denominator.
• The short-circuit diagnosis is now measured, not asserted. "Far too cheap for a kernel transition" was written around a 79 ns reading; here the empty submit is 216 ns against 206 ns for a real SetEvent, making the claim false. It is now decided against the probe's own measured syscalls.
• Two optimizations were credited with each other's savings -- recycling recovers the resolution, inline storage recovers the allocation, and both texts named one figure for both.

One finding is documented rather than settled, and is for the engineer: time_loop drops each returned value inside the timed region, so the three heap-owning timings are construct-and-destroy cycles reported as construction. The obvious fix is not obviously better -- retaining every value would hold 100_000 live allocations and measure an allocator that never reuses a block, and a bounded queue is neither regime. The captured-handle loop is restructured, because dropping one calls CloseHandle and that is a second kernel transition rather than a free.

Verification

• 150 tests pass (148 + the two park-and-wake handshake tests, bounded by construction: the handshake's first implementation deadlocked, and a probe that can hang is a probe that can hang a build)
• cargo check --all-targets clean across the workspace; clippy, cargo fmt --check and cargo doc clean
• both probes run under --release and exit 0; the emitted NDJSON parses
• encoding (622 files) and workflow-reference checks pass; the workflow parses as YAML

Also adds the two probe-table rows in lib.rs that the source branch never had -- the table is headed "what each probe establishes" and lists binary-only probes, so omitting these made it wrong about its own scope. The rows were themselves over-claimed on the first attempt and are corrected in fabda22.

[Copilot]

🟡 Changes recommended

request_cost::measure reintroduces a hard-coded C: path (can fail on non-C: installs) and the new module docs include stray //! text that should be corrected before merging.

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

Pull request overview

Adds two new windows-platform-probes binaries (probe-doorbell-cost, probe-request-cost) to measure nanosecond-scale costs that inform the two-layer ring “doorbell vs batching” design decision, and wires them into CI under --release for stable ratios.

Changes:

• Introduce doorbell_cost and request_cost probe modules plus corresponding binaries and CI steps (release-only for these two).
• Add an asserted deadlock-regression test for the doorbell park-and-wake handshake.
• Extend the crate’s probe table and dependencies to include the new probes and the real request type.

File summaries

File	Description
crates/windows-platform-probes/src/tests.rs	Adds bounded tests for measure_park_and_wake to prevent deadlock regressions and NaN averages.
crates/windows-platform-probes/src/request_cost.rs	New probe module to measure request/path/handle duplication construction costs.
crates/windows-platform-probes/src/lib.rs	Exposes new modules and documents them in the probe capability table.
crates/windows-platform-probes/src/doorbell_cost.rs	New probe module to measure doorbell-related syscall/atomic costs and a bounded park-and-wake handshake.
crates/windows-platform-probes/src/bin/request_cost.rs	New binary that renders the request-cost report and emits NDJSON.
crates/windows-platform-probes/src/bin/doorbell_cost.rs	New binary that renders the doorbell-cost report and emits NDJSON.
crates/windows-platform-probes/Cargo.toml	Registers the two new bins and adds windows-namespace-request-sys + a windows-sys feature.
Cargo.lock	Adds windows-namespace-request-sys to the probes crate dependency set.
.github/workflows/ci.yml	Runs the two new probes in CI under --release (and only these two).

Review details

Suppressed comments (1)

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

• This doc comment line ends with a stray //!, which will show up in rustdoc output as literal text.

//! which half is which is the point of measuring both.//!

• Files reviewed: 8/9 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.

[MikeGrier]

Correction to the description above, from a second review round (ec4e2b5).

The "What it found" section quoted the probe's own closing advice -- that the skip-when-busy rule is a refinement and a first implementation can always-signal. That was wrong, and it reversed the probe's own arithmetic.

The batching table divides ONE doorbell across a batch of N. That is the cost of signalling once per drained batch, i.e. coalescing on the empty-to-non-empty edge. A producer that signals on every push amortizes nothing: it pays a redundant SetEvent per operation, which this probe measures directly at 208 ns against a 2.1 ns push -- 100x, at every batch size. So the data supports "you must coalesce", not "you need not".

The corrected finding, which is the one to read:

Coalescing is the prerequisite; the parked-consumer check is the refinement. Signalling once per empty-to-non-empty edge is what drives the doorbell below the push, and it needs no eventcount -- only the queue's own emptiness. Tracking whether a consumer is actually parked is a further saving on top of that, and it is the part carrying the lost-wakeup risk, so it can wait for a measurement against real work.

The design conclusion is therefore narrower than the description claimed: the risky eventcount can still be deferred, but the cheap edge-triggered coalescing cannot.

Six other findings in the same round, all fixed in ec4e2b5: the park-and-wake round trip was described as what a wake costs while the function producing it documents an upper bound over two handoffs; the short-circuit diagnosis survived in the module and accessor docs after the binary was fixed to decide it per host; GetFullPathNameW was attributed to a syscall when windows-namespace-request-sys documents it as lexical and filesystem-free; "comparable, and neither dominates" was concluded from a test for order that also matches a ratio of 0.01; the construct-and-destroy cycles were documented in source but still emitted as build_open_request_ns to machine consumers, now *_cycle_ns; and two doc lines carried a stray //! from my own splices.

Three of those seven were introduced or left behind by the previous round's fixes, which is why the round ran before merge rather than after.

[Copilot]

🔵 Needs a closer look

request_cost::measure still hard-codes a C:\... long-path sample which can panic on non-C installations, undermining the probe’s stated host portability.

Review details

Suppressed comments (2)

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

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

• doorbell_share_of_submit is named like a meaningful "share" of a submit, but it is explicitly a ratio over an empty SubmitIoRing. The probe’s NDJSON already exposes this as doorbell_over_empty_submit; renaming this accessor (and updating the couple of call sites/docs) would make the API harder to misread and keep the terminology consistent.

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

• long_text hard-codes a C:\... absolute path, which can still panic on machines without a C: volume (the same class of failure the surrounding comment says this probe is avoiding for system_dll). Build the long-path sample off the probed system directory (or at least its drive prefix) so the "prepare_long_path" loop remains runnable on non-C installations.

    let long_text = format!(r"C:\{}\file.txt", vec!["directory"; 24].join("\\"));
    let long = Wtf16String::from(long_text.as_str());

• Files reviewed: 8/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

[MikeGrier]

Two corrections from review, in 35c4919.

1. The reference figures were measured on a Snapdragon X2 (ARM64), and did not say so.

The constants and everything derived from them said "the development machine". That is a
label only its author can resolve, and it was then read as though it described machines in
general. It does not, and the failure is not one of magnitude:

	ARM64 (dev machine)	x86_64 (review host)
doorbell cycle	164.9 ns	~531 ns
build_open_request	--	~210 ns
empty SubmitIoRing	~79 ns, below that host's syscalls	216 ns, among that host's 206 ns syscalls

So "which half is larger" inverts between the two, and the short-circuit reading of the
empty submit holds on one and fails on the other. Two observations across two architectures
is precisely the sample from which no universal follows -- which is the point, and is now
stated at each site rather than left for a reader to discover by running it.

Worth noting for anyone auditing this: both probes already emitted an arch field in their
NDJSON. Only the prose was anonymous. The machine-readable half was honest the whole
time, and the human-readable half was the one making the unqualified claim.

2. My earlier correction in this PR was itself wrong, and I want that on the record.

I replaced "a syscall cost" with "lexical" for GetFullPathNameW and said so in a comment
above. Lexical is also wrong. GetFullPathNameW resolves against the process current
directory, and for a drive-relative path (C:foo) against the per-drive current directory
held in the =C: environment variables. That is process state, not string work. The first
claim asserted a kernel transition a timing loop cannot establish; the second asserted pure
string work it equally is not. Both named a mechanism the run never measured.

What the run established is the cost. The reports now say that, add that it touches no
filesystem and is not an allocation -- the conclusion that actually mattered, and which
survives either framing -- and explicitly decline to say whether it enters the kernel.

This leaves a real question one layer down, queued as M2.6 rather than taken here:
windows-namespace-request-sys carries the same imprecision in its own doc ("This call is
lexical. It resolves relative components and ./.. against the process current
directory" -- those two sentences disagree), and it is the crate that owns the answer. The
mono-repo rule says fix the layer, but that crate is outside this peel and is
release-managed, so it gets its own commit. The item also asks the decision question a
terminology fix would otherwise bury: a genuinely lexical canonicalizer exists
(PathCchCanonicalizeEx) and is cheaper, and the expected answer is still no -- resolving
against the CWD at submission is the property the namespace design is buying, since the
CWD is shared mutable state and resolving later is racy. That should be recorded rather than
re-derived by the next reader.

[Copilot]

🔵 Needs a closer look

The probe logic has a couple of correctness/convention issues (unvalidated WaitForSingleObject result, reintroduced C: assumption, and probe mains not using emit_report) that should be fixed before approval.

Review details

Suppressed comments (4)

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

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

• wait_zero_signalled is documented as WaitForSingleObject(handle, 0) on a signalled event, but the return value is currently ignored. If the wait fails (WAIT_FAILED) or times out, the probe will still record a plausible timing under a label that claims the satisfied path, which can mislead both the prose and NDJSON consumers.
  crates/windows-platform-probes/src/bin/doorbell_cost.rs:26
• This probe calls emit(&mut Stdout, &render(...)), which means a panic inside render (e.g., from assertions in the measured loops) would drop the partially composed report on the floor. The crate convention is to use report::emit_report/emit_report_to so partial output is still printed on unwinding panics (documented in crates/windows-platform-probes/src/report.rs:112-170). Using that here would keep probe failures diagnosable in CI logs.
  crates/windows-platform-probes/src/bin/request_cost.rs:48
• This probe uses emit(&mut Stdout, &render()), which loses all already-composed output if render() panics before returning. The crate’s established pattern is to wrap probe rendering in report::emit_report/emit_report_to so partial output is still emitted on unwinding panics (see crates/windows-platform-probes/src/report.rs:112-170). Aligning this probe with that convention keeps failures diagnosable.

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

• This probe goes out of its way to avoid assuming Windows is installed on C: (by querying GetSystemDirectoryW), but the long-path test case reintroduces a hard-coded C:\... absolute path. prepare() ultimately calls GetFullPathNameW and returns an error on resolution failure, so this can make the probe panic on machines where C: is absent/unmapped, defeating the earlier portability fix.

    let long_text = format!(r"C:\{}\file.txt", vec!["directory"; 24].join("\\"));
    let long = Wtf16String::from(long_text.as_str());

• Files reviewed: 9/10 changed files
• Comments generated: 0 new
• Review effort level: Lite

[MikeGrier]

Addressed in f9081f1. Note this review ran against fabda22, two commits back — the stray //! finding was already fixed in ec4e2b5, and 35c4919 landed since. The other two are live at HEAD and both land, though the second for a different reason than stated.

1. doorbell_share_of_submit -> doorbell_over_empty_submit. You're right, and the miss is instructive.

ec4e2b5 renamed the serialized field for exactly the reason given here, and left the accessor alone. The comment making the argument — "the name states its own denominator, so a query that wants a real share of a syscall does not find this field by looking for that" — was sitting six lines from the sibling it didn't reach. A rename for precision isn't finished until every name for the quantity moves; the field and the method are one fact with two spellings. Renamed, with the miss recorded at the site so the next partial rename is the one that gets noticed.

2. The hard-coded C: does not panic on a non-C: machine — and that's now measured, not argued.

The stated failure mode isn't there. GetFullPathNameW normalizes a fully-qualified path without consulting a device, so no volume is needed behind the letter. Verified by preparing a 24-component path on a drive letter with nothing mounted:

absent drive D: prepare ok? true

But two passes flagged the same line, which is its own evidence — the code was communicating a portability bug it doesn't have. So rather than reply with the argument, it's a test:

fn preparing_a_path_needs_no_volume_behind_its_drive_letter()

That earns its place twice over. It answers this permanently, and it's the first thing in the crate to pin "touches no filesystem" — which is the claim the probe's entire account of where its nanoseconds go rests on, corrected only one commit ago (in 35c4919, after "syscall" and then "lexical" were both wrong), and until now supported by nothing executable. Your finding pointed at a real hole; it just wasn't the hole named.

On why the sample stays hard-coded rather than taking the resolved drive: the two paths have opposite requirements, and making them match would hide that.

• short names a file that is really opened — it must exist, hence system_directory().
• The long sample is only ever normalized — it must not need to exist, and a fixed 24 components keep its length identical on every host. Deriving it from the local system directory would make the length vary per machine, and the report explicitly asks the reader to compare figures across machines.

Building the second off the first would imply it needs a real volume when the opposite is the requirement. The comment now states that, and points at the test.

[Copilot]

🟡 Changes recommended

Several timing loops and a new test can silently produce misleading measurements or introduce CI flakiness without small correctness checks (Win32 return-value validation and drive-letter selection via GetLogicalDrives).

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

Review details

Suppressed comments (5)

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

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

• The event-manipulation timings don't check SetEvent's return value. If SetEvent ever fails (e.g., invalid handle), the probe will still record a plausible timing while no longer measuring the labeled operation, which is exactly the failure mode this probe warns about for SubmitIoRing. Consider asserting success here too (and inside the timed loop, since the check cost is negligible compared to losing measurement validity).

This issue also appears in the following locations of the same file:

• line 179
• line 185
  crates/windows-platform-probes/src/request_cost.rs:187

• system_directory() is obtained as a Windows path, but it's converted to UTF-8 with to_str().expect(...) before building a Wtf16String. That reintroduces an avoidable panic (and can lose unpaired surrogates), even though Wtf16String already has a lossless Windows OsStr bridge.
  crates/windows-platform-probes/src/request_cost.rs:304
• system_directory() falls back to C:\Windows\System32 when the fixed buffer is too small (the documented Win32 "required size" return), which can silently point the probe at the wrong file on unusual installs. Also, the doc comment says the failure is kept visible, but the fallback currently leaves no signal. Retrying with a dynamically sized buffer avoids the silent wrong-path case.

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

• set_reset_event is intended to time a real SetEvent/ResetEvent cycle, but the code doesn't validate either call succeeded. A failing BOOL return would silently turn this into a timing of a failing syscall path (still a measurable transition), which would make the report misleading.

    unsafe { ResetEvent(event) };
    timings.push(time_loop("set_reset_event", ITERATIONS, || unsafe {
        SetEvent(event);
        ResetEvent(event);
    }));

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

• wait_zero_signalled is documented as a satisfied wait (WAIT_OBJECT_0), but the timing loop ignores the return value. If the wait ever returns WAIT_FAILED/WAIT_TIMEOUT, the probe would still emit a plausible timing under the satisfied-wait label.

    unsafe { SetEvent(event) };
    timings.push(time_loop("wait_zero_signalled", ITERATIONS, || {
        unsafe { WaitForSingleObject(event, 0) };
    }));

• Files reviewed: 9/10 changed files
• Comments generated: 1
• Review effort level: Lite

[MikeGrier]

All six addressed in 22bce47. The rule adopted is flat, per @MikeGrier: every call that returns a status has that status checked, inside the timed region — no per-call-site argument about whether this one is worth it. If a check ever costs enough to distort a figure, that shows up as data and gets tuned then, at that site.

The three event loops (comments 1–3)

Right, and the miss is instructive. Twelve lines below those loops, the SubmitIoRing loop already carried a careful note explaining why discarding a status lets a failing call report a plausible time for an operation that never happened — "the failure mode this whole crate exists to avoid." The argument was correct and had been applied to the one call that looked expensive enough to deserve it. "Trivial enough not to check" is a fact about how hard anyone looked, not a property of the call.

Rather than assert that, I measured it — ran the probe against a deliberately invalid handle so every event call fails, with and without the checks:

	unchecked, bad handle	true
set_event_already_signalled	212.7	205
set_reset_event	422.9	531
wait_zero_signalled	231.5	280

Not one of those looks wrong. The redundant-SetEvent figure is within 4% of the real one. The entire doorbell-versus-build conclusion — the thing this PR exists to inform — would have been drawn from a run in which no event operation ever succeeded. A failing syscall is not cheap enough to be conspicuous; that is the whole hazard, and it's now recorded at the site with the numbers.

Cost of the fix, same host, three runs with checks in place: 204–206, 528–534, 280.3–280.7. That's run-to-run spread, not a shift.

One extra: I also check CloseHandle as a post-condition, on the reasoning that a close failing means the handle was already bad and every figure above it is discredited. That turned out not to be theoretical — in the bad-handle run with every other check stripped, CloseHandle was the one that caught it.

system_directory (comment 5)

Took the dynamic-buffer retry. On the fallback I went further than suggested: it now panics rather than substituting C:\Windows\System32. Your observation that the doc claimed the failure was kept visible while the code hid it is exactly right, and the two ways to reconcile that are to make the fallback signal or to remove the guess. Substituting a guess for an answer the system declined to give is the same defect as discarding a status — a plausible result standing in for one never obtained — so the guess goes.

The UTF-8 round-trip (comment 4)

Took it, and swept the sibling: the path also went out through String::from_utf16_lossy inside system_directory, which silently replaces unpaired surrogates before to_str() ever gets a chance to panic on them. Fixing only the reported half would have left the lossy conversion upstream. It's now wide the whole way from the Win32 call to Wtf16String.

The drive probe (comment 6)

Took it as written. Path::exists() on 23 roots touches real devices, and an offline mapped network drive blocks until the redirector times out — on exactly the CI machine most likely to have one. GetLogicalDrives also answers the sharper question: a letter absent from the mask has no volume, where exists() also returns false for a present drive with no media.

Gate: 151 tests, clippy, cargo doc, both probes under --release with parsing NDJSON, encoding clean.

[Copilot]

🟡 Changes recommended

There are correctness/robustness issues around unchecked Win32 return values in the handshake and JSON-invalid NaN defaults in NDJSON emission (plus a test robustness tweak needed).

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

Review details

Suppressed comments (3)

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

crates/windows-platform-probes/src/bin/doorbell_cost.rs:248

• The NDJSON line uses unwrap_or(f64::NAN) for required timings, but JSON does not permit NaN/Infinity literals. If any label lookup ever fails (e.g., a renamed label), the probe would emit invalid JSON rather than a clear failure, undermining the “NDJSON parses” guarantee.

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

• measure_park_and_wake ignores the return value of SetEvent(ping) in the timed loop. If it ever fails, the following wait will time out and the failure will look like a liveness issue rather than a failed wake attempt.

    for _ in 0..rounds {
        // SAFETY: both handles are live for the whole loop.
        unsafe { SetEvent(ping) };
        if unsafe { WaitForSingleObject(pong, WAIT_TIMEOUT_MS) } != WAIT_OBJECT_0 {
            ok = false;

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

• measure_park_and_wake closes ping/pong without checking CloseHandle's return value. A close failure would leak handles across test runs and also suggests the handles were invalid, which would undermine the timing result.

    // SAFETY: the peer has been joined, so nothing else holds these.
    unsafe {
        CloseHandle(ping);
        CloseHandle(pong);
    }

• Files reviewed: 9/10 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It contains a real handle-leak / incorrect-error-reporting edge case in measure_park_and_wake and a new test that can fail spuriously on hosts with all drive letters mapped.

Review details

Suppressed comments (2)

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

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

• In measure_park_and_wake, the combined assert!(!ping.is_null() && !pong.is_null(), ...) can leak a handle (if one CreateEventW succeeds and the other fails) and can also report the wrong last_os_error() (the last call may have succeeded, making the error code stale/undefined). Handle each creation separately, and close ping if creating pong fails.

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

• This test hard-fails if every drive letter A-Z is in use (e.g., many mapped network drives). That environment condition is unrelated to the behavior under test (that prepare doesn’t consult the filesystem), so it can make the suite fail spuriously. Consider skipping the test when no unused letter is available.

    let absent = (b'A'..=b'Z')
        .find(|&byte| used & (1 << u32::from(byte - b'A')) == 0)
        .map(char::from)
        .expect("every drive letter A-Z is in use, so this test cannot run here");

• Files reviewed: 10/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[MikeGrier]

Addressed in c6684cf. One taken, one declined with the reasoning recorded at the site.

Taken: the combined CreateEventW assertion

Real, and it was introduced by the previous commit — the one that added last_os_error() to every assertion in this file specifically to improve diagnosis.

Both problems named are there. It leaks: ping created, pong fails, panic without closing ping. And it misreports, which is the more interesting half — last_os_error() was read after both calls, so a failure of the first is overwritten by the success of the second, and the message can read:

CreateEventW failed: The operation completed successfully

The lesson generalizes past this site, so it is now stated there: an error code is only meaningful read immediately after the single call whose failure is being reported. Attaching one to a condition spanning two calls does not improve a diagnosis, it fabricates one — the code belongs to whichever call ran last, not whichever failed. The new failure path reads its own error before the close rather than reusing an earlier one.

I swept the file for that shape and found one more, milder, in request_cost: the GetSystemDirectoryW retry asserted retried != 0 && retried < heap.len() under a single error code, where only the first half is an OS failure — the second means the directory grew between calls, and GetLastError says nothing about it. Split into two assertions so neither carries a code that would be fiction.

Declined: skipping the drive-letter test

libtest has no runtime skip — a test that "skips" is a test that PASSES. This is the test pinning the claim that prepare needs no volume, so a pass that established nothing is the single outcome most worth avoiding here. It is the same vacuous-green hazard as the unchecked GetLogicalDrives you flagged two rounds ago and I fixed in c0a0021; converting it into a silent pass would reintroduce that defect in a different shape.

It also diverges from what this crate already does with the identical condition. Two existing device-map tests do exactly this:

panic!("no free drive letter on this host, so the probe cannot run");

and they search only H..=Z. This test searches all 26 letters, so it fails strictly less often than sites that already chose to fail. The condition requires A and B to be mounted as well, which is floppy-era and essentially unreachable — and a host in that state is worth hearing about loudly.

The message now says plainly that it is an environment limitation rather than a failure of the behaviour under test, and the reasoning is recorded in the test so this does not come up a fourth time.

Gate: 151 tests, clippy, both probes strict-parsed, encoding clean.

[Copilot]

🔵 Needs a closer look

Two concrete issues were found that impact CI diagnosability/behavior (missing OS error details for CreateEventW failure, and the new probe steps may be skipped after earlier job failures unless guarded like the topology probe).

Review details

Suppressed comments (2)

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

.github/workflows/ci.yml:293

• These two probes are described as running in CI for the nanosecond comparisons, but without an explicit if: '!cancelled()' they will be skipped whenever an earlier step in the job fails (default if: success()). Topology is already guarded to still run on failed jobs for exactly this reason; consider doing the same here so the probe output is still available when a failure elsewhere makes the log more valuable.
  crates/windows-platform-probes/src/doorbell_cost.rs:172
• The CreateEventW failure path drops the OS error code, even though the surrounding assertions consistently include last_os_error() for diagnosability in CI logs. Including the error here makes failures actionable (e.g., invalid parameter vs handle quota) without needing a debugger.

• Files reviewed: 10/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[MikeGrier]

Both taken in 4f50485. Both were siblings walked past, not new ground.

The bare CreateEventW assertion

That was the last site in the file still saying only "CreateEventW failed". bdc45cc added last_os_error() to every other assertion here, and c6684cf fixed the creation pair in measure_park_and_wake — this one sat between the two sweeps and neither reached it. Swept the crate afterwards to confirm nothing else in this peel's files is bare.

The CI guard, which is the better catch

Right, and the reason is already written four lines above the steps in question, at the topology step: a probe step exists to emit diagnostics, so skipping it on failure suppresses it in exactly the run that wanted it. The argument was general and had been applied to one step.

It matters more for these two than for a pass/fail probe, and this branch is why. Over the last several commits both probes were changed to assert every status they take, panic on a missing NDJSON label, and route through emit_report so that a panic still prints what was already established. That is a lot of machinery for making failures legible — and it only reaches a reader if the step runs at all. I spent the branch making these probes fail loudly and then left them in the one configuration where the failure isn't printed.

Two sweeps queued rather than taken

Checking the whole file rather than the two lines reported: twelve probe steps, and only one was guarded before this.

• M2.7 — the other nine, including the long-path pair, whose own comment says either half alone "says nothing" because the finding is the difference between two executables. So a partial run of that pair is worse than useless.

  Queued rather than done because there is a genuine tradeoff and it is your call, not a consistency argument: !cancelled() also runs a step when the build failed, where cargo run cannot compile, turning skipped (grey) into failed (red). That trades quieter broken-build output for better broken-test output. The topology step already took that trade; whether all twelve should is a judgement about how the CI log gets read.

• M2.8 — four assertion messages in probes this peel didn't touch (completion_port.rs:224/:234, ioring.rs:320, pool_growth.rs:62) name what was attempted without saying why it failed. The item carries the two rules this peel learned expensively: read the error immediately after the single call whose failure is reported, and attach one only where the condition is genuinely an OS failure.

Verified the YAML by indentation comparison against the known-good topology step (all three guards at identical depth, no tabs) since there's no YAML parser on this host.

Gate: 151 tests, clippy, encoding clean.

[Copilot]

🟢 Approval recommended

The changes are cohesive and low-risk (probe-only additions plus CI wiring), and the only finding is a minor comment/doc mismatch noted inline.

Review details

Suppressed comments (1)

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

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

• The comment says a machine under load may make each round “arbitrarily slow” without making it wrong, but the handshake implementation uses a 5-second timeout per wait and returns None on timeout. If a round takes >5s (e.g., extreme load/suspension), this test will fail, so the comment should reflect the bounded nature of the measurement.

• Files reviewed: 10/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[MikeGrier]

Fixed in b90d897. Real mismatch, and the contradiction was tighter than reported.

WAIT_TIMEOUT_MS = 5_000, per wait — so a round exceeding it makes measure_park_and_wake return None and fails the expect. "Arbitrarily slow without making it wrong" is simply not true of the code it describes.

What makes it worth more than a wording tweak is where the correct statement already was. Two places, both within a few lines:

• The section header directly above states the 5-second bound and gives the reason for it.
• The expect on the very next line reads "a bounded handshake of 64 rounds must complete rather than time out."

So the file said "bounded" twice and "unbounded" once, in adjacent lines. Nothing caught it because each statement is locally true and only the pair is contradictory — the exact failure mode that per-artifact review is structurally blind to, which is why it took an outside reader to spot.

Reworded to state the real bound and keep the reason it exists: a test that can hang takes the whole suite with it, which is worse than the defect it guards. The margin is enormous — sub-microsecond round trip against a 5-second ceiling — so a failure means the machine stalled for seconds, which deserves a red test rather than a silently slow pass.

Swept the crate for other unbounded-tolerance claims (arbitrarily slow, however slow, never times out, and similar); this was the only one.

Gate: 151 tests, clippy, encoding clean.

[Copilot]

🔵 Needs a closer look

doorbell_cost reports last_os_error() for a WAIT_TIMEOUT wait result where GetLastError is undefined/stale, producing misleading diagnostics.

Review details

Suppressed comments (1)

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

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

• WaitForSingleObject only sets GetLastError when it returns WAIT_FAILED; if it returns WAIT_TIMEOUT (a successful return), last_os_error() is stale and can mislead the diagnosis. Capture the return value and only include last_os_error() for the WAIT_FAILED case, otherwise report the wait code itself.

• Files reviewed: 10/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[MikeGrier]

Both handled. e03d88b for the review finding, dc39ae3 for the CI failure.

The CI failure was not this PR, and that is established rather than argued

96 failures, all in windows-namespace-request-sys — a crate this branch does not touch. It is one defect plus 95 collateral: close::tests::a_caller_supplied_routine_is_carried failed assertion failed: !was_still_open(raw) while holding the write guard on handle_allocation(), poisoning it for every test that took it afterward (the lock is not poisoned: PoisonError { .. } x95).

Evidence it is intermittent:

• The same commit b90d897 was re-run with no change and passed. Identical SHA, identical code.
• The commit that "failed" was comment-only, in a different crate (verified: zero non-comment lines in the diff).
• The two commits before it passed the identical job.
• The crate passes 5/5 locally.

The mechanism is measured. was_still_open probes a raw value by attempting to close it, and its own comment states the precondition — "because these tests hold the allocation lock." That precondition does not hold process-wide: 218 tests in the crate, and 11 open handles without taking it (open::tests::a_missing_path_reports_the_raw_code_unaltered and friends). cargo test runs tests as threads in one process, so a freed handle value can be reallocated by one of those 11 concurrently, and the probe finds it open.

Worth flagging: the failed assertion is the benign outcome. was_still_open closes the handle when the probe succeeds — so in the losing interleaving this test closes a live handle belonging to another test. That surfaces later as an unrelated failure somewhere else entirely, which is far harder to trace than the version we got.

Recorded in a new crates/windows-namespace-request-sys/UNRESOLVED-TEST-FAILURES.md per the repo's test-tracking rule, with three directions. Not fixed here: it is another component, and it wants a real decision (lock the 11, or remove the need for the lock by asking the close routine's own observation static instead of the OS — the latter cannot race at all).

The wait-result finding

Right, and it breaks a rule stated in the same file. WaitForSingleObject is the one call here returning a status code rather than a BOOL, and only WAIT_FAILED sets the last-error value — WAIT_TIMEOUT and WAIT_ABANDONED are successful returns, so the code printed beside them belonged to whatever ran previously.

The sweep in bdc45cc that added last_os_error() to every assertion treated a status-returning call like the BOOL-returning ones around it. That is the second time on this branch a diagnosability sweep introduced a wrong diagnosis — after the combined CreateEventW assertion in c6684cf. Both times the sweep was right and its uniform application was not.

Now goes through describe_wait, which names the code and attaches an OS error only for WAIT_FAILED. Swept the crate: the other two waits here compare and return without claiming an error; pool_growth's discards its result, which belongs to the workspace audit.

Gate: 151 tests, clippy, encoding clean.

[Copilot]

🟡 Changes recommended

A factual statement in the newly added UNRESOLVED-TEST-FAILURES.md contradicts the PR’s actual touched paths and should be corrected to keep the diagnostic record accurate.

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

Review details

• Files reviewed: 11/12 changed files
• Comments generated: 1
• Review effort level: Lite

[MikeGrier]

Fixed in 7c18511, and checking the rest of the record turned up a second false claim you didn't flag.

The one you found

Correct, and neatly self-referential: the paragraph said the branch "touches only crates/windows-platform-probes/, .github/workflows/ci.yml and Cargo.lock" — which was untrue the moment it was written, because the file saying it lives under crates/windows-namespace-request-sys/. Your reading of the intent is right; the accurate claim is "no source change to this crate", verified against the diff: its only file here is the record itself.

The one I found while checking the rest

Your comment made me re-verify every checkable claim in the entry rather than just the reported one. Most held — the 96/95 split, the identical-SHA rerun, 5/5 locally, 218 tests with 11 unlocked, the root DESIGN-NOTES citation. One did not.

The entry proposed, as the most promising fix, that was_still_open be replaced by asking the close routine, "and the close routines already have observation statics for that purpose."

They do not. That is windows-threadpool-sys's pattern for its wait targets, and I imported it here by mistake. This crate calls the real CloseHandle and FindCloseChangeNotification directly with no shim — which src/close.rs records as a deliberate property — so there is nothing observable to ask. was_still_open, used at seven sites in close/tests.rs, is the only mechanism the crate has for the question.

That mattered more than the path error: I had recommended it as the direction, so the next person would have started on a change substantially larger than advertised, requiring a test-only routine be introduced first. The directions are reordered — having Fixture/captured_duplicate take the lock themselves, so omitting it isn't something a test can do by omission, is now second and is the smallest change that stops recurrence.

Both corrections are recorded in place rather than silently rewritten. A diagnostic record's value depends on a reader being able to trust it, and seeing what it once claimed is part of that.

Also added the RESOLVED-TEST-FAILURES.md sibling the record links to. It didn't exist — a link broken on creation, which the repo's own cross-reference rule forbids. All four links in both files now resolve.

Worth noting the shape: a record written to prevent someone re-deriving things contained two assertions that would have cost them time. The rule I keep relearning on this branch is that a claim is only worth writing down if it was checked, and prose about code gets no automatic verification the way code does.

[Copilot]

🟢 Approval recommended

The changes are self-contained, well-defended with tests/documentation where behavior is subtle, and the CI wiring is consistent with the probes’ measurement goals.

Review details

• Files reviewed: 12/13 changed files
• Comments generated: 0 new
• Review effort level: Lite