Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a bounded TTL negative cache (_FailedGetCache) to the Mooncake store to temporarily suppress existence checks for physical keys that recently failed to load, preventing redundant remote get requests. The feedback focuses on optimizing performance and reducing lock contention in the critical path by introducing batch-oriented methods (add_batch, remove_batch, filter_failed, and update_batch) to the cache, allowing lock acquisition to occur once per batch operation instead of repeatedly in loops.
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.
Add a bounded short-TTL negative cache for failed physical keys so HiCache falls back instead of repeatedly timing out during stale metadata windows. Batch cache updates to take one lock per storage operation. Assisted-by: OpenAI Codex <codex@openai.com> Signed-off-by: 彦纾 <wangyanshu.wys@alibaba-inc.com>
Assisted-by: OpenAI Codex <codex@openai.com> Signed-off-by: 彦纾 <wangyanshu.wys@alibaba-inc.com>
bf11f96 to
427ac11
Compare
|
/tag-and-rerun-ci |
|
CI status clarification: the Ready-state rerun reached pr-gate successfully, but every platform workflow stopped before tests with |
|
@huangtingwei9988 All review threads are resolved in |
|
@ispobock @xiezhq-hermann Could one of the KV Cache merge oncalls please run |
|
/tag-and-rerun-ci |
Thanks for submitting this PR! Could you share under what circumstances you encountered this issue? |
|
/rerun-failed-ci |
|
Thanks for asking. To be precise, I did not first observe this as a production incident; I isolated it while testing the stale data-plane failure mode of the Mooncake-backed HiCache path. The concrete sequence is:
The regression test models that exact observable contract ( I also fixed the registered-test entry-point issue in |
I think the probability of these two situations happening at the same time is quite low. Even if they do occur simultaneously, I personally think a slight performance degradation for up to 10 seconds is acceptable, especially compared to introducing a more complicated prevention mechanism into SGLang. That said, this is just my personal opinion. Ultimately, whether such a mechanism should be added is up to the HiCache maintainers. |
|
CI refresh: merged the latest |
|
/rerun-failed-ci |
|
FYI: this PR helped a lot in our case when replicas exit ungracefully. |
|
Thanks for sharing the production evidence — the 12-replica Qwen3.5 397B case is especially useful. If you have it available, could you share whether the main observed improvement was fewer repeated Mooncake gets, lower error amplification during the master’s failure-detection window, or improved tail latency after an ungraceful replica exit? Even a qualitative breakdown would help validate the negative-cache boundary. |
|
/rerun-failed-ci |
…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
Motivation
Mooncake metadata may briefly report a page as present after the page's only data segment becomes unavailable. During this stale window, SGLang currently repeats
batch_exists -> get timeout -> abortfor every request touching the same physical page key. Under load, repeated transfer timeouts can create a request-queue bubble even though Mooncake eventually removes the stale segment.This PR addresses the remaining live-replica tolerance gap discussed in #29955. It does not change graceful shutdown, does not call
remove_all()during process teardown, and does not overlap the SIGTERM work in #16484.I searched open issues and PRs for
Mooncake stale negative cache,Mooncake failed get cache,HiCache fallback, and #29955 immediately before opening this PR and found no implementation in flight. I also left the intended scope on #29955 before starting.Modifications
batch_is_existhits for those keys during the short TTL so HiCache falls back instead of repeating the remote timeout.failed_get_ttl_seconds(default1.0;0disables the cache)failed_get_cache_max_entries(default65536)The cache affects performance/fallback decisions only. It never returns KV data. Once the TTL expires, the next existence check reaches Mooncake again, so recovered data becomes visible without manual invalidation.
Accuracy Tests
This change does not affect model computation or model output.
Unit and regression tests
Run on commit
427ac11cbin an isolated cluster container:Result:
17 passed, 21 warnings in 19.43s; both CI registry checks passed. The new cases cover partial get failure, get exception, successful and failed put behavior, TTL expiry/retry, explicit clear, and capacity eviction.Real Mooncake TCP backend smoke
Environment:
lingjun-099, Linux5.10.134-16.3.al8.x86_64580.105.08sha256:b8fdd23aa507e59a2d892f82cf6d04c8c86c876235d9ef5e2ab98842c4bea59c3.12.3, PyTorch2.11.0+cu129, Mooncake Transfer Engine0.3.11.post1:18080, master:15051, metrics:19003, store:18081; TCP protocol; 256 MiB client segmentCommand:
Result: all five real-backend flows passed:
The single get reported 0.131 ms, and the four batch gets reported 0.238-0.478 ms for 13 logical pages (13 or 26 physical keys).
Speed Tests and Profiling
The benchmark models the exact stale-window sequence: the backend reports a hit, each data get waits for an injected transfer delay and fails, and the caller retries the same physical page 200 times. Each point has five independent repetitions. The enabled case uses a 1-second TTL.
xychart-beta title "Speedup for 200 repeated stale-page accesses" x-axis "Injected failed-get latency (ms)" [1, 5, 10, 50] y-axis "Speedup (x)" 0 --> 210 bar [164.7, 191.3, 195.3, 199.1]Raw elapsed values (ms):
Five repetitions per point
All 20 runs verified that the first existence check after TTL expiry reached the backend and observed the backend hit. Raw JSON was retained on the test host with one file per repetition; the aggregate SHA-256 manifest hash is
6a9ddfd5bee5e50d695642489e53f6fe5b4d68827299cfb5a9e2eb16e21bfb0d.Formatting
The full pre-commit suite passed on all changed files in a clean Python 3.12 hook environment, including isort, Ruff, Black, codespell, executable-shebang validation, and the SGLang CI registry validators.
Checklist
AI assistance was used for code drafting, test generation, and experiment/report preparation. The resulting changes were validated on the cluster described above.
CI States
Latest PR Test (Base): ✅ Run #30060739341
Latest PR Test (Extra): ❌ Run #30060739169