Skip to content

fix(kv-router): fence stale TTL removals after refresh - #15476

Open
bojiang-li wants to merge 3 commits into
ai-dynamo:mainfrom
bojiang-li:bojiang-li/prune-refresh-ordering-20261001
Open

bojiang-li wants to merge 3 commits into
ai-dynamo:mainfrom
bojiang-li:bojiang-li/prune-refresh-ordering-20261001

Conversation

@bojiang-li

@bojiang-li bojiang-li commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Overview:

Approximate TTL removals can erase refreshed KV coverage: an expired removal may already be queued when the same block is stored again, and the thread-pool caller only registers the refreshed TTL after receiving its store acknowledgement. Cancelling that caller after enqueueing can also leave a successful store without any TTL.

Order successful-store registration, expiration validation and reset cleanup with the worker's mutation stream. A queued expired removal can no longer delete refreshed or newly LRU-owned coverage; an applied store retains its TTL even if its caller is cancelled.

Details:

Expiration creates removable ownership that refresh, Forget and worker/rank reset invalidate. A removal consumes that ownership immediately before backend application. Successful stores register TTL before their callers are acknowledged. Rank reset bookkeeping completes before the next mutation; whole-worker cleanup remains at the existing final removal barrier after all lanes have drained. The direct indexer's actor applies the same expiration check.

Keep the public WorkerTask variants and constructors and the existing SyncIndexer::worker signature unchanged. An additive provided worker_with_retention method accepts an opaque WorkerTaskReceiver that exposes only original worker tasks. Compressed radix, positional and lower-tier workers reuse their existing loops and perform bookkeeping between tasks, retaining one thread and one input queue per lane. The default preserves existing external implementations through a compatibility dispatch thread; those implementations can override the additive method to use the same inline path. This extra thread/queue cost applies only to legacy external backends with retention enabled. Queue-depth observations include both queues on that fallback path.

The receiver settles ready final store/worker-removal acknowledgements even when a worker returns before its next receive. Drop never waits for an outstanding acknowledgement. An unacknowledged rank removal does not claim completion when its lane exits.

Validation:

  • Deterministic real-worker regressions failed before the fix for queued refresh/removal, cancelled store acknowledgement and queued Store→reset ordering. Coverage includes refreshed eventual expiry, failed stores, TTL→LRU handoff and delayed delivery after a later legitimate expiration.
  • cargo test --locked --offline -p dynamo-kv-router --lib --features bench: 1083 passed, 2 existing ignored. Includes an exhaustive downstream-style backend using the original worker API, early worker return, caller cancellation, panic/error/dropped acknowledgements, single-thread-runtime shutdown, and fallback queue-depth observations.
  • cargo clippy --locked --offline -p dynamo-kv-router --lib --tests --features bench -- -D warnings: passed.
  • cargo fmt --all -- --check and git diff --check: passed.
  • CPU-only release benchmark: seven alternating fresh-process pairs per workload,168 runs on macOS arm64/Rust1.96.1 with other agent compilation paused. Repeated1/16/128-block stores,1 or4 mutation lanes,1 or16 clients,10,000 or48,000 acknowledged requests/run. Builtin thread counts remain unchanged. Concurrent1/16-block TTL throughput fell4.1%/6.8%; aggregate CPU/request rose from3.11→3.47µs and3.05→3.49µs. At128 blocks, concurrent throughput increased10.1% while CPU/request rose4.02→4.53µs. Sequential TTL throughput varied−3.7% to+2.2%; event controls varied−2.2% to+0.5%. These are steady-state KV-indexer operations, not GPU inference or token throughput, and are not a general speedup claim. The final plain-receiver/default-adapter early-return correction is outside this unchanged builtin benchmark path.

AI assistance: Codex helped investigate, implement and test this change. The patch also received independent agent review; reported tests ran locally on CPU.

Main refresh validation

Merged main c872a52bb in 784b6dbf9, retaining the new LaneLookup implementation inside the existing retention worker wrapper. No additional tree or locking behavior was changed during conflict resolution.

On DGX Spark, Linux ARM64 and Rust 1.96.1: cargo test --locked -p dynamo-kv-router --lib --features bench passed 1131 tests with 3 ignored. Strict Clippy for the library and tests with the same features passed, as did Rust formatting and diff checks. This is CPU validation, not an inference benchmark.

Where should the reviewer start?

Start with indexer/worker_receiver.rs for acknowledgement ownership, inline completion, legacy adaptation and shutdown. Then review indexer/pruning.rs for expiration authorization and invalidation, and the gated mutation tests in indexer/thread_pool.rs. The three builtin backend changes reuse their existing worker loops; tree algorithms and structural locking are unchanged.

Related Issues

Part of #12785. This PR addresses stale queued expiration only; the per insert deadline work remains separate.

Related work: #12793 separately adds per-insert TTL support and defers the stale-removal ordering problem. This PR does not implement its per-insert TTL feature or change the expiry-heap rebuild policy, and does not close that PR.

