Repository navigation
Conversation
|
👋 Hi Shang-Pin! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
|
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 (1)
WalkthroughThe radix tree gains weak parent backlinks and cascading removal of empty nodes. Worker-removal paths invoke detachment, while tests cover split-ancestor cascades, shared-node retention, and chain recreation. ChangesRadix tree dead-node detachment
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
This comment has been minimized.
This comment has been minimized.
apply_removed / remove_worker clear worker state and lookup entries (so the tracked-blocks gauge stays bounded) but never detached the node from its parent's children map — nodes had no parent link. Every unique stored chain therefore permanently retained its edge Vec and edge_index map after its blocks were removed: ~65B per block ever stored, ~1GB/h of frontend RSS at ~30rps of unique-heavy traffic (jeprof: 9.6GB in 11h, 98% under RadixTree::apply_event_with_counters, while kv_indexer_tracked_blocks oscillated healthily at 350-870k). Give each node a Weak parent back-link and detach it once it holds no workers and no children, cascading upward while ancestors become empty. Applies to block removes, worker removal, and Cleared events alike; the TTL prune path also benefits since it funnels through apply_removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 3d72f5e) Signed-off-by: Shang-Pin <shang-pin@deepinfra.com>
270e4ef to
8537539
Compare
Unsigned Commits DetectedThe following commits are not GPG-signed and must be signed before CI can run:
Please sign your commits and push again. See the GitHub docs on commit signature verification for help. |
|
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. |
Overview:
RadixTreenever frees the memory of removed blocks:apply_removed(and workerremoval /
Cleared) clears worker state and lookup entries, but a node whoseworkers are all gone is never detached from its parent's
childrenmap — nodeshave no parent link, and
clear_children_if_unreachableonly clears the node'sown children. Every unique stored chain therefore permanently retains its
edgeVec andedge_indexhashbrown map as an unreachable-but-alive "zombie"under the immortal root.
On a production disaggregated frontend serving unique-heavy traffic (~30 rps),
this leaked ~1 GB/h of router memory indefinitely: jeprof showed 9.6 GB of live
allocations after 11 h, 98% under
RadixTree::apply_event_with_counters→RadixBlock::for_blocks, while the tracked-blocks gauge stayed healthilybounded (350–870k, oscillating with load) — the tree forgets blocks but never
frees their nodes. The arithmetic closes: ~1M requests × ~140 blocks/chain ×
~65 B/block ≈ the measured heap. An A/B with only this fix reverted reproduced
the ~1 GB/h growth; with the fix, the same frontend held a flat ~1 GB working
set over a 3-day soak.
The leak affects every removal path, including the TTL prune (it funnels
through the same
apply_removed); kv-events mode is hit hardest becausenothing else bounds the tree. Hot shared prefixes are unaffected (dead nodes
under them get revived by the next store) — it is the unique-suffix tail that
accumulates, so severity scales with prompt uniqueness.
Details:
Weakparent back-link toRadixBlock(noRccycles), wired at allnode creation/move sites:
apply_stored's two child-creation sites, andsplit_node(the new suffix node, plus re-pointing the children it inherits).detach_if_dead: once a node holds no workers and has no children,remove it from its parent's
childrenmap, cascading upward while ancestorsbecome empty; stops at the root (dangling parent). Guarded by
Rc::ptr_eqagainst slot reuse.
apply_removed,remove_or_clear_worker_blocks(also coversCleared), andremove_worker_dp_rank.live children) are untouched; a re-stored chain after detach recreates
cleanly.
multi-worker sparing, worker-removal detach, store-after-detach recreation.
Where should the reviewer start?
lib/kv-router/src/indexer/radix_tree.rs—detach_if_deadand its three callsites; then the parent wiring in
split_node(the inherited-childrenre-pointing is the one non-obvious spot).
Related Issues
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit