Reshape the topology model around observed domains by MikeGrier · Pull Request #61 · MikeGrier/windows-threadpool-sys

MikeGrier · 2026-09-05T13:50:45Z

Third peel from #56. Reshapes windows-topology-sys around what the platform
actually reports, and updates the one consumer that breaks.

Replaces the flat Topology with a model built from observation: domains that
carry their own identity and provenance, an explicit record of the anomalies a
walk encountered, and observation types that distinguish what was measured
from what was inferred.

Breaking

Topology and Domain::id are removed, so every caller changes. release-please
will cut 0.2.0 from 0.1.0.

Why `windows-ioring-sys` is in the diff

The ring_copy example consumes both removed items, so it is updated in the
same commit -- splitting it would leave a commit that does not compile, which
this repository's release-scope rule names as the worse defect of the two.

This is a genuinely coupled change, not a ride-along:
windows-ioring-sys' library does not depend on the topology crate at all
(confirmed from cargo metadata, not by grepping manifests). Only the example
does. So no windows-ioring-sys bump is implied or wanted, and the commit is
scoped feat(topology)! accordingly.

Documentation links

Thirteen links in the crate's docs pointed at files outside this change --
topology-planner, two workspace-root checklists, and a design session. Link
text is kept and only the dangling target removed, so the provenance survives
without a reader of the published crate meeting a dead link.

I checked this proactively rather than waiting for review, because dangling doc
links were the finding in two separate rounds on #59. Verified by resolving
every relative link in the changed files, not by reading them.

crates/topology-planner/ was considered for inclusion and deliberately left
out: it is documentation for a component that has no code yet, and bringing it
in would have added five more dangling links while fixing six.

Verification

Run in a clean worktree against main:

236 tests with --all-features, 162 with default features
107 tests in windows-ioring-sys -- the consumer this could have broken
clippy clean under -D warnings in both feature configurations
rustdoc clean under CI's flags (-D rustdoc::broken_intra_doc_links -D rustdoc::private_intra_doc_links -D rustdoc::invalid_rust_codeblocks) in
both configurations
cargo check --workspace --all-targets --all-features --locked clean
encoding check: 537 files clean

Checking docs in both feature configurations is deliberate: on #59 a link to a
feature-gated item passed --all-features and failed the default build.

Peel sequence

With this merged, #56 drops from 196 files to 161. windows-platform-probes
(34 files) and windows-placement-probe (30) both depend on topology, so they
unblock and are the natural next peel.

Replaces the flat `Topology` with a model built from what the platform actually reports: domains carrying their own identity and provenance, an explicit record of anomalies the walk encountered, and observation types that distinguish what was measured from what was inferred. **Breaking.** `Topology` and `Domain::id` are gone, so every caller changes. The `ring_copy` example in `windows-ioring-sys` is updated in the same commit because it consumes both -- splitting it would leave a commit that does not compile, which this repository's release-scope rule names as the worse defect. `windows-ioring-sys`' library does not depend on the topology crate at all, so nothing else there moves and the crate earns no release from this. Thirteen documentation links pointed at files that live outside this change -- `topology-planner`, and two workspace-root checklists and a design session. The link text is kept and only the dangling target removed, so the provenance survives without a reader of the published crate meeting a dead link. Verified by resolving every relative link in the changed files rather than reading them. `topology-planner` was considered for inclusion and left out: it is documentation for a component with no code yet, and bringing it would have added five more dangling links while fixing six. Verified against main: 236 tests with all features, 162 by default, and 107 in `windows-ioring-sys`, which is the consumer this could have broken. Clippy clean under `-D warnings` in both feature configurations, rustdoc clean under CI's flags in both, and `cargo check --workspace --all-targets --all-features --locked` clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

There is a confirmed overflow/OOB-read risk in the new shared record walker and a correctness issue in proximity’s finer_unobserved computation that should be fixed before approval.

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

Pull request overview

This PR continues the topology “peel” by reshaping windows-topology-sys around what Windows actually reports (observations, provenance, and explicitly represented absences), and updates the windows-ioring-sys ring_copy example to the new API surface.

Changes:

