diff --git a/TECHNICAL_REPORTS/1288-eviction-sort-total-order-20260822.en.md b/TECHNICAL_REPORTS/1288-eviction-sort-total-order-20260822.en.md new file mode 100644 index 000000000..19bfe66c9 --- /dev/null +++ b/TECHNICAL_REPORTS/1288-eviction-sort-total-order-20260822.en.md @@ -0,0 +1,80 @@ +# Technical Report: PR #1288 - A total order for eviction candidate sorts + +## Executive Summary + +Three eviction paths built a candidate list from a `HashMap`, sorted it, and consumed a prefix. Sorting looks like it makes the outcome deterministic and does not: `slice::sort_by_key` is a **stable** sort, so entries comparing equal keep their input order, and the input order is `HashMap` iteration order. Among candidates tied on the sort key, which ones landed in the consumed prefix varied per run. + +This is the subtle member of the defect class fixed by #1265, #1267, #1276 and #1277. Those four had no sort at all, so the defect was visible on inspection. These three do sort, which is precisely why they read as safe. + +## 1. Problem Statement + +- `src/distributed/tensor_parallel/cache_manager.rs`: `select_eviction_candidates` collects `allocations.values()` and sorts on `last_accessed` (LRU) or `current_offset` (LeastTokens). Its caller `check_pressure` iterates and breaks as soon as `projected_used <= target_bytes`, making it a prefix consumer. Ties decide which sequences actually lose their cache, and under LeastTokens a shared token count is an ordinary occurrence rather than a corner case. +- `src/distributed/pipeline/cache_manager.rs`: the same shape across three `PreemptionPolicy` arms, published as `PreemptionSignal.sequence_ids` and documented as eviction priority order. No prefix consumer exists in-tree today, so what was broken was the published contract rather than observed behavior. +- `src/distributed/request_tracker.rs`: `evict_if_needed` collects terminal requests, sorts on `created_at`, then takes an explicit prefix with `.take(to_remove)`. + +Nothing corrupts and nothing crashes: the eviction relieves the pressure it set out to relieve and the counts are right. What was lost is reproducibility of **which** sequence was sacrificed, which is exactly what an operator needs when a request is preempted and they are working out why. + +## 2. Technical Decisions + +### 2.1 Make the sort key a total order + +Each sort now includes the unique id as a tiebreaker, so no two elements compare equal and stability stops mattering. Both the comparator form and the tuple-key form are correct; the request tracker uses the comparator form specifically so its `String` key is not cloned into a tuple on every comparison, while the two cache managers use the tuple form over `Copy` fields. + +`sort_unstable_by` was explicitly not the fix. It would replace one arbitrary tie order with another rather than remove the arbitrariness. + +### 2.2 Pin the pipeline contract at its boundary anyway + +The issue required an end-to-end assertion only for the tensor-parallel site, since it is the one with a live prefix consumer. The pipeline site got one too, asserting on the published `PreemptionSignal.sequence_ids` rather than only on the private helper, so the ordering is already pinned at the boundary a future consumer will read. + +## 3. Change Summary + +| File | Change | +| --- | --- | +| `src/distributed/tensor_parallel/cache_manager.rs` | Both policy arms sort on `(key, sequence_id)` | +| `src/distributed/pipeline/cache_manager.rs` | All three policy arms sort on `(key, sequence_id)`; the garbled `PreemptionSignal.sequence_ids` doc corrected to `first = evict first` | +| `src/distributed/request_tracker.rs` | `created_at` sort tie-breaks on the request key via a comparator | +| the three matching `*_tests.rs` | 8 new tests | + +No public setter and no `#[cfg(test)]` accessor was added. All three test files attach via `#[path]` inside the module under test, so the private helpers and the private state were already reachable, and widening the API to make testing easier would have been the wrong trade. + +## 4. Review Findings + +The tests were written first and run against the still-unfixed sources, rather than the fix being written and then reverted to check. Three ways to write a vacuous test here were identified in advance and all three turned out to matter: + +1. A reused map passes without the fix, because `RandomState` randomizes per `HashMap` instance. Each test builds a fresh map per iteration and loops 32 or 64 times. +2. Test data with distinct sort keys passes without the fix, because distinct keys already give a total order. Every test constructs a real tie. +3. `Instant::now()` resolves to nanoseconds on this platform and never collides on its own, so the LRU and `created_at` tests write equal `Instant` values in deliberately. + +Pre-fix output, where `left` is raw hash order passed through by the stable sort: + +``` +---- eviction_candidates_lru_tie_break_is_deterministic ---- + left: [5, 3, 1, 4, 7, 2, 8, 6] + right: [1, 2, 3, 4, 5, 6, 7, 8] +---- check_pressure_prefix_is_deterministic_under_ties ---- + left: [15, 11, 14] + right: [11, 12, 13] +---- eviction_tie_break_is_deterministic ---- + left: ["req-0", "req-1", "req-4", "req-5"] + right: ["req-2", "req-3", "req-4", "req-5"] +``` + +One trap not named in the issue, recorded here because it will catch the next person: `evict_if_needed` runs inside `submit_with_id` before the insert, so a test that completes each request as it submits triggers an unintended eviction on the sixth submit. The test submits all six, then completes them, then calls `evict_if_needed` directly. + +## 5. Validation + +Measured on GB10 (DGX Spark, CUDA sm_121, Linux aarch64), rebased onto `main` at `8fcc01f2` before gating so the gate ran on the tree that merges. + +- `make verify-test-cuda`: **8246 passed, 0 failed, 311 ignored**, 101 suites, exit 0. That is +8 against `main` (8238), and the diff adds exactly 8 `#[test]` functions and removes none. +- The three module filters: 48, 47 and 21 passed, exit 0 each, and green again across five further separate processes, which reseeds `RandomState` per run. +- `cargo fmt --all -- --check`: exit 0. `cargo clippy --lib --tests --features cuda -- -D warnings`: exit 0 (the `err_expect` that previously reddened this was fixed by #1283 / PR #1285). + +The gate log was scanned for process-level aborts as well as failing tests. A per-suite tally cannot see a teardown crash: an earlier run of this same gate reported 0 failed and still exited 101, because every test passed and the process then aborted with `Destroy(handle_) failed: driver shutting down` while a second cargo process saturated the GPU. Run alone, both this gate and #1283's are clean. + +## 6. Related Work + +- #1286: the issue this closes, filed from a sweep of `src/distributed/` during #1277. +- #1265 and PR #1266, #1267 and PR #1269, #1276 and PR #1281, #1277 and PR #1284: the sibling instances. +- #1287: the proposal to record this class in `docs/code-guidelines.md` and decide whether a static check can catch it. + +Two near-misses were checked and left alone deliberately. `src/distributed/routing.rs` stable-sorts and takes `online[0]`, and an idle cluster ties on every component, but PR #1284 already cured it upstream by giving `Registry::all_nodes` a defined order, so that path silently depends on the accessor fix. And `nodes_at_stage` / `nodes_at_rank` sort on `Option` keys through `unwrap_or(u32::MAX)`, which looks tie-able, but `ClusterConfig::validate` rejects a PPTP node with either field unset before it can reach the registry, and neither accessor has a non-test consumer. diff --git a/TECHNICAL_REPORTS/1288-eviction-sort-total-order-20260822.ko.md b/TECHNICAL_REPORTS/1288-eviction-sort-total-order-20260822.ko.md new file mode 100644 index 000000000..94929789f --- /dev/null +++ b/TECHNICAL_REPORTS/1288-eviction-sort-total-order-20260822.ko.md @@ -0,0 +1,80 @@ +# 기술 보고서: PR #1288 - 축출 후보 정렬에 전순서 부여 + +## 요약 + +축출 경로 셋이 `HashMap`에서 후보 목록을 만들고, 정렬한 뒤, 접두사를 소비하고 있었다. 정렬은 결과를 결정적으로 만드는 것처럼 보이지만 아니다. `slice::sort_by_key`는 **안정 정렬**이라 같다고 비교되는 원소가 입력 순서를 유지하고, 그 입력 순서가 곧 `HashMap` 순회 순서다. 정렬 키가 동점인 후보들 중 어느 것이 소비되는 접두사에 들어가는지가 실행마다 달라졌다. + +#1265, #1267, #1276, #1277이 고친 결함 계열의 미묘한 쪽이다. 그 넷은 정렬이 아예 없어서 눈으로 보였다. 이 셋은 **정렬을 하고 있어서** 안전해 보였다. + +## 1. 문제 + +- `src/distributed/tensor_parallel/cache_manager.rs`: `select_eviction_candidates`가 `allocations.values()`를 모아 `last_accessed`(LRU) 또는 `current_offset`(LeastTokens)로 정렬한다. 호출자 `check_pressure`는 `projected_used <= target_bytes`가 되는 즉시 `break`하므로 접두사 소비자다. 동점이 어느 시퀀스가 실제로 캐시를 잃는지를 정하고, LeastTokens에서는 토큰 수가 겹치는 것이 예외가 아니라 일상이다. +- `src/distributed/pipeline/cache_manager.rs`: `PreemptionPolicy` 세 갈래에 같은 형태. 결과가 `PreemptionSignal.sequence_ids`로 발행되고 축출 우선순위 순서로 문서화돼 있다. 트리 안에 접두사 소비자가 아직 없으므로 깨져 있던 것은 관측된 동작이 아니라 발행된 계약이다. +- `src/distributed/request_tracker.rs`: `evict_if_needed`가 종료된 요청을 모아 `created_at`으로 정렬한 뒤 `.take(to_remove)`로 명시적 접두사를 취한다. + +손상되는 것도, 죽는 것도 없다. 축출은 해소하려던 압박을 해소하고 개수도 맞다. 잃은 것은 **어느 시퀀스가 희생됐는가의 재현성**이고, 그것이 바로 요청이 선점됐을 때 운영자가 이유를 따지려고 필요로 하는 정보다. + +## 2. 기술적 판단 + +### 2.1 정렬 키를 전순서로 + +각 정렬에 고유 id를 타이브레이커로 넣어 두 원소가 같다고 비교되는 일이 없게 했고, 그러면 안정성 여부가 무의미해진다. 비교자 형태와 튜플 키 형태 둘 다 옳다. request tracker는 `String` 키가 비교마다 튜플로 클론되지 않도록 비교자 형태를 쓰고, 두 캐시 매니저는 `Copy` 필드라 튜플 형태를 쓴다. + +`sort_unstable_by`는 수정이 아니라고 명시했다. 임의 순서를 다른 임의 순서로 바꿀 뿐 임의성을 없애지 않는다. + +### 2.2 파이프라인 계약은 경계에서 고정 + +이슈는 종단 단언을 tensor-parallel 쪽에만 요구했다. 거기만 실제 접두사 소비자가 있기 때문이다. 파이프라인 쪽에도 하나 넣어, 비공개 헬퍼가 아니라 발행되는 `PreemptionSignal.sequence_ids`에 단언을 걸었다. 소비자가 생기는 날 그 경계에서 이미 순서가 고정돼 있다. + +## 3. 변경 요약 + +| 파일 | 변경 | +| --- | --- | +| `src/distributed/tensor_parallel/cache_manager.rs` | 두 정책 갈래를 `(키, sequence_id)`로 정렬 | +| `src/distributed/pipeline/cache_manager.rs` | 세 정책 갈래를 `(키, sequence_id)`로 정렬, 뒤엉킨 `PreemptionSignal.sequence_ids` 문서를 `first = evict first`로 정정 | +| `src/distributed/request_tracker.rs` | `created_at` 정렬이 요청 키로 타이브레이크(비교자 형태) | +| 대응 `*_tests.rs` 3개 | 신규 테스트 8건 | + +공개 setter도 `#[cfg(test)]` 접근자도 추가하지 않았다. 세 테스트 파일 모두 대상 모듈 안에서 `#[path]`로 붙으므로 비공개 헬퍼와 비공개 상태에 이미 닿을 수 있었고, 테스트 편의를 위해 API를 넓히는 것은 잘못된 교환이었을 것이다. + +## 4. 리뷰 지적사항 + +테스트를 먼저 쓰고 아직 수정되지 않은 소스에 돌렸다. 수정을 쓴 뒤 되돌려 확인하는 방식이 아니다. 여기서 공허한 테스트를 쓰는 방법 셋을 미리 식별했고 셋 다 실제로 중요했다. + +1. 맵을 재사용하면 수정 없이도 통과한다. `RandomState`가 `HashMap` 인스턴스마다 무작위화하기 때문이다. 각 테스트는 반복마다 맵을 새로 만들고 32회 또는 64회 돈다. +2. 정렬 키가 서로 다른 데이터는 수정 없이도 통과한다. 서로 다른 키는 이미 전순서이기 때문이다. 모든 테스트가 실제 동점을 만든다. +3. `Instant::now()`는 이 플랫폼에서 나노초 해상도라 스스로 충돌하지 않는다. LRU와 `created_at` 테스트는 같은 `Instant` 값을 의도적으로 써 넣는다. + +수정 전 출력. `left`가 안정 정렬이 그대로 통과시킨 생 해시 순서다. + +``` +---- eviction_candidates_lru_tie_break_is_deterministic ---- + left: [5, 3, 1, 4, 7, 2, 8, 6] + right: [1, 2, 3, 4, 5, 6, 7, 8] +---- check_pressure_prefix_is_deterministic_under_ties ---- + left: [15, 11, 14] + right: [11, 12, 13] +---- eviction_tie_break_is_deterministic ---- + left: ["req-0", "req-1", "req-4", "req-5"] + right: ["req-2", "req-3", "req-4", "req-5"] +``` + +이슈에 없던 함정 하나를 기록해 둔다. 다음 사람이 걸릴 것이기 때문이다. `evict_if_needed`는 `submit_with_id` 안에서 insert **전에** 돌기 때문에, 제출과 동시에 완료 처리하는 테스트는 여섯 번째 제출에서 의도치 않은 축출을 일으킨다. 테스트는 여섯 개를 모두 제출하고, 모두 완료한 뒤, `evict_if_needed`를 직접 호출한다. + +## 5. 검증 + +GB10(DGX Spark, CUDA sm_121, Linux aarch64)에서 실측. 머지될 트리를 검증하려고 게이트 전에 `main`(`8fcc01f2`) 위로 리베이스했다. + +- `make verify-test-cuda`: **8246 통과, 0 실패, 311 무시**, 101 스위트, exit 0. `main`(8238) 대비 +8이고 diff가 `#[test]`를 정확히 8개 추가하고 하나도 제거하지 않는다. +- 세 모듈 필터: 각각 48, 47, 21 통과, exit 0. 별도 프로세스 5회 추가 실행에서도 green이며, 이는 실행마다 `RandomState`를 다시 시드한다. +- `cargo fmt --all -- --check`: exit 0. `cargo clippy --lib --tests --features cuda -- -D warnings`: exit 0 (전에 이걸 red로 만들던 `err_expect`는 #1283 / PR #1285가 수정). + +게이트 로그를 실패 테스트뿐 아니라 **프로세스 수준 abort**까지 훑었다. 스위트별 집계는 teardown 크래시를 볼 수 없다. 같은 게이트의 앞선 실행이 0 실패를 보고하면서도 101로 종료했는데, 모든 테스트가 통과한 뒤 다른 cargo 프로세스가 GPU를 포화시킨 상태에서 `Destroy(handle_) failed: driver shutting down`으로 abort했기 때문이다. 단독 실행하면 이 게이트도 #1283의 게이트도 깨끗하다. + +## 6. 관련 작업 + +- #1286: 이 PR이 닫는 이슈. #1277 작업 중 `src/distributed/` 스윕에서 나왔다. +- #1265와 PR #1266, #1267과 PR #1269, #1276과 PR #1281, #1277과 PR #1284: 형제 인스턴스들. +- #1287: 이 계열을 `docs/code-guidelines.md`에 기록하고 정적 검사 가능성을 판단하자는 제안. + +의도적으로 손대지 않은 근접 사례 둘을 확인했다. `src/distributed/routing.rs`는 안정 정렬 후 `online[0]`을 취하고 유휴 클러스터는 모든 성분이 동점이지만, PR #1284가 `Registry::all_nodes`에 정의된 순서를 주면서 상류에서 이미 치유했다. 그 경로는 접근자 수정에 조용히 의존한다. 그리고 `nodes_at_stage` / `nodes_at_rank`는 `Option` 키를 `unwrap_or(u32::MAX)`로 정렬해 동점이 날 것처럼 보이지만, `ClusterConfig::validate`가 두 필드 중 하나라도 비어 있는 PPTP 노드를 레지스트리에 닿기 전에 거부하고, 두 접근자 모두 비테스트 소비자가 없다. diff --git a/src/distributed/pipeline/cache_manager.rs b/src/distributed/pipeline/cache_manager.rs index b6d32ea97..0da923c25 100644 --- a/src/distributed/pipeline/cache_manager.rs +++ b/src/distributed/pipeline/cache_manager.rs @@ -351,7 +351,7 @@ impl fmt::Display for EvictionEvent { /// Signal emitted when memory pressure requires preemption. #[derive(Debug, Clone)] pub struct PreemptionSignal { - /// Sequences recommended for eviction, in priority order (evict first last). + /// Sequences recommended for eviction, in priority order (first = evict first). pub sequence_ids: Vec, /// Stage that detected the pressure. pub source_stage: u32, @@ -696,15 +696,20 @@ impl PipelineCacheManager { fn select_eviction_candidates(&self) -> Vec { let mut entries: Vec<_> = self.allocations.values().collect(); + // The `sequence_id` component is what makes each sort key a TOTAL + // order, and it must stay. `sort_by_key` is stable and `entries` comes + // out of a `HashMap` whose iteration order `RandomState` randomizes per + // instance, so without the tie-break the priority order published in + // `PreemptionSignal.sequence_ids` is not reproducible across runs. match self.preemption_policy { PreemptionPolicy::LRU => { - entries.sort_by_key(|a| a.last_accessed); + entries.sort_by_key(|a| (a.last_accessed, a.sequence_id)); } PreemptionPolicy::Shortest => { - entries.sort_by_key(|a| a.current_offset); + entries.sort_by_key(|a| (a.current_offset, a.sequence_id)); } PreemptionPolicy::Longest => { - entries.sort_by_key(|a| std::cmp::Reverse(a.current_offset)); + entries.sort_by_key(|a| (std::cmp::Reverse(a.current_offset), a.sequence_id)); } } diff --git a/src/distributed/pipeline/cache_manager_tests.rs b/src/distributed/pipeline/cache_manager_tests.rs index 8ffa8e847..542eb3e01 100644 --- a/src/distributed/pipeline/cache_manager_tests.rs +++ b/src/distributed/pipeline/cache_manager_tests.rs @@ -300,6 +300,87 @@ fn eviction_candidates_longest() { assert_eq!(candidates[2], 2); } +// --- Eviction tie-break determinism (issue #1286) --- + +/// Number of freshly built managers each determinism test iterates over. +/// +/// `RandomState` seeds every `HashMap` instance separately, not merely once +/// per process, so probing a single map twice proves nothing. Rebuilding the +/// manager on every iteration is what exercises a different hash iteration +/// order each time. +const DETERMINISM_ITERATIONS: usize = 32; + +/// Helper: build a manager holding eight equally sized allocations under the +/// given preemption policy, with sequence IDs inserted out of order. +fn tied_manager(policy: PreemptionPolicy) -> PipelineCacheManager { + let mut cfg = test_config(0, 0..1); + cfg.memory_budget_bytes = 1_000_000; + cfg.max_sequences = 10; + let mut mgr = PipelineCacheManager::new(cfg) + .unwrap() + .with_preemption_policy(policy); + + for id in [7, 3, 8, 1, 6, 2, 5, 4] { + // Equal prompt lengths, so `current_offset` ties across all eight. + let req = CacheAdmissionRequest::new(id, 10); + assert_eq!(mgr.request_admission(&req), AdmissionDecision::Admitted); + } + mgr +} + +#[test] +fn eviction_candidates_lru_tie_break_is_deterministic() { + let expected: Vec = vec![1, 2, 3, 4, 5, 6, 7, 8]; + + for _ in 0..DETERMINISM_ITERATIONS { + let mut mgr = tied_manager(PreemptionPolicy::LRU); + // `Instant::now()` has nanosecond resolution on Linux and will never + // tie naturally, so write one shared timestamp into every allocation. + let tied = Instant::now(); + for alloc in mgr.allocations.values_mut() { + alloc.last_accessed = tied; + } + + assert_eq!(mgr.select_eviction_candidates(), expected); + } +} + +#[test] +fn eviction_candidates_shortest_tie_break_is_deterministic() { + let expected: Vec = vec![1, 2, 3, 4, 5, 6, 7, 8]; + + for _ in 0..DETERMINISM_ITERATIONS { + let mgr = tied_manager(PreemptionPolicy::Shortest); + assert_eq!(mgr.select_eviction_candidates(), expected); + } +} + +#[test] +fn eviction_candidates_longest_tie_break_is_deterministic() { + let expected: Vec = vec![1, 2, 3, 4, 5, 6, 7, 8]; + + for _ in 0..DETERMINISM_ITERATIONS { + let mgr = tied_manager(PreemptionPolicy::Longest); + assert_eq!(mgr.select_eviction_candidates(), expected); + } +} + +#[test] +fn preemption_signal_order_is_deterministic_under_ties() { + let expected: Vec = vec![1, 2, 3, 4, 5, 6, 7, 8]; + + for _ in 0..DETERMINISM_ITERATIONS { + let mut mgr = tied_manager(PreemptionPolicy::Shortest); + // Eight 10-token allocations use 81_920 B; drop the budget so the + // manager is over its 0.8 threshold and publishes a signal. + mgr.config.memory_budget_bytes = 100_000; + let signal = mgr + .check_memory_pressure() + .expect("81_920 B of 100_000 B must exceed the 0.8 threshold"); + assert_eq!(signal.sequence_ids, expected); + } +} + // --- Metadata sync tests --- #[test] diff --git a/src/distributed/request_tracker.rs b/src/distributed/request_tracker.rs index 9345408f9..46210e8b6 100644 --- a/src/distributed/request_tracker.rs +++ b/src/distributed/request_tracker.rs @@ -338,7 +338,14 @@ impl RequestTracker { .map(|(k, l)| (k.clone(), l.created_at)) .collect(); - completed.sort_by_key(|(_, t)| *t); + // The key component is what makes the sort key a TOTAL order, and it + // must stay. `sort_by_key` is stable, `completed` comes out of a + // `HashMap` whose iteration order `RandomState` randomizes per + // instance, and the `take(to_remove)` below consumes only a prefix. + // Without the tie-break, requests sharing a `created_at` tick keep hash + // order and which of them is dropped changes from run to run. The + // comparator form avoids cloning the key that `sort_by_key` would need. + completed.sort_by(|(key_a, t_a), (key_b, t_b)| t_a.cmp(t_b).then_with(|| key_a.cmp(key_b))); // Remove oldest completed until we are under the limit. let to_remove = inner.requests.len().saturating_sub(self.config.max_tracked) + 1; diff --git a/src/distributed/request_tracker_tests.rs b/src/distributed/request_tracker_tests.rs index 44e061334..863f4178f 100644 --- a/src/distributed/request_tracker_tests.rs +++ b/src/distributed/request_tracker_tests.rs @@ -175,6 +175,74 @@ fn eviction_removes_oldest_completed() { assert!(tracker.tracked_count() <= 4); } +/// Number of freshly built trackers the determinism test iterates over. +/// +/// `RandomState` seeds every `HashMap` instance separately, not merely once +/// per process, so probing a single tracker twice proves nothing. Rebuilding +/// the tracker on every iteration is what exercises a different hash +/// iteration order each time. +const DETERMINISM_ITERATIONS: usize = 32; + +/// Issue #1286: requests that share a `created_at` tick must be evicted in a +/// defined order rather than in `HashMap` iteration order. +#[test] +fn eviction_tie_break_is_deterministic() { + let keys: Vec = ["req-4", "req-1", "req-5", "req-0", "req-3", "req-2"] + .iter() + .map(|k| (*k).to_string()) + .collect(); + // Six tracked against max_tracked = 5 means to_remove = 2, and every + // entry ties on created_at, so the two lexicographically smallest keys go. + let expected_remaining: Vec = ["req-2", "req-3", "req-4", "req-5"] + .iter() + .map(|k| (*k).to_string()) + .collect(); + + for _ in 0..DETERMINISM_ITERATIONS { + let tracker = RequestTracker::new(RequestTrackerConfig { max_tracked: 5 }); + + // Submit all six before completing any, so the eviction that runs + // inside submit_with_id finds no terminal entries to drop. + for key in &keys { + tracker.submit_with_id(RequestId::from_string(key.clone()).unwrap()); + } + for key in &keys { + let id = RequestId::from_string(key.clone()).unwrap(); + assert!(tracker.transition(&id, RequestState::Routing)); + assert!(tracker.transition( + &id, + RequestState::Processing { + node_id: "node-0".to_string(), + }, + )); + assert!(tracker.transition(&id, RequestState::Completed)); + } + + { + let mut inner = tracker.inner.write().expect("tracker lock poisoned"); + // `Instant::now()` has nanosecond resolution on Linux and will + // never tie naturally, so write one shared timestamp into every + // lifecycle. + let tied = Instant::now(); + for lifecycle in inner.requests.values_mut() { + lifecycle.created_at = tied; + } + tracker.evict_if_needed(&mut inner); + } + + let mut remaining: Vec = tracker + .inner + .read() + .expect("tracker lock poisoned") + .requests + .keys() + .cloned() + .collect(); + remaining.sort(); + assert_eq!(remaining, expected_remaining); + } +} + #[test] fn remove_request() { let tracker = RequestTracker::new(RequestTrackerConfig::default()); diff --git a/src/distributed/tensor_parallel/cache_manager.rs b/src/distributed/tensor_parallel/cache_manager.rs index 96c4c1700..b3c5bace4 100644 --- a/src/distributed/tensor_parallel/cache_manager.rs +++ b/src/distributed/tensor_parallel/cache_manager.rs @@ -710,12 +710,19 @@ impl TPCacheManager { fn select_eviction_candidates(&self) -> Vec { let mut entries: Vec<_> = self.allocations.values().collect(); + // The `sequence_id` component is what makes each sort key a TOTAL + // order, and it must stay. `sort_by_key` is stable, `entries` comes out + // of a `HashMap` whose iteration order `RandomState` randomizes per + // instance, and `check_pressure` consumes only a prefix of this list. + // Without the tie-break, allocations that tie on the policy key keep + // hash order and which of them actually loses its cache changes from + // run to run. match self.eviction_policy { EvictionPolicy::LRU => { - entries.sort_by_key(|a| a.last_accessed); + entries.sort_by_key(|a| (a.last_accessed, a.sequence_id)); } EvictionPolicy::LeastTokens => { - entries.sort_by_key(|a| a.current_offset); + entries.sort_by_key(|a| (a.current_offset, a.sequence_id)); } } diff --git a/src/distributed/tensor_parallel/cache_manager_tests.rs b/src/distributed/tensor_parallel/cache_manager_tests.rs index 480b2d9d1..c79f10c67 100644 --- a/src/distributed/tensor_parallel/cache_manager_tests.rs +++ b/src/distributed/tensor_parallel/cache_manager_tests.rs @@ -395,6 +395,105 @@ fn eviction_candidates_least_tokens() { assert_eq!(candidates[2], 1); } +// --- Eviction tie-break determinism (issue #1286) --- + +/// Number of freshly built managers each determinism test iterates over. +/// +/// `RandomState` seeds every `HashMap` instance separately, not merely once +/// per process, so probing a single map twice proves nothing. Rebuilding the +/// manager on every iteration is what exercises a different hash iteration +/// order each time. +const DETERMINISM_ITERATIONS: usize = 32; + +/// Helper: a tight-budget config where six equally sized allocations put the +/// manager over the pressure threshold and a three-sequence prefix brings it +/// back under the target. +fn tied_pressure_config() -> TPCacheConfig { + let mut cfg = mha_config(0, 4); + cfg.memory_budget_bytes = 10_000_000; + cfg.pressure_threshold = 0.5; + cfg +} + +#[test] +fn eviction_candidates_lru_tie_break_is_deterministic() { + let ids: [SequenceId; 8] = [7, 3, 8, 1, 6, 2, 5, 4]; + let expected: Vec = vec![1, 2, 3, 4, 5, 6, 7, 8]; + + for _ in 0..DETERMINISM_ITERATIONS { + let mut mgr = TPCacheManager::new(mha_config(0, 4)).unwrap(); + for &id in &ids { + mgr.allocate_cache(id, 1).unwrap(); + } + // `Instant::now()` has nanosecond resolution on Linux and will never + // tie naturally, so write one shared timestamp into every allocation. + let tied = Instant::now(); + for alloc in mgr.allocations.values_mut() { + alloc.last_accessed = tied; + } + + assert_eq!(mgr.select_eviction_candidates(), expected); + } +} + +#[test] +fn eviction_candidates_least_tokens_tie_break_is_deterministic() { + let ids: [SequenceId; 8] = [7, 3, 8, 1, 6, 2, 5, 4]; + let expected: Vec = vec![1, 2, 3, 4, 5, 6, 7, 8]; + + for _ in 0..DETERMINISM_ITERATIONS { + let mut mgr = TPCacheManager::new(mha_config(0, 4)) + .unwrap() + .with_eviction_policy(EvictionPolicy::LeastTokens); + // Equal token counts, so `current_offset` ties across all eight. + for &id in &ids { + mgr.allocate_cache(id, 10).unwrap(); + } + + assert_eq!(mgr.select_eviction_candidates(), expected); + } +} + +#[test] +fn check_pressure_prefix_is_deterministic_under_ties() { + let ids: [SequenceId; 6] = [14, 11, 16, 12, 15, 13]; + // Six 10-token allocations use 7_864_320 B against a 10 MB budget with a + // 0.5 threshold, so `check_pressure` consumes a three-candidate prefix to + // get projected usage under the 5_000_000 B target. Every candidate ties, + // so the prefix is exactly the three lowest sequence IDs. + let expected: Vec = vec![11, 12, 13]; + + for _ in 0..DETERMINISM_ITERATIONS { + // LeastTokens: identical token counts tie `current_offset`. + let mut mgr = TPCacheManager::new(tied_pressure_config()) + .unwrap() + .with_eviction_policy(EvictionPolicy::LeastTokens); + for &id in &ids { + mgr.allocate_cache(id, 10).unwrap(); + } + let signal = mgr + .check_pressure() + .expect("six 10-token allocations must exceed the 0.5 threshold"); + assert_eq!(signal.sequence_ids, expected); + + // LRU: the same allocations with one shared `last_accessed`. + let mut mgr = TPCacheManager::new(tied_pressure_config()) + .unwrap() + .with_eviction_policy(EvictionPolicy::LRU); + for &id in &ids { + mgr.allocate_cache(id, 10).unwrap(); + } + let tied = Instant::now(); + for alloc in mgr.allocations.values_mut() { + alloc.last_accessed = tied; + } + let signal = mgr + .check_pressure() + .expect("six 10-token allocations must exceed the 0.5 threshold"); + assert_eq!(signal.sequence_ids, expected); + } +} + // --- Coordinated eviction --- #[test]