Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces synchronization and validation logic for hybrid prefetch results across attention groups to enforce an all-or-nothing policy. If any hybrid pool falls short, the prefetch result is discarded and resources are released. Feedback on the changes points out a potential race condition in hiradix_cache.py where extra_pools are released unconditionally even if the pool transfers are not yet complete, suggesting to only release them when operation.pool_transfers_done is true.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| self.cache_controller.append_host_mem_release( | ||
| host_indices=operation.host_indices[:completed_tokens], | ||
| extra_pools=pool_transfers, | ||
| ) |
There was a problem hiding this comment.
In _sync_and_check_hybrid_prefetch_result, extra_pools is released unconditionally as pool_transfers. However, if the prefetch operation has not completed its pool transfers yet (operation.pool_transfers_done is False), the background IO thread might still be holding or writing to these transfer buffers. Releasing them prematurely can lead to a race condition or use-after-free/double-free issues.
To prevent this, we should only release pool_transfers if operation.pool_transfers_done is True, matching the logic used in unified_radix_cache.py.
| self.cache_controller.append_host_mem_release( | |
| host_indices=operation.host_indices[:completed_tokens], | |
| extra_pools=pool_transfers, | |
| ) | |
| self.cache_controller.append_host_mem_release( | |
| host_indices=operation.host_indices[:completed_tokens], | |
| extra_pools=pool_transfers if operation.pool_transfers_done else None, | |
| ) |
b7c5c2b to
edb953b
Compare
edb953b to
9b2a519
Compare
| and getattr(operation, "pool_transfers", None) | ||
| and not getattr(operation, "pool_transfers_done", True) | ||
| ): | ||
| can_terminate = False | ||
|
|
||
| operation_terminated = operation.is_terminated() |
There was a problem hiding this comment.
But under can_terminate_prefetch, does wait_complete still need to check pool_transfers_done?
|
/tag-and-rerun-ci |
7e6f321 to
6de9b08
Compare
|
Rebase to latest code of main branch. |
| transfer.host_indices = operation.host_indices | ||
| if transfer.keys is None: | ||
| transfer.keys = operation.hash_value | ||
| pass |
There was a problem hiding this comment.
should this be passed?
for dsv4, some sidecar components may share indices with swa, not kv
There was a problem hiding this comment.
This else branch is transfer.indices_from_pool == PoolName.KV. The case you mentioned are in the previous if branch, not removed.
| self._barrier_attn_groups() | ||
| self.dec_host_lock_ref(last_host_node_id, anchor_lock_params) | ||
| del self.ongoing_prefetch[rid] | ||
| pool_transfers = [x for xfers in comp_xfers.values() for x in xfers] |
There was a problem hiding this comment.
pls add some unit test for this case
There was a problem hiding this comment.
Done. Added test_release_aborted_request_l3_prefetch_io_in_progress and test_release_aborted_request_l3_prefetch_io_done.
hzh0425
left a comment
There was a problem hiding this comment.
Almost lgtm, just a bit worried that this might break the dsv4 hicache
| prefix_keys = operation.prefix_keys | ||
| kv_derived_transfers = [ | ||
| transfer | ||
| for transfer in getattr(operation, "pool_transfers", []) |
There was a problem hiding this comment.
For full-only model, this might throw error?
There was a problem hiding this comment.
Fixed. Tested Qwen3-32B with L3.
| @@ -28,6 +28,7 @@ | |||
| HiCacheStorageExtraInfo, | |||
There was a problem hiding this comment.
btw, We need to benchmark to see if the current segmented KV + sidecar mode causes any performance regression.
We can use moocnake to benchmark the latency under patterns like 256k and 512k
There was a problem hiding this comment.
Done. I have tested that for 256K. My box cannot test 512K.
a25e5ea to
b137657
Compare
|
I tested GLM 5.2 w4a8 on H20 box with 256K L3 prefetch. Testing method: # sglang
nohup env HF_HUB_OFFLINE=1 \
SGLANG_ENABLE_UNIFIED_RADIX_TREE=1 \
sglang serve \
--model-path=PhalaCloud/GLM-5.2-W4AFP8 \
--tp-size=8 \
--kv-cache-dtype fp8_e4m3 \
--enable-hierarchical-cache \
--hicache-storage-prefetch-policy wait_complete \
--hicache-ratio=2 \
--enable-metrics \
--host=0.0.0.0 \
--mem-fraction-static=0.75 \
--hicache-ratio=2 \
--reasoning-parser glm45 \
--tool-call-parser glm47 \
--hicache-storage-backend=mooncake \
--hicache-storage-backend-extra-config '{"master_server_address": "127.0.0.1:50051", "local_hostname": "localhost", "metadata_server": "http://127.0.0.1:8080/metadata", "global_segment_size": "512gb", "protocol": "tcp"}' \
> sglang.log 2>&1 &
# Run this command for 11 times. The first time will fill L3 cache. The rest are prefetch from L3.
python benchmark/hicache/bench_multiturn.py \
--num-clients=1 \
--num-rounds=1 \
--disable-auto-run \
--model=PhalaCloud/GLM-5.2-W4AFP8 \
--request-rate 1 \
--request-length $((256*1024)) \
--output-length 64I ran this for baseline (131bd51) and this PR (b137657). The results are very interesting. After this PR, TTFT is reduced ~10s -> ~9s.
The reason is, before this PR, there is a large sidecar read in the end of prefetch, which results into long-tail. Here are a prefetch for baseline and PR. The first is baseline. There are large requests that other requests in the end. |
77e3feb to
079467d
Compare
|
Rebased to latest code. |
079467d to
ee76c62
Compare
|
Replay the same benchmark as #30393. Run sharegpt benchmark. Flush cache. Re-run sharegpt benchmark, loading from L3. The accept len and accept rate seems similar before flush cache. Before flush cache: accept len ≈ 3.58,accept rate ≈ 0.52 The results are similar to the results in #30393. |
|
Tested dsv4 flash 0731 with EAGLE3. Got similar results as in link. Need to patch #33344. Before flush cache: 2.461 / 0.487 |
…project#32035 sgl-project#33656 sgl-project#32183 sgl-project#33145) Applied PRs (latest from GitHub): sgl-project#33288 Indexer logits OOM fix sgl-project#30393 HiCache packed/sidecar draft caches sgl-project#31170 DPA prefix_affinity load balancing sgl-project#33795 DSpark compact ragged-verify CUDA graph JIT race sgl-project#32467 C128 plan-kernel warp barrier sgl-project#33865 DSpark x prefill CP unblock sgl-project#30371 SWA state pool sizing (storage page) sgl-project#33358 FlashMLA norm-rope K-tokens-per-block ILP sgl-project#33872 num_draft_tokens clamp + extend_len==0 skip (supersede sgl-project#32183) sgl-project#34002 Sidecar backup vacuously-successful fix (replaces sgl-project#33656, with tests) sgl-project#33862 Reclaim redundant host mirrors after storage backup sgl-project#31315 Avoid repeated Mooncake gets after stale hits sgl-project#32327 Q8KV8 sparse MLA prefill backend (flashmla_sparse_q8) sgl-project#31668 Fix sidecar pool life-time (use-after-free on prefetch abort) sgl-project#31195 TP0 verify-token-budget broadcast (adapted to get_schedule() API) Dropped (per user request or superseded): sgl-project#32771 IndexCache C4 top-k reuse — has bug sgl-project#32035 DSpark C128 online compressor — has bug sgl-project#33656 Superseded by sgl-project#34002 (same fix + unit tests) sgl-project#32183 Superseded by sgl-project#33872 (included in supersede PR) sgl-project#33145 Base f01f706 already has superior reasoning-effort profile system Conflicts resolved: sgl-project#31195: adapted to base get_schedule().disable_overlap_schedule API sgl-project#32327: path remapped jit_kernel/ -> kernels/jit/ and kernels/ops/attention/ sgl-project#31668: applied cleanly on top of sgl-project#30393+sgl-project#34002+sgl-project#33862 modifications
|
This PR is moved to #27010. |
… use-after-free)
|
Close this issue, as #27010 has been merged. |

