Skip to content

kv-cache : fix crash in state save/restore - #21576

Draft
ehotting wants to merge 1 commit into
ggml-org:masterfrom
ehotting:fix/iswa-null-kv-tensor-guard
Draft

kv-cache : fix crash in state save/restore#21576
ehotting wants to merge 1 commit into
ggml-org:masterfrom
ehotting:fix/iswa-null-kv-tensor-guard

Conversation

@ehotting

@ehotting ehotting commented Apr 7, 2026

Copy link
Copy Markdown

Status (2026-07-12): draft, superseded. Do not merge.

The guards described below stop the assert, but they silently omit one layer's K and V from the saved state. Measured on current master: exactly 151,576 bytes missing, which is one layer's K plus V. The root cause is an uninitialised view after a Vulkan buffer split (suballocation_block_size, default 1 GiB), and it should be fixed in the allocator.

Full reproduction, instrumentation and measurements: #21576 (comment)

Kept open as a tracker until a root fix lands (@Yoshi4470's 99655d5 or an equivalent). See also #23737 and #19839.


K and V tensors in layer.k_stream/v_stream can have tensor->data == NULL if the tensor object exists but has no GPU memory allocated. The existing V loops guarded for !v but not !v->data, and the K loops had no guard at all, causing GGML_ASSERT(tensor->data != NULL) in ggml_backend_tensor_get during cache state save/restore.

Add !tensor->data null guards matching the existing pattern for both K and V tensors in state_write_data and state_read_data.

Overview

Related to #21468

I ran into this running Gemma 4 26B-A4B with cache reuse and multiple slots. The server would crash with SIGABRT every few minutes. Both write and read paths in llama_kv_cache iterate all layers but some K/V tensors have data == NULL. The write path for V already had if (!v) continue but K had no guard. Both lacked a check for tensor->data.

Additional information

Tested on Gemma 4 26B-A4B, Vulkan backend, 4 slots, cache reuse enabled. 20/20 multi-turn requests stable, previously crashed every 4-5 min.

No impact on models without iSWA since all tensors are fully allocated and the guards never trigger.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES - AI assisted in finding the root cause and setting up local testing.

For iSWA models (e.g. Gemma 4), some K and V tensors in
layer.k_stream/v_stream have tensor->data == NULL. The existing
V loops guarded for !v but not !v->data, and the K loops had
no guard at all, causing GGML_ASSERT(tensor->data != NULL)
in ggml_backend_tensor_get during cache state save/restore.

Add null guards matching the existing pattern for both K and V
tensors in state_write_data and state_read_data.
@ehotting
ehotting requested a review from ggerganov as a code owner April 7, 2026 19:25
@liminfei-amd

Copy link
Copy Markdown
Contributor

This looks like it also fixes the same assert reported in #19839 and #23737 (GGML_ASSERT(tensor->data != NULL) in ggml_backend_tensor_get during state save/restore) — they aren't cross-linked yet, so noting it in case it helps consolidate.

While reproducing that assert I traced it to the per-stream views: layer.k_stream[s] / v_stream[s] are created with ggml_view_2d() over the per-layer cache tensors and are never ggml_backend_view_init()-ed, so view->data == NULL while view->view_src (the cache tensor) is allocated. That's what trips the assert once the host/file state-IO writer calls the backend get/set directly on the view.

One thing worth considering about the added !tensor->data guard (separate from the existing !tensor one): when it triggers, the tensor is a per-stream view whose view_src is allocated, so it still points at real KV data — skipping it omits that data from the saved/restored state. An alternative that preserves the data is to resolve the view to its source before the backend call, carrying the offset:

if (tensor->view_src) { offset += tensor->view_offs; tensor = tensor->view_src; }

which is effectively what the on-device state writer/reader already do for their sub-views via ggml_backend_view_init(). I have a small ggml harness that reproduces the exact assert and shows the resolved read returns the correct bytes — happy to share it, or open it as an alternative, if that's useful.

Either way it'd be good to get this assert fixed; it bites multi-stream slot save/restore (--parallel > 1). Thanks for putting up the fix.

@ehotting

Copy link
Copy Markdown
Author

Reviving this...

Still reproduces on current master (b9967). The per-stream views at kv-cache creation are made with ggml_view_2d() and no ggml_backend_view_init(), so on an iSWA model (Gemma) k_stream[s] / v_stream[s] can be null or have view->data == NULL while view_src is allocated. In state_write_data / state_read_data the k-stream path has no null guard and neither path checks ->data, so state save/restore trips GGML_ASSERT(tensor->data != NULL) in ggml_backend_tensor_get.

I've been running this small patch succesfully on the model that hit it for months.

This is the state-IO path, distinct from #25215 (which fixed the rotation input in llama-graph.cpp). It looks like the same assert as the still-open #23737 (Vulkan since b9318) and the earlier #19839; @Yoshi4470 also has a parallel fix commit referenced on #23737. Cross-linking so the duplicates can be consolidated.

On my approach: this PR is the minimal guard in the state-IO paths (+10/-4, CI green, still mergeable 1277 commits later 😄 ). @liminfei-amd 's trace points at the root cause being the uninitialised views, so initialising them at creation would be the alternative. Happy either way.

@Yoshi4470

Copy link
Copy Markdown

Thanks for the mention. After my earlier comment on #23737, I dug further into the root cause and landed on fixing the missing view init in the allocator.

The change is here:
Yoshi4470@99655d5

The case I reproduced is with Qwen3.6-27B (with MTP layer): when a parent KV tensor exceeds the backend buffer-type max_size (e.g. Vulkan's default 1 GiB), ggml_backend_alloc_ctx_tensors_from_buft splits the allocation. If the context then ends with a view-only tail (e.g. MTP / hybrid layer k_stream / v_stream), that final range can skip ggml_backend_view_init, leaving tensor->data == NULL while view_src is allocated. That trips GGML_ASSERT(tensor->data != NULL) in host state IO (ggml_backend_tensor_get).

The fix separates parent allocation from view init: allocate only parents in each split range, then run a final pass over the context to init all uninitialized views.

This is complementary to the state-IO guards in this PR. Guarding on !tensor->data avoids the assert, but when the view's view_src still holds real KV data, skipping the tensor can omit that data from save/restore. Initializing the views at alloc time addresses that class of failure upstream.

Hope this is useful.

@liminfei-amd

Copy link
Copy Markdown
Contributor

@ehotting Thanks for reviving this. One correction to my earlier comment: the normal KV allocator does initialize stream views via ggml_backend_view_init(). I could not naturally reproduce a null data view on either b8699 or current master with Gemma 4 26B, Vulkan, four slots, including a 262K-context configuration.

I did test the exact claimed state through fault injection: view->data == NULL while view_src->data remains allocated. Unpatched master reproduces the original assertion, but the guards in this PR make restore fail with a mismatched value type. The state still advertises all layers while the guarded layer's metadata and payload are omitted, so the serialized stream becomes misaligned.

Resolving the unallocated view to view_src and carrying view_offs instead preserves the data. The full file, host, and on-device state round trips passed on Vulkan and CPU, including continuation-token equality.

Could you share the exact current-master command and failing log, including the context size, KV-unified setting, model hash, and the first null tensor's layer/stream and view_src values? I have a tested view-preserving patch that I can share. Based on these results, I think the PR should replace the skip guards with that approach.

@liminfei-amd

Copy link
Copy Markdown
Contributor

@Yoshi4470 Thanks, this explains the missing trigger.

I ported 99655d5 to current master and reproduced the oversized-parent/view-only-tail case. Unpatched master leaves the view uninitialized and aborts; your fix passes the focused regression, the existing allocator tests, and a Gemma 4 26B Vulkan state round trip.

Would you consider opening this against current upstream master with the regression test? It would give @ehotting and the maintainers a root-fix option when deciding how to update #21576.

@ehotting

Copy link
Copy Markdown
Author

Disclosure: @liminfei-amd asked for details that are well past my depth in this part of the codebase, so I used Claude Code to help me run these experiments on my machine and to write this up. The builds, the runs and the numbers below are real and reproducible on the hardware described; the analysis and the wording are AI assisted. I am happy to re-run anything, vary any parameter, or share the raw logs.

I reproduced this on current master and ran the experiments. Short version: the assert reproduces, the guards in this PR are the wrong fix, and @Yoshi4470's allocator fix resolves it correctly. I am moving this PR to draft so that it cannot be merged as it stands.

Setup

  • llama.cpp b9976 (e3546c794), Vulkan / RADV (Mesa 26.1.4), AMD Ryzen AI Max+ 395 with Radeon 8060S (gfx1151), 96 GB VRAM carveout
  • Model: gemma-4-26B-A4B-it Q8_0, sha256 7778df567283364f98693690c5b45ebba63eb5658e6e20f2546acce6e8b52e7c
  • Command:
llama-server -m gemma-4-26B-A4B-it-Q8_0.gguf \
  -c 1048576 -np 4 -ngl 999 --no-mmap --flash-attn on \
  --cache-type-k f16 --cache-type-v f16 \
  --slot-save-path /tmp/slots --port 8099

which gives n_slots = 4, n_ctx_slot = 262144, kv_unified = 'false'.

  • Trigger: POST /slots/0?action=save after a 37-token prompt.

The failing tensor

@liminfei-amd, this is what you asked for:

il=29  strm=0  k->data=(nil)  view_src->data=0x1000  view_offs=0

Layer 29 of 30 (the last one), stream 0, both K and V. view_src is allocated, the view is not. Path:

llama_kv_cache_iswa::state_write -> state_write -> state_write_data
  -> ggml_backend_tensor_get -> GGML_ASSERT(tensor->data != NULL)   (ggml-backend.cpp:348)

Why it did not reproduce for you

The trigger is the buffer split. Vulkan's get_max_size returns suballocation_block_size, default 1 GiB. In this config the per-layer non-SWA K tensor is 262144 cells x 1024 (n_embd_k_gqa) x 2 B x 4 streams = 2 GiB, so the allocation splits. At -c 262144 the same tensor is 512 MiB, stays under the limit, and there is no split and no bug.

I confirmed that causally: with -c 262144 (which works normally) plus GGML_VK_SUBALLOCATION_BLOCK_SIZE=268435456, the same assert fires. The context size therefore only matters because it decides whether a single KV tensor exceeds the block size. Backends that do not implement get_max_size (it defaults to SIZE_MAX) never split, so they never hit this at all.

I have only tested one GPU, so the rest is inference from the code rather than measurement: suballocation_block_size defaults to 1 GiB independently of the device, so I would expect any Vulkan backend to hit this once a single KV tensor crosses the block size. Happy to be corrected there.

Results

Same prompt, 37 tokens saved, bytes written by the state save:

build -c state save bytes written
master 1048576 abort n/a
master 262144 ok 8,337,184
master + this PR's guards 1048576 ok 8,185,608
master + this PR's guards 262144 ok 8,337,184
master + @Yoshi4470's 99655d5 1048576 ok 8,337,184

The 151,576-byte gap is exactly one layer's K and V (37 x 1024 x 2 B x 2, plus a 24-byte header). So @liminfei-amd and @Yoshi4470 are right: these guards trade the assert for silently dropping layer 29 from the saved state. The -c 262144 control writes the full amount, which shows the guards only drop data when the split occurs.

@Yoshi4470's 99655d5 applies cleanly to current master, prevents the assert, and writes the full 8,337,184 bytes with a clean restore. That is the correct fix.

Where this leaves the PR

I have moved this to draft. The guards are wrong, and silently omitting a layer is the worst of the available failure modes: the save reports success, the file restores "cleanly", and the model then runs with a missing attention cache and no error anywhere. An assert is at least loud. I am keeping the PR open as a tracker rather than closing it, so that the issue stays visible until a root fix actually lands.

@liminfei-amd, I could not find your view-preserving patch in a branch or a PR, so I assume it is not public yet. Where can I get it? I would like to run it here against the 2 GiB tensor case above.

The same offer stands for @Yoshi4470's 99655d5, which I have already verified on this machine (no assert, the full 8,337,184 bytes, clean restore), and for any combination of the two. I have the reproduction and the hardware, so I am glad to act as the test bed and report byte counts and round trips for whatever you want to land.

If the maintainers conclude that the allocator fix alone is sufficient and no state-IO guard is wanted, I will close this PR.

@ehotting
ehotting marked this pull request as draft July 12, 2026 09:06
@Yoshi4470

Copy link
Copy Markdown

@ehotting @liminfei-amd I opened an allocator-side fix for the missing view init after buft max_size splits:

#25584

It includes a focused regression in test-alloc. On current master with the fix, the previously failing tensor->data == NULL assert no longer triggers in my Vulkan repro. I still have a separate amdvlk64.dll crash afterward; I plan to investigate that separately and do not treat it as part of this allocator fix.

Additional testing on your side would be very welcome, especially around the state save/restore cases you have been hitting. Thanks.

@ehotting

Copy link
Copy Markdown
Author

For anyone landing here from #23737 or #19839: the root cause is in the allocator, not in the state-IO path, and @Yoshi4470 has now fixed it upstream in #25584.

I have tested that PR on the hardware and model this one was opened for. The assert is gone and the saved state is byte-identical to the non-splitting path, including under a forced buffer split.

Full report: #25584 (comment)

The guards in this PR remove the assert but silently omit one layer's K and V from the saved state, so they should not be merged. Keeping this open as a tracker only until #25584 lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants