QVAC-23752 feat[bc]: remove sliding-context support from the llm-addon - #3938
QVAC-23752 feat[bc]: remove sliding-context support from the llm-addon#3938yingying0906 wants to merge 12 commits into
Conversation
Review StatusCurrent Status: ❌ PENDING Pending reviews: Needs 1 Management or Team Lead, and 1 more from Management, Team Lead, or Member. |
License compliance — cleanNo new dependency license findings in this PR. Warn-only (shadow) mode — this check does not block merges yet. Updated automatically by the canonical license compliance workflow. NOTICE presence (advisory)Missing NOTICE (advisory, does not block):
|
12d3096 to
ea16aa0
Compare
The addon could evict tokens from the middle of the KV cache once the context filled, during prefill or decode, and tracked `firstMsgTokens` so the system prompt and tool definitions were never the tokens dropped. It was opt-in through `n_discarded` and defaulted to off, so the shipped behaviour was already the non-sliding path while the slide machinery added state every other path had to keep correct. The generation-time slide also invalidated a tracked `<think>` span, which wiped the sequence and hard-failed instead of shifting the span. - Removed `ContextSlider` (slide policy) and `ContextShifter` (slide budget and counter). The surviving KV primitives, the ops seam plus `compactKvRange`, move to `KvCacheOps` since reasoning-block compaction still needs them. - Removed `firstMsgTokens` / `protectedPrefix` from both contexts and the `LlmContext` / `SequenceDriver` accessors that exposed them, along with `slideCapable` admission handling, `applySlide`, `supportsSliding` and `SequenceStepResult::discarded`. - Removed the compactor's slide-invalidation state and its `FailedKvWiped` branch, which nothing could reach once the shifter went. - Overflow is now the single path `n_discarded=0` already took. A prefill that does not fit throws `ContextOverflow`. A generation that fills the window stops with `stopReason=contextOverflow` and still returns what it produced. The generation check is a plain "no room for even one more token" test, so a caller can tell a full context from a prediction-limit cutoff. - A batched slot never reached that check. `advance` marks the request the moment `currentPos` hits `maxTokensPerSequence` and `isFinished` includes that, so the slot is filtered out before the driver is asked, and ordinary text generation reported `sequenceLimit` for a full window. That limit IS the slot's share of the context, and `submit` rejects any request whose prompt plus a positive `n_predict` would not fit, so a full window is the one thing it can mean. It now says so. - Both prefill guards name the quantity they report, so `cached tokens N plus prompt tokens M` is no longer formatted into the same field as the slice alone. - Session metadata keeps its four-slot width so a file written by either build still loads, with slots 1 and 3 written as 0. `ContextSlideFailed` stays reserved so a new code does not reuse 26, which builds up to 0.45.0 emit. Rolling this package back over an existing cache dir does drop the system prompt, because the old build reads slot 1 as its protected prefix and 0 makes it evict from position 0. - Leftovers cleared: a dead `applyContextDiscard` declaration, the unused `IKvCacheOps::nCtx`, comments still describing sliding, `%zu` for two `size_t` counts formatted `%ld`, and the OpenCL error string that still said "sliding context". `sliding-context.test.js` and `mrope-sliding-context.test.js` are deleted, with all three mobile registries updated so a run does not abort on an unregistered test. `KvCacheOpsTest` holds the 7 `compactKvRange` cases moved out of `test_context_slider.cpp`, and the plain text overflow case is back as a direct test in `api-behavior.test.js`. `n_discarded` is no longer consumed, so it reaches llama's own argument parser and fails model load as an unknown option. The SDK half is in #3999.
aegioscy
left a comment
There was a problem hiding this comment.
The end − start arithmetic is the textbook llama hole-punch, but the range/branching around it can leave a hole without a shift, abort on M-RoPE after seq_rm already mutated cache, or OOM on the next decode's K-shift graph. Inline comments on the spots that need to change.
| if (endPos <= startPos || startPos < 0 || endPos > nPast) { | ||
| return {CompactRangeOutcome::Kind::NoOp, nPast, 0}; |
There was a problem hiding this comment.
Please don't treat every out-of-range input as a silent NoOp.
endPos > nPast is a stale span, not an empty one. llama's own p1 = -1 means "to the end of the sequence"; if a caller ever passes that as nPast, this guard is true for any endPos >= 0 and compaction never runs. SWA / a cursor that lags seq_pos_max has the same shape: we refuse instead of shifting what is actually resident.
Requested changes:
- Cross-check
nPastagainstllama_memory_seq_pos_max(mem, seqId) + 1(orseq_addwithp1 = -1and then readnewNPastback fromseq_pos_max). Do not trust the software cursor alone. - If
endPos > nPastbutstartPos < nPast, clamp the hole to[startPos, nPast)rather than NoOp. LeaveNoOpfor empty/inverted/start < 0only. - Reject
nPast < 0explicitly so the llama-1sentinel cannot fall through.
There was a problem hiding this comment.
Update: removed in 68087763b, so this thread is moot.
Context shifting has been removed from reasoning-block compaction entirely. Every model now rewinds to the end-of-prefill boundary and replays the answer, which is what the recurrent path already did. So compactKvRange and KvCacheOps.hpp/.cpp are gone, along with the nPast < 0 reject, the endPos > nPast question and the seq_pos_max readback we were discussing here. Compaction now rewinds to the end-of-prefill boundary and re-decodes the kept tokens, which on pure attention is a tail trim and needs no cursor arithmetic at all.
Leaving the original reply below for the record.
Addressed in d0bdab79, bb2d1e15 and 6ba83952.
nPast < 0 is rejected now. llama uses -1 as the "to the end of the sequence" sentinel, so it would have made endPos > nPast true for every range and stopped compaction silently.
On not trusting the software cursor, you were right and I went with the readback. compactKvRange takes the new cursor from seq_pos_max after the shift instead of from nPast - discarded, and a disagreement means the shift did not land where the arithmetic said. So a cursor that has drifted from live memory is caught rather than propagated as a cursor no live cell backs.
I didn't add the pre-call seq_pos_max + 1 cross-check. The readback catches the same drift without needing a rule about what a healthy sequence looks like going in, and I didn't want a hard pre-check failing a cache that loaded with gaps. Tell me if you think the pre-check catches something the readback doesn't and I'll add it.
endPos > nPast is still NoOp rather than a clamp. ReasoningBlockCompactor::compact already does end = std::min(recordedEnd, pos) before it calls in, so a range past the cursor means the caller is inconsistent with itself, and a silent clamp in a primitive that owns no policy would hide that.
Worth saying, this function is byte identical to origin/main:ContextSlider.cpp:106. The PR only moved it, so none of it was new behaviour.
| // llama_memory_seq_add is void / infallible by API contract. | ||
| ops.seqAdd(mem, seqId, endPos, nPast, -discarded); | ||
| return {CompactRangeOutcome::Kind::Compacted, nPast - discarded, discarded}; |
There was a problem hiding this comment.
This is the part that looked funny, and it is not infallible.
llama_memory_seq_add is void, but:
GGML_ASSERT(n_pos_per_embd == 1)— M-RoPE (Qwen2-VL / Qwen3-VL) aborts afterseq_rmalready punched the hole. Mtmd's attention path calls this.- shared/
otherstreams no-opseq_addwhileseq_rmsucceeded → hole, no shift,newNPaststill decreases. seq_addonly updates cellpos/has_shift. K RoPE is rewritten later inllama_decode→init_update→build_graph_shift, which allocates an I32 tensor of sizen_ctx * n_streamplus a RoPE pass over every K layer. Compact runs when the window is full of thinking; that graph is the OpenCL/GPU OOM. On alloc failure llama logs and does notreset_shift, so every later decode retries it.!get_can_shift()(M-RoPE, Step35)GGML_ABORTs on that next decode.
p1 = nPast also disagrees with llama's own slide examples, which use -1 so stray cells with pos >= nPast still move.
Requested changes, before seq_rm:
- Refuse unless the memory module can shift (
n_pos_per_embd == 1,get_can_shift()). Never punch a hole we cannot shift. seq_add(..., endPos, /*p1=*/-1, -discarded)or assertnPast == seq_pos_max+1.- Apply
init_update/ K-shift here. If the shift graph fails to allocate, reportMemoryOperationFailed— do not returnCompactedwith stickyhas_shiftand a lyingnewNPast. - Drop the "infallible" comment.
There was a problem hiding this comment.
Update: removed in 68087763b, so this thread is moot.
Context shifting has been removed from reasoning-block compaction entirely. Every model now rewinds to the end-of-prefill boundary and replays the answer, which is what the recurrent path already did. seq_add no longer exists anywhere in the addon, so the can-shift guard, the seq_pos_max readback and the MemoryInconsistent kind all go with KvCacheOps.hpp/.cpp.
That also settles the K-shift graph point. Nothing in the addon schedules a shift any more, so has_shift is never set and the fabric bug can't be reached from here. It is a real bug on the fabric we pin, and it is fixed in tetherto/qvac-fabric-llm.cpp#213. Worth knowing if you review that one: making apply() return the failure was not enough on its own. llama_context::memory_update logged it and returned true anyway, and decode() discarded that, so the failure still never reached llama_decode. Both layers had the same shape, a bool where false meant both "nothing to do" and "it failed", so it now returns a three-state result instead.
Leaving the original reply below for the record.
Addressed in d0bdab79, bb2d1e15 and 6ba83952.
compactKvRange checks llama_memory_can_shift before seq_rm now, so no hole is opened that the shift can't close, and it reads seq_pos_max back after seq_add instead of trusting nPast - discarded. Shift passes p1 = -1 and the infallible comment is gone.
6ba83952 fixes a mistake in the first version of that readback. It reported MemoryOperationFailed, which maps to FailedKvIntact, but seq_rm has already run by then, so the hole is mid-cache and a tail trim can't reach it. There's a MemoryInconsistent kind now that maps to FailedKvWiped, and compact() wipes.
On the K-shift graph, this is a fabric bug and the addon can't reach it. llama_kv_cache_context::apply() at src/llama-kv-cache.cpp:2742 discards the bool from kv->update() and returns true, so llama_decode returns 0 even when the shift graph failed to allocate, and the decode runs with K unshifted. Silently wrong rather than an error. The failure path at :986 also skips reset_shift(), which only runs at :1005 on success, so has_shift sticks and every later decode retries. And there's no per-sequence reset: llama_kv_cells::rm at src/llama-kv-cells.h:251 clears only the per-cell shift, and whole-cache clear() is the one thing that clears the flag. So the addon can neither detect it nor clear it for one sequence. Your concern stands and this PR doesn't fix it. I'd rather file it against fabric, where apply() propagating the false is the first thing to fix, than work around it here.
Two premises don't match this fabric though. seq_add asserts on get_can_shift(), not n_pos_per_embd == 1, and get_can_shift at :1289 allows n_pos_per_embd == 4 under llama_kv_cache_uses_mrope_shift, so Qwen2-VL and Qwen3-VL don't abort, only LLM_ARCH_STEP35. And shared cells no-op seq_rm and seq_add together at :479 and :667, so no hole is left unshifted.
Four new tests cover the refusal, the readback and the wipe mapping. The fake models a resident sequence and fails the test if one isn't modelled, so a readback assertion can't become a rubber stamp. reasoning.test.js does 10 real compactions on Metal and all of them pass the readback.
| if (!needsRecurrentSnapshot_) { | ||
| const CompactRangeOutcome rangeOutcome = | ||
| compactKvRange(ctx, seqId, start, end, pos, sliderOps); | ||
| compactKvRange(ctx, seqId, start, end, pos, kvCacheOps); |
There was a problem hiding this comment.
compactKvRange has three outcomes. The fall-through below collapses NoOp and MemoryOperationFailed into FailedKvIntact, and the driver then removeLastNTokens of the entire request — answer included.
NoOp means the cache was not touched (empty/inverted/out-of-range). That is not a seq_rm rejection. A stale end > nPast in the primitive becomes a hard-fail wipe instead of a clamp.
Requested change: map Kind::NoOp → Outcome::Kind::NoOp, and only Kind::MemoryOperationFailed → FailedKvIntact. Please add a unit test that a primitive NoOp does not take the wipe path.
There was a problem hiding this comment.
Update: removed in 68087763b.
Context shifting has been removed from reasoning-block compaction entirely. Every model now rewinds to the end-of-prefill boundary and replays the answer, which is what the recurrent path already did. With the shift gone there is no compactKvRange and no attentionOutcomeFor, so the NoOp mapping this thread was about no longer exists. Compaction now has a single path for every model.
Leaving the original reply below for the record.
Addressed in d0bdab79, bb2d1e15 and 6ba83952.
Kind::NoOp maps to Outcome::Kind::NoOp now. MemoryOperationFailed goes to FailedKvIntact, and MemoryInconsistent, the kind 6ba83952 adds for a shift whose removal ran but whose move didn't land, goes to FailedKvWiped since the cache has been written to by then.
For the regression test, I pulled the mapping out into ReasoningBlockCompactor::attentionOutcomeFor, a pure function, and compact() calls it. Three tests under ReasoningBlockCompactorOutcomeMapping pin all four kinds, including NoOpDoesNotBecomeTheWipePath for the one you asked about.
Reaching that branch through compact() isn't possible, setOpenSpan refuses start < 0, the degenerate check catches end <= start, and end = std::min(recordedEnd, pos) caps the rest. But that's a reason to make the mapping testable on its own, not a reason to skip the test.
gianni-cor
left a comment
There was a problem hiding this comment.
Inline notes from an automated-assisted review — five consolidation cleanups the refactor left behind. None are blocking bugs; each is a follow-up-sized change that removes a footgun or dead state.
| shifter_.discardBudget() == 0) { | ||
| // The context is 100% full on either measure: no room for one more | ||
| // token, and nothing is evicted to make room any more. | ||
| if (current_.pos + 1 > |
There was a problem hiding this comment.
Context-full detection is now hand-rolled at five divergent sites. This check compares against raw llama_n_ctx, as do the seven guards in evalMessageWithTools (~456–471), while onLogitsReady (~1639) and both TextLlmContext sites compare against ctxCeiling(); the sites also mix >= with +1 > and their warning strings have already drifted ("context is full" vs "per-slot context is full"). The next boundary change (e.g. reserving EOT headroom, or a per-seq ceiling path reaching evalMessageWithTools, where perSeqCtxCeiling_ would currently be ignored) has to be applied at all five sites, and missing one yields an off-by-one overflow that manifests on only one model family or only in batch mode. A single shared contextWindowFull(pos, ceiling) helper on the LlmContext base would serve all five.
There was a problem hiding this comment.
Done. Added contextWindowFull(pos, ceiling) next to exceedsContextWindow in SequenceDriver.hpp, and all five sites go through it.
No behaviour change today. evalMessageWithTools only runs on the single request context, built by the ctor that never sets perSeqCtxCeiling_, so ctxCeiling() there is already llama_n_ctx. It becomes correct if a per-slot ceiling ever reaches it, which was your point.
I left the two warning strings as they are. "context is full" and "per-slot context is full" are different paths and the wording is what tells them apart in a log.
| static_cast<llama_token>(context.getCacheTokens()); | ||
| tokens[static_cast<size_t>(Field::FirstMsgCacheTokens)] = | ||
| static_cast<llama_token>(context.getFirstMsgCacheTokens()); | ||
| // Slots 1 and 3 are unused and stay 0. |
There was a problem hiding this comment.
The on-disk 4-slot session-metadata layout {NPast, 0, CacheTokens, 0} is hand-encoded at three sites that this PR had to edit in lockstep. This anonymous-namespace SessionMetadata struct, TextLlmContext::saveCache, and MtmdLlmContext::saveCache (a verbatim copy) each independently encode the "slots 1 and 3 retired, write 0" contract, and TextLlmContext::loadCache hand-reads slots 0/2. Since the PR itself documents that a wrong slot-1 value makes an older build evict from position 0, a future writer that forgets one of these sites is a silent cache-corruption bug. Suggest hoisting a shared capture/apply next to SessionMetadataField in LlmContext.hpp so the layout is written and read in exactly one place.
There was a problem hiding this comment.
Done. SessionMetadata moved to LlmContext.hpp next to SessionMetadataField, with capture and applyTo. CacheManager, both saveCache and TextLlmContext::loadCache all use it now, so the {nPast, 0, cacheTokens, 0} layout is written and read in one place.
I left mtmdSessionMetadataIsComplete alone. It's == 4 on purpose while the text path takes >= 4, so folding them would change behaviour.
| // slot's share of the context window, so reaching it is ContextOverflow. | ||
| // Kept so the numbering and any explicit `markFinished` caller still | ||
| // resolve; `sequenceLimit` is unreachable while nothing sets this. | ||
| LimitReached, |
There was a problem hiding this comment.
StopReason::LimitReached is dead — no producer remains. The comment itself says sequenceLimit is unreachable while nothing sets this, and grep confirms no production assignment is left. Yet the LimitReached → GenerationStopReason::SequenceLimit mapping in ContinuousBatchScheduler.cpp (~111–112) and the unit test LimitReachedPropagatesSequenceLimit in test_continuous_batch_finalize.cpp (~112–115) both survive, asserting behavior the program can never reach. The enum is in-memory only, so numbering stability buys nothing here (unlike the reserved error code 26, which is externally visible). Suggest deleting the enum value, the switch case, and the dead-path test — and dropping the now-unreachable "sequenceLimit" from the JS-facing stopReason union in index.d.ts alongside the SDK PR.
There was a problem hiding this comment.
Done for the C++ side. Dropped the enum value, the SequenceLimit mapping in ContinuousBatchScheduler.cpp and LimitReachedPropagatesSequenceLimit.
I kept sequenceLimit in the JS union though. GenerationStopReason::SequenceLimit is ordinal 4, pinned by the static_assert in SequenceDriver.hpp and by the positional STOP_REASONS array in addon.js, so dropping it renumbers contextOverflow from 5 to 4. That's a real break for a string that only can't be produced any more.
| // dropped-token count, and the kept-prefix end (used by callers to | ||
| // adjust `firstMsgTokens_` / `protectedPrefix_`). The compactor | ||
| // itself does not write to the caller's position fields. | ||
| // dropped-token count, and the kept-prefix end. The compactor itself |
There was a problem hiding this comment.
Outcome::keptPrefixEnd (declared at line 251) is write-only after this PR. Its production consumers were the firstMsgTokens_ / protectedPrefix_ adjustments deleted from both compactThinkSpan implementations. What remains is exactly: the declaration, the two assignments in ReasoningBlockCompactor.cpp (356, 496), and two EXPECT_EQs in test_reasoning_block_compactor.cpp (916, 1100) — no production reader. This doc comment was reworded to keep mentioning "the kept-prefix end" without the (now deleted) purpose. Dead state the feature removal should have taken with it: suggest dropping the field, both assignments, the two test assertions, and this mention.
There was a problem hiding this comment.
Done. Dropped the field, both assignments, the two EXPECT_EQs and the doc mention.
| // prompt, so the stop is always the context rather than the cap. | ||
| const { model } = await setupModel(t, { ctx_size: '512', n_predict: '512' }) | ||
|
|
||
| const filler = 'word '.repeat(430) |
There was a problem hiding this comment.
This overflow scenario is copy-pasted, and its sizing is tokenizer-sensitive. reasoning.test.js (~1024–1084) hard-codes the same recipe — 'word '.repeat(430) against a 512 window, the same three assertions (stopReason === 'contextOverflow', output survives, oversized prompt refused at prefill matching /context overflow/i) and the same recovery turn. The 430 must tokenize to just under 512 so the prompt is accepted at prefill but generation fills the window; a chat-template or model-pin bump can silently flip both copies into asserting the prefill-rejection path instead, and both then need the same recalibration (the darwin-x64 CPU fallback is already implemented differently in each file — file-wide useCpu here vs a per-test device override there). Suggest a shared helper in the test utils that owns the sizing and assertions.
Related duplication on the C++ side: the RejectingKvCacheOps fake is defined near-verbatim in both test/unit/test_cancel_rollback.cpp (1274) and test/unit/test_reasoning_block_compactor.cpp (861), and test_kv_cache_ops.cpp holds a third IKvCacheOps fake that subsumes both — one configurable fake in a shared test header would cover all three.
There was a problem hiding this comment.
Done. The sizing and the assertions live in test/integration/_context-overflow.js now and both files call it, so a chat-template or model pin bump gets recalibrated once.
The three IKvCacheOps fakes are one configurable FakeKvCacheOps in test/unit/test_kv_cache_ops_fake.hpp, used by test_kv_cache_ops, test_cancel_rollback and test_reasoning_block_compactor.
Update after 68087763b. The _context-overflow.js half is unchanged. The fake consolidation survived the shift removal but changed shape: with compactKvRange gone there is no IKvCacheOps left to fake, so it is now one FakeReasoningRewindOps in test/unit/test_reasoning_rewind_fake.hpp, shared by test_reasoning_block_compactor and test_cancel_rollback. test_kv_cache_ops.cpp was deleted with the primitive.
| "[TextLlm] context overflow at batch prefill step: cached tokens %d " | ||
| "plus prompt tokens %zu exceed the max context tokens %d\n", |
There was a problem hiding this comment.
The second prefill-overflow guard, whose message this PR rewrote, has no desktop integration coverage after the deletions.
TextLlmContext.cpp carries two prefill guards: :626 reports prompt tokens alone, and this one reports cached tokens %d plus prompt tokens %zu exceed the max context tokens %d. Nothing exercises the second — grepping test/ at this head, no file contains the string cached tokens.
The deleted sliding-context.test.js held two scenarios configured n_discarded: '0' — testing behaviour this PR keeps, not the feature it removes:
:156'Generation stops at the context boundary when sliding is disabled':309'Cached follow-up overflows when sliding is disabled and context is full'
The replacement at api-behavior.test.js:306-320 sends one oversized prompt, which trips the first guard — this PR's CI shows prompt tokens 4013, max context tokens 512, the prompt-alone wording. It also asserts only /context overflow/i (:318), which matches both guards, so it would not pin either wording. Also lost in the deletion: the stats.generatedTokens < n_predict assertion, which proved the prediction cap was not the stopper.
Impact: a change to this guard's wording or boundary would not fail desktop CI — while #3999's overflow parser is being taught to match this exact wording.
Suggested fix: prime a cacheKey with saveCacheToDisk, then send a follow-up that no longer fits. That reaches this guard rather than the prompt-alone one, because nPast_ is non-zero on the second turn:
t.ok(/cached tokens .* plus prompt tokens/.test(err.message))Restoring the generatedTokens < predict assertion to the generation case would close the other half.
Distinct from the comment on api-behavior.test.js:287: that one is about the two overflow recipes duplicating each other; this is about the guard neither of them reaches.
There was a problem hiding this comment.
You're right, nothing reached that guard. Added run | context full: cached follow-up is refused at prefill in api-behavior.test.js. It primes a cacheKey with saveCacheToDisk and then sends a small follow-up, which is your suggested shape. Local run lands on it:
[TextLlm] context overflow at batch prefill step: cached tokens 512 plus prompt tokens 17 exceed the max context tokens 512
Both overflow tests pin the wording of the guard they actually hit now instead of /context overflow/i, which matched either one. Also restored the generatedTokens < predict assertion.
Answers the three review rounds on #3938. The KV primitive hardening is pre-existing behaviour the removal only moved, kept here because the moved code is where a reviewer can see it. KV primitive: - `compactKvRange` now checks `llama_memory_can_shift` before `seq_rm`. `llama_memory_seq_add` GGML_ASSERTs on a module that cannot shift, and since `seq_rm` runs first that abort would land with the hole already punched and nothing able to close it. A refusal reports `MemoryOperationFailed`, so the caller rolls back instead of wiping. It also fixes the null-memory case, which used to report `Compacted` with a `newNPast` nothing backed. - `nPast < 0` is rejected. It is llama's "to the end of the sequence" sentinel, never a cursor, and it would make `endPos > nPast` true for every range and stop compaction silently. - The tail shift passes `p1 = -1` rather than `nPast`, matching llama's own slide callers, so a cell past the cursor moves with the tail instead of being stranded on top of it. - Dropped the "infallible by API contract" comment, which was wrong. Compactor: - A primitive `NoOp` maps to `Outcome::Kind::NoOp` instead of falling through to `FailedKvIntact`. The caller's clamp makes it unreachable today, but collapsing the two would roll the whole request back, answer included, for a range the cache never saw. - Removed `Outcome::keptPrefixEnd`. Its only readers were the `firstMsgTokens` / `protectedPrefix` adjustments this PR deleted. Consolidation: - `contextWindowFull(pos, ceiling)` next to `exceedsContextWindow`, and the five hand-rolled context-full checks now go through it. Three of them compared against raw `llama_n_ctx` and so ignored `perSeqCtxCeiling_`; `evalMessageWithTools` only ever runs on the single-request context where that is zero, so the value is unchanged today and correct if a per-slot ceiling ever reaches it. - `SessionMetadata` moves to `LlmContext.hpp`. The `{nPast, 0, cacheTokens, 0}` layout was hand-encoded at three sites that had to stay in lockstep, and a writer that forgot a retired slot makes an older build evict from position 0. - Deleted `StopReason::LimitReached`, its `SequenceLimit` mapping and its unit test. Nothing has produced it since the slot-limit path became `ContextOverflow`. The JS `stopReason` union keeps `sequenceLimit`: the ordinal is pinned by a static_assert and by the positional `STOP_REASONS` array, so removing it would renumber `contextOverflow`. Tests: - The second prefill guard, `cached tokens N plus prompt tokens M`, had no desktop coverage after the deleted `sliding-context.test.js`. Added a cached-follow-up case that reaches it, and both overflow tests now pin the wording of the guard they hit rather than a `/context overflow/i` that matches either. The SDK's overflow parser matches each separately. - Restored the `generatedTokens < predict` assertion that proved the prediction cap was not the stopper. - `test/integration/_context-overflow.js` owns the tokenizer-sensitive sizing and the assertions both files shared, so a chat-template or model pin bump is recalibrated once. - One configurable `FakeKvCacheOps` in `test/unit/test_kv_cache_ops_fake.hpp` replaces the three near-identical `IKvCacheOps` fakes.
…ome mapping Second review pass on #3938. Closes the two gaps the first pass argued around instead of fixing. `compactKvRange` now reports the cursor that memory has, not the one the arithmetic predicts. `seq_add` is void, and there are ways for the cells not to move while `seq_rm` still reports success: a memory module sharing cells with another no-ops both halves, and a software cursor that has drifted from live memory gets a shift that lands elsewhere. Either way the old code returned `Compacted` with a cursor no live cell backed, and the driver then wrote a cache header describing memory that does not exist. `seq_pos_max` is read back after the shift and a disagreement with `nPast - discarded` is `MemoryOperationFailed`. `IKvCacheOps` gains `seqPosMax` for it. This is the reachable half of the K-shift request. Applying `init_update` from the addon is not: this fabric exposes no `llama_memory_update` or `kv_self_update`, the K-shift runs inside `llama_decode`. The readback catches the same symptom from the other side, which is a shift that did not land. `ReasoningBlockCompactor::attentionOutcomeFor` extracts the primitive-to-outcome mapping as a pure function so the `NoOp` case is pinned by a direct test. Reaching it through `compact()` is impossible, its guards clamp every span first, which is why the earlier attempt could not write the regression. Test fake models a resident sequence now, so `seqPosMax` answers from that model rather than echoing the primitive. `withResidentTokens` has no default and `seqPosMax` fails the test without it, so a readback assertion can never become a rubber stamp.
… not land The readback added in bb2d1e1 reported `MemoryOperationFailed`, which the compactor maps to `FailedKvIntact`. That outcome promises live KV still matches the caller's cursor and asks the caller to unwind by trimming its tail. Neither holds here: `seq_rm` has already run by the time the readback fires, so the hole is in the middle of the cache and a tail trim cannot reach it. The driver would have trimmed against a cursor that no longer described memory. `CompactRangeOutcome::Kind::MemoryInconsistent` splits the mutated case from the refused-before-any-write one, and `attentionOutcomeFor` maps it to `FailedKvWiped`, whose contract is the one that fits: clear the sequence, reset positional accounting to zero. `compact()` now wipes on that path, so the outcome and live memory agree. Also trimmed the comments added across this review round down to the non-obvious why.
Reasoning-block compaction still shifted the cache on pure-attention models: `seq_rm` the reasoning span, then `seq_add` the tail down over it. That is context shifting, which this ticket is removing, so it goes too. Every model now does what Qwen3.5 already did, rewind to the end-of-prefill boundary and replay the answer after it. Pure attention does not need the full-state snapshot that drove Qwen3.5 there. Its cells are positionally indexed, so the boundary is just a position and rewinding to it is `llama_memory_seq_rm(mem, seqId, boundaryPos, -1)`, a tail trim. Replaying the kept tokens rebuilds exactly what a state file would have restored, so `RecurrentStateSnapshot` grows a position-only mode instead and recurrent memory keeps the file it cannot do without. - Deleted `KvCacheOps.hpp/.cpp` and `compactKvRange`. No `seq_add` remains in the addon, so nothing sets `has_shift` and the deferred K-shift never runs. That also takes the can-shift guard, the `seq_pos_max` readback and the `MemoryInconsistent` kind, which only existed to make the shift safe. - `recurrentReasoningBoundaryDecision` no longer gates on memory kind. Every model anchors a boundary; only the anchor's form differs. - `Outcome::Kind` loses `CompactedAttention` / `CompactedRecurrent` for a single `Compacted`, since there is one path now. - `Outcome::Kind::FailedKvIntact` removed with its hook and both drivers' roll-back-to-pre-request-cursor recovery. Compaction rewinds before it replays, so by the time anything can fail the cache has been written to and only a wipe leaves it coherent. Nothing produced `FailedKvIntact` any more. - The `IKvCacheOps` test seam becomes `IReasoningRewindOps`, covering the two operations compaction actually performs. Behaviour change worth calling out: an unfinished reasoning span now rewinds to the prefill boundary rather than dropping `[start, pos)`. Generation never left the think block in that case, so there is no answer to keep. And compaction now costs a re-decode of the answer rather than pointer math over cells, which is the price of not shifting.
Compaction replays the kept tokens through `llama_decode` after generation ends. Two stats broke on that, both because they read llama's perf counters. `llama_perf_context` keys on batch size, not on meaning: `n_queued_tokens == 1` increments `n_eval`, anything larger adds to `n_p_eval` (`llama-context.cpp`). Generation decodes one token at a time, so `n_eval` and "tokens generated" used to agree by coincidence. Replay decodes in batches, so the coincidence ended and a generated token started being reported as a prompt token. - `generatedTokens` is now counted in the generation loops, incremented where a token is committed to the cache. A terminal EOG breaks out before its decode and never reaches KV, so it is not counted, which keeps the stat consistent with the cache growth callers can observe. - `compactThinkSpan` freezes the user-visible prompt counters on every memory kind, not just recurrent. Its guard was written when pure-attention compaction had no `llama_decode` at all; that stopped being true when it started replaying, and replay decodes were landing in the caller's `promptTokens` (24 reported as 26). The unit test that pinned the old snapshot contract now pins the new one.
Compaction rewinds to the end-of-prefill boundary and replays what it keeps. The replay seeded a single close-marker token, so a marker that tokenises to several pieces could not restore a balanced `<think>...</think>` span, and the boundary policy refused those models outright. That refusal was survivable while pure-attention models compacted through `seq_rm + seq_add`, which removed the whole span whatever its tokenisation. It is not survivable now that every model replays: the policy would throw `FailedToDecode` and wipe the sequence for a model that used to work. - `ReasoningState` keeps `cached_close_tag_tokens`, the whole canonical marker, and `recordCloseMarkerForReplay` gained a sequence overload that seeds every piece. Marker length no longer decides whether compaction is possible. - `close_is_single_token` stays for EOS-inside-reasoning substitution, which swaps a sampled EOS for one close token and so genuinely needs a single id. - `RecurrentReasoningBoundaryDecision::UnsupportedMultiTokenClose` has no producer left, so it goes along with both switch cases, the two warnings that can no longer fire, and the now-unused `throwUnsupportedRecurrentReasoningCompaction` and `recurrentReasoningBoundaryFailureReason` helpers. Also from the same pass: - `handleReasoningEOS` counts every token it commits. It decodes the substituted close tag plus up to two newlines, and the caller counted one, so the generated-token stat was short by up to two on an EOS-in-reasoning turn. - Generation TPS is derived from that same count instead of `n_eval`, so rate and token total describe the same set. - A comment claiming the replay seed is a "No-op on pure-attention paths" and an `architecture.md` line naming the deleted `KvCacheOps` are both corrected.
Eight comments still described compaction as `seq_rm + seq_add` on pure-attention models. No such call remains in the addon: every model rewinds to the end-of-prefill boundary and replays.
Follow-up on #3938. Six code fixes plus the stale prose the redesign left behind. Replay correctness: - The multimodal single-prompt close-detection site still seeded the scalar `cached_close_tag_token`. `ReasoningUtils` only assigns that when the marker is one token, so a multi-piece marker seeded nothing and the replay wrote an unbalanced span. It was unreachable while the boundary policy refused those models; it stopped being unreachable when the policy started accepting them. Now passes `cached_close_tag_tokens`, matching the other three sites. - `recordCloseMarkerForReplay` gained the close-side half of the single-block policy. `setOpenSpan` already ignored a second opener; without the mirror a second `</think>` appended its marker behind the captured answer and bumped the seed count, which raised the clip cap so the stray marker survived into the replay and `discarded` went negative. - `handleReasoningEOS` stops before decoding a newline into a cell that does not exist. The generation guard proves room for one token and the substituted close tag takes it, so with nothing left to evict the injection used to log a decode ERROR on an ordinary full-context boundary. Stats: - `sampleAndAppendIdle` no longer records a `LLAMA_TOKEN_NULL` sample. The MTMD context-overflow return produces one, and `generatedTokens` is both the feed queue and the runtime-stats count, so it queued an invalid id and reported one token more than reached the cache. - Corrected the `singleRuntimeStatsLocked` comment. A replay of exactly one token decodes with `n_queued_tokens == 1` and so does land in `t_eval_ms`, which the old wording denied. Reading the snapshot instead would drop the final decode from every request, the wider of the two errors. Other: - The two multimodal prefill guards go through `exceedsContextWindow(..., isPrefillOnlyRequest_)` like every other admission site, so a cache-warm prompt that exactly fills the window is accepted rather than refused. - Reserve the replay buffer on first use. Tests: - `NoOpForPureAttentionModels` and its pre-reasoning twin asserted that pure attention never seeds the replay buffer, which the replay redesign made false. They passed only because the fixture seeded no boundary, making them duplicates of the two `NoOpWhenBoundaryNotCaptured` cases. Both now seed a boundary and assert the seed lands. - Added `IgnoresCloseMarkerAfterSpanClosed` and `NullSampleIsNotRecorded`. Prose the redesign left stale: - The `RecurrentReasoningBoundaryDecision` doc still named the single-token close as a hard requirement and referred to an `Unsupported*` state that no longer exists. - Two compactor comments still described two `Failed*` outcomes and a `preRequestCursor` recovery, both removed with `FailedKvIntact`. - The lifecycle table still had `onPrefillComplete` triggering a context-shift check. - The OpenCL note still listed KV-cache shifts as a trigger; no `seq_add` call site remains in the addon. - A reasoning integration comment still called `generatedTokens` raw `n_eval`. C++ unit tests: 835 pass, 2 skipped for missing model fixtures. clang-format and clang-tidy clean on every changed translation unit.
Pure attention reaches `compact()` with an open span. The recurrent guard `shouldRollbackInterruptedReasoning` requires `needsRecurrentSnapshot`, so only recurrent and hybrid models divert to a rollback; pure attention falls through to compaction, and the open-ended branch deliberately lets it through. That was safe while pure attention compacted with `seq_rm` + `seq_add` over `[start, pos)`, which removed the whole span including the pieces that opened it. It stopped being safe when every model started replaying: the seeded prefix runs up to AND INCLUDING the open marker, those tokens live inside the dropped range, and no close marker is ever captured on an unfinished span. So the replay rebuilt a `<think>` with nothing to close it and the next turn resumed from an open block. Reachable on any pure-attention reasoning model with `remove_thinking_from_context` on, whenever generation stops inside the think block: an `n_predict` cutoff, an antiprompt hit, or a full context. `clipSeededPrefix` drops the seeded tokens past a cap, keeping any captured tail behind them, and the open-ended path caps at `start - snapshotPos`, what sat before the span. A closed span is unaffected: its opener is balanced by the close marker the replay also seeds. `OpenSpanDoesNotReplayTheOpener` pins it. The existing `ResidentOpenSpanRewindsToBoundary` passed with an empty replay buffer, so it never saw the seeded prefix production builds. C++ unit tests: 836 pass, 2 skipped for missing model fixtures. clang-format and clang-tidy clean on both changed translation units.
…mit site Three fixes that were left as follow-ups on the first review pass. **Compaction replay no longer runs with the scheduler mutex held.** `finalizeTerminalDriver` reaches `onGenerationFinished`, and for a reasoning turn that means `compactThinkSpan()` rewinding and replaying the kept tokens through `llama_decode`. `stepLocked` already drops the lock for the main decode and for media eval; `drainFinishedLocked` did not, so a replay stalled every co-tenant slot and blocked a cross-thread `cancel()` for its whole duration. On a phone that is a Stop tap that does not land. Unlike the decode window this one holds a reference into `slots_` across the unlock, so the usual reconcile-on-every-reacquisition is wrong here: a cancel recorded during the window still passes `slotOwnedByLocked`, because the slot keeps its `admissionId` until `freeSlot` and `extractFinished` only removed it from the batcher. Reconciling would run `onCancel` on a driver mid-finalize and free the slot the drain loop still holds. `TeardownDeferGuard` suspends application for the window. Nothing is dropped: `applyDeferredTeardownLocked` returns before it swaps the pending vectors out and `clearRequested_` stays set, so the worker applies everything at its next loop top, where the apply-time ownership re-check discards the records whose slot has since been freed. Guard order in the loop is deliberate, the unlock guard is destroyed first so it reacquires while suspension is still active. **`generatedTokens` counts the same thing on both paths.** The single-prompt path counts at the commit site, so a sample that ends the sequence is excluded. The batch path recorded whatever the sampler returned. A terminal EOG, an antiprompt hit, a prediction limit and a context overflow all mark the slot finished, `fillBatch` then filters it out, and the token is dropped without ever being decoded, so it was reported but never reached the cache. `generatedTokens` is also the feed queue, which is why the null id the MTMD overflow return produces had to stay out of it. **Session metadata fails closed on a downgrade.** Slots 1 and 3 stay retired and this build's readers still ignore them, but writing 0 there pointed an older build's eviction at position 0 and silently dropped the system prompt and tool definitions. They now mirror the live cursors, which drives that build's `leftTokens = currentPos - protectedPrefixPos - discard` negative so it refuses the slide and reports a context overflow with the cache intact. Tests: `TeardownDeferralKeepsPendingCancels`, `FinishingSampleIsNotRecorded` and `RetiredSlotsMirrorTheLiveCursors`. Each was confirmed to fail with its fix reverted. C++ unit tests: 839 pass, 2 skipped for missing model fixtures. clang-format and clang-tidy clean on every changed translation unit.
gianni-cor
left a comment
There was a problem hiding this comment.
Inline reasoning-compaction follow-up.
| @@ -459,9 +400,19 @@ ReasoningBlockCompactor::Outcome ReasoningBlockCompactor::compact( | |||
| // tail trim has since removed from the live cache, | |||
| // without touching the structural prefix. | |||
| const llama_pos snapshotPos = rollback_.reasoningBoundaryNPast(); | |||
There was a problem hiding this comment.
The fix should preserve the no-context-sliding direction, but compaction still needs a boundary before the reasoning span. Hybrid and pure-attention models should produce the same final cache: no reasoning body and no leftover <think> / </think> scaffold. Right now the shared boundary/replay path can restore from the end-of-prefill boundary, which is after a forced-open <think> suffix, so those opener tokens can survive the compaction. Suggested fix: make the compaction boundary semantically be spanStart for all model kinds, replay only the visible post-reasoning tail, and avoid seeding/replaying structural reasoning markers into the compacted cache. Hybrid/recurrent models can use a different implementation mechanism, such as a full-state snapshot before the span or a sanitized prompt replay, but the final cache should match pure attention. Please add coverage for forced-open pure-attention and hybrid/recurrent paths to verify the next cached turn does not resume inside reasoning.
gianni-cor
left a comment
There was a problem hiding this comment.
Inline pure-attention closed-span follow-up.
| if (hasCapturedCloseSpan()) { | ||
| return; | ||
| } | ||
| rollback_.appendPostReasoningToken(id); |
There was a problem hiding this comment.
Closed pure-attention spans have a related issue here: by removing the recurrent-only gate, pure-attention now seeds structural reasoning markers into the replay buffer too. Later clipPostReasoningTokens() preserves the seeded prefix, so a closed span can compact to a cache containing <think></think> plus the answer tail instead of removing the full reasoning scaffold. Recurrent/hybrid models may need structural replay to keep hidden state balanced, but pure-attention models should produce the same final logical cache without leftover reasoning markers. Suggested fix: split the replay policy by memory kind or by compacted-span semantics. For pure attention, trim from spanStart and replay only tokens outside the span, especially the visible answer tail; do not replay <think> / </think> marker seeds. Please add a closed-span pure-attention test that verifies the compacted cache contains the answer tail but no leftover reasoning scaffold.
🎯 What problem does this PR solve?
firstMsgTokensso the system prompt was never what got dropped. It was opt-in throughn_discardedand defaulted to off, so the shipped behaviour was already the non-sliding path while the slide machinery added state every other path had to keep correct.seq_rmthe<think>span, thenseq_addthe tail down over it. That is context shifting under another name, so it goes too.📝 How does it solve it?
Sliding removed.
ContextSliderandContextShifterare gone, along withfirstMsgTokens/protectedPrefix,slideCapableadmission,applySlide,supportsSlidingandSequenceStepResult::discarded.Overflow is now the single path
n_discarded=0already took. A prefill that does not fit throwsContextOverflow. A generation that fills the window stops withstopReason=contextOverflowand still returns what it produced, so a caller can tell a full context from a prediction-limit cutoff. Both prefill guards name the quantity they report.Compaction replays instead of shifting. Every model rewinds to the end-of-prefill boundary and re-decodes what it keeps, which is what the recurrent path already did. Pure attention does not need that path's full-state snapshot: its cells are positionally indexed, so the boundary is just a position and rewinding is a tail trim.
KvCacheOps.hpp/.cppandcompactKvRangeare deleted, so noseq_addremains in the addon and the deferred K-shift never runs.Outcome::Kindcollapses to oneCompacted, andFailedKvIntactgoes with both drivers' roll-back-to-pre-request-cursor recovery: compaction rewinds before it replays, so by the time anything can fail only a wipe leaves the cache coherent.Compaction was pointer math over cells and is now a re-decode of the kept tokens, once per reasoning turn. Measured on Qwen3-0.6B on Metal that is 0.85 ms to replay 30 tokens and 2.2 ms to replay 71, so it scales like prefilling those tokens rather than like generating them.
A multi-token reasoning close marker works now. Replay seeds the close marker to keep the restored span balanced and used to seed a single token, so the policy refused markers that tokenise to several pieces. Survivable while pure attention used
seq_rm + seq_add; not survivable once every model replays. The wholecached_close_tag_tokenssequence is seeded instead, and the refusal path is deleted with it.An unfinished reasoning span no longer replays the tokens that opened it. The seeded prefix runs up to and including the open marker, and those pieces sit inside the range compaction drops. Nothing balances them when generation never reached the close, so replaying them rebuilt a
<think>the next turn resumed from with nothing to close it.clipSeededPrefixcaps the prefix at what sat before the span. Only pure attention could reach this: the interrupted-reasoning rollback covers recurrent and hybrid, and pure attention used to drop the whole span withseq_rm + seq_add, so it appeared when every model started replaying. A closed span is unaffected, its opener is balanced by the close marker the replay also seeds.Compaction replay runs outside the scheduler mutex.
finalizeTerminalDriverreachescompactThinkSpan, so on a reasoning turn the drain now runs a realllama_decode.stepLockedalready drops the lock for the main decode and for media eval;drainFinishedLockeddid not, so a replay stalled every co-tenant slot and blocked a cross-threadcancel()for its whole length. This window holds a reference intoslots_across the unlock, so the usual reconcile on every reacquisition is wrong here: a cancel recorded during it still passesslotOwnedByLockedand would runonCancelon a driver mid-finalize.TeardownDeferGuardsuspends application for the window without dropping anything, and the worker applies the records at its next loop top where the ownership re-check discards the stale ones.generatedTokensno longer comes from llama's perf counters.llama_perf_contextkeys on batch size, not meaning:n_queued_tokens == 1bumpsn_eval, larger bumpsn_p_eval. Generation decodes singly, so the two agreed by coincidence until replay started decoding in batches. The stat is counted in the generation loops now, where a token is committed. On the single-prompt path that restores the old numbers rather than redefining them. The batch path used to count whatever the sampler returned, which included the sample that ended the sequence, so it now counts at the commit site too and reports one less. See Breaking Changes.Batch path.
advancemarked a slot finished the momentcurrentPoshitmaxTokensPerSequence, so it was filtered out before the driver's overflow check and ordinary generation reportedsequenceLimitfor a full window. That limit IS the slot's share of the context, andsubmitrejects any request that would not fit, so a full window is the one thing it can mean. It now says so.Shared helpers.
contextWindowFull(pos, ceiling)replaces five hand-rolled context-full checks that had drifted between>=,+1 >, rawllama_n_ctxandctxCeiling().SessionMetadatamoves intoLlmContext.hppso the{nPast, nPast, cacheTokens, cacheTokens}layout has one home instead of three.Session metadata keeps its four-slot width so files written by either build still load. Slots 1 and 3 are retired and this build's readers ignore them, but they are not written as 0: an older build reads slot 1 as its protected prefix and evicts from there, so a 0 would point that at position 0 and silently drop the system prompt and tool definitions. They mirror the live cursors instead, which drives that build's slide guard negative so it refuses the slide and reports a context overflow with the cache intact.
The SDK half is in #3999, and the two can land in either order:
sdkandinferencepin@qvac/llm-llamacpp@^0.45.0from npm, and #3999 still parses the published wordings. A fabric bug found on the way is fixed in tetherto/qvac-fabric-llm.cpp#213; this PR does not depend on it, since it removes the addon's only route into that code.🧪 How was it tested?
reasoning15/15 with 10 real compactions through the replay path,api-behavior14/14,cache-state-machine21/21,generation-params2/2.api-behavior.test.jscovers both prefill guards, including a cached follow-up that only the second guard can reject, and pins each guard's wording since the SDK parser matches them separately.test/integration/_context-overflow.jsowns the tokenizer-sensitive sizing both files share.sliding-context.test.jsandmrope-sliding-context.test.jsdeleted, with all three mobile registries updated so a run does not abort on an unregistered test.clang-formatandclang-tidyclean on every changedaddon/srctranslation unit.💥 Breaking Changes
n_discardedis no longer consumed, so it reaches llama's own argument parser and fails model load as an unknown option.BEFORE:
AFTER:
A batched sequence that fills its window reports the full context instead of the per-sequence cap.
BEFORE:
AFTER:
A batched request reports one less generated token. The sample that ends a sequence is never decoded, so it never reached the cache and is no longer counted. This matches what the single-prompt path has always reported.
TPSshifts with it.BEFORE:
AFTER:
🔌 API Changes
contextSlidesis gone from the runtime stats snapshot and fromRuntimeStatsin the type declarations.