fix(distributed): give registry node accessors a defined order

[inureyes]

Summary

RegistryInner.nodes is a HashMap, and two of its list accessors handed back .values() unsorted while their consumers select by position or tie-break on input order. RandomState seeds every map instance separately rather than once per process, so identical cluster state produced different node selections between runs. Both accessors now sort by config.id, which is what the sibling accessors nodes_at_stage and nodes_at_rank already did.

What changed

• src/distributed/registry.rs: all_nodes and nodes_with_role sort the returned Vec by config.id. Signatures and return types are unchanged; only the order becomes defined.
• src/distributed/registry.rs: peer_addresses and topology_summary get the same treatment. See the scope note below.
• src/distributed/disaggregated/request_router.rs: handle_node_failure sorts the affected request ids before the round-robin re-routing loop consumes them.
• src/distributed/registry_tests.rs: five ordering tests over 64 independently constructed registries each.
• src/distributed/disaggregated/request_router_tests.rs: four selection-determinism tests over 32 independently constructed routers each, driving the real routing entry points rather than the accessor.

Design choices

Sort in the accessor, not at the call sites. There are seven consumers across three modules and the set grows; a per-call-site fix means every future caller has to rediscover the hazard. The sort runs on a local Vec after the read lock has already been taken for the clone, so it holds no lock longer than before, and node counts are cluster-sized next to the existing clone of every RegisteredNode.

sort_by, not sort_by_key. The issue recommends this and the recommendation holds. sort_by_key re-evaluates the key on every comparison, so a String key allocates per comparison rather than per element. Measured on a 12-element scrambled Vec<String>: sort_by_key(|s| s.clone()) performed 78 key evaluations, each a clone, while sort_by(|a, b| a.cmp(b)) performed 39 comparisons and allocated nothing. sort_by_key also cannot borrow the key out of the element, so a clone is the only way to spell it.

BTreeMap stays rejected. Per the issue: it would penalize the O(1) get_node / set_node_status / local_pp_tp_coords lookups for a property only the list accessors need. Nothing found during implementation changes that.

Scope: which accessors actually carry the exposure

The issue describes nodes as having four list accessors and puts two of them in scope. Swept the file and the module; the corrected picture:

Accessor	Before	Consumer sensitivity	Action
all_nodes	unordered	positional and tie-break, primary request path	sorted
nodes_with_role	unordered	positional, failover path	sorted
nodes_at_stage / nodes_at_rank	already sorted	positional	unchanged
peer_addresses	unordered, not named in the issue	only consumer (heartbeat.rs:214) broadcasts to every peer, so insensitive today	sorted
topology_summary	unordered, not named in the issue	operator-facing text printed at discovery.rs:96 and cluster_init.rs:253	sorted
find_pp_tp_node	.values().find(..)	deterministic only because ClusterConfig::validate rejects duplicate (stage, rank) pairs (config.rs:465)	unchanged

peer_addresses and topology_summary are beyond the issue's stated in-scope list. Including them is deliberate: they are the same defect in the same file, the fix is two lines each, and leaving them means the next caller of peer_addresses inherits a hazard the sibling accessors no longer have. topology_summary is also the one place where an operator can see the disorder directly, since re-running it on an unchanged cluster reshuffles the node list.

The order-insensitive consumers the issue cleared are confirmed: discovery.rs:50 (probes run concurrently), heartbeat.rs:131 (per-node registration), router_front.rs:409 (per-node health probe). router_front.rs:561 (/stats) is confirmed cosmetic and now stable as a side effect. One consumer the issue does not mention, router_front.rs:1218, takes only .len() of nodes_with_role and is insensitive.

A gap in the issue's own instance 1

Sorting nodes_with_role alone does not make failover re-routing reproducible. handle_node_failure collects the affected request ids from self.requests, which is also a HashMap, and the loop assigns candidates round-robin in exactly that sequence. With sorted candidates but unsorted request ids, the set of targets is stable while the request-to-node pairing still moves between runs, which is the part an operator is trying to reproduce. affected.sort() closes it. The new failover_reroute_assignment_is_stable_across_routers test asserts the full pairing, so it covers both halves.

Acceptance criteria

Verified by test:

