Close the ioring mutation survivors, drop the topology dev-dependency pin, and queue the NUMA repairs

[MikeGrier]

Peels the windows-ioring-sys work out of PR #56: eight mutation survivors closed, a dev-dependency pin removed, and the milestone that the 2026-08-30 NUMA-sharding measurement made necessary.

No crate is bumped, and that was verified rather than assumed -- see the bottom of this description.

The tests

The 2026-09-02 sweep ran this crate with its default features, and eight mutants survived across the batch, error, event-delivery, and ring surfaces. Two needed a seam before any test could reach them, and both are #[cfg(test)] pub(crate), so neither exists in a real build:

• Ring::set_supported_ops_for_test builds an arbitrary OpSupport mask, so the refusal path for an unsupported opcode can be exercised without needing a kernel that happens to lack one. It builds the mask the same way OpSupport::probe does (1 << Op::ALL.position(op)) and OpSupport::contains reads it -- a helper that indexed differently would let tests pass over broken production code.
• DROP_RUNS counts drop-glue runs through a thread-local, which is what makes "the drop ran exactly once" assertable at all.

One test replaces a comment. ring/tests.rs previously carried a "deliberately not tested" note claiming the information: 0 zeroing on an injected failure was unobservable. It is observable from the child module, so the note is gone and an_injected_failure_zeroes_the_transferred_byte_count asserts it -- reading the pre-injection value first, so it cannot pass against a completion that was already zero.

The dev-dependency pin

windows-topology-sys carried both a path and a version, and the version was doing harm rather than work.

It was never checked where it was supposed to matter: publish verification builds the library alone, not examples or tests, so a dev-dependency requirement is never exercised at that moment. And cargo omits a versionless dev-dependency from the published manifest entirely -- confirmed by packaging the crate and reading the result, which contains serde_json, windows-overlapped-io-sys and windows-sys under [dev-dependencies] and no windows-topology-sys entry at all.

What it did do was break the workspace. Cargo enforces path + version agreement at every build regardless of dependency kind, so a pin reading 0.1.0 against a windows-topology-sys bumped to 0.2.0 failed cargo metadata for every crate here -- turning a release of an unrelated crate into a red main.

The examples still build from a checkout, which is where anyone reads them. They were only ever incidentally buildable from the packaged tarball.

The milestone, and the design session it rests on

