feat(kv-router): index Mooncake KV events - #11239
Conversation
This comment has been minimized.
This comment has been minimized.
WalkthroughChangesThe Mooncake shared-cache client now indexes KV events locally, validates group generations, clears state on stream inconsistencies, and uses the index for cache checks. Registration publishes the event endpoint, Hicache starts the subscriber, tests cover event behavior, and SGLang documentation reflects the new configuration. Mooncake HiCache event index
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
|
Once sgl-project/sglang#32302 goes in we can also push this to main and then use it in 0.5.17 sglang that will come out in 2.5ish weeks |
Merged. |
3cf225a to
7d3b1af
Compare
|
/ok to test 7d3b1af |
|
/ok to test 114474d |
|
Addressed the unresolved review feedback in 114474d:
Validation: |
There was a problem hiding this comment.
Trimmed this review down to what isn't already covered by the existing bot comments. I agree with and won't repeat: the stale verified-group cache when removed events omit group_id, the unbounded group_states growth, the missing subscriber dedup, and the lack of reconnect — see Devin's and CodeRabbit's comments for those. The first two are what I'd consider merge blockers.
Overall the architecture is right and the conservative bias is correct — every failure path degrades to "no shared hits" rather than false positives. Removing HTTP from the routing hot path is a clear win, and the draft-gating on unmerged upstream deps is correctly called out. The items below are the ones I didn't see raised elsewhere. Line references are against lib/llm/src/kv_router/shared_cache.rs on this branch.
Correctness & operations
1. Subscribing to all topics lets one stray message wipe the index
connect_sub_socket(&endpoint, None) (L186) subscribes to "". If the Mooncake master publishes anything else on that PUB socket, it lands in the frames.len() != 3 branch at L212, which calls self.clear() — dropping the entire index on every such message. That's a steady-state thrash, not a one-off: the index would be repeatedly emptied for as long as the other publisher is active.
Pass the object-event topic to connect_sub_socket, and reserve clear() for genuine sequence gaps. A single malformed frame doesn't imply the rest of the index is stale.
2. Subscriber death is invisible, and shared_cache_errors is now dead
Separate from the missing-reconnect issue already flagged: every path in the new check_blocks returns Ok, so route_lookup.rs:209's inc_shared_cache_errors() is unreachable for this impl and the shared_cache_errors metric no longer fires at all.
So when the subscriber does exit, there's one warn! and then silence — shared_cache_hit_rate sagging to 0 is the only remaining signal, and the doc's troubleshooting table points operators at "check the frontend subscriber log," which is a one-line needle hours back. Whatever shape the reconnect fix takes, please also surface a subscriber_connected gauge (or repurpose the error counter) so this is alertable.
3. DYN_MOONCAKE_KV_EVENTS_ENDPOINT on the worker is an awkward home for a cluster-global value
The endpoint belongs to the Mooncake master and is consumed by the frontend, but it's routed through per-worker env → registration metadata.
Because kv_events_endpoint is part of SglangHicacheMooncakeConfig's PartialEq, a single worker missing the env var makes resolve_mooncake_config return None (L113-119) and silently disables shared caching cluster-wide, with only a warn!. That's a rough failure mode for a value that has to be identical everywhere by construction. A frontend-side flag with the worker metadata as fallback would be more robust and matches where the subscriber actually runs.
4. Dead config fields still gate that equality check
master_server_address and master_metrics_port are no longer read by the router now that mooncake_batch_query_endpoint is gone, but they still participate in the cross-worker comparison. A cosmetic port difference between workers disables shared caching for fields nothing uses. Either drop them or exclude them from the equality check.
Tests
Ten tests pass, but the gaps line up with the risks:
- No test for the stale verified-group case (the issue Devin flagged). Verified group, then a groupless
removed— it's the case most likely to bite in production and would be ~15 lines. sglang_group_idwithextra_backend_tagis untested.mooncake_config()sets it toNone, so only thesglang-hicache:{hash}branch is exercised. Thesglang-hicache:{tag}_{hash}form is a guess at an unmerged upstream format, and it's exactly the branch nobody will notice is wrong until someone runs with a tag set.- The generation-guard race (L297-305) has no test. That guard is the whole soundness argument for group caching, and right now it's only reasoned about in the PR description.
test_check_blocks_skips_mooncake_for_cache_namespacestill spins up amockito::Serverand parses its URL for a code path that no longer makes HTTP calls. That's what's keepingmockitoandreqwest::Urlin the test imports — the scaffolding can go.
Nits
MooncakeEventBatch = (i64, Vec<MooncakeObjectEvent>, u32)is a positional tuple decoded from an external wire format, with two silently-discarded fields and no doc comment. A named struct, or at least a comment recording the frame layout ([topic, be_u64_seq, msgpack(batch)]) and what fields 0 and 2 are, would survive the upstream PR shifting.clear()resetslast_sequenceto 0, so the batch after any malformed frame is guaranteed to log a spurious "sequence gap" warning (L126:previous == 0 && sequence != 1). Harmless, but it'll get reported as a bug.#[serde(default)]is applied to theOptionfields inMooncakeObjectEventbut not to those inSglangHicacheMooncakeConfig, in the same file. Both work — serde'smissing_fieldhandlesOptionviavisit_none— so this is purely a consistency nit.state.0 != generation → return false(L301-303) discards an already-computedtrue. Conservative and correct, but worth a one-line comment since it silently drops a real hit.start_subscriber(&Component)uses the component solely fordrt().child_token(). Taking aCancellationTokendirectly would make the subscriber loop unit-testable against a local PUB socket (prior art inlib/kvbm-consolidator/tests/common/mod.rs) — and would compose with whatever per-router token the dedup fix introduces.- The description lists "Apply shared-cache credit only beyond the selected worker's device-local prefix" under How This Was Implemented, but no scoring code is in this diff — likely carried over from an earlier iteration. Worth trimming so reviewers don't go looking for it.
Summary
Requesting changes primarily on #1 — the all-topics subscribe combined with the clear-on-bad-frame path is a stability issue I didn't see raised elsewhere, and it interacts badly with the reconnect work. #2 is cheap and makes the difference between "degrades gracefully" and "degrades invisibly." #3 and #4 are worth settling while the config surface is still in draft.
Given the feature is already gated on unmerged upstream work, there's room to nail down the group-metadata contract with the Mooncake publisher before relying on it.
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
ae27936 to
3db6754
Compare
|
/ok to test 3db6754 |
|
/ok to test d5a31a9 |
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
|
/ok to test 7d782e2 |
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
|
/ok to test 73a58c4 |
Summary
This adds an event-driven Mooncake shared-cache index for SGLang HiCache and uses
group_idas a verified per-page fast path. It removes Mooncake HTTP queries from Dynamo's routing hot path while keeping exact physical object keys as the correctness source of truth.CLOSES: DYN-3578
Important
Draft only. Do not merge until the Mooncake event publisher lands, SGLang updates its Mooncake integration to the matching revision, and Dynamo's SGLang dependency includes those changes. Mooncake #2214 provides the current publisher, #2663 is the semantic follow-up, and SGLang #29948 is related worker-local L3 event work.
How This Was Implemented
group_idlookup, then cache one group result until a relevant event invalidates it.Walkthrough
Mental model
flowchart LR SGLang["SGLang HiCache"] -->|"stores physical KV objects"| Mooncake["Mooncake master"] Mooncake -->|"ordered object events + group_id"| Index["Dynamo shared-cache index"] Request["Request page hashes"] --> Index Index -->|"shared prefix hits"| Router["KV router scoring"]Dynamo keeps this global shared-pool index separate from its worker-rooted radix index. A logical page is a hit only after all physical objects implied by the worker's TP/PP/layout metadata are present.
State and ordering
Exact keys are updated from each event batch. Sequence zero is a valid initial sequence, duplicate batches are ignored, and a gap conservatively clears the index. A group stores the event sequence at which it was last invalidated; concurrent updates cannot make a physically present page report as a miss.
Request lifecycle
The first request for a group expands the logical SGLang page hash into physical object keys and checks each key locally. Later requests use one group lookup until a relevant stored or removed event invalidates that group; events without
group_idalways use exact-key checks.Boundaries and limitations
There is no snapshot or replay protocol yet, so a late subscriber or sequence gap conservatively misses existing objects until new store events arrive. The current shared-cache request contract also remains scoped to Mooncake's default tenant.
Validation
cargo test -p dynamo-llm shared_cache— 19 passed.cargo test -p dynamo-llm remove_hicache_caches— 1 passed.cargo clippy -p dynamo-llm --lib -- -D warningscargo fmt --all -- --checkandpre-commit run --files …Benchmark Results
group_idlookup: 1.83 ms mean versus 2.12 ms for exact event-key lookup, a 13.8% reduction.Summary by CodeRabbit
New Features
Documentation