Repository navigation
feat(router): approx indexer support per-insert pruning TTLs - #12793
michaelfeil wants to merge 7 commits into
Conversation
Signed-off-by: Michael Feil <63565275+michaelfeil@users.noreply.github.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
feat
external-contribution
|
/ok to test df9de92 |
michaelfeil
changed the title
GitHub Actions
Inactive
|
/ok to test c351350 |
GitHub Actions
Inactive
|
/ok to test 18c05c5 |
This comment has been minimized.
This comment has been minimized.
|
@codex review |
GitHub Actions
Inactive
|
/ok to test 332a213 |
|
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 (14)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughChangesThe routing indexer now accepts optional per-request TTL overrides. The override flows through request processing, recording, worker insertion, and pruning. Existing expiry behavior remains the default, while later expiries are preserved during reinsertion. Routing TTL override
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/kv-router/src/indexer/tests.rs`:
- Around line 1099-1103: Flush and settle the indexer after
process_routing_decision_for_request and before the first assert_request_score
call in the single-variant test. Reuse the existing
route_approx_tokens/flush_and_settle behavior or invoke the established flush
helper directly so the queued routing event is applied before assertions.
In `@lib/kv-router/src/indexer/thread_pool.rs`:
- Around line 810-822: The EventWithAck acknowledgement is only being awaited,
so failed event application can leave pruning registrations active. In
lib/kv-router/src/indexer/thread_pool.rs lines 810-822, inspect the
acknowledgement boolean and fail or roll back when it is false; in
lib/kv-router/src/indexer/pruning.rs lines 381-408, ensure
enqueue_and_insert_worker_block_entries delays timer registration until
successful application or restores every prior expiry during rollback.
🪄 Autofix
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: Enterprise
Run ID: abd3e512-e445-46e3-9f69-ba4db6aeabf3
📒 Files selected for processing (14)
lib/bench/tests/mooncake_trace.rslib/kv-router/src/indexer/branch_sharded.rslib/kv-router/src/indexer/kv_indexer.rslib/kv-router/src/indexer/local.rslib/kv-router/src/indexer/pruning.rslib/kv-router/src/indexer/tests.rslib/kv-router/src/indexer/thread_pool.rslib/kv-router/src/indexer/traits.rslib/kv-router/src/indexer/types.rslib/llm/src/kv_router.rslib/llm/src/kv_router/indexer/mod.rslib/llm/src/kv_router/indexer/recording.rslib/llm/src/kv_router/indexer/remote.rslib/llm/src/kv_router/indexer/side.rs
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 332a21305c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
GitHub Actions
Inactive
|
/ok to test 4c53db4 |
|
@codex review |
Signed-off-by: Michael Feil <63565275+michaelfeil@users.noreply.github.com>
Signed-off-by: Michael Feil <63565275+michaelfeil@users.noreply.github.com>
Signed-off-by: Michael Feil <63565275+michaelfeil@users.noreply.github.com>
Signed-off-by: Michael Feil <63565275+michaelfeil@users.noreply.github.com>
4c53db4 to
3b29e65
Compare
GitHub Actions
Inactive
|
/ok to test 3b29e65 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c53db4571
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Michael Feil <63565275+michaelfeil@users.noreply.github.com>
GitHub Actions
Inactive
|
/ok to test 2e93c8d |
|
@CodeRabbit review |
|
@codex review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
GitHub Actions
Inactive
|
/ok to test e86da26 |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
PeaBrane
left a comment
PeaBrane
left a comment
There was a problem hiding this comment.
AI-generated neutral review; human evaluation is requested. This is neither an approval nor a request-changes verdict.
For bounded, trusted TTLs, the existing FxHashMap/BTreeMap plus per-worker-minimum heap design appears compatible with heterogeneous expiries: this PR does not introduce per-block timer tasks or a fundamentally heavier scheduling container. The main concerns are where expiry authority and identity are lost across boundaries.
A possible architectural direction is to treat expiry as validated, versioned state: expose a bounded receiver-owned retention policy on the wire; compute an absolute internal deadline at admission so queue/transport time counts; carry a generation with heap entries and pending removals so stale work is conditional; and publish heap rebuilds with an epoch check or reconciliation. That should preserve the bucketed scheduler and hot read path while addressing the inline policy, race, and coverage gaps.
| for key in keys { | ||
| match self.timers.entry(key.clone()) { | ||
| Entry::Occupied(entry) if *entry.get() == expiry_time => { | ||
| Entry::Occupied(entry) if *entry.get() >= expiry_time => { |
There was a problem hiding this comment.
AI review — human evaluation requested: Please re-evaluate the previously discussed stale-removal race against #12785's acceptance criterion. This max-expiry check only helps while the key remains in the timer map. Once queue_due removes the timer and queues a bare BlockEntry, a refresh can install a later timer, yet the stale queued Removed can still delete the refreshed tree entry, including during the worker-ack window before the later expiry is registered. If this PR intends to close that criterion, pending work likely needs to carry an expiry/generation and apply removal only when it is still current. Scanning the pending queue during insertion would not cover a removal already enqueued downstream.
| /// Locally-computed rolling sequence hashes for the routed request. | ||
| pub sequence_hashes: Vec<SequenceHash>, | ||
| /// Optional pruning TTL for this routing decision. | ||
| pub ttl_override: Option<Duration>, |
There was a problem hiding this comment.
AI review — human evaluation requested: Please assess whether a raw, arbitrary Duration is safe on this served-indexer wire boundary. The receiver forwards it into pruning without a cap; a large value can bypass the configured retention bound, increase live entries and expiry buckets, and an extreme representable value can make Instant + ttl panic. A bounded receiver-owned retention class, or receiver-side validation/capping plus checked_add, would keep untrusted policy outside the indexer.
| worker, | ||
| local_hashes, | ||
| sequence_hashes, | ||
| ttl_override, |
There was a problem hiding this comment.
AI review — human evaluation requested: This carries a relative TTL through the queue/RPC, but the pruning manager samples Instant::now() only when it later processes the request. Local backpressure, a worker acknowledgement, or remote transport therefore does not consume the requested lifetime; a request with five seconds remaining after ten seconds in transit receives another five seconds. If the intended contract is event-age-bounded retention, consider computing a validated absolute internal deadline at admission and carrying that through private queues rather than restarting a relative duration at each boundary.
| new_next >= old_next, | ||
| "approximate prune expiry moved earlier during insert" | ||
| ); | ||
| self.inner.push_worker_expiry(worker, new_next); |
There was a problem hiding this comment.
AI review — human evaluation requested: Please verify the interaction with concurrent heap rebuilds. A rebuild snapshots per-worker minima without serializing against this push, then replaces the heap wholesale. An insertion can publish a newly earlier minimum after its worker was snapshotted, only for the rebuild to overwrite and lose that entry; if another worker remains heap-top, stale-top validation does not rediscover the omitted worker until later. Heterogeneous TTLs make new_next < old_next routine. A mutation epoch/retry, serialization, or post-build reconciliation would make publication robust.
| let long_expiry = *pm.get_expiry(&2).expect("long expiry missing"); | ||
| pm.insert_at(vec![2], now, Some(Duration::from_secs(5))); | ||
|
|
||
| assert!(short_expiry < long_expiry); |
There was a problem hiding this comment.
AI review — human evaluation requested: This checks the leaf map's computed expiries, but it does not demonstrate that the background scheduler wakes for a newly earlier deadline. The interface-level test later calls flush, which synchronously drains due work and can pass even if the wake-up path is wrong. A deterministic test could insert a long TTL followed by a shorter TTL for another block on the same worker, observe ordered background removal without relying on flush, and repeat descending deadlines past the heap-rebuild threshold.
| let encoded = rmp_serde::to_vec_named(&decoded).unwrap(); | ||
| let round_trip: IndexerRecordRoutingDecisionRequest = | ||
| rmp_serde::from_slice(&encoded).unwrap(); | ||
| assert_eq!(round_trip.ttl_override, decoded.ttl_override); |
There was a problem hiding this comment.
AI review — human evaluation requested: This proves only the MessagePack shape. It does not exercise RouteRecordingTarget → RemoteIndexer → ServedIndexerRecordEngine, where TTL forwarding can be lost even while serialization still round-trips. Please consider a behavioral remote-path test using a long configured default and Some(Duration::ZERO) (or another short override), then assert that observed expiry follows the override.
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
here is my idea for next changes 1. unbounded wire 2. Concurrent heap rebuild losing an earlier minimum. If two rebuilds race, the older minimum can overwrite the newer state and a TTL fires late. @PeaBrane what do you think of these ideas? |
External-PR triage (head
|
Summary
Closes #12785
Validation
cargo fmt --allcargo clippy -p dynamo-kv-router --all-targets -- -D warningscargo test -p dynamo-kv-router --lib ttl_override -- --nocapture(6 passed)cargo test -p dynamo-kv-router --lib(836 passed, 2 ignored)Summary by CodeRabbit
New Features
Bug Fixes
Tests