Introduces new model primitives (Observation/Source, Observed<T>, Provenance, granularity/proximity queries) and threads enumeration anomalies through discovery.
Deduplicates record-buffer walking into a shared records module and adds malformed-input coverage tests for both enumerations.
Updates docs, plans/checklists, and the ring_copy example to account for the removed Topology and Domain::id.

File summaries

File	Description
crates/windows-topology-sys/src/walk/tests.rs	Adds anomaly-focused decode helpers and malformed-input walk tests.
crates/windows-topology-sys/src/relation.rs	Carries walk decode anomalies into `Relations`.
crates/windows-topology-sys/src/records/tests.rs	Adds focused tests for the shared record walker.
crates/windows-topology-sys/src/records.rs	New shared `Size`-chained record walker with anomaly reporting.
crates/windows-topology-sys/src/provenance/tests.rs	Adds tests for provenance ordering/default semantics.
crates/windows-topology-sys/src/provenance.rs	New `Provenance` type and downgraded deserialization helper.
crates/windows-topology-sys/src/processor_set/tests.rs	Adds tests for new `is_subset` and `is_empty` coverage.
crates/windows-topology-sys/src/processor_set.rs	Adds `can_represent` and `is_subset` for inclusion-based ordering.
crates/windows-topology-sys/src/observed/tests.rs	Adds tests for `Observed<T>` semantics and serde round-trip.
crates/windows-topology-sys/src/observed.rs	New `Observed<T>` type distinguishing absence reasons.
crates/windows-topology-sys/src/observation/tests.rs	Adds tests asserting `Source` is not a trust ordering.
crates/windows-topology-sys/src/observation.rs	New observation/provenance vocabulary for relations/attributes.
crates/windows-topology-sys/src/lib.rs	Updates crate docs/exports and wires README doctests.
crates/windows-topology-sys/src/granularity/tests.rs	Adds extensive tests for inclusion-based granularity/proximity.
crates/windows-topology-sys/src/granularity.rs	Implements inclusion-ordered `Granularity` + `minimal_shared`/`proximity`.
crates/windows-topology-sys/src/domain.rs	Removes `Domain::id`, adds observations, adopts `Observed` for memory bytes, updates serde.
crates/windows-topology-sys/src/cpu_set/tests.rs	Adds CPU-set decode/flag/walk robustness tests.
crates/windows-topology-sys/src/cpu_set.rs	New safe CPU-set enumeration and decode built on shared record walk.
crates/windows-topology-sys/src/anomaly.rs	New anomaly types for record-walk decode failures.
crates/windows-topology-sys/README.md	Updates public README to new model and availability claims.
crates/windows-topology-sys/PLANS.md	Updates plan status text and links.
crates/windows-topology-sys/COMPLETED-PLANS.md	Records completion entries for the reshape and shared walk work.
crates/windows-topology-sys/CHECKLIST.md	Archives plan pointers and records explicit deferrals.
crates/windows-topology-sys/examples/print_topology.rs	Updates example to `MachineMemoryTopology`.
crates/windows-topology-sys/Cargo.toml	Adds `Win32_System_Threading` feature for `GetCurrentProcess`.
crates/windows-ioring-sys/examples/ring_copy/policy.rs	Migrates example policy selection to new topology types.
crates/windows-ioring-sys/examples/ring_copy/plan.rs	Reworks labeling/NUMA selection for removed `Domain::id` and new observation semantics.
crates/windows-ioring-sys/examples/ring_copy/main.rs	Updates topology loading/discovery and remote placement validation logic.

Review details

Files reviewed: 34/34 changed files
Comments generated: 4
Review effort level: Lite

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

+        let header_end = self.offset + self.size_offset + size_of::<u32>();
+        if header_end > self.length {
+            if self.offset < self.length {
+                self.anomaly = Some(EnumerationAnomaly::trailing_bytes(
+                    self.source,
+                    self.offset,
+                    self.length - self.offset,
+                ));
+            }
+            return None;
+        }
+
+        // SAFETY: the bound above proves the `Size` field is within the buffer
+        // the caller guaranteed is initialized.
+        let size = unsafe {
+            self.base
+                .add(self.offset + self.size_offset)
+                .cast::<u32>()
+                .read_unaligned()
+        } as usize;


+        Proximity {
+            shared: self.minimal_shared(&set),
+            finer_unobserved: processors
+                .iter()
+                .any(|id| !self.kinds_covering(*id).is_empty()),
+            unrepresentable,
+        }


