llama : fix K/V and recurrent state cleanup after failed restores - #27530
Open
CHIPMUNK-T0T wants to merge 5 commits into
Open
llama : fix K/V and recurrent state cleanup after failed restores#27530CHIPMUNK-T0T wants to merge 5 commits into
CHIPMUNK-T0T wants to merge 5 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
A failed per-sequence state restore rolls back sequence metadata with
seq_rm(), but does not clear K/V or recurrent-state tensor data already written by the failed restore. In contrast, the corresponding whole-context failure path callsclear(true), so the existing rollback is asymmetric.This PR adds failure containment for failed state restores in both attention and recurrent memory.
llama_kv_cacheandllama_memory_recurrentclear data written by a failed restore.Per-sequence failures before modifying the target sequence leave it unchanged. Failures after modification remove the affected state and prevent tensor writes from that restore attempt from surviving rollback.
A failed per-sequence restore leaves other sequences and subsequent inference unaffected. A failed whole-context restore still clears the whole memory through
clear(true)as before, so all sequences are emptied rather than left holding stale data.This does not provide transactional rollback to the previous sequence state. Return values and server HTTP error behavior are unchanged.
Fixes #27068
Additional information
Failure containment for file and buffer restores
File and buffer restores require different cleanup on failure.
File restores may already have written tensor data when validation fails, so the affected K/V or recurrent-state ranges must be cleared. Buffer restores can still have deferred tensor writes pending at that point, so those writes must be discarded before the reader is destroyed after failure handling.
The memory cleanup and reader
discard()therefore address separate parts of the same failure path rather than providing redundant rollback.Mutation testing confirms that each part is required: removing K/V or recurrent cleanup breaks the corresponding file-restore case, removing host-reader
discard()breaks buffer restore for both memory types, and removing whole-contextdiscard()breaks whole-context restore for both. Removing device-readerdiscard()reachesGGML_ASSERT(buf_size == 0)during reader destruction.Why recurrent memory is included
The original report focused on the K/V cache, but the Qwen3.5 hybrid reproduction can fail in
llama_memory_recurrent, where the same per-sequence rollback asymmetry exists.The fix therefore applies the same containment rule to both attention and recurrent memory, with independent regression coverage for each implementation.
The recurrent cleanup takes a
(head, cell_count)range, andheadis not reliable after a failure:state_read_meta()removes the target sequence first, then returns early whencell_count == 0and returns false whenfind_slot()fails, leavingheadon another sequence's cells. The cleanup is therefore gated onstate_read_meta()having succeeded and uses theheadcaptured before the rollback; on that pathhead + cell_count <= sizeis already asserted instate_read_meta().Why whole-context buffer restore is also changed
The original report treated whole-context restore as contained by
clear(true). That handles tensor data already applied to memory, but not deferred buffer writes that can still be flushed after the rollback.Whole-context buffer restore therefore uses the same
discard()handling so that pending writes cannot overwrite the cleanup performed on failure. The corresponding mutation test fails when thisdiscard()is removed.Tensor clearing
Failed file restores need to clear backend tensor ranges, but
ggml_backend_tensor_memset()is not supported by every buffer backend and can assert on unsupported implementations.The cleanup instead uses
ggml_backend_tensor_set()through the shared internalllama_clear_tensor_data()helper. The helper lives inllama-implbecause the same operation is required by both attention and recurrent memory.The cleanup only runs on restore failure and adds no cost to normal decoding or successful restore.
Tests
Added two regression tests covering
llama_kv_cacheandllama_memory_recurrent.They exercise failed per-sequence file/buffer restore, fragmented restore with another live sequence, the guarded ON_DEVICE restore path, and whole-context buffer restore, with Flash Attention both disabled and enabled.
Without the changes in this PR the new tests fail: the file and buffer restore cases produce logits that do not match the clean baseline, and the ON_DEVICE case aborts in
~llama_io_read_device()atGGML_ASSERT(buf_size == 0). They pass with the changes applied.On the current
d775b8967base:The 8 matching tests include the two new regression tests and existing related state tests.
CUDA ctest validation also passed 8/8 on the same base, with the K/V cache and the recurrent state allocated on the device.
Server-level fault injection was verified on an RTX 4070 with CUDA and Flash Attention enabled using Gemma 3 (
llama_kv_cache_iswa) and Qwen3.5 (llama_memory_hybrid): the corrupted restore poisons subsequent inference before the fix, while inference remains clean after the fix.Non-goals
This PR provides failure containment, not transactional restore. If a failed restore has already modified the target sequence, its previous contents are not restored.
It also does not provide atomic rollback across multiple components of hybrid/iSWA memory.
DSV4-specific restore paths are not changed here; related DSV4 rollback work is tracked separately in #26756.
Related ON_DEVICE issue
The separate ON_DEVICE pre-validation failure is tracked in #27439, with a fix proposed in draft PR #27487 from another contributor.
Known limitations
The regression fixtures directly cover
llama_kv_cacheandllama_memory_recurrent. Hybrid and iSWA paths are covered by the server-level Gemma 3 and Qwen3.5 validation above.Requirements
I have read and agree with the contributing guidelines
AI usage disclosure: YES
llama-serverbuild, reported it in Eval bug: failed slot restore leaves corrupted K/V data that breaks subsequent inference #27068, and defined the scope as failure containment rather than transactional restore.