diff --git a/TECHNICAL_REPORTS/1290-hashmap-order-guideline-20260822.en.md b/TECHNICAL_REPORTS/1290-hashmap-order-guideline-20260822.en.md new file mode 100644 index 00000000..1d692d11 --- /dev/null +++ b/TECHNICAL_REPORTS/1290-hashmap-order-guideline-20260822.en.md @@ -0,0 +1,70 @@ +# Technical Report: PR #1290 - Recording the HashMap iteration order class, and declining to gate it + +## Executive Summary + +Seven instances of one defect class were found and fixed in a three-day sweep. This PR writes the class down in `docs/code-guidelines.md` and records, with numbers, the decision not to enforce it with a static check. No Rust changed. + +The class: a `HashMap` iteration result becomes ordered state or feeds an order-sensitive consumer. Nothing in the toolchain flags it. The types are correct, `cargo check` and clippy are clean, and the tests pass most of the time. + +## 1. Problem Statement + +Seven instances across five modules, none caught by review or CI: #1265 (four test fixtures), #1267 (`lang_bias.rs`), #1276 (RT-DETRv2 layout sniffing), #1277 (four registry accessors) and #1286 (three eviction paths). Two of them reached primary request paths, and one silently double-transposed conv weights into a shape-valid tensor nothing downstream could flag. + +Recurrence at that rate is the argument for writing the rule down rather than relying on each reviewer rediscovering it. + +## 2. Technical Decisions + +### 2.1 What the guideline emphasizes + +Two points get the most space, because they are the ones that cost the most to rediscover. + +**`RandomState` seeds per map instance, not per process.** The common mental model, "iteration order is randomized per run", implies a fixed binary on a fixed machine sees a fixed order. It does not. Measured here: build ten maps from the same five keys inside one process, over 200 processes; 127 runs produced 10 distinct orders out of 10, 62 produced 9, 11 produced 8. Not one of the 200 had the ten maps agree. The split moves between measurement runs because the experiment is itself random, and the guideline says so rather than presenting one split as a constant. + +**A sort is not automatically a fix.** `sort_by_key` and `sort_by` are stable, so ties keep input order, and when the input came from a `HashMap` that retained order is hash order. Three of the seven sorted their candidate list and were still nondeterministic. `sort_unstable_by` is not the remedy either: it substitutes a different arbitrary tie order rather than defining one. The remedy is a key that is a total order. + +The guideline also records the testing rule, because every fix in the family needed it and each of its three points is a way to write a test that passes against unfixed code: rebuild the map inside each iteration and loop 32 or more times; construct an actual tie, since distinct keys already give a total order; and write equal `Instant` values in deliberately, since `Instant::now()` never collides at nanosecond resolution. PR #1281's test failed on 27 of 64 freshly built maps, which is why a single-shot version would have passed and shipped nothing. + +### 2.2 Declining the static check, and the number that decided it + +Two candidate checks were prototyped in the shape of `check_kernel_dtype_keys.py` and run over all 1,248 `.rs` files, against the current tree and against the reconstructed pre-fix tree of each fix. + +The `min_by_key` / `max_by_key` on an unordered-map receiver rule looked strongest on its face: 4 flags, 0 false positives. Its catch rate against the seven is **0 of 7**, a number the issue never computed. None of the seven used either method. Gating on it would mean suppressing four of four findings on the day it landed, and a suppression comment does not re-arm on the change that would make the pattern real, which is the key becoming coarser than `Instant`. Those four hits are true positives for the lint and zero live defects: every key is an `Instant` stamped once per call, so the tie is unreachable today. Reporting them as four bugs would have been false and would have cost the rule its credibility on first inspection. + +The `Vec` from a map view rule fails for a different and more decisive reason: it flags all three #1286 sites **after** PR #1288 fixed them. The fix keeps the shape and changes only whether the sort key is total, a distinction no regex can draw. Its 3-of-7 held only against pre-fix trees, so on any fixed tree those three become permanent false positives and it can never be a gate. + +The general form is undecidable from source alone, since whether an order matters depends on a consumer that is frequently in another module. The guideline says the rule is enforced by review and by the testing rule, and to re-open the question if a future instance takes a shape either candidate would have caught. + +One improvement over the issue's analysis survives: a tree-wide scan for `type X = HashMap<...>` removes the type-alias blind spot that `WeightMap` creates, and it resolves `Memo` and `DetachedMap` as well. That limit is fixable. The other three misses are not. + +## 3. Change Summary + +| File | Change | +| --- | --- | +| `docs/code-guidelines.md` | New `## HashMap Iteration Order` section: the rule with wrong and right examples, the per-instance measurement, order-sensitive consumers, the stable-sort subsection, the total-order remedies, the testing rule, the `BTreeMap` alternative, the seven instances, and the enforcement decision with its numbers | +| `docs/README.md` | Index entry | + +## 4. Review Findings + +The implementation corrected the issue that specified it, in ways worth keeping. + +The issue described #1286 as open with its three instances unfixed on `main`. That was stale by roughly an hour: PR #1288 merged before the work started. It is not cosmetic, because it inverts the `Vec` rule's evaluation, turning three catches into three false positives. The stale text is a process error on the filing side: #1287 was filed to record a class while instances of that class were still being fixed, and it was not refreshed before implementation. + +The `RandomState` split did not reproduce exactly (127/62/11 measured against 133/52/15 in the issue). Correctly treated as the experiment being random rather than as an error, with the invariant that the ten maps never agree holding in both runs, and the guideline states the caveat. + +The reconstruction recipe in the issue, `git show ^:`, understates any tree-wide rule, since both file-local type detection and the alias table need the whole tree. `git archive ^ src` is the working form. + +## 5. Validation + +No cargo gate was run and none applies: the diff is two Markdown files with zero Rust changed, so `make verify-test-cuda` would exercise nothing this PR touches. CI's `changes` path filter reached the same conclusion and skipped every Rust job while `crate versions`, `kernel dtype keys` and `cross-repo refs` passed. + +Checks that do apply, all exit 0: `make verify-fmt`; the new section's relative link to `scripts/ci/check_kernel_dtype_keys.py` resolves; `docs/README.md` indexes it; and the added lines carry no em dash and no AI attribution. + +Every in-tree citation the section makes was verified rather than carried over: the four test-loop constants and their line numbers, the 27-of-64 figure in PR #1281, the withdrawal at `Makefile:604-616`, and the seven issue and PR pairs. + +## 6. Related Work + +- #1287: the issue this closes. +- #1265 and PR #1266, #1267 and PR #1269, #1276 and PR #1281, #1277 and PR #1284, #1286 and PR #1288: the seven instances the guideline documents. +- #1283 and PR #1285: found in the same window. A different class, an `err_expect` that reddened the lint gate for every contributor, but the same underlying gap, which is that nothing ran the check at PR time. + +Left for a separate decision: the four `min_by_key` LRU eviction sites are genuinely fragile and now have no mechanical guard. They are safe only because every key is an `Instant` stamped once per call. Pinning them with a total-order tie-break on the map key is small, and `src/server/responses_store.rs` and `src/server/conversation_store.rs` are already being touched by #1248. diff --git a/TECHNICAL_REPORTS/1290-hashmap-order-guideline-20260822.ko.md b/TECHNICAL_REPORTS/1290-hashmap-order-guideline-20260822.ko.md new file mode 100644 index 00000000..83aa7fdf --- /dev/null +++ b/TECHNICAL_REPORTS/1290-hashmap-order-guideline-20260822.ko.md @@ -0,0 +1,70 @@ +# 기술 보고서: PR #1290 - HashMap 순회 순서 계열의 기록, 그리고 정적 검사를 두지 않기로 한 결정 + +## 요약 + +사흘간의 스윕에서 같은 결함 계열 인스턴스 일곱 개를 찾아 고쳤다. 이 PR은 그 계열을 `docs/code-guidelines.md`에 적고, 정적 검사로 강제하지 않기로 한 결정을 **숫자와 함께** 기록한다. Rust 변경은 없다. + +계열은 이것이다. `HashMap` 순회 결과가 순서 있는 상태가 되거나 순서에 민감한 소비자로 흘러간다. 툴체인 어느 것도 잡지 못한다. 타입은 맞고, `cargo check`도 clippy도 깨끗하며, 테스트는 대체로 통과한다. + +## 1. 문제 + +다섯 모듈에 걸친 일곱 인스턴스, 리뷰도 CI도 못 잡았다. #1265(테스트 픽스처 4개), #1267(`lang_bias.rs`), #1276(RT-DETRv2 레이아웃 판별), #1277(레지스트리 접근자 4개), #1286(축출 경로 3개). 그중 둘은 주 요청 경로에 닿았고, 하나는 conv 가중치를 조용히 이중 transpose해 하류에서 아무도 못 잡는 형상상 유효한 텐서로 만들 수 있었다. + +이 빈도의 재발이 곧 "리뷰어마다 다시 발견하게 두지 말고 규칙으로 적자"는 근거다. + +## 2. 기술적 판단 + +### 2.1 가이드라인이 무게를 싣는 두 지점 + +다시 발견하는 비용이 가장 큰 둘에 가장 많은 지면을 줬다. + +**`RandomState`는 프로세스가 아니라 맵 인스턴스마다 시드된다.** "순회 순서는 실행마다 무작위"라는 흔한 모델은 고정된 기계의 고정된 바이너리가 고정된 순서를 본다는 뜻을 함축한다. 아니다. 여기서 실측: 한 프로세스 안에서 같은 다섯 키로 맵 열 개를 만들고, 200 프로세스에 걸쳐 세었다. 127회가 10개 중 고유 순서 10개, 62회가 9개, 11회가 8개였다. **200회 중 열 개가 일치한 경우는 한 번도 없었다.** 실험 자체가 무작위라 분포는 측정마다 움직이며, 가이드라인은 한 분포를 상수처럼 제시하지 않고 그 단서를 적는다. + +**정렬이 자동으로 수정이 되지는 않는다.** `sort_by_key`와 `sort_by`는 안정 정렬이라 동점이 입력 순서를 유지하고, 입력이 `HashMap`에서 왔다면 그 유지된 순서가 곧 해시 순서다. 일곱 중 셋이 후보 목록을 정렬하고도 비결정적이었다. `sort_unstable_by`도 해법이 아니다. 순서를 정의하지 않고 다른 임의 순서로 바꿀 뿐이다. 해법은 키를 전순서로 만드는 것이다. + +테스트 규칙도 적었다. 이 계열의 모든 수정이 필요로 했고, 세 항목 각각이 미수정 코드에서도 통과하는 테스트를 쓰는 방법이기 때문이다. 반복마다 맵을 새로 만들고 32회 이상 돌 것, 실제 동점을 만들 것(서로 다른 키는 이미 전순서다), 같은 `Instant` 값을 의도적으로 써 넣을 것(`Instant::now()`는 나노초 해상도에서 충돌하지 않는다). PR #1281의 테스트는 새로 만든 맵 64개 중 27개에서 실패했고, 그래서 단발 버전이었다면 통과하고 아무것도 못 실었을 것이다. + +### 2.2 정적 검사를 두지 않기로 한 결정과 그것을 가른 숫자 + +`check_kernel_dtype_keys.py` 형태로 후보 검사 둘을 프로토타이핑해 `.rs` 파일 1,248개 전체에, 현재 트리와 각 수정의 재구성된 수정 전 트리에 대해 돌렸다. + +"맵 수신자에 대한 `min_by_key`/`max_by_key`" 규칙은 겉보기에 가장 강했다. 플래그 4건, 오탐 0. 그런데 일곱 인스턴스에 대한 검출률이 **0 of 7**이다. 이슈가 계산하지 않은 숫자다. 일곱 중 어느 것도 그 두 메서드를 쓰지 않는다. 이걸로 게이트를 걸면 도입 당일에 4건 중 4건을 억제해야 하고, 억제 주석은 그 패턴을 실재하게 만들 변경(키가 `Instant`보다 거칠어지는 것)에 다시 무장하지 않는다. 그 4건은 린트 참양성이자 live 결함 0건이다. 키가 전부 호출당 한 번 찍히는 `Instant`라 동점이 지금은 도달 불가다. 이를 "버그 4개를 찾았다"고 보고했다면 거짓이고, 처음 확인하는 사람에게 규칙의 신뢰를 잃었을 것이다. + +"맵 뷰에서 만든 `Vec`을 순서 민감하게 소비" 규칙은 더 결정적인 이유로 탈락했다. **PR #1288이 고친 뒤의 #1286 세 지점을 그대로 플래그한다.** 수정이 형태는 유지하고 정렬 키가 전순서인지만 바꾸는데, 그 구분은 어떤 정규식도 그릴 수 없다. 3-of-7은 수정 전 트리에서만 성립했고, 계열이 고쳐진 트리에서는 그 셋이 영구 오탐이 되므로 게이트가 될 수 없다. + +일반형은 소스만으로 판정 불가다. 순서가 중요한지가 종종 다른 모듈에 있는 소비자에 달려 있기 때문이다. 가이드라인은 이 규칙이 리뷰와 위 테스트 규칙으로 강제된다고 적고, 두 후보 중 하나가 잡았을 형태의 인스턴스가 나오면 질문을 다시 열라고 닫는다. + +이슈 분석보다 나아진 것 하나는 남는다. `type X = HashMap<...>`을 트리 전역으로 훑으면 `WeightMap`이 만드는 타입 별칭 사각지대가 사라지고, `Memo`와 `DetachedMap`도 함께 풀린다. 그 한계는 고칠 수 있다. 나머지 셋은 아니다. + +## 3. 변경 요약 + +| 파일 | 변경 | +| --- | --- | +| `docs/code-guidelines.md` | 신규 `## HashMap Iteration Order` 절: 규칙과 wrong/right 예시, 인스턴스별 무작위화 실측, 순서 민감 소비자 목록, 안정 정렬 하위절, 전순서 해법 3형태, 테스트 규칙, `BTreeMap` 대안, 일곱 인스턴스, 그리고 숫자를 동반한 강제 결정 | +| `docs/README.md` | 색인 항목 | + +## 4. 리뷰 지적사항 + +구현이 그것을 지시한 이슈를 정정했고, 남길 가치가 있다. + +이슈는 #1286을 열려 있고 세 인스턴스가 `main`에서 미수정이라고 서술했다. 약 한 시간 낙후였다. PR #1288이 작업 시작 전에 머지됐다. 표면적 문제가 아니다. `Vec` 규칙의 평가를 뒤집어 검출 3건을 오탐 3건으로 만들기 때문이다. 이 낙후는 발행 쪽의 절차 오류다. 그 계열의 인스턴스를 아직 고치는 중에 계열을 기록하는 이슈를 냈고, 구현 전에 갱신하지 않았다. + +`RandomState` 분포는 정확히 재현되지 않았다(측정 127/62/11 대 이슈 133/52/15). 오류가 아니라 실험이 무작위인 것으로 옳게 처리했고, 열 개가 결코 일치하지 않는다는 불변식은 양쪽에서 유지됐으며, 가이드라인이 그 단서를 적는다. + +이슈의 재구성 레시피 `git show ^:<파일 하나>`는 트리 전역 규칙을 과소평가한다. 파일 지역 타입 탐지와 별칭 표 둘 다 트리 전체가 필요하다. `git archive ^ src`가 동작하는 형태다. + +## 5. 검증 + +cargo 게이트는 돌리지 않았고 해당되지도 않는다. diff가 마크다운 두 파일이고 Rust 변경이 0이라 `make verify-test-cuda`는 이 PR이 건드린 것을 아무것도 실행하지 않는다. CI의 `changes` 경로 필터도 같은 결론에 도달해 Rust 잡을 전부 건너뛰었고 `crate versions`, `kernel dtype keys`, `cross-repo refs`가 통과했다. + +해당되는 검사는 전부 exit 0이다. `make verify-fmt`, 신규 절의 `scripts/ci/check_kernel_dtype_keys.py` 상대 링크 해석, `docs/README.md` 색인 등록, 그리고 추가된 줄에 em dash와 AI attribution 없음. + +절이 인용하는 트리 내 사실은 옮겨 적지 않고 전부 확인했다. 테스트 루프 상수 4개와 그 줄 번호, PR #1281의 27-of-64 수치, `Makefile:604-616`의 철회, 일곱 개 이슈/PR 쌍. + +## 6. 관련 작업 + +- #1287: 이 PR이 닫는 이슈. +- #1265와 PR #1266, #1267과 PR #1269, #1276과 PR #1281, #1277과 PR #1284, #1286과 PR #1288: 가이드라인이 문서화하는 일곱 인스턴스. +- #1283과 PR #1285: 같은 기간에 나왔다. 계열은 다르지만(모든 기여자의 린트 게이트를 red로 만든 `err_expect`) 밑에 깔린 구멍은 같다. PR 시점에 그 검사를 도는 것이 없었다는 것. + +별도 판단으로 남긴 것: `min_by_key` LRU 축출 4곳은 실제로 취약하고 이제 기계적 가드가 없다. 키가 전부 호출당 한 번 찍히는 `Instant`라서만 안전하다. 맵 키로 전순서 타이브레이크를 거는 것은 작은 작업이고, `src/server/responses_store.rs`와 `src/server/conversation_store.rs`는 #1248이 이미 건드리고 있다. diff --git a/docs/README.md b/docs/README.md index 4c48cc5f..d1cca3e9 100644 --- a/docs/README.md +++ b/docs/README.md @@ -33,7 +33,7 @@ Current GitHub-facing docs: 17. `cascade-attention.md` — shared-prompt-prefix (cascade) decode: computing a prefix shared by several concurrent sequences once per step instead of once per sequence, the two-level decomposition, the flags, and how to tell which path a launch took. 18. `sparse-paged-decode.md` — sparse attention reduced to page indirection over the fused v2 decode kernel: the addressing argument, the MiniMax-M3 routing, why DeepSeek Sparse Attention is not routed, where the dispatch floor sits, the kill switch, and the benchmark harness. 19. `mtp-policy-api.md`: the supported read interface for the adaptive B=1 MTP verdict (`GET /v1/internal/mtp-policy`): the response body, the four states, the unavailable reasons, and the schema versioning and compatibility policy. -20. `code-guidelines.md`: the file-size and module-split thresholds, including when to extract a `_helpers.rs` and when inline tests move to a sibling `_tests.rs`, the `// Used by:` annotation convention for shared functions, and the JIT kernel rule that every varying input dtype must appear in `template_args` because CUDA keys the compiled-module cache on it. +20. `code-guidelines.md`: the file-size and module-split thresholds, including when to extract a `_helpers.rs` and when inline tests move to a sibling `_tests.rs`, the `// Used by:` annotation convention for shared functions, the JIT kernel rule that every varying input dtype must appear in `template_args` because CUDA keys the compiled-module cache on it, and the `HashMap` iteration-order rule covering what counts as an order-sensitive consumer, why a stable sort on a non-total key does not pin the result, the fresh-map-per-iteration testing requirement, and why no static check enforces it. ## Architecture Decision Records diff --git a/docs/code-guidelines.md b/docs/code-guidelines.md index 13425f4d..9a78c3c8 100644 --- a/docs/code-guidelines.md +++ b/docs/code-guidelines.md @@ -105,3 +105,95 @@ The entry may stay unreferenced by the kernel body. Its job is the cache key, so **Enforcement:** `make verify-kernel-dtype-keys` (part of `make verify`, and the `kernel dtype keys` CI job) runs [`scripts/ci/check_kernel_dtype_keys.py`](../scripts/ci/check_kernel_dtype_keys.py). The rule is scoped by the presence of `cuda_kernel(` in the file rather than by a hand-maintained list, so a Metal-only launcher is out of scope until someone adds a CUDA port to it, at which point the check starts applying on its own. Read the script rather than trusting this section. + +## HashMap Iteration Order + +When a `HashMap` or `HashSet` iteration result becomes ordered state, or feeds a consumer that is sensitive to order, the ordering has to be established explicitly at the point of iteration: + +```rust +// Wrong. `entries` comes out of a HashMap, so its order is arbitrary, and +// `sort_by_key` is stable, so any two entries tied on `last_accessed` keep +// that arbitrary order. +let mut entries: Vec<_> = self.allocations.values().collect(); +entries.sort_by_key(|a| a.last_accessed); + +// Right. The id component makes the key a TOTAL order, so the stable sort is +// never asked to break a tie and hash order cannot reach the result. +let mut entries: Vec<_> = self.allocations.values().collect(); +entries.sort_by_key(|a| (a.last_accessed, a.sequence_id)); +``` + +**Why this matters:** + +`RandomState` seeds each map *instance*, not the process. The common mental model, "HashMap iteration order is randomized per run", is wrong in the direction that matters: it implies a fixed binary on a fixed machine sees a fixed order, and it does not. Two maps built from identical input, in the same function, microseconds apart, iterate differently. + +Measured on this repository's Linux box with `rustc -O` (1.97.1): build ten `HashMap<&str, u32>` instances from the same five keys inside one process, record each map's `.keys()` order, and count the distinct orders. Over 200 processes, 127 produced 10 distinct orders out of 10, 62 produced 9, and 11 produced 8. Not one process out of 200 had the ten maps agree. The exact split moves between measurement runs, because the experiment is itself random. What does not move is that the ten never agree. + +The result is a probabilistic defect, so a green test run is not evidence. `cargo check` is clean, clippy is clean, and the tests pass most of the time. + +**Order-sensitive consumers.** The iteration is only a problem once something downstream cares about position. These all care: + +- Positional indexing, including round-robin selection by index. +- `min_by_key` and `max_by_key`. Both return the FIRST extremum, so they tie-break on input order. +- A prefix: `take(n)`, or an early `break`, or an early `return` out of the loop. +- A loop-carried accumulator, such as a seed advanced once per element. +- Anything documented as a priority order, whether or not the current consumer reads it that way. + +The consumer is frequently in another module from the iteration, which is why this survives review: the accessor that returns the unordered list looks unremarkable on its own. + +### A sort is not automatically a fix + +This is the least obvious part of the rule and the part that costs the most to rediscover. `slice::sort_by_key` and `slice::sort_by` are **stable**: elements that compare equal keep their input order. When the input came out of a `HashMap`, that retained order is hash order. So a sort does not make a `HashMap`-derived list deterministic unless the sort key is a **total order** over the elements, meaning no two distinct elements can compare equal. + +Three of the seven instances below sorted their candidate list and were still nondeterministic for exactly this reason. A reader who saw the sort reasonably concluded the result was pinned. It was not. + +`sort_unstable_by` is not the remedy either. It does not preserve input order, but it does not define one; it substitutes a different arbitrary tie order, and the outcome is still not reproducible. + +The remedy is to make the key total by appending something unique to each element. All three forms below are correct and the choice between them is stylistic: + +```rust +// Tuple key. Cheapest when the unique component is Copy. +entries.sort_by_key(|a| (a.last_accessed, a.sequence_id)); + +// Comparator with `then_with`. Preferred when the unique component is a +// String, since `sort_by_key` would force a clone into the tuple. +completed.sort_by(|(key_a, t_a), (key_b, t_b)| t_a.cmp(t_b).then_with(|| key_a.cmp(key_b))); + +// Sort on the unique component alone, when the policy key is not needed. +nodes.sort_by(|a, b| a.config.id.cmp(&b.config.id)); +``` + +Add a comment at the call site naming the unique component as the thing that makes the key total. Without it the component reads as redundant and the next reader simplifies it away. + +### Testing the fix + +Every fix in this family needed a test shaped a particular way, and a test that misses any of these three points passes against the unfixed code: + +1. **Build a fresh map inside each iteration, and loop at least 32 times.** Because `RandomState` seeds per instance, probing one map repeatedly measures nothing; the map has to be rebuilt. The in-tree loop counts are `ORDER_RESOLVE_ITERATIONS = 32` (`src/lang_bias.rs:755`), `ITERATIONS = 64` (`src/vision/detection/rt_detr_v2/sanitize.rs:388`), `ORDER_ITERATIONS = 64` (`src/distributed/registry_tests.rs:257`) and `SELECTION_ITERATIONS = 32` (`src/distributed/disaggregated/request_router_tests.rs:865`). +2. **Construct an actual tie.** Distinct keys already give a total order, so a test built from distinct keys passes without the fix and proves nothing. The tie is the thing under test. +3. **Write equal timestamps in deliberately.** `Instant::now()` resolves to nanoseconds on the platforms this project targets, so two entries stamped by two separate calls effectively never collide. A test that stamps entries with `Instant::now()` and expects them to tie will never construct the case it is trying to cover. + +The reason for the loop is that the pre-fix failure rate is well under 100 percent. PR #1281's new test failed on **27 of 64** freshly built maps against the unfixed function. A single-shot version of that test would have passed on the majority of runs and shipped nothing. PR #1269's three ordering tests first failed at iteration 0, iteration 0 and iteration 2. + +Validate the test by running it against the unfixed code first. If it does not fail there, it is not testing the defect. + +### Choosing BTreeMap instead + +`BTreeMap` and `BTreeSet` have a defined iteration order, so switching the container is the structural fix and removes the whole class at that site. It costs O(1) `get` for O(log n), so it is the right move where lookups are not hot and the wrong one where they are. #1277 is the worked example of the other direction: the registry's node map is read on the primary request path, so it kept its `HashMap` and the four accessors sort on the node id instead. + +**The seven instances.** These were found in one review sweep between 2026-08-20 and 2026-08-22, across five modules, with no part of the toolchain flagging any of them: + +- **#1265** (PR #1266). Four `filled_weights` test fixtures walked a `WeightMap` and advanced one LCG seed per key, so every process built a different synthetic model. A loop-carried accumulator, with no sort and no index anywhere in sight. Before the cause was found it had already put two wrong conclusions into a tracked file, both since withdrawn in place at `Makefile:604-616`. +- **#1267** (PR #1269). `LangBiasYamlConfig::bias` was `Option>` feeding `LangBiasSet.ordered`, which is documented as the priority order and consumed first-language-wins. A multi-CJK YAML config therefore assigned different biases to shared Han tokens on every run, and the schema example in the doc comment is itself a three-CJK config, so copying it was enough to trigger this. +- **#1276** (PR #1281). `needs_sanitize` returned from inside a `weights.keys()` walk on the first marker it saw, so a checkpoint carrying both marker families got a coin-flip layout verdict. The wrong direction is the expensive one: re-running the sanitize pipeline over already-MLX weights double-transposes conv weights into a shape-valid tensor that nothing downstream can flag. +- **#1277** (PR #1284). Four registry accessors (`all_nodes`, `nodes_with_role`, `peer_addresses`, `topology_summary`) returned `HashMap` values unordered into consumers that index positionally or tie-break on input order, reaching node selection on the primary request path. +- **#1286** (PR #1288). Three eviction paths, in `src/distributed/tensor_parallel/cache_manager.rs`, `src/distributed/pipeline/cache_manager.rs` and `src/distributed/request_tracker.rs`. All three sorted their candidate list and were still nondeterministic, which is the case the stable-sort subsection above exists for. + +**Enforcement:** + +There is no `make verify-*` target, no CI job and no script for this rule. That is a measured decision rather than an omission, taken in #1287, and the numbers behind it are recorded here so the next person does not re-derive them. Two candidate checks in the shape of [`scripts/ci/check_kernel_dtype_keys.py`](../scripts/ci/check_kernel_dtype_keys.py) were prototyped and run over all 1,248 `.rs` files under `src/`, against the current tree and against the reconstructed pre-fix tree of each of the five fixes above: + +- **`min_by_key` / `max_by_key` on an unordered-map receiver.** Four flags on the current tree and zero false positives: the LRU eviction sites at `src/server/prompt_cache/store.rs:315` and `:336`, `src/server/responses_store.rs:243`, and `src/server/conversation_store.rs:151`. It catches **0 of the 7** instances above. Every reconstructed pre-fix tree produced those same four flags and nothing else, because none of the seven used `min_by_key` or `max_by_key`. The four are also not live defects: all four keys are `std::time::Instant`, and each writer computes `Instant::now()` once per call and stamps a single entry with it, so at nanosecond resolution the tie is not reachable today. They are true positives for the lint and zero live bugs. The distinction matters: saying the check found four bugs would be false, and the rule would lose its credibility the first time somebody checked. Gating on this would mean suppressing four of four findings on the day it landed, and a suppression does not re-arm on the change that would make the pattern real, which is the key becoming coarser. The pattern is genuinely fragile and worth knowing about; it is not worth a gate. +- **A `Vec` built from an unordered-map view and consumed order-sensitively.** Twenty-two flags on the current tree. It flags all three #1286 sites *after* PR #1288 fixed them, because the fix keeps the shape and changes only whether the sort key is total, a distinction no regex can draw. Its 3-of-7 catch rate held only against pre-fix trees; on any tree where the class has been fixed, those three become permanent false positives. It reaches none of the four separately filed instances, each for a different structural reason: #1265 is a loop-carried seed with no `Vec`, no sort and no index; #1267 iterates the map directly in a `for` and never calls `.keys()` or `.values()` at all; #1276 returns early out of a `for` with no `Vec` binding to bind a consumer to; and #1277's producer and its order-sensitive consumers are in different modules. A tree-wide scan for `type X = HashMap<...>` does remove the type-alias blind spot that `WeightMap` creates, so that one limit is fixable; the other three are not, at this level of analysis. + +The general form is undecidable from the source alone, because whether an iteration order matters depends on a consumer that is often in another module. So this rule is enforced by review and by the testing rule above. Re-open the question if a future instance takes a shape either candidate would have caught.