Repository navigation
Conversation
SGLang refreshes every radix node that a match or insert walks, including a child whose edge is only partially matched and then split, so the retained suffix and the new prefix node are both most recently used (legacy `_match_prefix_helper` and `_insert_helper` in python/sglang/srt/mem_cache/radix_cache.py L731-L739 and L794-L813 at v0.5.20; the default unified tree core touches the walked child and refreshes it again in `_split_node`, unified_cache/unified_tree_core.py L1196-L1200 and L1396). Re-inserting a sequence whose pages are all cached walks and refreshes the same path. The simulator split a partially matched child without refreshing it, so its suffix kept the old timestamp, and an insert with no new pages returned before refreshing the path. Refresh the child before splitting it on the match and insert paths, and refresh the start path even when the inserted suffix is empty. Read-only admission matching and Belady's page-wise eviction are unchanged. Signed-off-by: Rudy Pei <yanrpei@gmail.com> Co-authored-by: Isaac <no-reply@databricks.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title "fix: refresh SGLang radix recency on splits and cached re-inserts" names the actual behavioral change: recency refresh during partial-edge splits and cached-prefix reinserts in the radix ca… |
| Description check | |
The description follows the repository template. It explains the problem and behavior change, names the riskiest file, and states that the public contract is unchanged. Evidence is detailed and includ… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Cross-Layer Contract | |
PASS: The PR changes only Rust cache behavior and synchronized third-party notices. crates/core/src/engine/cache/mod.rs:6 keeps radix_cache crate-private, and the changed method signatures remain … |
| Modeling And Data Evidence | |
The PR provides provenance and reproducible behavioral evidence. THIRD_PARTY_NOTICES.md identifies SGLang v0.5.20, commit 94602c9c2b7cbdb8efd5c52802dac6a1c180089e, and the adapted source paths. Th… |
| Compatibility Boundaries | |
No compatibility boundary changed. The PR changes only private crates/core/src/engine/cache/radix_cache.rs behavior and two synchronized notice files; cache::radix_cache remains pub(crate) (crat… |
| Review Evidence | |
The PR description names the relevant local commands and exact results: the full cargo test -p aisimulate-core run and the focused radix-cache test run. It records boundary coverage for page sizes 1… |
Full details: Title check
-
Explanation
The title "fix: refresh SGLang radix recency on splits and cached re-inserts" names the actual behavioral change: recency refresh during partial-edge splits and cached-prefix reinserts in the radix cache. It matches the radix_cache.rs changes and the PR objectives. It is specific and avoids vague wording, so it passes the title requirements.
Full details: Description check
-
Explanation
The description follows the repository template. It explains the problem and behavior change, names the riskiest file, and states that the public contract is unchanged. Evidence is detailed and includes test commands, results, boundary cases, and a controlled-clock expected-value derivation. The Modeling or data provenance section gives the SGLang source and states that the tests use synthetic data. Minor gaps: the Review map does not state a CodeRabbit review (it says 'not reviewed', which is acceptable), the Fast CI and Full CI fields say 'not run' without explanation, and the Closes field says 'None' instead of being left as a plain link. The SPDX header from the template is omitted, which is acceptable for a PR body. Overall the description is mostly complete.
Full details: Cross-Layer Contract
-
Explanation
PASS: The PR changes only Rust cache behavior and synchronized third-party notices.
crates/core/src/engine/cache/mod.rs:6keepsradix_cachecrate-private, and the changed method signatures remain internal; no schema, CLI, serialization, or public output contract changed. Rust consumers are limited tosglang_backend.rscalls at lines 370, 598, 633, and 663. The new behavior is covered inradix_cache.rs:1644-1710for token and hashed insertion, locked and unlocked matching, empty-suffix reinsertion, and page sizes 1, 4, and 64. BothTHIRD_PARTY_NOTICES.mdfiles document the adapted source and behavior. Repository-wide search found no Python or CLI consumer of the changed cache APIs.
Full details: Modeling And Data Evidence
-
Explanation
The PR provides provenance and reproducible behavioral evidence.
THIRD_PARTY_NOTICES.mdidentifies SGLang v0.5.20, commit94602c9c2b7cbdb8efd5c52802dac6a1c180089e, and the adapted source paths. The changed tests use a controlled clock, fixed page sizes, and exact expected evicted page identities and retained prefix lengths (crates/core/src/engine/cache/radix_cache.rs:1597-1710). The PR description also reports deterministic before/after results: the new cases fail on the base revision and pass on the final revision. This is explained golden-output evidence, not binary-only validation.
Full details: Compatibility Boundaries
-
Explanation
No compatibility boundary changed. The PR changes only private
crates/core/src/engine/cache/radix_cache.rsbehavior and two synchronized notice files;cache::radix_cacheremainspub(crate)(crates/core/src/engine/cache/mod.rs:6), with no changed schemas, bindings, versions, manifests, or defaults. The root and wheel notice copies are byte-identical. The release contract still defines exactly oneaisimulatewheel and oneaisimulate-corecrate (docs/ci/release.md:8-28), and the release validator still enforces that manifest and artifact set (scripts/release/build_release_artifacts.py:45-97, 155-161). No Dynamo dependency or integration was added. This matches the downstream boundary, where Dynamo consumes separately publishedaisimulate-coreand a pinnedaisimulatewheel (ai-dynamo/dynamo@d043ae0, Cargo.toml:53-55; container/deps/requirements.aisimulate.txt:4-6).
Full details: Review Evidence
-
Explanation
The PR description names the relevant local commands and exact results: the full
cargo test -p aisimulate-corerun and the focused radix-cache test run. It records boundary coverage for page sizes 1, 4, and 64, locked and unlocked matches, token and hash insertion, and cached reinsertion, plus negative validation against unchanged main. It separates local test evidence from hosted CI by stating that Fast CI and Full CI were not run. It does not cite any inactive nested workflow as hosted CI evidence, and it makes no unsupported fake-runner, parity, or production claim.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
Superseded by #583, which now contains this change together with the related SGLang cache lifecycle, eviction, reporting, and KV-event corrections. The consolidated branch has passed local core and public-API validation. |
Why and what changed
A partial radix match or insert splits a cached edge without refreshing its retained suffix. Finishing a request whose entire sequence is already cached can also return before refreshing the path. Both leave recently accessed KV older than an unrelated prompt and change the next eviction victim.
Refresh a matched child before splitting it, and refresh the start path even when an insertion has no new pages. Apply this to token and page-hash entrypoints.
Review map
crates/core/src/engine/cache/radix_cache.rs.Evidence
cargo test -p aisimulate-core— 2,446 passed, zero failures, 1 ignored. This full suite ran before the final comment/legal-only changes; final-head focused results follow.cargo test -p aisimulate-core --lib -- engine::cache::radix_cache::tests— 40 passed, zero failures. Nightly formatting and packaged legal-file checks passed.Modeling or data provenance
The behavior follows SGLang v0.5.20 (radix matching and insertion). Tests use synthetic token sequences and a controlled clock; no measured performance claim.
Tracking
None.
This pull request and its description were written by Isaac.