Repository navigation
perf(agents): remove quadratic scaling traversals for sessionprefixin… - #15362
Conversation
…dexer Signed-off-by: Karen Chung <karenc@nvidia.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe indexer now clears session frontiers when workers or worker ranks are removed. Residency versions track both workers and worker ranks. Session-prefix updates reject IDs over 256 bytes, and stored-chain validation checks repeated hashes, cycles, and parent conflicts. ChangesWorker frontier lifecycle
Session-prefix validation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to No confirmed issue currently blocks merging. Worker removal and stored-chain validation preserve the examined behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: Stored session mutations remain unversioned and apply unconditionally, so an in-flight store can recreate a removed worker's frontier after worker or rank cleanup.
- Original discussion: The previously reported stale-store race remains present:
Storedmutations carry no residency version and apply unconditionally, so a store queued before or during worker/rank removal can run afterWorkerRemoved/Clearedand recreate a frontier for removed residency. The author explicitly deferred this lifecycle enforcement. - Original discussion: The reported race remains present: Stored mutations carry no residency generation and are applied unconditionally. A store already enqueued to the primary can enqueue its session update after worker/rank cleanup, recreating a frontier for removed residency.
Overview
This PR follows up on #13807 by improving
SessionPrefixIndexerworker lifecycle handling, bounding session identifier storage, and reducing stored-chain validation overhead.Details
Worker removal
Session identifier bound
Session IDs longer than 256 bytes are ignored before they create persistent
SessionPrefixIndexerstate.Stored-chain validation
Definitions:
k: blocks in one stored event.d: depth of the stored event's parent lineage.N: total lineage length across successive events.w: worker/rank entries with tracked session frontiers.b: frontier bindings for a removed worker.O(k + d)time and temporary memoryO(k)time and memoryO(k + d)per event and potentiallyO(N^2)across one-block replayO(k)per event andO(N)across replayO(k + d)O(k)O(k + d)time and memoryO(k + d)time andO(k)memoryO(w + b)frontier cleanupThe validator now avoids an ancestry walk for new blocks, existing edges with matching parents, and parentless leaf grafts. It walks parent pointers only when grafting an existing subtree. This avoids adding a persistent ancestor-query index.
Validation
cargo fmt --all -- --checkcargo test -p dynamo-kv-router session_prefix_index --libcargo test -p dynamo-kv-router --features standalone-indexer services::indexer::backend::session_tests --libcargo clippy -p dynamo-kv-router --all-targets --features standalone-indexergit diff --checkResults:
SessionPrefixIndexertests passed.Where should the reviewer start?
lib/kv-router/src/session_prefix_index.rsvalidate_chain().lib/kv-router/src/services/indexer/session_updates.rsWorkerRemovedsession mutation.lib/kv-router/src/services/indexer/backend.rsRelated Issues
🔗 This PR is linked to an issue:
Summary by CodeRabbit