lib/kv-router/src/nested_map.rs (1)

303-310: Pre-existing TOCTOU on worker_blocks — consider entry API.

Lines 305–310 have a check-then-act pattern (contains_key → insert → get().unwrap()) that's racy if two threads hit the same worker key simultaneously. The sticky routing in ThreadPoolIndexer makes this safe in practice, but using the entry API would be both cleaner and formally correct:

♻️ Suggested simplification

-        if !worker_blocks.contains_key(&worker) {
-            worker_blocks.insert(worker, RwLock::new(FxHashMap::default()));
-        }
-
-        let worker_blocks_entry = worker_blocks.get(&worker).unwrap();
-        let mut worker_map = worker_blocks_entry.write();
+        let worker_blocks_entry = worker_blocks
+            .entry(worker)
+            .or_insert_with(|| RwLock::new(FxHashMap::default()));
+        let mut worker_map = worker_blocks_entry.write();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/kv-router/src/nested_map.rs` around lines 303 - 310, The current
contains_key → insert → get().unwrap() pattern on worker_blocks is racy; replace
it with the HashMap entry API: use worker_blocks.entry(worker).or_insert_with(||
RwLock::new(FxHashMap::default())) to obtain the RwLock for that worker
atomically, then call .write() on the returned entry to get worker_map; update
the code paths that reference worker_blocks, worker, RwLock, and FxHashMap
accordingly to remove the check-then-act and the unwrap.

lib/kv-router/src/naive_indexers.rs (4)

119-143: Actor threads spawn a full Tokio runtime just for mpsc::Receiver::recv().

Each actor creates a dedicated OS thread + a current_thread Tokio runtime solely to .await on the mpsc::Receiver. Since the inner logic is entirely synchronous, a simpler approach would be to use std::sync::mpsc (or flume) and avoid the Tokio runtime overhead altogether. This is bench-only code so it's not blocking, but the extra runtime may add noise to benchmark measurements.

Also applies to: 323-347

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/kv-router/src/naive_indexers.rs` around lines 119 - 143, The actor thread
currently spawns a full Tokio current_thread runtime and uses rt.block_on to
await rx.recv(), which is unnecessary because NaiveNestedMapInner and message
handling (ActorMessage variants: Event, Match, RemoveWorker) are fully
synchronous; replace the async mpsc::Receiver usage with a synchronous channel
(std::sync::mpsc or flume) and convert the thread closure to a plain blocking
loop that calls recv() (or recv_timeout if needed) to pull messages and handle
them by calling inner.apply_event, inner.find_matches (sending replies), and
inner.remove_worker, removing the Tokio runtime creation
(tokio::runtime::Builder, rt.block_on) and simplifying the spawn to a standard
thread loop over the sync receiver.

---

175-177: Silently dropped send errors in apply_event.

Both apply_event implementations discard the Result from self.tx.send(...). If the actor thread panics or the channel fills up (bounded at 2048), events are silently lost with no log or metric. For benchmarking correctness validation, consider at least a tracing::warn! on failure.

Also applies to: 379-381

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/kv-router/src/naive_indexers.rs` around lines 175 - 177, The apply_event
implementations silently drop the Result from
self.tx.send(ActorMessage::Event(...)), which loses events if the channel is
closed or full; update both apply_event (the one using
self.tx.send(ActorMessage::Event(event)).await and the other occurrence around
the second implementation) to handle the Result: match or if let Err(e) =
self.tx.send(...).await and emit a tracing::warn! (including context like
"failed to send ActorMessage::Event" and the error) and/or increment a metric
counter for dropped events, so failures are observable and include the error
details for debugging.

---

244-260: Redundant re-check of position 0 in the loop.

active is cloned from self.index.get(&sequence[0]) on line 238, then the loop starting at line 244 re-checks sequence[0] (since enumerate() starts at 0). The first iteration is always a no-op. Starting from skip(1) and initializing scores accordingly would be slightly cleaner:

♻️ Suggested fix

-        for (depth, local_hash) in sequence.iter().enumerate() {
+        for (depth, local_hash) in sequence.iter().enumerate().skip(1) {
             let empty = HashSet::new();
             let workers_here = self.index.get(local_hash).unwrap_or(&empty);
 
             active.retain(|w| {
                 if workers_here.contains(w) {
                     true
                 } else {
                     scores.scores.insert(*w, depth as u32);
                     false
                 }
             });
 
             if active.is_empty() {
                 break;
             }
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/kv-router/src/naive_indexers.rs` around lines 244 - 260, The loop
redundantly re-checks sequence[0] because active was already cloned from
self.index.get(&sequence[0]); change the iterator to skip the first element (use
sequence.iter().enumerate().skip(1)) so the first no-op iteration is removed,
keep the body that looks up workers_here via self.index.get(local_hash) and
updates scores.scores and active as before (symbols to edit: active, sequence,
self.index, scores.scores, the enumerate loop). Ensure behavior is otherwise
unchanged.

---

47-67: find_matches doesn't verify prefix/positional ordering — intentional but worth a note.

This checks set membership (blocks.contains(local_hash)) without verifying that blocks appear at the correct positions in the sequence. A worker with blocks {A, C} queried with [A, B, C] correctly returns depth 1, but [C, A] would also return depth 2 even though the prefix order differs. The doc says "O(W × D) per find_matches" which matches, but this semantic gap vs. the production indexer could produce misleading benchmark comparisons if the query workload has reordered tokens.

Given this is bench-only and the doc already caveats "cut corners," this is just a heads-up for interpreting results.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/kv-router/src/naive_indexers.rs` around lines 47 - 67, The current
find_matches implementation only checks set membership via
blocks.contains(LocalBlockHash) and thus counts matches regardless of
positional/prefix order; update find_matches to verify positional ordering by
using each worker's ordered block sequence (self.index entry) rather than set
membership: build or use a mapping from LocalBlockHash to its index in the
worker's block list and then, for each query sequence, only increment depth
while the next sequence hash maps to the next consecutive index (ensuring
prefix/positional order), then insert the final depth into OverlapScores; adjust
any types as needed so blocks is accessible as an ordered sequence or index map
instead of an unordered set.