Repository navigation
fix(ArcInfer/ArcGraph): cross-request token leak — parked burst tokens no longer outlive their sequence - #220
Conversation
Code Metrics Report━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Language Files Lines Code Comments Blanks ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ C Header 5 305 210 52 43 CSS 2 1181 1036 34 111 CUDA 81 29475 20161 6275 3039 Dockerfile 1 39 22 8 9 JavaScript 16 3546 2676 482 388 Jinja2 7 694 656 5 33 JSON 75 4896 4893 0 3 Makefile 1 6 5 0 1 Metal Shading Lan| 33 12224 9431 1142 1651 PowerShell 1 300 227 30 43 Python 147 15371 12677 824 1870 Shell 42 10304 6858 2769 677 Plain Text 4 3801 0 2479 1322 TOML 33 1498 1294 54 150 YAML 3 25 23 2 0 ───────────────────────────────────────────────────────────────────────────────── HTML 4 2687 2604 43 40 |- CSS 2 543 479 37 27 |- JavaScript 1 1233 1215 12 6 (Total) 4463 4298 92 73 ───────────────────────────────────────────────────────────────────────────────── Jupyter Notebooks 4 122 83 23 16 |- Markdown 1 60 30 22 8 |- Python 1 122 113 1 8 (Total) 304 226 46 32 ───────────────────────────────────────────────────────────────────────────────── Markdown 213 48932 0 38081 10851 |- BASH 72 1655 1203 331 121 |- C 4 19 19 0 0 |- CUDA 2 84 56 16 12 |- JSON 19 779 779 0 0 |- PowerShell 1 1 1 0 0 |- Python 23 1008 787 113 108 |- Rust 68 2063 1727 78 258 |- TOML 6 207 164 0 43 |- YAML 5 41 36 5 0 (Total) 54789 4772 38624 11393 ───────────────────────────────────────────────────────────────────────────────── Rust 681 340988 293252 18096 29640 |- Markdown 504 32562 471 28063 4028 (Total) 373550 293723 46159 33668 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Total 1353 516771 363188 99077 54506 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ |
…equence — one funnel, one leak test A device-loop burst runs with eos_token_id = -1, so it always parks its full burst; a sequence stopping mid-burst (stop string, max_tokens, cancel, error, panic, eviction) left its undrained tokens parked, and the NEXT sequence's first sample_sequence — including the one after its prefill — consumed them as its own. One user's tokens in another user's response. clear_pending_tokens documented the contract but its only production call sites were fallback paths. The fix is one call at the one funnel every completion path passes through: Sequence::set_state stands the device loop down on every transition out of the running set (Done, Error, FinishedAborted, FinishedIgnored, Waiting, Swapped). Clearing on preemption is deliberate fail-closed: the sequence re-prefills on re-admission, so its parked tokens are stale. To make the funnel testable on machines without CUDA — the cfg(cuda) darkness is exactly what let this ship — arc-cuda-graph is now a non-optional dependency of mistralrs-core (its host surface is plain Rust; CUDA plumbing stays behind its own cuda feature, still enabled by mistralrs-core/cuda) and device_loop_pre_sampled_token loses its cfg split. The new leak test drives the real sample_sequence path both ways: wired, B's first token is its own argmax and the queue is empty; with the funnel removed, the queue holds 3 of A's tokens and B's first sampled token is 22 — A's parked id. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WK8VgocBUrr5djjE1ZfCNf
1366369 to
be7f971
Compare
…s structurally impossible, not funnel-dependent The completion funnel cannot see a sequence the DefaultScheduler moves aside WITHOUT a state change (the bucketing waitlist says so in its own doc), and batch growth has the same shape. So the queue itself now refuses: tokens are parked tagged with the owning sequence's id (published by the sampling pre-hook with eligibility), take requires a matching id, and a foreign queue is dropped whole with the grep-able instrument line "ArcGraph: dropped N foreign parked tokens". The forward's pending>0 short-circuit gains the two guards its aliasing contract always needed: seq_len == 1 (a parked queue at a prefill step must not replace the prefill) and pending_owned_by_current(). If the short-circuit fired and the sample still cannot take (scheduler swapped sequences in between), the step fails loudly via the aliased-logits marker instead of host-sampling another sequence's stale graph output. DeviceDecodeLoop finally resets: every stand_down (including the funnel's) bumps a device-state generation; run() compares and zeroes ring, cursors and fault word before a newer-generation burst. Reset at engagement, not completion, because the funnel cannot reach the pipeline-owned loop and between bursts is the only point with no in-flight ring writes. ARC_GRAPH_DEVICE_LOOP is now registered in capability_reachability.rs (Symbol: device_loop_enabled), closing the refuted premise from the previous commit. Interleave test (A waitlisted, no set_state): wired, B samples its own argmax 9 and the foreign tail is dropped; with the owner comparison removed, B's first sampled token is 22 — A's parked id. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WK8VgocBUrr5djjE1ZfCNf
Follow-up commit
|
Stacked on #217 (
arcgraph/device-decode-loop-rel); merge after it (D20). Until #217 merges, this PR shows its commits too — the leak fix itself is136636973, four files.The bug
device_decode_burstruns every burst witheos_token_id = -1(normal.rs:2193), so the device loop always parks its full burst in the thread-local pending queue. The engine drains one token per step. A sequence that stops mid-burst — stop string,max_tokens, client cancel, forward error, panic, eviction — left its undrained tail parked, and the next sequence's firstsample_sequence(including the one right after its prefill) consumed those tokens as its own: one user's tokens inside another user's response. Worse, with anything parked,device_decode_burst'spending > 0short-circuit (normal.rs:2168) returns stale graph logits without launching anything — for a new request's prefill step too.clear_pending_tokensdocumented the completion contract, but its only production call sites were fallback paths (normal.rsdecline/fall-back arms,sampling.rsineligible arm,kill_device_loop);DeviceDecodeLoop::reset()— the other caller — has zero production call sites. The existing unit test covered the function, not that anything calls it.The fix: one funnel, not N scattered calls
Every path that takes a sequence out of the running set passes through
Sequence::set_state. That is where the stand-down now lives — an exhaustive match, so a future state variant forces a decision:finish_or_add_toks_to_seq->set_state(Done(reason))(sampling.rs:267/:277, :74)set_state(Done(Canceled))(sampling.rs:246)default_scheduler.rs:500, pagedscheduler.rs:449->Done(Canceled)TERMINATE_ALL_NEXT_STEPdefault_scheduler.rs:556->Done(Canceled)step_catching_panics->handle_pipeline_forward_error!)utils/mod.rs:403->set_state(Error); alsohandle_seq_error_stateaware_ok!(utils/mod.rs:272)scheduler.rs:288/:299->FinishedIgnored; enginemod.rs:839scheduler.rs:532->Waiting;Swappeddefault_scheduler.rs:263)Why the dependency became non-optional
The funnel could have been
#[cfg(feature = "cuda")]— and then it could not be compiled, run, or mutation-tested on any machine that reviews it, which is the exact class that let this ship (and the recorded polarity-bug pattern).arc-cuda-graph's host surface (pending queue, burst driver, admission) is plain Rust already exercised by the CPU clippy lane, and it adds zero new transitive deps tomistralrs-core. So: dep promoted to non-optional (arc-cuda-graph/cudastill rides thecudafeature), anddevice_loop_pre_sampled_tokenloses its cfg split — the real take path now compiles and runs everywhere. On non-CUDA builds nothing ever parks, so behaviour is unchanged.The test that fails on the leak — mutation-checked both ways
pipeline/sampling.rs::device_loop_cross_sequence_leak_testsdrives the realsample_sequence: A (greedy, eligible) samples once, its burst parks[21, 22, 23, 24](eos -1 => full length), A drains21— a built-in control proving the take path is live in the test build, so the assertions cannot be vacuous — then A stops mid-burst viaDone(Length(2)), and B samples with its own logits peaking at 9.2 passed.set_state): queue assertion failsleft: 3, right: 0— A's undrained tail survives; with that assertion skipped to let execution reach B: "cross-request token leak: sequence B's first sampled token is 22, one of sequence A's parked burst tokens." The second test fails the same way forDone(Canceled)(left: 4, right: 0) and every other finished transition.A second test pins the whole funnel map:
Done(Canceled),Done(StopString),Error,FinishedAborted,FinishedIgnored,Waiting,Swappedeach clear the queue.Same-shape audit (report, not fixed here)
DeviceDecodeLoopper-sequence device state outlives completion. Ring cursors, ring head, the device position buffer the commit kernel advances, and the last committed id in the pinned graph input buffer all live in the pipeline-lifetimeloop_slot;DeviceDecodeLoop::reset()exists for exactly this and has no production caller. A second sequence engaging the loop would decode on-device as a continuation of the first. Correctly resetting requires the new sequence's prefill to re-prime graph-mode KV/positions — capture-lane work innormal.rs, outside this PR's fence.seq.id()with eligibility;push_pending_tokensstamps the owner; take requires a match, mismatch clears fail-closed). Contained toarc-cuda-graph+sampling.rs; not shipped here to keep this PR single-mechanism and its mutation semantics clean.pending > 0short-circuit lacks aseq_len == 1guard (normal.rs:2168): any stale pending token converts a prefill forward into a no-op returning stale decode logits. Unreachable across sequences with the funnel in place; cheap armor for thenormal.rsowner.stand_down. Sampler RNG: none per-sequence (greedy, fixed seed).DEVICE_LOOP_KILLED: process-wide by design.Premise corrections against the tree
ARC_GRAPH_DEVICE_LOOPis not registered incapability_reachability.rs— no entry exists to keep or update; the crate is covered only by the dead-symbol ratchet (which passes).ARC_CAPTURE_STREAM=1->mistralrs_for_server_builder.rs:1238init_device->Device::new_cuda_with_stream(0)(a real capturable stream); default off viaCAPTURE_STREAM_DEFAULT = false(:1182), auto-refused under NCCL or paged-attn. Named blast radius (in-source): NCCL conflict, PagedAttention device expectations (device_map::get_all_similar_devices), cudarc event tracking disabled, persistent cuBLAS workspace. Flipping the default is one line + its test, gated on a hardware-verified replay (ARC_GRAPH_VERIFY_REPLAYS). Note this entry point covers the server builder only; other entry points still bind the legacy NULL stream andgraph.rs:143-167logsArcGraph INERT.+1-step replay divergence (report-only; capture stays default-off)
Wiring is proven innocent (same-step delta = 0.000e0), so the divergence is state carried across the replay boundary. On-box probe to localize it: run with
ARC_CAPTURE_STREAM=1 ARC_V4_CAPTURE_PROBE=1 ARC_CANDLE_ALLOC_CACHE=1 ARC_GRAPH_VERIFY_ALWAYS=1and the layer bisect (arc_layer_bisect/arc_layer_trace_begin, already wired atnormal.rs:2802): at step N (replay) and step N+1 (first divergent step), trace eager vs replay per-layer checksums — the first layer index whose delta goes nonzero at N+1 names the tensor whose address or value went stale across the boundary (per-step inputs: positions, KV write index, mask window, input id). The existingARC_GRAPH_VERIFY_REPLAYScomparison already computes both forwards; extending it to dump per-layer sums at N and N+1 is the whole probe. D14: nothing here is validated until the box runs it.Verification
cargo check -p mistralrs-core(macOS, no CUDA): clean.cargo test -p mistralrs-core --lib: 695 passed.--test capability_reachability: 12 passed (ratchet clean).--test doc_citations: 11 passed.cargo test -p arc-cuda-graph: 76 passed.arc-bench arc-engine arc-cuda-graph arc-cli mistralrs-quant --tests --examples -D warnings): clean.cuda-feature build ofmistralrs-corewith the reworked feature list (macOS cannot build it) — relies on thecargo check (cuda, workspace)and nvcc sm_80/sm_90 lanes. No.cuor cfg(cuda) code was modified.🤖 Generated with Claude Code
https://claude.ai/code/session_01WK8VgocBUrr5djjE1ZfCNf