diff --git a/TECHNICAL_REPORTS/1301-eviction-victim-total-order-20260822.en.md b/TECHNICAL_REPORTS/1301-eviction-victim-total-order-20260822.en.md new file mode 100644 index 00000000..49f95bc9 --- /dev/null +++ b/TECHNICAL_REPORTS/1301-eviction-victim-total-order-20260822.en.md @@ -0,0 +1,83 @@ +# Technical Report: PR #1301 - A total order for preemption victim selection + +## Executive Summary + +`BatchScheduler::select_eviction_victim` chose which in-flight sequence to preempt with `max_by_key` on `generated_tokens.len()` under `LongestFirst`, and `min_by` on priority-then-length under `LowestPriority`. Both ran over `ActiveBatch::iter_sequences`, which is `HashMap::values`, so ties fell through to hash order. + +This is the eighth instance of the class recorded in `docs/code-guidelines.md`, and the only one whose tie is reachable through ordinary state rather than through a coarsening of the key. Sequences admitted together decode in lockstep and therefore share a token count, and both `PreemptionPolicy::LongestFirst` and `RequestPriority::Normal` are the shipped defaults. Two identically configured workers under identical load preempted different requests, and an operator could not reproduce the choice from the batch state. + +## 1. Problem Statement + +Nothing was corrupted and the policy's stated intent was never violated: any tied-longest sequence does satisfy "evict the longest". What was lost is reproducibility of which user's request was sacrificed, which is exactly what someone investigating a preemption needs. + +The reachability is what separates this from #1291's four sites, whose keys are `Instant` values stamped once per call and therefore effectively never collide. Here the key is a small integer that collides routinely. + +## 2. Technical Decisions + +### 2.1 Smallest `seq_id`, and why `created_at` was rejected + +`seq_id` is monotonic, but `try_evict_for_preemption` reallocates its victim under a fresh, higher id, so a large id marks a recently preempted sequence rather than a late arrival. Smallest-id-wins therefore rotates preemption onto sequences that have not been hit yet. + +`created_at` was considered as the more operator-meaningful key and rejected for the mirror-image reason: it survives preemption, so smallest-`created_at`-wins would keep selecting the same oldest request forever, since nothing about being preempted moves the field. It is also an `Instant`, so `seq_id` would have had to sit behind it regardless. Both arms resolve to the same direction, so the choice does not vary by policy. + +### 2.2 Opposite-facing tie components for the same direction + +`max_by_key` returns the last maximum and `min_by` the first minimum, so the two arms need the tie component facing opposite ways to produce the same outcome. + +- `LongestFirst` keys on `(generated_tokens.len(), std::cmp::Reverse(seq_id.as_u64()))`. The maximum of that key is the longest sequence and, among equals, the largest `Reverse(id)`, which is the smallest id. +- `LowestPriority` appends `.then_with(|| a.seq_id.as_u64().cmp(&b.seq_id.as_u64()))` ascending. The minimum is the lowest priority, then the longest, then the smallest id. + +Because both keys are now total, neither the last-maximum nor the first-minimum rule can fire at all. `SequenceId` does not derive `Ord`, so both go through `.as_u64()`, the same trap `PromptCacheKeyDigest` posed in #1291. + +### 2.3 One copy of the policy, with the filter inside it + +The two existing tests reimplemented the selection expression inline rather than calling the private method, and had already drifted: neither carried the `.filter(|seq| seq.structured.is_none())` guard that production has. A regression test written in their shape would have pinned a copy. + +The policy is now a free function, `select_eviction_victim_from(sequences, policy)`, with the method reduced to a one-line call. A free function rather than an `ActiveBatch` method because `active.rs` currently knows nothing about `PreemptionPolicy`. The `structured.is_none()` filter moved **inside** the function, so no caller can lose it again, and its doc comment says to extend the policy there and never at a call site. + +## 3. Change Summary + +| File | Change | +| --- | --- | +| `src/server/batch/scheduler.rs` | `select_eviction_victim_from` extracted; both arms given a total order; filter moved inside; comments naming `seq_id` as the total-order component | +| `src/server/batch/scheduler_tests.rs` | The two drifted tests rewritten to call the extracted function; 3 new tests | +| `docs/code-guidelines.md` | The instance list bumped to eight, plus the arithmetic that depended on "seven" | + +## 4. Review Findings + +The tests were built before the fix rather than written after and validated by reverting, so no `git stash` and no file surgery was needed. Against the unfixed expression: + +``` +LongestFirst resolved a fully tied batch to something other than seq_id 2 on 58 of 64 +freshly built batches; the tie is falling through to HashMap order + +LowestPriority resolved a batch tied on priority and length to something other than seq_id 2 +on 39 of 64 freshly built batches; the tie is falling through to HashMap order +``` + +The tests accumulate mismatches over all 64 iterations rather than panicking on the first, which is what produces the "N of 64" figure and shows the pre-fix failure rate directly. + +Three things worth recording: + +Updating the guideline's instance count to eight made three other sentences arithmetically wrong, which the issue did not anticipate. The dependent figures were corrected, and the measurement sentence was scoped to "the first five fixes above (#1293 was found after the measurement ran)" rather than silently restating #1287's measurement as covering something it never measured. + +One nuance was added to the enforcement record rather than left to be misread: #1293 **does** use `max_by_key`, so the candidate check's "0 of 8" is not because the rule ignores that method. It was missed because the receiver is a cross-module accessor (`ActiveBatch::iter_sequences`) rather than a literal `.values()`, which is the limitation the same section already describes for #1277. + +The `structured.is_none()` guard is now in the executed path but no test constructs a `Some(..)`, because `StructuredOutputConstraint` has no test constructor and building one needs a real tokenizer plus a compiled grammar. Stated in the PR body rather than left implicit. + +## 5. Validation + +Measured on GB10 (DGX Spark, CUDA sm_121, Linux aarch64), branch current with `main` at gate time, run with no other cargo process on the box. + +- `make verify-test-cuda`: recorded in the PR thread. +- `cargo test --profile test-fast --features cuda --lib server::batch::scheduler`: 111 passed, 7 ignored, exit 0. `server::batch`: 373 passed, exit 0. The eviction filter alone: 5 passed. +- `cargo fmt --all -- --check`, `cargo check --lib --tests --features cuda`, `cargo clippy --lib --tests --features cuda -- -D warnings`: all exit 0. + +Neither pre-existing eviction test changed its expected victim, because neither constructs a tie. + +## 6. Related Work + +- #1293: the issue this closes. +- #1287 and PR #1290, plus the correction in PR #1294: the guideline this instance is added to, and the `max_by_key` direction fact that the fix depends on. +- #1291 and PR #1299: the four latent siblings, where the opposite call was made on testing because the tie is unreachable. +- #1265 and PR #1266, #1267 and PR #1269, #1276 and PR #1281, #1277 and PR #1284, #1286 and PR #1288: the rest of the class. diff --git a/TECHNICAL_REPORTS/1301-eviction-victim-total-order-20260822.ko.md b/TECHNICAL_REPORTS/1301-eviction-victim-total-order-20260822.ko.md new file mode 100644 index 00000000..22e2444e --- /dev/null +++ b/TECHNICAL_REPORTS/1301-eviction-victim-total-order-20260822.ko.md @@ -0,0 +1,83 @@ +# 기술 보고서: PR #1301 - 선점 victim 선택에 전순서 부여 + +## 요약 + +`BatchScheduler::select_eviction_victim`은 어느 진행 중 시퀀스를 선점할지를 `LongestFirst`에서는 `generated_tokens.len()`에 대한 `max_by_key`로, `LowestPriority`에서는 우선순위 다음 길이에 대한 `min_by`로 골랐다. 둘 다 `ActiveBatch::iter_sequences` 즉 `HashMap::values` 위에서 돌았으므로 동점이 해시 순서로 떨어졌다. + +`docs/code-guidelines.md`에 기록된 계열의 여덟 번째 인스턴스이고, **동점이 키의 거칠어짐이 아니라 평범한 상태에서 도달 가능한 유일한 것**이다. 함께 승인된 시퀀스들은 나란히 디코딩하므로 토큰 수를 공유하고, `PreemptionPolicy::LongestFirst`와 `RequestPriority::Normal` 둘 다 출하 기본값이다. 동일하게 설정된 워커 둘이 동일한 부하에서 서로 다른 요청을 선점했고, 운영자는 배치 상태로부터 그 선택을 재현할 수 없었다. + +## 1. 문제 + +손상된 것은 없고 정책의 명시된 의도도 위배되지 않았다. 동점인 가장 긴 시퀀스 중 무엇이든 "가장 긴 것을 축출한다"를 만족한다. 잃은 것은 **어느 사용자의 요청이 희생됐는가의 재현성**이고, 선점을 조사하는 사람이 정확히 필요로 하는 정보다. + +도달 가능성이 #1291의 네 지점과 이것을 가른다. 그쪽 키는 호출당 한 번 찍히는 `Instant`라 사실상 충돌하지 않는다. 여기 키는 자주 충돌하는 작은 정수다. + +## 2. 기술적 판단 + +### 2.1 최소 `seq_id`, 그리고 `created_at`을 기각한 이유 + +`seq_id`는 단조 증가하지만 `try_evict_for_preemption`이 victim을 **새로운, 더 큰** id로 재할당한다. 따라서 큰 id는 늦게 도착한 것이 아니라 최근에 선점된 것을 뜻한다. 최소 id 우선은 아직 안 맞은 시퀀스로 선점을 돌린다. + +운영자에게 더 의미 있는 키로 `created_at`을 검토했다가 정반대 이유로 기각했다. 이 필드는 선점을 넘어 살아남으므로, 최소 `created_at` 우선은 같은 최고령 요청을 영원히 계속 고른다. 선점당한다고 필드가 움직이지 않기 때문이다. 게다가 `Instant`라 어차피 뒤에 `seq_id`를 놓아야 한다. 두 갈래가 같은 방향으로 수렴하므로 선택이 정책에 따라 달라지지 않는다. + +### 2.2 같은 방향을 위해 서로 반대로 향하는 타이브레이크 성분 + +`max_by_key`는 마지막 최댓값을, `min_by`는 첫 번째 최솟값을 반환한다. 그래서 같은 결과를 내려면 두 갈래의 타이브레이크 성분이 서로 반대를 향해야 한다. + +- `LongestFirst`는 `(generated_tokens.len(), std::cmp::Reverse(seq_id.as_u64()))`로 키잉한다. 그 키의 최대는 가장 긴 시퀀스이고, 같은 길이에서는 `Reverse(id)`가 최대인 것, 즉 **id가 최소**인 것이다. +- `LowestPriority`는 `.then_with(|| a.seq_id.as_u64().cmp(&b.seq_id.as_u64()))`를 오름차순으로 덧붙인다. 최소는 가장 낮은 우선순위, 다음 가장 긴 것, 다음 **id 최소**다. + +두 키가 이제 전순서이므로 마지막-최댓값 규칙도 첫-최솟값 규칙도 발동할 여지가 없다. `SequenceId`는 `Ord`를 파생하지 않아 둘 다 `.as_u64()`를 거친다. #1291에서 `PromptCacheKeyDigest`가 놓았던 것과 같은 함정이다. + +### 2.3 정책 사본은 하나, 필터는 그 안에 + +기존 테스트 둘은 비공개 메서드를 호출하지 않고 선택 표현식을 인라인으로 재구현했고 이미 드리프트했다. 프로덕션에 있는 `.filter(|seq| seq.structured.is_none())` 가드가 둘 다 없었다. 그 형태로 쓴 회귀 테스트는 사본을 고정했을 것이다. + +정책은 이제 자유 함수 `select_eviction_victim_from(sequences, policy)`이고 메서드는 한 줄 호출로 줄었다. `ActiveBatch`의 메서드가 아니라 자유 함수인 이유는 `active.rs`가 현재 `PreemptionPolicy`를 전혀 모르기 때문이다. `structured.is_none()` 필터는 함수 **안으로** 옮겨서 어떤 호출자도 다시 잃을 수 없게 했고, 문서 주석에 정책은 여기서 확장하고 호출부에서는 절대 하지 말라고 적었다. + +## 3. 변경 요약 + +| 파일 | 변경 | +| --- | --- | +| `src/server/batch/scheduler.rs` | `select_eviction_victim_from` 추출, 두 갈래에 전순서 부여, 필터 내부 이동, `seq_id`를 전순서 성분으로 지목하는 주석 | +| `src/server/batch/scheduler_tests.rs` | 드리프트한 테스트 둘을 추출 함수 호출로 재작성, 신규 테스트 3건 | +| `docs/code-guidelines.md` | 인스턴스 목록을 여덟으로, "일곱"에 의존하던 산술도 함께 | + +## 4. 리뷰 지적사항 + +테스트를 수정 전에 먼저 만들었고, 수정 후 되돌려 확인하는 방식이 아니었다. 그래서 `git stash`도 파일 수술도 필요 없었다. 미수정 표현식에 대고: + +``` +LongestFirst resolved a fully tied batch to something other than seq_id 2 on 58 of 64 +freshly built batches; the tie is falling through to HashMap order + +LowestPriority resolved a batch tied on priority and length to something other than seq_id 2 +on 39 of 64 freshly built batches; the tie is falling through to HashMap order +``` + +테스트가 첫 실패에서 패닉하지 않고 64회 전체의 불일치를 누적한다. 그래서 "64 중 N"이라는 수치가 나오고 수정 전 실패율이 그대로 보인다. + +기록할 것 셋: + +가이드라인의 인스턴스 수를 여덟으로 올리자 다른 문장 셋의 산술이 틀리게 됐다. 이슈가 예상하지 못한 부분이다. 의존하는 수치를 고쳤고, 측정 문장은 "the first five fixes above (#1293 was found after the measurement ran)"로 좁혔다. #1287의 측정을 그것이 재지 않은 것까지 덮는 것처럼 조용히 재진술하지 않기 위해서다. + +오독을 막으려고 강제 조항 기록에 단서를 하나 더했다. #1293은 **실제로 `max_by_key`를 쓴다.** 따라서 후보 검사의 "0 of 8"은 그 규칙이 해당 메서드를 무시해서가 아니다. 수신자가 리터럴 `.values()`가 아니라 모듈 간 접근자(`ActiveBatch::iter_sequences`)라서 놓친 것이고, 같은 절이 #1277에 대해 이미 서술한 한계다. + +`structured.is_none()` 가드가 이제 실행 경로에 있지만 어떤 테스트도 `Some(..)`을 만들지 않는다. `StructuredOutputConstraint`에 테스트 생성자가 없고, 만들려면 실제 토크나이저와 컴파일된 문법이 필요하다. 암묵으로 두지 않고 PR 본문에 적었다. + +## 5. 검증 + +GB10(DGX Spark, CUDA sm_121, Linux aarch64)에서 실측. 게이트 시점에 브랜치가 `main`과 동기였고, 박스에 다른 cargo 프로세스 없이 돌렸다. + +- `make verify-test-cuda`: PR 스레드에 기록. +- `cargo test --profile test-fast --features cuda --lib server::batch::scheduler`: 111 통과, 7 무시, exit 0. `server::batch`: 373 통과, exit 0. 축출 필터만: 5 통과. +- `cargo fmt --all -- --check`, `cargo check --lib --tests --features cuda`, `cargo clippy --lib --tests --features cuda -- -D warnings`: 전부 exit 0. + +기존 축출 테스트 둘은 예상 victim이 바뀌지 않았다. 둘 다 동점을 만들지 않기 때문이다. + +## 6. 관련 작업 + +- #1293: 이 PR이 닫는 이슈. +- #1287과 PR #1290, 그리고 PR #1294의 정정: 이 인스턴스가 추가되는 가이드라인, 그리고 수정이 의존하는 `max_by_key` 방향 사실. +- #1291과 PR #1299: 잠재적 형제 넷. 동점이 도달 불가라 테스트에 대해 반대 판단을 했다. +- #1265와 PR #1266, #1267과 PR #1269, #1276과 PR #1281, #1277과 PR #1284, #1286과 PR #1288: 계열의 나머지. diff --git a/docs/code-guidelines.md b/docs/code-guidelines.md index 5bebc5fc..023cffa9 100644 --- a/docs/code-guidelines.md +++ b/docs/code-guidelines.md @@ -145,7 +145,7 @@ The consumer is frequently in another module from the iteration, which is why th 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. +Three of the eight 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. @@ -181,20 +181,21 @@ Validate the test by running it against the unfixed code first. If it does not f `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: +**The eight instances.** These were found in one review sweep between 2026-08-20 and 2026-08-22, across six 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. +- **#1293** (PR #1301). `BatchScheduler::select_eviction_victim` chose the preemption victim with `max_by_key` on `generated_tokens.len()` under `LongestFirst` and `min_by` on priority-then-length under `LowestPriority`, both over `ActiveBatch::iter_sequences`, which is `HashMap::values`. The only one of the eight whose tie is reachable through ordinary state rather than through a coarsening of the key: sequences admitted together decode in lockstep and therefore share a token count, and both `PreemptionPolicy::LongestFirst` and `RequestPriority::Normal` are the shipped defaults. Nothing was corrupted, since any tied-longest sequence does satisfy the policy, but two identically configured workers under identical load preempted different requests and an operator could not reproduce the choice from the batch state. Fixed by appending `seq_id`, facing opposite ways in the two arms because `max_by_key` takes the last maximum and `min_by` the first minimum. **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: +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 first five fixes above (#1293 was found after the measurement ran): -- **`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. +- **`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 8** instances above. Every reconstructed pre-fix tree produced those same four flags and nothing else, because none of the seven then known used `min_by_key` or `max_by_key`. #1293 does use `max_by_key` and was missed anyway, for the reason recorded at the end of this section. 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-8 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. diff --git a/src/server/batch/scheduler.rs b/src/server/batch/scheduler.rs index 58629173..67fcc2e0 100644 --- a/src/server/batch/scheduler.rs +++ b/src/server/batch/scheduler.rs @@ -750,6 +750,84 @@ fn lookahead_teardown_positions(next_prime_issued: bool) -> usize { if next_prime_issued { 2 } else { 1 } } +/// Pick the preemption victim for `policy` out of a candidate iterator. +/// +/// This is the single implementation of the eviction policy. +/// [`BatchScheduler::select_eviction_victim`] is a thin wrapper over it, and +/// unit tests call it directly. The indirection exists because `BatchScheduler` +/// needs a real model to construct, so a test cannot reach the method: the two +/// pre-existing eviction tests worked around that by copying the selection +/// expression inline, and both copies then drifted from production by losing +/// the `structured.is_none()` guard. Extend the policy here, never at a call +/// site. +/// +/// Sequences carrying a structured-output constraint are filtered out inside +/// this function rather than by the caller, so the exclusion cannot be lost by +/// a caller that forgets it. See [`BatchScheduler::select_eviction_victim`] for +/// why those sequences cannot be preempted. +/// +/// # Tie-break: smallest `seq_id` +/// +/// Both arms append `seq_id` so the order is total. `seq_id` is unique across +/// the batch (it is the `ActiveBatch` map key), so no two candidates can +/// compare equal and the victim can no longer be decided by `HashMap` +/// iteration order, which `RandomState` re-seeds per map instance. This is the +/// reachable case rather than a theoretical one: the primary key of the +/// `LongestFirst` arm is `generated_tokens.len()`, a small integer that +/// sequences admitted together and decoding in lockstep share routinely. +/// +/// The direction is smallest-id-wins, and it is chosen for anti-starvation +/// rather than only for totality. `SequenceId` is handed out monotonically by +/// `CachePool`, and preemption reallocates its victim under a *fresh*, higher +/// id (see `BatchScheduler::try_evict_for_preemption`), so a large id marks a +/// sequence that was preempted recently. Preferring the smallest id therefore +/// rotates preemption onto sequences that have not been hit yet; preferring the +/// largest would keep re-picking the request that was just replayed. +/// `SequenceInfo::created_at` survives preemption and would give true arrival +/// order, but for that reason it points the other way: it would repeatedly +/// select the same oldest request. It is also an `Instant`, so it would still +/// need `seq_id` behind it to be total. +/// +/// Because the two arms use opposite-facing selectors, they need +/// opposite-facing tie components to break the same direction: +/// `Iterator::max_by_key` returns the LAST maximum while `Iterator::min_by` +/// returns the FIRST minimum (`docs/code-guidelines.md`, "HashMap Iteration +/// Order"). +pub(crate) fn select_eviction_victim_from<'a>( + sequences: impl Iterator, + policy: PreemptionPolicy, +) -> Option { + let candidates = sequences.filter(|seq| seq.structured.is_none()); + match policy { + // Evict the sequence with the most generated tokens. `seq_id` is the + // component that makes this key a TOTAL order; without it, tied + // sequences resolve through `HashMap` iteration order. Do not + // simplify it away. It is wrapped in `Reverse` because `max_by_key` + // takes the LAST maximum, and reversing the id is what makes the + // SMALLEST id win, matching the `LowestPriority` arm below. + PreemptionPolicy::LongestFirst => candidates + .max_by_key(|seq| { + ( + seq.generated_tokens.len(), + std::cmp::Reverse(seq.seq_id.as_u64()), + ) + }) + .map(|seq| seq.seq_id), + // Evict the lowest-priority sequence; break ties by longest, then by + // smallest `seq_id`. As above, `seq_id` is the component that makes + // the comparator a TOTAL order and must not be simplified away. It is + // compared ascending here because `min_by` takes the FIRST minimum. + PreemptionPolicy::LowestPriority => candidates + .min_by(|a, b| { + a.priority + .cmp(&b.priority) + .then_with(|| b.generated_tokens.len().cmp(&a.generated_tokens.len())) + .then_with(|| a.seq_id.as_u64().cmp(&b.seq_id.as_u64())) + }) + .map(|seq| seq.seq_id), + } +} + impl BatchScheduler { fn release_sequence_caches(&mut self, seq_id: SequenceId) { self.model.release_sequence_state_by_id(seq_id); @@ -6813,6 +6891,10 @@ impl BatchScheduler { /// Select the eviction victim based on the configured policy. /// + /// The policy itself lives in [`select_eviction_victim_from`], which is its + /// only implementation and the entry point the unit tests use; this method + /// just supplies the batch and the configured policy. + /// /// follow-up: sequences with an attached structured-output /// constraint are excluded from the candidate set. Preemption resets /// `generated_tokens`, the streaming decoder, and the KV cache, but the @@ -6824,28 +6906,7 @@ impl BatchScheduler { /// available, `try_evict_for_preemption` falls through to its existing /// "no candidate" path and the new request stays queued. fn select_eviction_victim(&self) -> Option { - match self.preemption_policy { - PreemptionPolicy::LongestFirst => { - // Evict the sequence with the most generated tokens - self.active_batch - .iter_sequences() - .filter(|seq| seq.structured.is_none()) - .max_by_key(|seq| seq.generated_tokens.len()) - .map(|seq| seq.seq_id) - } - PreemptionPolicy::LowestPriority => { - // Evict the lowest-priority sequence; break ties by longest - self.active_batch - .iter_sequences() - .filter(|seq| seq.structured.is_none()) - .min_by(|a, b| { - a.priority - .cmp(&b.priority) - .then_with(|| b.generated_tokens.len().cmp(&a.generated_tokens.len())) - }) - .map(|seq| seq.seq_id) - } - } + select_eviction_victim_from(self.active_batch.iter_sequences(), self.preemption_policy) } // ------------------------------------------------------------------ diff --git a/src/server/batch/scheduler_tests.rs b/src/server/batch/scheduler_tests.rs index d13a0e4c..4530b15a 100644 --- a/src/server/batch/scheduler_tests.rs +++ b/src/server/batch/scheduler_tests.rs @@ -27,14 +27,15 @@ use mlxcel_core::generate::SamplingConfig; use super::{ MAX_CONSECUTIVE_EVAL_FAILURES, advance_eval_failure_count, effective_decode_storage_backend, - eval_failures_reached_limit, resolve_max_batch_prefill_tokens, vlm_prefix_sharing_allowed, + eval_failures_reached_limit, resolve_max_batch_prefill_tokens, select_eviction_victim_from, + vlm_prefix_sharing_allowed, }; use crate::server::batch::active::ActiveBatch; use crate::server::batch::queue::PrefillQueue; use crate::server::batch::sequence::{ BatchSchedulerAction, RequestPriority, SequenceInfo, SequenceState, }; -use crate::server::config::DecodeStorageBackend; +use crate::server::config::{DecodeStorageBackend, PreemptionPolicy}; use crate::server::model_provider::GenerateEvent; use crate::server::model_provider::model_worker::StreamingDecodeState; @@ -691,8 +692,6 @@ fn active_batch_iter_min_priority_empty_returns_none() { #[test] fn eviction_selects_longest_first_by_default() { - use crate::server::config::PreemptionPolicy; - let mut batch = ActiveBatch::new(4); let (mut s1, _r1) = make_test_sequence_with_priority(1, RequestPriority::Normal); @@ -706,22 +705,18 @@ fn eviction_selects_longest_first_by_default() { batch.add(s1).unwrap(); batch.add(s2).unwrap(); - // LongestFirst should pick s1 (3 tokens > 1 token) - let victim = match PreemptionPolicy::LongestFirst { - PreemptionPolicy::LongestFirst => batch - .iter_sequences() - .max_by_key(|seq| seq.generated_tokens.len()) - .map(|seq| seq.seq_id), - _ => None, - }; + // LongestFirst should pick s1 (3 tokens > 1 token). Call the production + // selector rather than restating its expression: this test and the one + // below used to carry their own copies, and both copies drifted by losing + // the `structured.is_none()` guard that production has. + let victim = + select_eviction_victim_from(batch.iter_sequences(), PreemptionPolicy::LongestFirst); assert_eq!(victim.unwrap().as_u64(), 1); } #[test] fn eviction_selects_lowest_priority_then_longest() { - use crate::server::config::PreemptionPolicy; - let mut batch = ActiveBatch::new(4); let (mut s1, _r1) = make_test_sequence_with_priority(1, RequestPriority::High); @@ -740,22 +735,152 @@ fn eviction_selects_lowest_priority_then_longest() { batch.add(s2).unwrap(); batch.add(s3).unwrap(); - // LowestPriority should pick s3 (Low + 4 tokens, longest of Low group) - let victim = match PreemptionPolicy::LowestPriority { - PreemptionPolicy::LowestPriority => batch - .iter_sequences() - .min_by(|a, b| { - a.priority - .cmp(&b.priority) - .then_with(|| b.generated_tokens.len().cmp(&a.generated_tokens.len())) - }) - .map(|seq| seq.seq_id), - _ => None, - }; + // LowestPriority should pick s3 (Low + 4 tokens, longest of Low group). + // No two candidates tie here, so the seq_id component cannot reach the + // result and the expected victim is the same as before the tie-break was + // added; the tie itself is covered by the two tests below. + let victim = + select_eviction_victim_from(batch.iter_sequences(), PreemptionPolicy::LowestPriority); assert_eq!(victim.unwrap().as_u64(), 3); } +/// Rebuild count for the eviction tie-break determinism tests. +/// +/// `RandomState` seeds each `HashMap` instance rather than the process, so the +/// `ActiveBatch` is rebuilt inside the loop; probing one batch repeatedly +/// measures nothing. The pre-fix failure rate is well under 100 percent, so a +/// single-shot version would pass most of the time and pin nothing. Matches the +/// in-tree precedent (`ORDER_ITERATIONS = 64` in `src/distributed/registry_tests.rs`). +/// See `docs/code-guidelines.md`, "HashMap Iteration Order" / "Testing the fix". +const EVICTION_TIE_ITERATIONS: usize = 64; + +/// The seq_id both tie-break tests expect to win: the smallest among the tied +/// candidates. See `select_eviction_victim_from` for why the smallest id is the +/// chosen direction. +const EXPECTED_TIE_VICTIM: u64 = 2; + +/// Build a fresh `ActiveBatch` holding one sequence per +/// `(seq_id, priority, generated_token_count)` triple. +/// +/// The returned receivers are the sequences' response channels; the caller +/// holds them so the batch's senders stay connected for its lifetime. +fn eviction_tie_batch( + specs: &[(u64, RequestPriority, usize)], +) -> (ActiveBatch, Vec>) { + let mut batch = ActiveBatch::new(specs.len()); + let mut receivers = Vec::with_capacity(specs.len()); + + for &(id, priority, generated) in specs { + let (mut seq, rx) = make_test_sequence_with_priority(id, priority); + seq.state = SequenceState::Decoding; + seq.generated_tokens = vec![7; generated]; + batch.add(seq).unwrap(); + receivers.push(rx); + } + + (batch, receivers) +} + +#[test] +fn eviction_longest_first_tie_resolves_to_smallest_seq_id() { + // Every candidate carries the same generated-token count, which is the + // reachable case: sequences admitted together decode in lockstep, one token + // each per step. Only the seq_id component of the key can separate them, + // and it has to separate them the same way on every batch. Without it the + // arm falls through to `max_by_key`'s last-maximum rule applied to + // `HashMap` order, which `RandomState` re-seeds for every new batch. + // + // The ids are inserted out of order on purpose, so a selector that returns + // the first- or last-inserted sequence cannot pass by coincidence. + let specs = [ + (9u64, RequestPriority::Normal, 4usize), + (2, RequestPriority::Normal, 4), + (14, RequestPriority::Normal, 4), + (5, RequestPriority::Normal, 4), + ]; + + // Every iteration is recorded rather than asserted, so a failure reports how + // many of the freshly built batches disagreed instead of stopping at the + // first one. The pre-fix rate is well under 100 percent, so the count is + // the useful number. + let mut mismatches: Vec<(usize, Option)> = Vec::new(); + + for iteration in 0..EVICTION_TIE_ITERATIONS { + let (batch, _receivers) = eviction_tie_batch(&specs); + + let victim = + select_eviction_victim_from(batch.iter_sequences(), PreemptionPolicy::LongestFirst) + .map(|id| id.as_u64()); + + if victim != Some(EXPECTED_TIE_VICTIM) { + mismatches.push((iteration, victim)); + } + } + + assert!( + mismatches.is_empty(), + "LongestFirst resolved a fully tied batch to something other than seq_id \ + {EXPECTED_TIE_VICTIM} on {} of {EVICTION_TIE_ITERATIONS} freshly built batches \ + (first {:?}); the tie is falling through to HashMap order", + mismatches.len(), + mismatches.first(), + ); +} + +#[test] +fn eviction_lowest_priority_tie_resolves_to_smallest_seq_id() { + // The `LowestPriority` arm needs a tie on BOTH axes to reach hash order, so + // the three `Low` candidates share a generated-token count as well. The + // `High` and `Normal` entries are longer than all of them and must still + // lose on priority, which pins that the appended seq_id component did not + // perturb the primary keys. + let specs = [ + (9u64, RequestPriority::Low, 4usize), + (2, RequestPriority::Low, 4), + (14, RequestPriority::Low, 4), + (5, RequestPriority::High, 9), + (11, RequestPriority::Normal, 12), + ]; + + let mut mismatches: Vec<(usize, Option)> = Vec::new(); + + for iteration in 0..EVICTION_TIE_ITERATIONS { + let (batch, _receivers) = eviction_tie_batch(&specs); + + let victim = + select_eviction_victim_from(batch.iter_sequences(), PreemptionPolicy::LowestPriority) + .map(|id| id.as_u64()); + + if victim != Some(EXPECTED_TIE_VICTIM) { + mismatches.push((iteration, victim)); + } + } + + assert!( + mismatches.is_empty(), + "LowestPriority resolved a batch tied on priority and length to something other than \ + seq_id {EXPECTED_TIE_VICTIM} on {} of {EVICTION_TIE_ITERATIONS} freshly built batches \ + (first {:?}); the tie is falling through to HashMap order", + mismatches.len(), + mismatches.first(), + ); +} + +#[test] +fn eviction_returns_none_for_an_empty_batch() { + let batch = ActiveBatch::new(4); + + assert_eq!( + select_eviction_victim_from(batch.iter_sequences(), PreemptionPolicy::LongestFirst), + None + ); + assert_eq!( + select_eviction_victim_from(batch.iter_sequences(), PreemptionPolicy::LowestPriority), + None + ); +} + #[test] fn preemption_disabled_by_default_never_triggers() { // When enable_preemption is false, should_preempt should never