[paolo-perrone]

Overview

PositionalIndexer never deletes anything from its (position, local_hash) map. Once the last worker holding a block removes it, the entry stays behind empty, so the map keeps one dead entry for every distinct block it's ever indexed. Under cache churn that's memory that only grows, for anyone using this backend, including the nested-map indexer in the Mooncake benchmarks.

Key Changes

About 16 lines of core logic, plus one test.

• positional.rs: a new remove_from_index helper calls DashMap::remove_if_mut. It removes the worker and drops the entry when SeqEntry::remove says it's empty, in one step under the shard lock.
• The three removal paths (Removed events, Cleared events and worker removal) now go through that helper instead of get_mut plus let _ = entry.remove(..).

Validation

SeqEntry::remove already returns true when the entry "is now completely empty and should be removed", and the indexer README lists dropping empty entries as step 4 of remove_blocks. All three callers just threw that result away. I used remove_if_mut rather than checking first and calling remove after, because a store from another event thread could land in between and get deleted.

Case	Before	After
Worker 0 stores [1, 2, 3], then a Removed event for them	3 entries left	0
Same store, then a Cleared event	3 left	0
Same store, then remove_worker	3 left	0
Worker 0 stores [1, 2, 3], worker 1 stores [1, 2], worker 0 removes its blocks	3 left	2 left, and worker 1 still scores 2

removals_drop_emptied_index_entries covers all four rows and fails on main with left: 3, right: 0. cargo test -p dynamo-kv-router --lib passes (1150 passed), and cargo fmt --check and pre-commit are clean. Routing doesn't change: lookups already treat an empty entry like a missing one, so this only gives the memory back.

Issues and Ongoing Work

• Closes [BUG]: PositionalIndexer keeps an empty entry for every block it ever stored #17961
• I didn't find an open PR or issue for this. Seven open PRs touch positional.rs (feat(kv-router): carry shared-cache eligibility in events #15260, fix(kv-router): fence stale TTL removals after refresh #15476, feat(kv-router): expose per-rank resident block counts to selection plugins #16078, feat(kv-router): classify shared-cache event eligibility (2b) #16431, feat(kv-router): preserve opt-in dump eligibility (2c) #16432, feat(kv-router): keep recurrent-group KV events and record their state presence #16876, feat(mm-routing): restore SGLang multimodal KV routing with routing-label spans #17082), and none of them changes the removal paths. fix(kv-router): reclaim CRTC dead leaves as they die #16313 did the same kind of cleanup for the CRTC tree's dead leaves; this does it for the positional backend.

Pull Request Checklist

• I used Dynamo's /pr-checklist skill.

Summary by CodeRabbit

• Bug Fixes
  • Removed search index entries when their final worker reference is cleared, while preserving entries still referenced by another worker.
  • Applied consistent cleanup across block removal, DP-rank clearing, and worker removal.

[copy-pr-bot]

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.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

Block, DP-rank, and worker removal now use a shared helper that removes positional index entries when their SeqEntry becomes empty. Tests cover these removal paths and confirm that entries held by another worker remain searchable.

Changes

Positional index cleanup

Layer / File(s)	Summary
Remove empty positional entries lib/kv-router/src/indexer/positional.rs	Block, DP-rank, and worker removal use remove_from_index to remove empty entries. Tests cover each removal path and verify that entries held by another worker remain searchable.

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Medium

Merge Risk: Minimal · up to 0c711

No merge-blocking risk is established by the available review evidence.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the primary change: removing emptied entries from the positional indexer.
Description check		The description includes all required sections, explains the issue and implementation, documents validation results, links the issue, and completes the checklist.
Linked Issues check		[#17961] requires that PositionalIndexer delete a (position, local_hash) entry when its last worker is removed, for Removed events, Cleared events, and worker removal. The diff at merge base 3aafb…
Out of Scope Changes check		The whole-PR diff touches only lib/kv-router/src/indexer/positional.rs. Its changes are the remove_from_index helper, the three call-site replacements that use it, and a new unit test in the exist…

Full details: Linked Issues check

Explanation

[#17961] requires that PositionalIndexer delete a (position, local_hash) entry when its last worker is removed, for Removed events, Cleared events, and worker removal. The diff at merge base 3aafbef to head 0c711ec replaces the three removal sites (remove_blocks_impl, remove_worker_dp_rank_impl, remove_worker_blocks_impl) with remove_from_index. That helper calls DashMap::remove_if_mut(&key, |_, entry| entry.remove(seq_hash, worker)). SeqEntry::remove returns true when the entry is empty, so the map drops the key under the shard lock. The added test removals_drop_emptied_index_entries asserts index.len() == 0 after a Removed event, a Cleared event, and remove_worker_blocks_impl. It also asserts that an entry still held by worker 1 survives and keeps scoring. This matches the issue's reproduction and acceptance criteria. Routing lookups are unchanged, which the issue also requires. Limitation: I did not run the tests. The result rests on the source diff and the PR's reported test run. I also did not re-read the Cleared branch of apply_event, which was truncated. The Cleared path is treated as routed through the changed helpers, per the PR description.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff touches only lib/kv-router/src/indexer/positional.rs. Its changes are the remove_from_index helper, the three call-site replacements that use it, and a new unit test in the existing tests module. The test imports make_clear_event and make_remove_event from crate::test_utils, which this PR does not modify. No public API, other indexer backend, or configuration changes appear in the diff. Every change maps to #17961 or to its test coverage. No unrelated change was found.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 1 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

Recent review info

Run configuration

• Source mode: Auto
• Repository: ai-dynamo/dynamo
• Branch: main
• File: .coderabbit.yaml
• Commit: c60e700
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 477f6fde-fdc3-46b1-b737-b97c6aec1977

Commits

Reviewing files that changed from the base of the PR and between c60e700 and 0c711ec.

Files selected for processing: 1

• lib/kv-router/src/indexer/positional.rs

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

[github-actions]

👋 Hi paolo-perrone! 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.

🚀

[jh-nv]

/ok to test 0c711ec

[devin-ai-integration]

✅ Dynamo PR CI passed — run 41976114117 (attempt 1) on 0c711ecd24

Gate checks: ✅ backend-status-check · ✅ deploy-status-check · ✅ dynamo-status-check

_{Posted automatically by Devin for run 41976114117. Updated on every full-CI run of this PR.}