+**Windows only.** The crate does not build on other platforms, and is not
+intended to: an earlier version of this line claimed it degraded to an empty
+shell elsewhere, which was never true and was never built in CI (raised in
+PR #56 review).


+//!   That is a claim about not calling the platform, **not** about other
+//!   platforms: this crate is Windows-only and does not build elsewhere. An
+//!   earlier version of these docs said it degraded to an empty shell on other
+//!   targets, which was never true and never built in CI -- raised in PR #56
+//!   review.


Four findings from the PR #61 review, all confirmed before being fixed. `Proximity::finer_unobserved` was computed over the whole argument slice, including ids already classified as `unrepresentable`. `ProcessorSet::contains` answers `false` for an out-of-range number at every kind, which is indistinguishable from "the platform never placed this processor here" -- so one bad id reported a coverage gap at every reported kind at once, on a machine that had in fact described all of its processors. It now considers the representable ids only. The existing `an_unrepresentable_processor_does_not_corrupt_the_answer_for_the_others` asserted only over `shared`, and its topology carried no domains, so `finer_unobserved` was false whatever the query and the test could not have caught this. Both were fixed: it now carries a core and compares the field. Verified by running both tests against the unfixed code first -- they fail. `RecordWalk::next` computed its header bound with unchecked addition three lines above an existing `checked_add` on the same walk. On `i686-pc-windows-msvc`, which D-18 keeps supported, `usize` and `u32` are the same width, so a buffer near `u32::MAX` would wrap the sum and let the bounds check pass on an offset past the end. That buffer cannot exist in a 32-bit Windows address space, so this is unreachable rather than a live overrun -- but an unchecked add guarding an `unsafe` read should not rest on an argument made two layers away. The crate documented itself as Windows-only and "does not build elsewhere" while `observed` and `provenance` stayed ungated. Confirmed by building it for `x86_64-unknown-linux-gnu`: it compiled, exporting a two-type API. A `compile_error!` now enforces the claim; verified by rebuilding for that target and getting exactly that one error. Notably the same paragraph records a PR #56 round correcting this very sentence -- the correction reached the prose and never reached the code, which is the restatement drift CONTRACT INTEGRITY names. One dangling `topology-planner` doc link remained in `granularity.rs`. The sweep that removed the other thirteen covered `.md` files only, and rustdoc cannot catch this class because a relative path in a link reference definition is not an intra-doc link. Re-run across `.rs` and `.md` together: zero. Verified: 237 tests all-features and 163 default (each one higher than before, the new test), 107 in `windows-ioring-sys`, clippy clean under `-D warnings` and rustdoc clean under CI's flags in both feature configurations, and `cargo check --workspace --all-targets --all-features --locked` clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

crates/windows-topology-sys/src/domain.rs has mis-indented comment blocks in the float conversion arms (see stored review comments), which should be cleaned up to avoid formatting churn and potential cargo fmt check failures.

Review details

Suppressed comments (2)

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

crates/windows-topology-sys/src/domain.rs:367

In as_u64, the comment block under the AttributeValue::Float(n) arm is mis-indented (some lines are aligned with the match body instead of the arm), which is hard to read and likely to churn under cargo fmt. Consider moving the comment above the arm and formatting the guard consistently.

This issue also appears on line 387 of the same file.

crates/windows-topology-sys/src/domain.rs:397

In as_i64, the float conversion arm has the same indentation issue as as_u64 (the comment block is not consistently indented under the match arm). Restructuring it avoids readability issues and reduces the chance of future cargo fmt churn.

            AttributeValue::Float(n)
                // Exclusive at the top for the reason `as_u64` gives, and found by
            // sweeping for the same shape rather than reported: `i64::MAX as
            // f64` rounds up to 2^63, which no `i64` can hold, and the cast
            // would saturate it to `i64::MAX`. The *lower* bound stays
            // inclusive because `i64::MIN as f64` is -2^63 exactly -- it is a
            // power of two and representable, so it converts back losslessly.
            if n.fract() == 0.0 && (i64::MIN as f64..i64::MAX as f64).contains(&n) =>
            {
                Ok(n as i64)
            }

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

The comment blocks in `as_u64` and `as_i64` sat between the arm's pattern and its `if` guard, leaving the first line indented to the guard and the rest to the match body. Moved above the arm, which is where they read from. The review that raised this attributed it to `cargo fmt` churn. Measured, that is backwards: `cargo fmt --all --check` exited 0 on the original, and the comment's position was the *reason* -- sitting inside the arm it blocked rustfmt from joining pattern and guard, so the ragged shape was stable rather than pending. Moving it out is what let rustfmt format the arms, which it then did: `as_u64` now fits on one line and `as_i64` stays wrapped. Swept the rest of the branch for the same shape -- adjacent comment lines at differing indentation, across every changed `.rs` file -- and found no others. Verified: 237 tests all-features, 163 default, clippy clean under `-D warnings`, `cargo fmt --all --check` exit 0, encoding check clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

The PR includes an apparently unintended repository-root scratch file (.git-commit-msg.txt) that should be removed before merging.

Review details

Suppressed comments (4)

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

crates/windows-ioring-sys/examples/ring_copy/main.rs:269

outcome uses RemoteNode::Other(0) as a sentinel when any remote node exists. This discards the actual node id and makes future use of outcome easy to get wrong. Prefer preserving the first observed Other(id) value.
crates/windows-topology-sys/src/records/tests.rs:58
This test mutates storage via a *mut derived from Vec::as_ptr() (cast_mut()), which is avoidable and easier to reason about if you instead make storage mutable and use as_mut_ptr() directly.

This issue also appears on line 70 of the same file.
crates/windows-topology-sys/src/walk/tests.rs:5

The rustdoc comment has a grammatical error (missing verb). As written, it reads like a fragment; prefer an imperative "Returns ..." style.

crates/windows-topology-sys/src/records/tests.rs:74

Same issue as above: mutate the backing buffer via as_mut_ptr() instead of writing through a *mut created from as_ptr().

    let storage = chain(&[8, 8]);
    let base = storage.as_ptr().cast::<u8>().cast_mut();
    // Second record claims more than the buffer holds.
    // SAFETY: offset 8 is the second record's `Size`, inside the buffer.
    unsafe { base.add(8).cast::<u32>().write_unaligned(4096) };

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

Copilot

🔵 Needs a closer look

It is a large, breaking API/model reshape spanning core topology logic, serde wire shape, and a downstream consumer, which warrants final human review despite strong test additions.

Review details

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

…ed node id Two of the three suppressed findings from the PR #61 review. The third is declined below. `records/tests.rs` wrote through `storage.as_ptr().cast::<u8>().cast_mut()`. `Vec::as_ptr` documents that the memory it points to must never be written to through that pointer or any pointer derived from it, and directs callers to `as_mut_ptr` for exactly this. Miri passes both before and after, under Stacked Borrows and Tree Borrows alike, so nothing was miscompiling today -- but that is incidental current behavior, and this repository's rule is to bind to the specified contract instead. Now `let mut storage` and `as_mut_ptr`. Swept the branch for the shape. The other hits are calls passing a pointer to a Windows API that takes `*mut` without writing through it -- these two were the only writes, and both are in this crate. `ring_copy`'s `outcome` used `RemoteNode::Other(0)` as a stand-in whenever any domain had a remote node, discarding the id actually observed and making an invented node 0 indistinguishable from a real one. It now keeps the first observed value. The fabricated payload had also made a second scan look necessary: `SameAsLocal` is reached only when no `Other` was found, so it *is* the "nothing remote" case, and matching on it directly replaces the recomputed `any_remote`. Declined: the review asks for imperative "Returns ..." on `decode_records`' doc comment in `walk/tests.rs`. The noun-phrase form is idiomatic rustdoc and is what its sibling `decode_anomalies` uses three lines below; rewriting one would make the pair inconsistent, and rewriting both is churn on private test helpers to satisfy a style the crate does not follow. Verified: 237 tests all-features, 163 default, 107 in `windows-ioring-sys`, Miri clean on the touched tests, clippy clean under `-D warnings` in both feature configurations, `cargo fmt --all --check` exit 0, encoding clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

The changes are a large, breaking model reshape with new invariants across enumeration, serialization, and query semantics, so it warrants final human review despite only a minor textual nit identified.

Review details

Suppressed comments (1)

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

crates/windows-topology-sys/src/walk/tests.rs:5

Doc comment grammar: "The records a buffer decodes to" is missing a word (reads like a fragment). Suggest "The records that a buffer decodes to" to keep it a complete sentence.

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

…omment Taken on readability rather than on the grammar claim. "The records a buffer decodes to" is a reduced relative clause, which is ordinary English and not a fragment -- but "records" is also a verb, so the opening admits a brief garden-path reading before "decodes" resolves it. The explicit "that" closes that off for a word. This is narrower than the previous round's version of the finding, which asked for an imperative "Returns ..." and was declined because it would have split this helper's style from `decode_anomalies` three lines below. Adding the pronoun leaves both noun phrases intact, so that objection does not apply. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

ring_copy’s NUMA labeling/remote-placement helpers can mis-handle topologies whose memory domains are labeled via Source::CpuSets (not RelationshipWalk), leading to incorrect refusal/degradation paths.

Review details

Suppressed comments (3)

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

crates/windows-ioring-sys/examples/ring_copy/plan.rs:105

numa_node_for only looks at Source::RelationshipWalk labels, but MachineMemoryTopology can also carry NUMA node labels from Source::CpuSets (e.g. if the relationship walk truncated early but CPU Sets still enumerated). That can incorrectly treat a measured topology as having an unknown local NUMA node and force --placement remote into the LocalUnknown refusal path.

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

line 159
line 181

crates/windows-ioring-sys/examples/ring_copy/plan.rs:163

names_any_numa_node currently treats the machine as "unnamed" unless a memory domain has a RelationshipWalk label, but memory domain labels may also come from Source::CpuSets. That can make --placement remote exit with the "restored description" message on a topology that actually has NUMA node numbering via CPU Sets.

pub fn names_any_numa_node(topology: &MachineMemoryTopology) -> bool {
    topology.domains.iter().any(|domain| {
        matches!(domain.kind, DomainKind::Memory { .. })
            && domain.label_from(Source::RelationshipWalk).is_some()
    })

crates/windows-ioring-sys/examples/ring_copy/plan.rs:183

remote_numa_node only considers NUMA node labels from Source::RelationshipWalk. If the topology's memory domains are labeled only via Source::CpuSets, this will incorrectly return RemoteNode::Unnamed/SameAsLocal and can cause --placement remote to refuse or degrade despite node numbers being available.

        let Some(id) = domain.label_from(Source::RelationshipWalk) else {
            continue;
        };

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

PR #61 landed the topology reshape and release-please released it, so main now carries the peeled work plus the four rounds of review fixes made on it. Fourteen conflicts, resolved by which side is newer rather than by side: Main's version taken for the topology sources, `ring_copy`, and the README -- this branch holds the pre-review form of the same work, and main's adds the `proximity` fix, the checked header math, the `compile_error!` enforcing Windows-only, and the `as_mut_ptr` correction. This branch's version kept for `windows-ioring-sys`' versionless `windows-topology-sys` dev-dependency (SH-2.5). Main still pins it, and that pin is exactly what release-please had to rewrite to 0.2.0 in the release being merged here -- the churn the removal exists to stop. The first pass at that file took `--ours` wholesale and so also reverted `windows-ioring-sys` from the released 0.3.0 back to 0.2.0. Caught because `Cargo.lock` had auto-merged to 0.3.0 and the lockfile diff read as a *downgrade*; the manifest now takes main's version and this branch's dev-dependency, and `.release-please-manifest.json` agrees with both. The thirteen documentation links the peel stripped are restored. They were removed because their targets -- `topology-planner`, two workspace-root checklists, a design session -- do not exist in main. They do exist here, so on this branch the stripped form was the wrong one. Verified by resolving every relative link in the merged files: seven resolve, none dangle. Verified: `cargo check --workspace --all-targets --all-features --locked` clean, 237 topology tests, 116 in `windows-ioring-sys`, clippy clean under `-D warnings`, rustdoc clean under CI's flags, `cargo fmt --all --check` exit 0, encoding check 622 files clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI lite review requested due to automatic review settings September 5, 2026 13:50

Copilot started reviewing on behalf of MikeGrier September 5, 2026 13:51 View session