M20 queues four documentation and policy-test repairs from the 2026-08-30 measurement: a shipping ARM laptop reports no L3 cache domain at all, which falsifies the stated justification for the last-level-cache heuristic (though not the heuristic's preference over the NUMA node, and not ring_copy, whose degraded fallback already handles it).

The design session those four items are queued from is peeled with them. A review of this branch caught it citing a file that lives only on the unmerged feature branch -- the same defect the previous peel in this series shipped, the referrer taken without the referent. The remedy is the same: peel the referent, not drop the pointer. I checked the session's own twenty-six outbound links before moving it, since that is the identical failure one level down; all resolve here, including the two NUMA spike sources, which landed on main in #65.

The same review found two smaller instances, both fixed: a Cargo.toml comment citing another feature-branch-only checklist, and a PLANS.md row still reading "leaving only M6+" while the same change added M20 to it.

One flaky test, recorded rather than fixed

flush_barrier::a_covering_flush_waits_for_preceding_writes_and_an_unordered_one_does_not failed once under cargo test --workspace --all-features on 2026-09-03 (2,899 of 2,900) and passes in isolation. It measures real I/O ordering, so it is load-sensitive: a full-workspace run has every other suite competing for the disk, and the window it asserts is a timing one. Whether to make it load-independent or mark it serial is a decision rather than a repair, so it is recorded in UNRESOLVED-TEST-FAILURES.md. It has not been seen to fail in CI.

Why nothing is bumped

release-please attributes a bump by the paths a commit touches, not by its scope, so this matters more than the subject lines suggest -- the chore(ioring) commit edits that crate's Cargo.toml and the test(ioring) commit edits six files under its src/. Either typed fix: would have taken an unearned patch bump.

check	result
commit types	docs, docs, chore, test -- no feat, fix, !, BREAKING CHANGE, or Release-As
repo's own rule (check-commit-scope.ps1)	releasing is feat/fix/!; clean on all four commits
version strings	no x-release-please-version line, manifest, or config touched -- the only version in the diff is the dev-dep requirement being removed
empirical	#67 was the same shape (nine test:/docs: commits over seven crates) and changed no Cargo.toml and no release manifest; no release PR is open

design-sessions/ sits outside crates/, so it maps to no package at all.

Verification

cargo fmt --check, cargo clippy --workspace --all-targets --all-features, and cargo test --workspace --all-features (2,964 passing, 99 suites) all clean. Encoding clean over 598 files; check-baseline, check-publishable, check-borrow-surface and check-workflow-refs all green.

Before opening this I re-ran the link sweep the review's first finding called for -- every relative link the peel adds, resolved against this branch. Zero dangling. That check is cheap and belongs before review, not after it.

[Copilot]

🟡 Changes recommended

New tests include unbounded/over-assumptive completion popping and brittle string-based error assertions that can cause hangs or flakiness under load.

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

Pull request overview

This PR peels out the windows-ioring-sys slice of earlier work by closing mutation-test survivors with targeted new tests and test-only seams, removing a harmful dev-dependency version pin, and queuing follow-up NUMA documentation/policy repairs based on the 2026-08-30 measurement/design session.

Changes:

• Add focused tests (and small #[cfg(test)] seams) to kill previously surviving mutants across ring/batch/error/event-delivery surfaces.
• Remove the windows-topology-sys dev-dependency version pin (keep path-only) to avoid workspace-wide path+version mismatch breakage.
• Add the NUMA-sharding design session record and queue the resulting documentation/policy repair milestone (M20), plus record a known flaky test in UNRESOLVED-TEST-FAILURES.md.

File summaries

File	Description
design-sessions/DESIGN-SESSION-2026-08-30-numa-sharded-io-execution-domains.md	Adds Tier-3 design-session record that motivates/feeds the queued M20 repairs.
crates/windows-ioring-sys/UNRESOLVED-TEST-FAILURES.md	Records one flaky flush_barrier test per repo policy.
crates/windows-ioring-sys/src/ring/tests.rs	Adds tests to close mutation survivors (supports set, drop runs, injection zeroing, Debug rendering).
crates/windows-ioring-sys/src/ring.rs	Adds #[cfg(test)] capability override seam and a thread-local drop counter; documents equivalent-mutant rationale.
crates/windows-ioring-sys/src/event_delivery/tests.rs	Adds test that RingScope::supports reflects a deliberately restricted capability set.
crates/windows-ioring-sys/src/error/tests.rs	Adds Debug formatting assertion to close an IoRingError Debug survivor.
crates/windows-ioring-sys/src/batch/tests.rs	Adds tests covering Batch::require, Debug formatting, and Windows behavior for empty buffer registrations.
crates/windows-ioring-sys/src/batch.rs	Documents why RegisteredBuffers::is_empty() is effectively unreachable today and ties it to a platform-behavior test.
crates/windows-ioring-sys/PLANS.md	Updates the crate plan summary and links in the new NUMA design session / M20 milestone.
crates/windows-ioring-sys/CHECKLIST.md	Queues M20 items (doc/policy/test repairs) from the NUMA-sharding measurement.
crates/windows-ioring-sys/Cargo.toml	Drops the dev-dependency version pin for windows-topology-sys while keeping the path dependency.

Review details

Suppressed comments (1)

crates/windows-ioring-sys/src/batch/tests.rs:531

• Asserting on error.to_string() is brittle (it depends on Display formatting staying exactly the same). Since this error should be an HRESULT-backed IoRingError, assert on the actual code via the sealed downcast helper instead.

    assert!(
        error.to_string().contains("0x80070057"),
        "expected E_INVALIDARG, got {error}"
    );

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

---

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

[MikeGrier]

Fixed in 669e23f -- same helper as the sibling comment, so both spellings of the assumption are gone.

You identified the exact contract line: submit_and_wait's own docs say "Returning does not mean wait_operations completions are poppable: the timeout can expire first, and the returned count is entries submitted rather than completed." The test read that guarantee as stronger than it is, and .expect("the registration completion is ready") would have failed under load with a message asserting something the API never promised.

It now goes through the bounded pop_within, which retries with a yield until a thirty-second deadline and then fails naming what it waited for.

Worth noting the two findings are one defect with two faces: a single try_pop flakes, and the obvious repair -- looping -- turns the flake into a suite-wide hang. The helper exists so neither has to be reasoned about at the call site.

[MikeGrier]

Fixed in 669e23f, at four sites rather than the one.

You are right, and the consequence is worse than a slow test. cargo test runs tests as threads in one process, so a hung test stops the whole harness reporting -- the run dies with no test name attached. This repository's own mutation-sweep notes document that exact failure mode (a timeout there is usually a detection that lost its name). An unbounded spin is the reliable way to produce it, and this one had no yield either.

Both sites now call one helper:

#[cfg(test)]
pub(crate) fn pop_within(ring: &mut IoRing, what: &str) -> Completion {
    const BOUND: std::time::Duration = std::time::Duration::from_secs(30);
    let deadline = std::time::Instant::now() + BOUND;
    loop {
        if let Some(completion) = ring.try_pop().expect("pop") { return completion; }
        assert!(std::time::Instant::now() < deadline, "timed out after {BOUND:?} waiting for {what}");
        std::thread::yield_now();
    }
}

Thirty seconds and yield_now match the idiom the crate's failure_paths integration test already uses. It is a hang bound, not a latency expectation. The bound and the message come from one constant so they cannot drift apart.

It is used at four sites, not the two the review named. The loop you flagged was a copy of two older ones in the same file -- including the one inside real_completion, which every injection test goes through. Fixing only the new one would have left the wrong idiom sitting directly beside the right one, for the next person to copy. The class is gone from these files rather than the reported instances.

Verified by sabotage, not by reading. With the helper's try_pop forced to yield nothing:

thread 'ring::tests::an_injected_failure_zeroes_the_transferred_byte_count' panicked at ring.rs:1063:9:
timed out after 30s waiting for the fixture read's completion
test result: FAILED. 0 passed; 1 failed; finished in 30.00s

30.00s and a named failure, rather than a hang. That is the behaviour the fix exists to produce, so it is the behaviour I measured.

[MikeGrier]

All three findings from this review are fixed in 669e23f, including the suppressed one.

The two inline comments have replies on the threads. The suppressed finding -- error.to_string().contains("0x80070057") being brittle -- was correct too, and I have taken it:

let code = crate::IoRingErrorExt::as_ioring_error(&error)
    .expect("a kernel refusal carries an HRESULT")
    .code();
assert_eq!(code, windows_sys::Win32::Foundation::E_INVALIDARG, ...);

The point of that test is which HRESULT the kernel returned when refusing an empty buffer registration. Matching Display's formatted text would also fail if the message were reworded -- reporting a documentation change as a platform change, which is the opposite of what the test is for. The downcast asserts the thing itself.

One note on scope

The completion-pop fix is applied at four sites, not the two flagged. The loop the review named was a copy of two pre-existing ones in the same file -- one of them inside real_completion, the helper every injection test routes through. Repairing only the new instance would have left the hazardous idiom beside the corrected one, which is how this class propagates in the first place. I would rather absorb three lines of out-of-scope diff than ship a file that teaches both patterns.

Verification

check	result
cargo test --workspace --all-features	2,964 passing, 99 suites, 0 failed
cargo clippy --workspace --all-targets --all-features	clean
cargo fmt --check	clean
bound fires rather than hangs	sabotage-verified: fails in 30.00s naming the test and what it waited for
version impact	none -- pop_within is #[cfg(test)] pub(crate), commit typed test, scope checker clean

[Copilot]

🔵 Needs a closer look

The new design-session doc contains at least one incorrect API reference (Topology::discover()), which should be corrected to avoid sending readers to a non-existent symbol.

Review details

Suppressed comments (1)

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

design-sessions/DESIGN-SESSION-2026-08-30-numa-sharded-io-execution-domains.md:137

• Topology::discover() does not exist in windows-topology-sys (the type was renamed to MachineMemoryTopology). As written, this session record points readers to a non-existent API; use the current type name in the backticked call.

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

[MikeGrier]

Fixed in 9f911d2 -- and there were two stale claims, not one, so the "at least one" hedge was warranted.

The judgment call, stated

The session is dated 2026-08-30; the rename landed 2026-09-02 22:25, three days later. So Topology::discover() was correct when it was written.

That makes this a Tier-3 question rather than a typo. Editing the transcript to match current code would make the record say something that was not said, which is the one thing a dated session record exists to prevent -- and the file's own header already declares DESIGN-NOTES.md authoritative on any conflict. But your point stands regardless: a reader following the reference today finds nothing.

So the text stays and the current name goes in brackets beside it:

Probed with `Topology::discover()` [now `MachineMemoryTopology::discover()`] on
the development machine.

with the convention stated once in the header, so the brackets read as annotation rather than as part of the discussion.

The second one

efficiency_class is described twice as "exposed on Core". There is no Core type at all now -- only CoreRelation, which is a different thing -- and the field moved to ProcessorFacts as an Observed<u8>.

That one is worth naming precisely rather than just repointing, because the type change is material to the session's own argument. It calls this an input that "exists"; it is now explicitly maybe-absent-with-a-reason, which is exactly the distinction Observed<T> was introduced to carry. A reader who repointed the name but kept the assumption would be wrong in a way the rename alone does not reveal.

Verified, since asserting a wrong name is the defect being fixed

claim	evidence
MachineMemoryTopology::discover exists	topology.rs:238, re-exported lib.rs:156
ProcessorFacts exists	re-exported lib.rs:139
efficiency_class is a field, not a method	pub efficiency_class: Observed<u8> on ProcessorFacts

My first wording said "reached through ProcessorFacts" -- true, but vaguer than it needed to be, so I tightened it after checking whether it was a method or a field rather than guessing.

Swept the whole document for the class: three sites, all three now annotated, none left bare. Encoding clean over 598 files.

[Copilot]

🟢 Approval recommended

The changes are scoped to tests/docs/planning plus a dev-dependency pin removal, and the added test seams are #[cfg(test)]-only with no runtime surface impact.

Review details

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