Repository navigation
feat(kv-router): warn on duplicate store replays - #8473
Merged
Merged
Conversation
Signed-off-by: PeaBrane <yanrpei@gmail.com>
ishandhanani
approved these changes
Apr 21, 2026
Contributor
WalkthroughThe changes implement duplicate store detection and warning metrics throughout the KV router indexing system. A new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
lib/kv-router/src/indexer/tests.rs (1)
381-405: Redundantmetricsre-binding; minor tidy.You construct
metricson Line 382 and then immediately receive anothermetricsback frommake_indexer_with_metricson Line 383. Since the helper just returns the sameArcyou passed in, the rebind is cosmetic. Either drop the second binding:♻️ Tidy
- let metrics = Arc::new(KvIndexerMetrics::new_unregistered()); - let (index, metrics) = make_indexer_with_metrics(variant, metrics); + let metrics = Arc::new(KvIndexerMetrics::new_unregistered()); + let (index, _) = make_indexer_with_metrics(variant, metrics.clone());or, better, make the helper only take
&strand return(Box<dyn KvIndexerInterface>, Arc<KvIndexerMetrics>)itself (constructing metrics internally). Same comment applies to Line 411-412.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/kv-router/src/indexer/tests.rs` around lines 381 - 405, The test re-binds the metrics Arc unnecessarily: you create metrics with KvIndexerMetrics::new_unregistered() then pass it to make_indexer_with_metrics and reassign metrics from the tuple; remove the redundant re-binding by either (a) keep your initially created `metrics` and destructure only the index from `make_indexer_with_metrics` (use its returned index and ignore the returned metrics), or (b) change `make_indexer_with_metrics` to not require an external metrics Arc and instead construct and return it; apply the same tidy to the other occurrences (e.g., the second test around lines 411–412) and update references to `metrics` in tests like test_duplicate_store_replay_warns_without_error accordingly.lib/kv-router/src/indexer/metrics.rs (2)
216-225: Consider takingEventWarningKindinstead of&'static strfor type safety.
PreBoundEventCounters::inc_warning(Line 355) already uses the typedEventWarningKindenum, which gives compile-time exhaustiveness.KvIndexerMetrics::increment_event_warningdiverges by taking a&'static str, which lets any ad-hoc label leak in and create unintended time series.♻️ Optional: stronger typing
- pub fn increment_event_warning(&self, warning_kind: &'static str) { + pub fn increment_event_warning(&self, warning_kind: EventWarningKind) { #[cfg(feature = "metrics")] { + let label: &'static str = match warning_kind { + EventWarningKind::DuplicateStore => METRIC_WARNING_DUPLICATE_STORE, + }; self.kv_cache_event_warnings - .with_label_values(&[warning_kind]) + .with_label_values(&[label]) .inc_by(1); } #[cfg(not(feature = "metrics"))] let _ = (self, warning_kind); }(Test on Line 2258 of
tests.rsand any other callers would need to switch toEventWarningKind::DuplicateStore.)This also mirrors the existing
EventKind/incpattern. I recognize that keeping parity withincrement_event_applied(&'static str, …)was likely the motivation, butEventKindis also used throughout theinc/prebindfast path, so the typed variant would be more consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/kv-router/src/indexer/metrics.rs` around lines 216 - 225, Change KvIndexerMetrics::increment_event_warning to accept the typed EventWarningKind instead of &'static str: update the function signature and use the enum to produce the label value (e.g., via a to_string()/as_str() or impl Display on EventWarningKind) when calling self.kv_cache_event_warnings.with_label_values(&[...]). Also update all callers (including tests referenced) to pass EventWarningKind variants (e.g., EventWarningKind::DuplicateStore) and ensure parity with PreBoundEventCounters::inc_warning and the EventKind/inc pattern so no ad-hoc strings can create unintended time series.
124-146: Partial-failure logging hides the second error.In
from_component, when bothcreate_intcounterveccalls fail, the(Err(e), _) | (_, Err(e))pattern only binds the first error, so you only log one of two registration failures. Minor, but operators may want both contexts.♻️ Log both on double-failure
- ) { - (Ok(kv_cache_events_applied), Ok(kv_cache_event_warnings)) => Arc::new( - Self::new(kv_cache_events_applied, kv_cache_event_warnings), - ), - (Err(e), _) | (_, Err(e)) => { - tracing::warn!("Failed to create kv indexer metrics from component: {}. Using unregistered metrics as fallback.", e); - Arc::new(Self::new_unregistered()) - } - } + ) { + (Ok(applied), Ok(warnings)) => { + Arc::new(Self::new(applied, warnings)) + } + (applied_res, warnings_res) => { + if let Err(e) = &applied_res { + tracing::warn!("Failed to register kv_cache_events_applied: {e}"); + } + if let Err(e) = &warnings_res { + tracing::warn!("Failed to register kv_cache_event_warnings: {e}"); + } + tracing::warn!("Using unregistered kv indexer metrics as fallback."); + Arc::new(Self::new_unregistered()) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/kv-router/src/indexer/metrics.rs` around lines 124 - 146, The match in from_component that calls component.metrics().create_intcountervec for KV_CACHE_EVENTS_APPLIED_SUFFIX and KV_CACHE_EVENT_WARNINGS_SUFFIX currently uses a combined arm (Err(e), _) | (_, Err(e)) which only binds one error and hides the other; change the matching to capture both Results (e.g., match (r1, r2) or pattern arms (Err(e1), Err(e2)), (Err(e1), Ok(_)), (Ok(_), Err(e2))) so you can construct and log both error contexts when both fail (referencing create_intcountervec, KV_CACHE_EVENTS_APPLIED_SUFFIX, KV_CACHE_EVENT_WARNINGS_SUFFIX) and fall back to Arc::new(Self::new_unregistered()) as before.lib/kv-router/src/indexer/positional.rs (1)
272-315: Duplicate-store detection logic looks correct.Tracing through the flow:
duplicate_storestartstrueonly when!blocks.is_empty(), and is flipped tofalsethe moment any block path indicates "new insertion" (vacant index entry, new worker inSeqEntry::insert, or a freshworker_blocks_entrymapping).num_blocks_addedis only incremented on genuinely new entries inworker_blocks_entry(theNonearm), so tree-size accounting and the warning flag stay mutually consistent: ifnum_blocks_added > 0thenduplicate_store == false.- The
SeqEntry::insertbool return threaded from Line 60 correctly distinguishes "worker newly associated with (seq_hash)" vs "worker already present", which is exactly what duplicate detection needs.One subtle case worth calling out: when
Some(existing)withexisting != (position, local_hash)is hit (Line 295), this implies the sameseq_hashwas previously mapped to a different(position, local_hash)for this worker — that normally shouldn't happen in a well-formed event stream, and silently clearingduplicate_storewithout any warn log may hide a real inconsistency. Consider at least atracing::warn!for that branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/kv-router/src/indexer/positional.rs` around lines 272 - 315, The branch handling match worker_blocks_entry.insert(seq_hash, (position, local_hash)) where Some(existing) != (position, local_hash) should emit a warning instead of silently clearing duplicate_store; update that arm to call tracing::warn! (include worker, seq_hash, existing, and the new (position, local_hash) values for context) and then leave the existing duplicate_store=false behavior intact so SeqEntry::insert, worker_blocks_entry, and duplicate_store logic remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/kv-router/src/indexer/kv_indexer.rs`:
- Line 52: The routing-decision path currently calls trie.apply_event(event) in
process_routing_decision_with_hashes which bypasses the warning counters; change
that call to use the same counters plumbing as the normal path by calling
trie.apply_event_with_counters(event, Some(counters)) so the duplicate-store
warning metrics are incremented consistently, ensuring both
trie.apply_event_with_counters (used at the earlier router event path) and the
process_routing_decision_with_hashes path use the same counters instance without
altering the existing counters.inc(...) behavior.
---
Nitpick comments:
In `@lib/kv-router/src/indexer/metrics.rs`:
- Around line 216-225: Change KvIndexerMetrics::increment_event_warning to
accept the typed EventWarningKind instead of &'static str: update the function
signature and use the enum to produce the label value (e.g., via a
to_string()/as_str() or impl Display on EventWarningKind) when calling
self.kv_cache_event_warnings.with_label_values(&[...]). Also update all callers
(including tests referenced) to pass EventWarningKind variants (e.g.,
EventWarningKind::DuplicateStore) and ensure parity with
PreBoundEventCounters::inc_warning and the EventKind/inc pattern so no ad-hoc
strings can create unintended time series.
- Around line 124-146: The match in from_component that calls
component.metrics().create_intcountervec for KV_CACHE_EVENTS_APPLIED_SUFFIX and
KV_CACHE_EVENT_WARNINGS_SUFFIX currently uses a combined arm (Err(e), _) | (_,
Err(e)) which only binds one error and hides the other; change the matching to
capture both Results (e.g., match (r1, r2) or pattern arms (Err(e1), Err(e2)),
(Err(e1), Ok(_)), (Ok(_), Err(e2))) so you can construct and log both error
contexts when both fail (referencing create_intcountervec,
KV_CACHE_EVENTS_APPLIED_SUFFIX, KV_CACHE_EVENT_WARNINGS_SUFFIX) and fall back to
Arc::new(Self::new_unregistered()) as before.
In `@lib/kv-router/src/indexer/positional.rs`:
- Around line 272-315: The branch handling match
worker_blocks_entry.insert(seq_hash, (position, local_hash)) where
Some(existing) != (position, local_hash) should emit a warning instead of
silently clearing duplicate_store; update that arm to call tracing::warn!
(include worker, seq_hash, existing, and the new (position, local_hash) values
for context) and then leave the existing duplicate_store=false behavior intact
so SeqEntry::insert, worker_blocks_entry, and duplicate_store logic remain
consistent.
In `@lib/kv-router/src/indexer/tests.rs`:
- Around line 381-405: The test re-binds the metrics Arc unnecessarily: you
create metrics with KvIndexerMetrics::new_unregistered() then pass it to
make_indexer_with_metrics and reassign metrics from the tuple; remove the
redundant re-binding by either (a) keep your initially created `metrics` and
destructure only the index from `make_indexer_with_metrics` (use its returned
index and ignore the returned metrics), or (b) change
`make_indexer_with_metrics` to not require an external metrics Arc and instead
construct and return it; apply the same tidy to the other occurrences (e.g., the
second test around lines 411–412) and update references to `metrics` in tests
like test_duplicate_store_replay_warns_without_error accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4310a3ff-7f86-477a-86a6-c73d84bef5cc
📒 Files selected for processing (10)
lib/kv-router/src/indexer/concurrent_radix_tree.rslib/kv-router/src/indexer/concurrent_radix_tree_compressed.rslib/kv-router/src/indexer/kv_indexer.rslib/kv-router/src/indexer/metrics.rslib/kv-router/src/indexer/positional.rslib/kv-router/src/indexer/radix_tree.rslib/kv-router/src/indexer/tests.rslib/mocker/src/scheduler/sglang/tests.rslib/mocker/src/scheduler/test_utils.rslib/mocker/src/scheduler/vllm/tests.rs
1 task done
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a warning-only router metrics path for duplicate store replays without changing the hard-error event counters. Extend the router and scheduler test surfaces so clean streams assert zero warnings while duplicate no-op stores increment the new warning counter.
Summary by CodeRabbit
New Features
Tests