Repository navigation
Conversation
SGLang v0.5.20 decode caches a transferred prompt only when the request is selected into a PREBUILT running batch, and only with decode radix caching enabled (default off, which leaves a chunk cache that inserts nothing): - https://github.com/sgl-project/sglang/blob/94602c9c2b7cbdb8efd5c52802dac6a1c180089e/python/sglang/srt/disaggregation/decode.py#L2707-L2750 - https://github.com/sgl-project/sglang/blob/94602c9c2b7cbdb8efd5c52802dac6a1c180089e/python/sglang/srt/disaggregation/decode_schedule_batch_mixin.py#L123 - https://github.com/sgl-project/sglang/blob/94602c9c2b7cbdb8efd5c52802dac6a1c180089e/python/sglang/srt/arg_groups/fields/disagg.py#L80-L83 - https://github.com/sgl-project/sglang/blob/94602c9c2b7cbdb8efd5c52802dac6a1c180089e/python/sglang/srt/arg_groups/pd_disaggregation_hook.py#L77-L111 A request aborted from the decode waiting queue is released through the inserting release_kv_cache path: - https://github.com/sgl-project/sglang/blob/94602c9c2b7cbdb8efd5c52802dac6a1c180089e/python/sglang/srt/managers/scheduler.py#L5246-L5248 Activation now keeps the transferred pages private and locks only the matched prefix. Promotion into the running batch caches the prompt's full pages and bumps the destination capacity generation, since the new prefix can let a pending reservation fit. Cancelling a ready request caches its full pages and frees the tail. Handoff conformance expects no SGLang destination stores on activation. handoff_prefill_to_reserved_decode_owns_kv_until_prebuilt_admission asserted publication at activation, which SGLang does not do; it now asserts that the prompt stays invisible while the running batch is full and is published once at running admission. 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.
|
| Layer / File(s) | Summary |
|---|---|
Refresh radix-cache recency on partial access crates/core/src/engine/cache/radix_cache.rs, python/aisimulate/THIRD_PARTY_NOTICES.md |
Partial matches and reinsertion without new pages refresh cache-path timestamps. Tests check eviction ordering across page sizes and matching paths. |
Cache prefixes and publish KV events at defined boundaries crates/core/src/engine/kv_manager/sglang_backend.rs, crates/core/src/engine/scheduler/sglang/core.rs, crates/core/src/engine/scheduler/sglang/host_loop.rs, crates/core/src/engine/scheduler/sglang/tests.rs, THIRD_PARTY_NOTICES.md, python/aisimulate/THIRD_PARTY_NOTICES.md |
Transferred prompts are cached at running admission rather than activation. The scheduler caches materialized prefixes on source holds and cancellation. Running-request cancellation can defer cleanup until an in-flight result is observed. KV events are queued, coalesced, and assigned IDs when flushed at defined boundaries. Tests cover transfer, cancellation, retries, and event cadence. |
Order decode retraction and defer decoded-prefix caching crates/core/src/engine/scheduler/sglang/decode.rs, crates/core/src/engine/scheduler/sglang/tests.rs, THIRD_PARTY_NOTICES.md, python/aisimulate/THIRD_PARTY_NOTICES.md |
Retraction uses output-length and prompt-length ordering, excludes terminal ghost rows from retraction, and evicts cache toward the survivor reserve. Prefill commits happen before decode-page reservation. Decoded output is cached only when a request finishes. |
Record first-admission reuse and validate handoff behavior crates/core/src/replay/report.rs, crates/core/src/replay/canonical.rs, crates/core/src/replay/handoff.rs, crates/core/src/replay/disagg_tests.rs, crates/core/tests/agentx_qualification.rs, crates/tests/public-api/src/lib.rs |
Per-request reports add first_admission_reused_input_tokens, with serialization and public API coverage. SGLang handoff validation allows zero destination storage at activation, and a conformance test checks destination-first completion. |
Document cache behavior and adaptation sources docs/replay/engine/kv-cache.md, THIRD_PARTY_NOTICES.md, python/aisimulate/THIRD_PARTY_NOTICES.md |
Documentation describes cache insertion, KV-event cadence, decode retraction, and replay reuse fields. The notices record SGLang v0.5.20 provenance and Apache-2.0 attribution. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Possibly related PRs
- ai-dynamo/aisimulate#332: Changes destination reservation, admission headroom, and capacity-generation retries used by the handoff lifecycle updated here.
- ai-dynamo/aisimulate#363: Changes the native SGLang radix-cache core used by the cache recency updates in this pull request.
- ai-dynamo/aisimulate#581: Changes partial-edge recency and cached-prefix reinsertion in the same radix-cache adapter.
Merge Risk: 
Minimal · up to fdaad
-
This change aligns SGLang cache and KV-event behavior with the radix-cache lifecycle. Cancelled in-flight requests no longer expose unobserved KV to followers, and the author reports passing tests. No actionable merge-blocking risk remains in the supplied evidence.
Pre-merge checks: 
5 · 
2 · 
1
-
Failed checks: 2 warnings · 1 inconclusive Passed checks: 5 passed-
Full details: Title check
-
Explanation
The title "fix: align SGLang cache lifecycle, eviction, and KV events" names the three main areas of the changeset: SGLang radix-cache lifecycle, eviction/retraction, and KV-event publication. It is specific to the change, follows the conventional prefix, and is not vague. The title does not mention the new report field or the public API change, but those are secondary to the main change.
Full details: Compatibility Boundaries
-
Explanation
Compatibility boundaries remain synchronized.
PerRequestRecordadds the new field at the Rust public/JSON surface, populates it, updates the external public-API literal test, and documents its meaning (crates/core/src/replay/report.rs:938-987,2289-2292;crates/tests/public-api/src/lib.rs:790-852;docs/replay/engine/kv-cache.md:98-108). The Python runner preserves native per-request records as JSON without a second strict model (python/aisimulate/src/aisimulate/runner.py:2265-2288). Replay input schema version and package versions are unchanged, and no defaults or bindings were omitted. The PR adds no Dynamo dependency or import. Dynamo consumes the records through serialization and has noPerRequestRecordstruct literal at commitd043ae04174571aeac28dccb156f52d59c3f77fd(lib/bindings/python/rust/llm/replay.rs:875-891;lib/mocker/src/replay/entrypoints.rs:3049). The one-wheel/one-crate release checks remain unchanged, and the root and packaged notices are byte-identical. The release contract still permits exactly oneaisimulatewheel and oneaisimulate-corecrate (scripts/release/build_release_artifacts.py:45-92,155-169).
Full details: Description check
-
Explanation
The pull request description follows the required template. It documents the motivation, behavior changes, review map, public contract impact, compatibility concerns, test evidence, review status, boundary cases, expected-value derivation, provenance, and tracking information. It also clearly states that Fast CI and Full CI were not run or claimed.
-
Full details: Cross-Layer Contract
-
Explanation
The cross-layer contract remains incomplete. This PR adds and populates
PerRequestRecord.first_admission_reused_input_tokensincrates/core/src/replay/report.rs:984-986,2289-2298and covers it in the Rust canonical/public-API tests. Generic Rust serialization converts each record withserde_json::to_value(crates/core/src/python.rs:3038-3048), and the CLI writes the resulting mappings torequests.jsonl(python/aisimulate/src/aisimulate/main.py:235-241). The PR also updates the AISimulate KV-cache documentation. However, the downstream Dynamo documentation atai-dynamo/dynamo@d043ae04174571aeac28dccb156f52d59c3f77fd,docs/fern/pages/reference/components/dynosim-replay-cli-reference.mdx:326-327, still lists only the G1 and host first-admission fields. A repository-wide search of that downstream commit finds nofirst_admission_reused_input_tokens. Its consumer tests atcomponents/src/dynamo/replay/tests/test_agentx_g2.py:200-366and binding tests atlib/bindings/python/tests/replay/test_replay_host_offload.py:1-90assert older fields only. No changed PR file updates those downstream documentation or consumer contracts.Resolution
Update the downstream Dynamo per-request documentation to define
first_admission_reused_input_tokensand its readmission semantics. Add a readmission case where first-admission reuse differs fromreused_input_tokens. Assert the new field in the native-report, Python-binding, CLI, andrequests.jsonlconsumer tests. Retain the existing Rust public-API and canonical serialization assertions.
Full details: Modeling And Data Evidence
-
Explanation
The PR still refreshes all 16 AgentX qualification digests without reproducible before/after evidence. The changed test hashes each report, per-request record set, and lifecycle JSONL into an opaque 16-character BLAKE3 value (
crates/core/tests/agentx_qualification.rs:637-653), and the diff only replaces the expected constants. No report artifacts, semantic golden diff, or machine-readable anomaly checks were added. The test comment requires semantic comparison before re-pinning (:657-659), but does not perform or record that comparison. SGLang source provenance and focused lifecycle assertions do not explain the AgentX qualification output changes. The newfirst_admission_reused_input_tokensfield may affect serialized reports, but the PR provides no per-field before/after evidence for the 16 cases.Resolution
Add reproducible before/after report, per-request, and lifecycle artifacts for all 16 qualification cases, or commit an explained machine-readable golden diff with anomaly checks for counts, statuses, and changed fields. Tie each changed value to the cache-lifecycle or report-schema change, then update the digest constants. Otherwise revert the digest refresh.
Full details: Review Evidence
-
Explanation
The visible description names commands and results, lists boundary cases, and states that Fast CI was not run locally and Full CI was not claimed. It also separates SGLang reference/parity scope from local AISimulate tests and disclaims native-engine and production evidence. However, the authored description is explicitly truncated. This check requires the complete description to verify that omitted text does not cite an inactive nested workflow as hosted CI evidence. Repository policy confirms that workflows under
python/aisimulate/are not hosted-CI evidence (REVIEW.md:138-140), but the omitted prose cannot be checked.Resolution
Provide the complete pull-request description, or confirm that the omitted text contains no hosted-CI claim for nested workflows and preserves the stated local, simulator, parity, and production evidence distinctions.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Publish radix inserts at the prefill, completion, and transferred-request lifecycle boundaries. Batch compatible KV events and flush them from the observed forward in overlap mode. Align partial-edge recency and stable retraction/reserve eviction, and expose total first-admission reuse. Port the earlier event-cadence contribution onto current request leases and page metadata. Preserve current forward materialization and timing behavior. Signed-off-by: Rudy Pei <yanrpei@gmail.com> Co-authored-by: Isaac <no-reply@databricks.com>
tedzhouhk
left a comment
There was a problem hiding this comment.
Reviewed commit 78e6e4aa5d27723c9cb311a951b56a0a840b5685 against the pinned SGLang source.
Validation: core suite 2,466 passed / 1 ignored; public API consumer 16 passed; all 16 AgentX qualification digests match the base after removing only the added per-request field. One reproduced P2 concerning overlap cancellation is recorded inline. Fast CI is green; Full CI and native-engine/GPU parity were not established by this review.
| let capacity_improved = if was_running { | ||
| let releases_kv = request.kv_lease.is_active(); | ||
| cleanup_completed_request(&mut request, &mut self.kv_manager, self.config.block_size); |
There was a problem hiding this comment.
[P2] Defer caching canceled in-flight requests until result observation
My main concern is that this immediately inserts an unobserved forward's KV when host_loop is enabled. With a 16-token prompt and a 20 ms prefill, the first pass returns at t=0 without observing the result; canceling there caches all 16 prompt tokens, and a same-prompt follower selected at t=0 reuses 15 tokens instead of 0. Both one-token and multi-token output cases fail on this head, pass after reverting only this cancellation hunk, and fail again when the hunk is restored.
The pinned SGLang abort_request only sets to_finish on a running request; process_batch_result_prefill releases/caches it after result observation. Could we preserve deferred cleanup for in-flight cancellation and add cache-visibility/follower-reuse assertions to the existing overlap cancellation test? Otherwise canceled requests expose future KV to admission and the router.
Running cancellation under the overlap host loop discards pending outputs, but immediately finishing the lease exposes KV from an unobserved forward. A follower selected before that forward ends can then reuse future KV. Keep canceled running requests with the pending batch until observation, then cache and release their leases without retiring reused request IDs. Preserve immediate cleanup when no forward is pending. Cover first-token and continuing requests, both cache modes and page sizes, router events, idle decode cleanup, repeated cancellation, and request-ID reuse. Signed-off-by: Rudy Pei <yanrpei@gmail.com> Co-authored-by: Isaac <no-reply@databricks.com>
Why and what changed
SGLang cache insertion, eviction recency and retraction order determine which prompts can be reused and what the KV router sees. AISimulate currently announces some pages at allocation or destination activation and caches unfinished decode output, which makes those observations differ from SGLang's radix lifecycle.
This consolidates the cache corrections and the older event-cadence change:
Review map
sglang_backend.rs(publication queue and destination lease),sglang/core.rs,decode.rs,host_loop.rs, andradix_cache.rs. Scheduler tests cover their interactions;replay/report.rsadds the report field.PerRequestRecord.first_admission_reused_input_tokensand its JSON field. Rust consumers constructing that public struct must initialize the new field. Existing reuse fields retain their meanings. SGLang handoff conformance expects zero stores at activation; other backends retain activation stores.Evidence
cargo test --locked -j 4 -p aisimulate-core— 2,475 passed, zero failures, one ignored across 19 unit/integration/doc-test binaries on the current tree. The focused overlap-loop run passed all 22 cases, also covered by the full run. The eight prefill cancellation cases reproduced premature caching or release before the fix; the regressions now verify zero pre-observation follower reuse, delayed router stores, eventual caching/release, idle decode cleanup, repeated cancellation, and request-ID reuse.cargo test -j 4 --manifest-path crates/tests/public-api/Cargo.toml— 16 passed, zero failures at the original consolidated head78e6e4aa5d27723c9cb311a951b56a0a840b5685; doc-test binary also passed (zero cases). This cancellation follow-up does not change the public API; the standalone consumer was not rerun.rustfmt --edition 2024 --checkfor every changed Rust file andpython/aisimulate/.venv/bin/python scripts/ci/check_packaged_legal_files.pypassed; root/package notices are identical.fdaad18890a863ea27a0af673731a1ababe1d248; bounded gap review of the cancellation follow-up. The original combined diff was reviewed at78e6e4aa5d27723c9cb311a951b56a0a840b5685.Modeling or data provenance
Modified behavioral adaptations of SGLang v0.5.20, immutable commit
94602c9c2b7cbdb8efd5c52802dac6a1c180089e:Synthetic fixtures only; source/license attribution is recorded in the source headers and matching root/package notices.
Tracking
Supersedes #410, #580, #581 and #582.
This pull request and its description were written by Isaac.