Skip to content

Fix incomplete HiCache restores that can corrupt GLM-5.3-Flash output - #38161

Closed
ormandj wants to merge 4 commits into
sgl-project:xinyuan/glm-5.3-flash-supportfrom
ormandj:pr/hybrid-dsa-hicache
Closed

ormandj wants to merge 4 commits into
sgl-project:xinyuan/glm-5.3-flash-supportfrom
ormandj:pr/hybrid-dsa-hicache

Conversation

@ormandj

@ormandj ormandj commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Superseded by #38212 against main. This PR auto-closed after #36507 merged and its base branch was deleted. The historical description and validation below refer to this original revision.

Motivation

GLM-5.3-Flash can resume a cached prompt with incorrect attention state after HiCache moves that prompt from GPU memory to host memory and back. The KV values and recurrent state are restored, but the DSA index buffers used to select which tokens to attend to are not. Attention can therefore select the wrong parts of the prompt even though the cache reports a successful restore. This affects the target model even with speculative decoding disabled.

A second defect lets different prompt suffixes share a compressed index row when a prefix-cache split falls inside a compression group. The change keeps those groups intact so one suffix cannot overwrite another suffix's index state.

#38031 reports incorrect GLM-5.3-Flash output after HiCache loadback, including with MTP disabled. This PR repairs the missing-index and shared-row paths; the reporter's 8x H100 full-model configuration has not been validated with this patch. Stacked on #36507; includes the adapted packed-row dependency from #37534.

Modifications

  • Back up and restore target and draft index buffers, skipping empty shared-topk layers.
  • Align radix ownership to complete compressed groups while retaining physical KV transfer pages.
  • Preserve the configured host-memory budget and reject incompatible storage/linker geometry.

Accuracy Tests

The GPU regression simulates eviction and loadback without model weights: fill target/draft index buffers with deterministic bytes, back up noncontiguous pages in two transfers, clear device storage, then restore into different pages and compare every byte. Moving the destination pages prevents an unchanged device buffer from hiding a missing transfer. It also checks that the index tier shares KV addresses without allocating or freeing those addresses independently. CPU tests check compressed-prefix alignment and host-pool construction. These are state-transfer regressions, not a full-model answer-quality reproducer.

PYTHONPATH=python compute-sanitizer --tool memcheck --error-exitcode 99 python test/manual/test_hybrid_dsa_hicache_gpu.py

Author run at 66f6da3a21 on RTX PRO 6000 Blackwell (SM120), Transformers 5.12.1 / Tokenizers 0.22.2: 20/20 transfer cases across four layout/backend pairs, 100/100 mocked controller cycles and ratio-mode assembly passed; Compute Sanitizer reported ERROR SUMMARY: 0 errors. The two hybrid-pool CPU test files passed 30 tests and six subtests.

Speed Tests and Profiling

No isolated speed benchmark.

Checklist

  • Add CPU and GPU regression coverage.
  • Format changed files with upstream tools.
  • Complete final rebased-head tests and review.

Developed with AI assistance.


CI States

Latest PR Test (Base): ❌ Run #34000673291
Latest PR Test (Extra): ❌ Run #34000673170
Latest PR Test (AMD ROCm 7.2): ❌ Run #34000673275

build_hybrid_mamba_stack built the MLA host pool through
build_kv_host_pool without override_kv_cache_dim, so
MLATokenToKVPoolHost sized host rows as kv_lora_rank + qk_rope_head_dim.
A DSA pool that stores packed fp8 rows has a wider kv_cache_dim, and
every HiCache transfer path assumes host rows have the device pool's
row geometry (the kernel paths use the host stride for both sides, the
direct path copies row slices).

Pass the device pool's kv_cache_dim when use_mla, as the DSA strategy
and the HiRadixCache DSA entrypoint already do through
build_anchor_sidecar_stack / build_kv_only_group. The override is gated
on use_mla because MHA host pools take no such keyword.
DecodeKVCacheOffloadManager had the same gap (its MLATokenToKVPool
isinstance gate admits DSATokenToKVPool); pass the override there too.

Make MLATokenToKVPoolHost check its row geometry at construction,
before allocating host memory: the host row width must equal
device_pool.kv_cache_dim (a narrower host row is exactly this defect,
so a caller that omits the override for a packed pool now fails
clearly), and every packed MTP draft pool must have the device pool's
store_dtype and kv_cache_dim (reachable with
--speculative-draft-kv-cache-dtype), instead of allocating and then
transferring with mismatched rows. MLATokenToKVPool always sets
kv_cache_dim; the lookup falls back to the host width only for
duck-typed device pools that do not model row packing.

On main a packed DSA pool does not reach build_hybrid_mamba_stack;
HybridLinearKVPool with use_mla builds a plain MLATokenToKVPool. It
does with the GLM-5.3-Flash series (sgl-project#36507), which makes a
DSATokenToKVPool the hybrid pool's full_kv_pool.

Add CPU unit tests: host row width follows a packed device pool, a
non-MLA pool exposing kv_cache_dim gets no override, a draft pool with
the target's geometry is packed, draft pools with a different dtype or
(same dtype) a different width are rejected, a packed device pool
without the override is rejected by the constructor (alone and with a
same-dtype nominal-width draft), a packed target with the override and
a matching draft allocates, and a nominal-width pool needs no override.
@d3lavar

d3lavar commented Sep 6, 2026

Copy link
Copy Markdown

Hi @ormandj — a heads-up and a big thank-you: your fix resolved a nasty production corruption for us (the exact #38031 symptom set: intermittent stop+null tool-call drops and degenerate repetition loops after HiCache host load-back on GLM-5.3-Flash). We applied the cherry-pick on our 8×H100 node and validated it thoroughly — 3 independent stress runs with fresh nonces (54/54 clean, including 12/12 byte-identical post-eviction load-back replays) plus a deterministic eviction→resume round-trip probe (15/15). Details in the validation comment on #38031.

One problem: #36507 just landed in main (97c6978) without this fix, and this PR got closed unmerged — so right now main (and any image built from it) is exposed to the corruption again, and there is no successor PR.

I verified the diff applies cleanly to current main @ 97c6978 — all 7 files (4 code + 3 tests), zero conflicts:

git apply --check pr38161.diff   # -> clean

Could you please re-file it against main? Happy to test the new PR on our node immediately. If you're short on time, say the word and we'll open the refile PR ourselves with full credit to you.

@ormandj

ormandj commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Refiled as #38212 against main after #36507 merged and this PR's base branch was deleted. The patch is unchanged (matching stable Git patch ID); author CPU regression checks on the new head passed 30 tests and 6 subtests. GPU validation attached here remains tied to the original revision and has not been rerun on the new main-based head. Please continue review on #38212. The reporter's H100 confirmation is linked from the replacement.

@ormandj ormandj mentioned this pull request Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hicache Hierarchical Caching for SGLang unified-radix-cache

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants