fix(kv-router): fail closed on unrecognized KV event media - #12456
Conversation
Unknown medium strings previously fell back to the Device tier in convert_event (indexing them on the G1 primary radix tree), flowed through the salted cache-namespace normalizer state, and were tracked as G1 by both consolidator ingresses. vLLM 0.26.0 ships FS/OBJ media (pre-#48123 wire) that hit all three paths. Drop unrecognized media in conversion with a rate-limited warning, bypass normalizer state for them (Device/HostPinned still flow through so CPU-offload salted propagation is preserved), and gate both consolidator ingresses; remove the now-unused from_kv_medium_or_default. Follow-up promised in ai-dynamo#11571. Signed-off-by: Change72 <changg@nvidia.com>
|
🎯 Code Coverage (details) 🔗 Commit SHA: 7e48754 | Docs | Datadog PR Page | Give us feedback! |
vLLM's unified STORAGE medium maps to the Disk lower tier (ai-dynamo#11571), and unrecognized media now fail closed instead of misrouting, so the router does tier-aware routing for vLLM disk offloading. Flip the Disk cell to ✅ and refresh the stale FS/OBJ wording (upstream emits STORAGE; legacy 0.26.0 FS/OBJ media are dropped, not misrouted). Signed-off-by: Change72 <changg@nvidia.com>
|
/ok to test d5834a5 |
…rsion Unrecognized media (vLLM 0.26.0 FS/OBJ) were bypassed to conversion, which dropped them -- but the LLM ZMQ listener increments next_event_id before normalize_preprocessed, so a conversion-time drop burned an event id. The event processor then reads the id gap as a false engines_dropped_events, and the event is also counted as accepted then conversion_none. Classify media in preprocess_with_reason: Device/HostPinned stay on the normalizer path, Disk/External bypass to conversion (kept there, no id wasted), and unrecognized media fail closed with a new UnknownMedium filter reason (label "unknown_medium") -- the same in-preprocess filtering the ai-dynamo#11571 locality gate uses to avoid this exact id gap. convert_event keeps its rate-limited defensive drop for direct callers that bypass preprocess. Add a preprocess unit test (FS/OBJ/XYZ -> UnknownMedium; recognized media pass), update the namespace-pollution test to assert the reject, and add a listener-level GPU -> FS -> GPU test proving the two GPU stores land on consecutive event ids (the FS store burns none). Signed-off-by: Change72 <changg@nvidia.com>
|
/ok to test 630b42f |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
WalkthroughUnknown storage media are now rejected instead of defaulting to the device tier. Router preprocessing, conversion, G1 ingress filters, documentation, and tests were updated to recognize ChangesStorage media filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Signed-off-by: Change72 <changg@nvidia.com>
|
/ok to test 896af5a |
…n vLLM 0.27+
Address PR review feedback:
- Empty-string `medium` ("") was routed to the unrecognized-media fail-closed
path, dropping events a producer may mean as unset. Normalize "" to absent at
the wire deserialize boundary (mirroring the empty `cache_salt` handling) so an
unset medium stays on the default device tier and is indexed. No dynamo/vLLM
producer emits `medium: ""` today; this preserves prior behavior for that
value and keeps fail-closed scoped to populated-but-unrecognized media.
- Router offloading matrix: the vLLM Disk ✅ depends on the unified STORAGE
medium (vLLM 0.27+). Qualify the cell with the 0.27+ gate so readers on the
pinned 0.26.0 (which emits FS/OBJ that Dynamo drops) are not misled.
Signed-off-by: Change72 <changg@nvidia.com>
|
/ok to test cd14d9f |
|
/ok to test 833fe06 |
Signed-off-by: Chang Guo <changg@nvidia.com>
|
/ok to test 5f352d7 |
jthomson04
left a comment
There was a problem hiding this comment.
Two source review findings.
…nce path normalize_medium applies to both the map and positional (tuple) decode branches, but the regression test only exercised map-encoded events. Add a sequence-encoded store + remove case asserting an empty-string medium decodes as absent and stays on the device tier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Change72 <changg@nvidia.com>
|
/ok to test 7e48754 |
Summary
dynamo was fail-open on KV-event
mediumstrings it doesn't recognize:convert_eventfell back to the Device tier viafrom_kv_medium_or_default, so an unrecognized medium was indexed on the G1 primary radix tree — and separately flowed through the salted cache-namespace normalizer state and was tracked as G1 by both consolidator ingresses. This PR makes all three paths fail closed.Concrete trigger: vLLM v0.26.0 (dynamo's current pin) emits
FS/OBJmedia (present since vLLM #47923; unified toSTORAGEonly in 0.27+ via #48123). dynamo doesn't recognizeFS/OBJ, so today they hit all three holes. This is the follow-up promised in #11571:Behavior
GPU/CPU/STORAGE/ other recognizedFS/OBJ, future strings)next_event_id(false engine-drop metric) → rejected in preprocess (unknown_mediumfilter reason); no event id burned; consolidator: tracked as G1 → droppedChanges
convert_event: drop unrecognized media with a rate-limited warning (was a silent Device fallback). Removes the now-unusedStorageTier::from_kv_medium_or_default. This drop is kept as a defensive backstop for direct callers that bypasspreprocess.preprocess_with_reason: reject unrecognized media with a newunknown_mediumfilter reason instead of letting them reach conversion. Filtering here — exactly like the feat(kv-router): route vLLM STORAGE KV events to the Disk tier with locality gating #11571 locality gate — means the LLM ZMQ listener records an intentional filter rather than accepting the event, incrementingnext_event_id, and only dropping it in conversion; that id gap is what the event processor misreads as an engine drop (engines_dropped_events). Recognized lower-tier media (STORAGE→ Disk/External) still bypass normalizer state; Device and HostPinned events (GPU, CPU offload feat(kv-router): Route vLLM CPU KV events to HostPinned and count lower-tier applies #10368) still flow through the normalizer so their salted namespaces keep propagating.kvbm-consolidator, LLMkv_consolidator): gate unrecognized media out of the G1-only tracker.The normalizer and consolidator conditions are intentionally different (normalizer keeps CPU on-path for salted propagation; the consolidator is G1-only and drops CPU), so there is no shared predicate.
Out of scope: locality semantics (settled in #11571), the CPU/HostPinned normalizer path (#10368), any
FS/OBJ→ tier mapping (dynamo deliberately does not consume them — this only flips them from fail-open to fail-closed), and removal/refcount semantics (#11930).Validation
All commands on a Linux workstation, all green:
New/updated tests:
preprocess_with_reasonrejectsFS/OBJ/XYZasunknown_medium(store + remove) while recognized media pass.FSevent reusing a salted GPU hash is rejected inpreprocess(so it never touches namespace state) and the salted GPU chain keeps inheriting the namespace.GPU → FS → GPU(real ZMQ publisher): the two GPU stores land on consecutive event ids (0, 1), proving the filteredFSstore burns nonext_event_id(no false engine drop).convert_eventstill dropsFS/OBJ/XYZas a backstop (store + remove, any locality); both consolidator ingresses dropFS.AI assistance
Implemented with AI assistance (Cursor); design, scoping, and review were human-directed.
Summary by CodeRabbit
New Features
STORAGEevents map to Dynamo’s Disk tier.Bug Fixes
Documentation
FSandOBJevent types.