• nodes_with_role returns nodes ordered by config.id ascending, for every role. Covered for Prefill, Decode, and Hybrid, plus the empty case for a role with no members.
• all_nodes returns nodes ordered by config.id ascending.
• A regression test registers at least four nodes whose ids do not match their insertion order and builds several independent registries. Five nodes, 64 registries per test.
• A test asserts the expected order explicitly, so it fails if the sort key is changed or dropped.
• The existing nodes_with_role_filter test still passes unchanged. It is untouched, and it structurally cannot observe this bug: it indexes a single-element list.
• Deterministic order reaches select_prefill_node and select_decode_node, at unit level, under RoundRobin and under the LeastLoaded / MemoryAware tie cases. The four router tests call route_to_prefill / route_to_decode / handle_node_failure on freshly built routers, so they exercise the real path from registry through get_load_infos_for_roles into the strategy arm.
• make verify-fmt passes. It is exactly cargo fmt --all -- --check, run here at exit 0.

Verified by construction, not by observation:

• The claim that a defined accessor order is sufficient for reproducible selection rests on every strategy arm being a pure function of the candidate sequence plus the atomic round-robin counters. That is true by inspection of select_prefill_node / select_decode_node, and the unit tests confirm it for the tie cases, but a live cluster adds real metrics that break most ties before the order matters.

Not verified by observation:

• The issue's manual check (three or more prefill nodes, restart the router repeatedly, confirm request N lands on the same node each time) cannot be run here. This box is a single node, so there is no multi-node disaggregated deployment to restart. The unit-level equivalent is covered; the end-to-end claim is not.
• make verify-clippy as written runs --features metal,accelerate and cannot execute on this Linux/CUDA box. The CUDA lint path was run instead; see the test plan.
• The full make verify-test-cuda merge gate is not run in this PR's verification and is left to the reviewer.

Test plan

Commands and their real exit codes, each captured directly rather than read off a pipeline:

• cargo test --profile test-fast --features cuda --lib --no-run then the test binary with narrow filters (one filter per invocation), all at exit 0: distributed::registry 18 passed, distributed::disaggregated::request_router 29 passed, distributed::scheduler 16 passed, distributed::routing 19 passed, distributed::heartbeat 9 passed, distributed::discovery 3 passed, distributed::cluster_init 14 passed, server::router_front 20 passed. 0 failed throughout.

• cargo fmt --all -- --check, exit 0.

• The nine new tests were run against the unfixed accessors first and all nine failed, most on iteration 0 or 1. For example all_nodes returned ["yankee-prefill", "alpha-decode", "mike-prefill", "bravo-decode", "zulu-hybrid"] on the first registry, and round_robin_selection_sequence_is_stable_across_routers produced ["prefill-0", "prefill-1", "prefill-0", "prefill-1"] on one router and ["prefill-1", "prefill-0", "prefill-1", "prefill-0"] on the next.

• cargo clippy --lib --tests --features cuda -- -D warnings -A clippy::err_expect, exit 0. Nothing this PR touches produces a lint.

• cargo clippy --lib --tests --features cuda -- -D warnings, exit 101, on one pre-existing error: clippy::err_expect at src/multimodal/host_preprocessor_tests.rs:416, a file this PR does not touch. The repo's verify-clippy lints --features metal,accelerate, so the CUDA lint path is ungated and this error predates the branch. It is the only error in the run and none of the four changed files appear in the output.

Follow-ups found while sweeping, not fixed here

The same defect class appears three more times in src/distributed/, all outside this issue's subsystem and all with the identical shape of a HashMap iteration collected into a Vec, handed to a stable sort_by_key, then consumed as a prefix:

• src/distributed/tensor_parallel/cache_manager.rs:711: check_pressure stops walking the candidate list once the memory target is met, so among allocations tied on current_offset or last_accessed the hash order decides which sequences apply_eviction destroys.
• src/distributed/pipeline/cache_manager.rs:697: same stable-sort-over-hash-order defect, published as PreemptionSignal.sequence_ids, a field documented as eviction priority order. No in-repo consumer takes a prefix yet.
• src/distributed/request_tracker.rs:333: sorts terminal requests by created_at and takes a prefix, so same-Instant ties are broken by hash order.

These change eviction behavior rather than routing, so they deserve their own issue and their own tests rather than a silent ride along with this one.

Closes #1277

[inureyes]

make verify-test-cuda on GB10 (CUDA sm_121, Linux aarch64) at a126cdba: 8238 passed, 0 failed, 311 ignored, 101 suites, exit 0.

That is +9 against main at 930fd83e (8229 passed), and this branch adds exactly 9 #[test] functions and removes none, so the totals reconcile.

cargo fmt --all -- --check is clean, and cargo clippy --lib --tests --features cuda -- -D warnings -A clippy::err_expect exits 0 with none of this branch's files appearing. Without the allow it exits 101 on the pre-existing clippy::err_expect at src/multimodal/host_preprocessor_tests.rs:416, which predates this branch and is tracked as #1283.