Motivation
This PR fixes #31445.
Modifications
There is a life-time issue for sidecar pools. If a prefetch request is aborted before prefetch reads in the IO thread, the host indices are freed, result into a use-after-free hazard.
There 3 changes:
a) Move the prefetch code KV-derived pool to
CacheController.page_transfer, and use the same variablePrefetchOperation.completed_tokensto track its progress. Do not save hits toPrefetchOperation.extra_pool_hit_pages. Only non-KV derived pools (e.g. SWA) are performed inHybridCacheController.b) Use
pool_transfer_doneas a completion mark for non-KV derived pools, e.g. SWA. Use that variable to determine whether the scheduler thread or the prefetch IO thread to release memory when the prefetch operation is terminated early, e.g. in best effort mode.c) There protection in the original code, as introduced by #27550, does not cover
best_effortcode path. It is removed in this PR. The original production (by checkingpool_transfers_doneincan_terminate) introduced another issue: If the prefetch thread stucks at reading side-car pool, the prefetch of that request will not be timeout in scheduler thread.Tests
SWE-Bench
Tested SWE-Bench on a single-box sglang server with best_effort configuration and assertion enabled in prefetch IO thread to detect use-after-free in host memory pool. Scores are good. No assertion failure occured.
DSv4 Flash
Cache hit: L1=8.2% L2=18.4% L3=38.7
(*) Testing scores may vary due to randomness and environment issues.
GLM-5.2
Cache hit: L1=3.7% L2=1.6% L3=14.1%
(*) Testing scores may vary due to randomness and environment issues.
Multi-turn benchmark
Tested multiturn_bench on DSv4 flash and GLM5.2. The L3 cahce hit ratio seems good. (The last two rounds should hit L3.)
DSv4 Flash
GLM-5.2
Reproduce case
Passed the reproduce case (#32197).
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ❌ Run #31145822964
Latest PR Test (Extra): ✅ Run #31145822817