Signed-off-by: bojiang-li <327132355+bojiang-li@users.noreply.github.com>
@bojiang-li
bojiang-li requested a review from a team as a code owner October 1, 2026 10:05
@copy-pr-bot

copy-pr-bot Bot commented Oct 1, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@bojiang-li
bojiang-li temporarily deployed to external_collaborator October 1, 2026 10:05 — with GitHub Actions Inactive
@bojiang-li
bojiang-li temporarily deployed to external_collaborator October 1, 2026 10:05 — with GitHub Actions Inactive
@github-actions github-actions Bot added the fix label Oct 1, 2026
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

👋 Hi bojiang-li! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions Bot added the external-contribution Pull request is from an external contributor label Oct 1, 2026
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: aef8709b-44ef-4bc8-b812-586eef90ea93

📥 Commits

Reviewing files that changed from the base of the PR and between d573cc7 and ea009e3.

📒 Files selected for processing (7)
  • lib/kv-router/src/indexer/concurrent_radix_tree_compressed/sync_impl.rs
  • lib/kv-router/src/indexer/kv_indexer.rs
  • lib/kv-router/src/indexer/lower_tier.rs
  • lib/kv-router/src/indexer/positional.rs
  • lib/kv-router/src/indexer/pruning.rs
  • lib/kv-router/src/indexer/thread_pool.rs
  • lib/kv-router/src/indexer/types.rs

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

Walkthrough

The change routes approximate-TTL stores and removals through worker backend-event lanes, tracks pending expiration authorization, and clears that state alongside worker and rank removal. Prune application now consumes only still-authorized entries. Tests cover refreshes, failed or canceled stores, LRU changes, and reset ordering.

Changes

Approximate TTL pruning

Layer / File(s) Summary
Track expiration authorization
lib/kv-router/src/indexer/pruning.rs, lib/kv-router/src/indexer/types.rs
Worker prune state tracks pending expiration authorizations. Refreshes and removals revoke authorizations. TTL tasks register entries after successful stores and consume authorizations during removal.
Apply TTL and removal tasks
lib/kv-router/src/indexer/concurrent_radix_tree_compressed/sync_impl.rs, lib/kv-router/src/indexer/lower_tier.rs, lib/kv-router/src/indexer/positional.rs, lib/kv-router/src/indexer/types.rs
Indexer workers apply approximate-TTL events and update counters. Worker and rank removal tasks notify the prune manager when present.
Route TTL work through worker lanes
lib/kv-router/src/indexer/thread_pool.rs
The thread pool routes TTL stores and removals through worker tasks and carries prune state through removal paths. Tests cover refreshes, failed and canceled stores, LRU changes, and worker or rank resets.
Filter prune removals
lib/kv-router/src/indexer/kv_indexer.rs
Prune application consumes pending authorizations before removing entries from the radix tree. A paused-time test checks that a refreshed block survives an earlier expiration and is removed after its refreshed TTL expires.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to ea009

Approximate-TTL stores and removals now run on each worker's ordered lane. A removal only takes effect if its expiration is still current, so a refreshed block is no longer erased by an older, stale expiration. A cancelled store request no longer leaves stored content without a TTL. Tests cover refresh, cancellation, failed stores, and reset ordering. No concrete merge-blocking issue was found.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 27.66% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 47 functions across 7 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the primary change: preventing stale TTL removals after a refresh.
Description check Passed The description is complete and relevant. It includes the required Overview, Details, reviewer guidance, and Related Issues sections, with implementation scope and validation results.
  • Fix all pre-merge checks with AI
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Comment @coderabbitai help to get the list of available commands.

Comment thread lib/kv-router/src/indexer/types.rs Resolved
@bojiang-li
bojiang-li marked this pull request as draft October 1, 2026 11:16
Signed-off-by: bojiang-li <327132355+bojiang-li@users.noreply.github.com>
@bojiang-li
bojiang-li temporarily deployed to external_collaborator October 1, 2026 11:43 — with GitHub Actions Inactive
@bojiang-li
bojiang-li marked this pull request as ready for review October 1, 2026 11:43
Signed-off-by: bojiang-li <327132355+bojiang-li@users.noreply.github.com>
@bojiang-li
bojiang-li temporarily deployed to external_collaborator October 14, 2026 03:41 — with GitHub Actions Inactive
@bojiang-li

Copy link
Copy Markdown
Contributor Author

Hi @ai-dynamo/dynamo-router-codeowners, I resolved the conflict with main in 784b6db, keeping the new LaneLookup and the TTL ordering fix. On Spark, 1131 tests passed with 3 ignored, and strict Clippy and formatting passed. I linked the relevant part of #12785. Could you review when you have a chance? Thanks!

Prepared with Codex assistance.

This branch was previously deployed

1 inactive deployment
external_collaborator — 784b6dbf Deployed Oct 14, 2026 by bojiang-li via ok-to-test #32145
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request is from an external contributor fix size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant