diff --git a/memory/mission/wave65-CR-alloc-cache-eviction-vs-baked-addresses.md b/memory/mission/wave65-CR-alloc-cache-eviction-vs-baked-addresses.md new file mode 100644 index 000000000..5eb63dbae --- /dev/null +++ b/memory/mission/wave65-CR-alloc-cache-eviction-vs-baked-addresses.md @@ -0,0 +1,295 @@ +# wave65-CR — LRU eviction cannot see which addresses a live graph baked + +Parent system: ArcInfer / ArcGraph, in the forked candle CUDA caching allocator +(`aeonmindai/candle`, `candle-core/src/cuda_backend/device.rs`). + +Status: **surfaced, not fixed.** Two hazards, both silent, both filed here +because they have no owner otherwise. + +Found while merging the two diverged candle lines — master's `89ab14ef` +("eviction must not run inside the capture window") and the capture branch's +`9211966e` ("a captured device->host copy writes into a FREED host Vec") — into +`9586979d` on `aeonmindai/candle@arcgraph/bounded-alloc-plus-prewarm`. + +**No hardware leg was taken. Nothing here is measured on a GPU.** Every claim +below is derived from source. arc line numbers are read at `bb30559`; candle +claims are read at `9586979d`. Where a number came from someone else's recorded +measurement it is attributed inline and marked as theirs. + +> **candle is cited by symbol, never by line — deliberately.** candle is not +> vendored in this repo, so a `device.rs:N` citation either fails to resolve or, +> worse, resolves by component-suffix match to `arc-profiler/src/device.rs` +> (268 lines) and lands on unrelated code. Both are `SYMBOL-ROT` under +> `mistralrs-core/tests/doc_citations.rs`, which has no waiver path. Every +> candle reference below therefore names the file and the symbol and lets the +> reader search at the stated rev. **Do not "fix" these by adding line +> numbers.** + +--- + +## Hazard 1 — eviction frees buffers an instantiated graph writes into + +### The mechanism + +`AllocCache` gained a capacity and LRU eviction in candle `b2a4dd80` (default +cap 1 GiB, `DEFAULT_CAPACITY_BYTES`). Eviction sorts `free` by +`SizeClass::tick` and hands the oldest buffers back to the driver +(`evict_if_over_capacity`). + +The cache stores `(bytes, ptr)` and nothing else. In particular it does **not** +record that an address is baked into a `CUgraphExec`'s memory nodes. After a +successful capture, the buffers the captured forward used are dropped by the +eager tensors that held them and land straight back in `free`, indistinguishable +from any other cached buffer. Evicting one returns to the driver memory that the +next `cuGraphLaunch` will write into. + +### Why this is the steady state, not a corner case + +The tempting reading is "the graph's buffers are hot, so LRU protects them." +It does not, and the reason is structural: + +* **Replay never touches the allocator.** A `cuGraphLaunch` executes baked + addresses. It calls no `cache_take`, so it refreshes no `tick`. The graph's + buffers age monotonically from the moment the capture ends. +* **Eager traffic continues.** `default_verify_replays()` is 3 + (`arc-cuda-graph/src/graph.rs:278`), so eager forwards run alongside replays + and keep stamping fresh ticks on every *other* size. +* **Nothing drains in between.** `drain_alloc_cache_and_free` is called only on + pool-destroying paths — graph teardown (`graph.rs:56`) and cancel + (`graph.rs:214`). A successful instantiate leaves the graph's buffers resident + in the cache for the life of the graph. + +So the graph's buffers start as the newest entries and become the oldest. The +first eager put that crosses the cap evicts them first. Master's own test docs +record the cap being reached in practice on V4 — *"Measured on V4 at a 1024 MiB +cap, the peak was 1026.1 MiB"* (candle `859c49c8` commit message; **their +measurement, not ours**), so "the cap never bites" is not available as a defence. + +### The pre-warm makes it likelier, and multi-`bs` makes it certain + +`prewarm_alloc_cache` (candle `1fa534db`) deliberately installs a large, +long-lived set of buffers at exactly the sizes the capture is about to bake. +arc's caller comments size that pool at *"~315 MiB"* (held-out arc commit +`492b7baa7`; **their figure**). Against a 1 GiB cap that is a third of the +budget in one bite. + +And captures are keyed by batch size — `end_capture_and_cache(bs, ...)` +(`graph.rs:517`). Pre-warming for `bs=2` while `graph(bs=1)` is instantiated puts +graph(bs=1)'s baked addresses in the eviction pool. + +For that reason the merge makes the pre-warm **non-evicting** by construction +(`install_prewarmed`). That closes the pre-warm's own window. It does **not** +close the general one: ordinary decode puts still evict. + +### Why the obvious mitigations are not the fix + +* **Raise the cap.** Turns a correctness bug into a tuning parameter, and the + cap exists because the unbounded cache leaked (candle `b2a4dd80`: *"6.04 MiB + per decoded token with no plateau"* — **their measurement**). Trading one back + for the other is not progress. +* **Drain after instantiate.** The drain doc is explicit that draining under a + replayable graph *"trades one use-after-free for another"* + (`drain_alloc_cache_and_free`). +* **Never evict while a graph is live.** Restores the leak for the entire life of + the graph, which is the whole run. + +### The proposed fix — a pin the allocator can express + +LRU cannot represent "this address is baked", so give it a way to say so. + +1. A `pinned: HashSet` (or a `SizeClass::pinned` flag) marking byte sizes + the capture path has declared replay-critical — naturally, the keys of the + demand profile at instantiate time. +2. `evict_if_over_capacity` skips pinned sizes entirely, exactly as it already + skips `deferred`. +3. `CudaGraphRunner` pins on successful instantiate and unpins in the same place + it already drains (`graph.rs:56`, `:214`) — after `cuGraphExecDestroy`, before + `cuMemPoolDestroy`. The lifetime is already correct there; only the pin is + missing. +4. Retention stays bounded because the pinned set is bounded by the demand + profile, which is bounded by one forward's allocations. If the pinned set + alone exceeds the cap, that is a **reportable** condition, not a silent one. + +This is deliberately not shipped in `9586979d`: it is a new mechanism on both +sides of the candle boundary, and the merge's job was the union of two existing +lines, not a third. + +### The probe that settles it + +One counter already exists for this, added by the merge: +`AllocCacheStats::evicted_at_demand_size` — eviction victims whose byte size +is in the capture demand profile. + +> With capture enabled and a graph instantiated, poll `alloc_cache_stats()` per +> decode step and assert **`evicted_at_demand_size == 0`** for the life of the +> graph. + +Non-zero is the fingerprint, observed *before* the crash rather than inferred +from it. The existing per-step logger (`mistralrs-core/src/pipeline/normal.rs:299`) +is where it belongs; it reads `s.misses` / `s.frees()` / `s.hits` today and does +not print this field yet. + +Secondary A/B, same run: `capture_miss_count()` after the capture forward with +pre-warm on vs off (expect `>0 → 0`), and `alloc_cache_stats().prewarmed` +against the demand-profile sum. + +--- + +## Hazard 2 — `ARC_NO_DEFERRED_FREE` silently disables the capture instruments + +`set_capture_mode` returns early when `ARC_NO_DEFERRED_FREE` is set. That +predates both merged candle lines and was only ever documented as affecting +deferred-free. It does more than that now: + +`capturing` never becomes true, therefore + +* no capture window ever opens ⇒ `demand` stays empty ⇒ `prewarm_alloc_cache` + plans nothing and returns `(0, 0)`; +* no miss is ever filed in `capture_misses` ⇒ `capture_miss_count()` + returns 0 ⇒ the caller's "refuse to instantiate on a miss" + gate passes **unconditionally**. + +**A clean capture and a silently disabled capture are indistinguishable from +their instruments.** A run with this variable set reports the same numbers as a +perfect one. + +This is the same class as the instrument failures already logged this session, +and it belongs to neither candle line, so nobody owns it unless it is written +down. `9586979d` adds an in-code warning comment in `set_capture_mode`'s preamble and +changes no behaviour — a runtime print would alter what log scrapers see, which +is a separate call. + +> **Probe:** assert `ARC_NO_DEFERRED_FREE` is unset before trusting a zero from +> `capture_miss_count()` or a `(0, 0)` from `prewarm_alloc_cache`. + +--- + +## Interaction with PR #182 and the KV `cat` growth + +Three things are in flight at once and it is worth stating exactly how they sit +together, because two of them look independent and are not. + +### #182 is not a neighbour of hazard 1 — it is its precondition + +**PR #182** ("Bound the caching allocator: +6.04 MiB/token leak -> flat, for +0.15 ms/token", branch `perf/alloc-cache-bounded`, touching `Cargo.toml` and +`mistralrs-core/src/pipeline/normal.rs`) is the **arc-side landing of the same +allocator line hazard 1 is about**. Its candle side is master's `b2a4dd80` → +`89ab14ef` — one of the two lines merged into `9586979d`. + +That matters more than a cross-reference. **Before the bound there was no +eviction at all**: the only routes back to the driver were +`set_alloc_cache_enabled(false)` and `drain_alloc_cache_and_free()`, both called +only on pool-destroying paths. Hazard 1 had nothing to fire with. After #182 it +does. + +So #182 converts a bounded memory leak into a correctness risk that is live +exactly when a graph is live. That is still the right trade — an unbounded leak +is fatal on a long generation — but it **is** a trade, and it should be on the +record as one rather than discovered later. The pin proposed above is what pays +for it. + +### The KV `cat` growth sets hazard 1's *rate* + +Tonight's hardware run — **not ours; we took no hardware leg** — reports the +eager path OOMing at 22–30 tokens with ~11 MiB/token of genuine growth, the card +full at 143,151 of 143,771 MiB, traced to `cat_contiguous` (a candle function, +`candle-core/src/tensor_cat.rs`) on an F8E4M3 KV buffer. + +Whatever that turns out to be, it bears directly on hazard 1: **retention only +reaches the cap because something presents byte sizes nothing asks for again.** A +buffer whose width tracks KV length is exactly that shape — it is the shape +`b2a4dd80` named when it measured *"a family of ~132 buffers stepping by 8 KiB +per token"* (**their measurement**). More distinct-size traffic per token means +the cap is crossed sooner and the LRU sweep runs more often, and the sweep is +precisely what hazard 1 says can reach a graph's baked buffers. + +### This does not settle whether #182 and the KV growth are one bug or two + +Another agent owns that question and this document must not pre-empt it. It does +not have to: **hazard 1's mechanism is unchanged under either verdict.** + +* **One bug** — the growth *was* the cache retaining the `cat` chain's dead + sizes, and #182's bound fixes it. Then eviction now fires by design, at the + rate #182 measured, and hazard 1 fires with it. +* **Two bugs** — the ~11 MiB/token is genuinely live tensors the cache never + holds. Then the OOM is independent of the allocator, and hazard 1 still fires + whenever ordinary retention crosses the cap. + +Either way the fix for hazard 1 is the same pin, and neither verdict makes it +unnecessary. + +### Consequence for sequencing: hazard 1 is currently *masked* + +A process that dies at token 22–30 never keeps a `CUgraphExec` alive long enough +for its baked buffers to age to the front of the eviction queue. Replay does not +refresh a tick, but ageing still takes tokens, and there are not enough of them +before the OOM. + +So the `evicted_at_demand_size` probe cannot return a meaningful **zero or** +non-zero until the OOM is fixed. **A green probe today would be a green light +from an instrument that never had the chance to go red** — the same failure +shape as hazard 2, arriving by a different road. Fix the OOM first, then run the +probe, and treat any reading taken before that as void. + +--- + +## Cross-reference — two root causes, one observed crash + +Do not read either of these as closing the other. + +**This item** is a *device-side* use-after-free: the allocator frees device +memory whose address a graph node writes to on replay. + +**The `compressed_row_positions` thrash** found independently by the capture-lane +rebase is a *host-side* dangling pointer. `compressed_row_positions` +(`deepseek4.rs:3013`) memoizes into a **single-slot** thread-local +`Option<(t_c, ratio, Tensor)>` (`deepseek4.rs:3009`) keyed on `(t_c, ratio, +device)`. V4's layers strictly alternate ratio — *"Layers 0/1 = standard, even +2..=42 = CSA(4), odd 3..=41 = HCA(128)"* (`deepseek4.rs:6031-6032`, asserted +in-tree at `deepseek4.rs:6033-6041`) — so **every** consecutive compressed-layer +pair evicts the other's entry and the cache misses on all 41 compressed layers +of every forward. Each miss runs the strided `arange` constructor (`deepseek4.rs:3033`) → +`storage_from_cpu_storage` → `clone_htod`, +which under capture records the *host* pointer of a `Vec` that dies with the +expression → SIGSEGV on the first `cuGraphLaunch`. + +Two distinct mechanisms, the same observed symptom. Both must hold. + +Worth noting for sequencing: `9586979d` carries `arc_capture_retain_host` +(candle `864cd047`), which retains the host source of **every** capture-time H2D +copy including the `clone_htod` path — so it mitigates the second mechanism's +*crash* generically at the candle level, without fixing the cache thrash that +causes 41 redundant strided-`arange` builds per forward. That thrash is a real cost item +in its own right and is adjacent to wave65-CQ's item 2, which proposes memoizing +the `cos`/`sin` gathers on the same `(t_c, ratio, device)` key — **and would +inherit the same single-slot alternation defect if it copies the existing +pattern verbatim.** Whoever lands CQ item 2 should key on the pair, or use a +small map, not one slot. + +--- + +## Related + +* **PR #205** — the capture lane. Five commits were held out of it because they + call candle APIs that exist only on the branch line: `492b7baa7`, `1c371b2cc`, + `f7c65cb72`, `8968c1fa3`, `4475f9e71`. Against `9586979d` the first, second and + fourth resolve; **`f7c65cb72` and `4475f9e71` must never be cherry-picked** — + they pin candle to `5bdddcf` / `9211966`, which would revert master's + eviction-inside-capture fix (`89ab14ef`). They collapse into one rev bump plus + a lock regen. +* **PR #207 / wave65-CQ** — the V4 compressed-KV re-RoPE recompute, on the same + `compressed_row_positions` code path. See the caveat above before landing its + item 2. +* **`CAPTURE_LANE`** — op count is retired as a lever. Neither hazard here is an + op-count item; both are correctness. +* Filed here rather than as a GitHub issue because `aeonmindai/arc` has issues + disabled; `memory/mission/` is the tracker this project uses. + +--- + +## What would change this document + +Either hazard becomes a **defect with a number** the moment someone runs the +probe. Until then both are derivations from source, and the honest status is +"reachable by construction, unobserved on hardware."