fix: improved leader clean up

[ryanolson]

Under the condition request_finished marks the slot as Finishing, but not Finished; there is no future re-evaluation of the slot to transition it to Finished and no calls to remove that slot from the slot manager, resulting in minor host memory leak.

Summary by CodeRabbit

• Chores
  • Updated configuration files for consistency.
  • Enhanced internal connector lifecycle management with improved request handling and state transitions.

[ryanolson]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Walkthrough

Added a new trait method update_connector_output to the Leader lifecycle with implementations in KvConnectorLeader (no-op) and KvConnectorLeaderRecorder (delegation). Enhanced unscheduled request handling with state-aware branching logic for Finishing/Finished slot transitions. Updated .gitignore formatting.

Changes

Cohort / File(s)	Summary
Gitignore formatting \.gitignore	Added blank lines and START/END Ruler Generated Files block in Direnv section
Leader trait lifecycle hook lib/kvbm/src/block_manager/vllm/connector/leader.rs	Introduced update_connector_output() trait method; enhanced unscheduled request handling with state-aware branching for slot state Finishing/Finished transitions; modified request_finished() to conditionally remove slots based on state; added lifecycle documentation
Recorder proxy delegation lib/kvbm/src/block_manager/vllm/connector/leader/recorder.rs	Added update_connector_output() method delegating to wrapped connector_leader instance

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• leader.rs requires careful attention to the new state branching logic in unscheduled request handling—verify correct slot state transitions between Finishing and Finished, and confirm the skip-path preservation logic integrates correctly with the new lifecycle hook
• leader.rs request_finished() changes need validation that slot removal and inflight request cleanup occur at the correct state boundaries
• Ensure the new update_connector_output() method placement and lifecycle documentation align with intended future migrations mentioned in comments

Poem

🐰 A trait takes flight with fresh lifecycle grace,
State branches bloom where Finishing finds its place,
Slots transition smoothly from pending to done,
The recorder relays with a delegated run—
New hooks connect hearts as the leader grows strong! 🌿

Pre-merge checks

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description identifies the core issue but lacks structure; it does not follow the repository's template with Overview, Details, Where to start, and Related Issues sections.	Restructure the description using the template: add Overview, Details sections explaining the changes, identify files to review closely, and add Related Issues with appropriate GitHub issue references.
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title 'fix: improved leader clean up' is vague and generic; it does not clearly convey the specific issue being fixed (memory leak when slot transitions to Finishing but not Finished).	Consider a more specific title such as 'fix: prevent memory leak from unfinished slot cleanup in leader' or 'fix: ensure slot removal when request_finished marks slot as Finishing'.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

lib/kvbm/src/block_manager/vllm/connector/leader/recorder.rs (1)

323-325: Record update_connector_output events for replay parity.

Every other Leader method recorded here emits an Action so the recorder/replayer path stays faithful. When update_connector_output gains real behavior, having no corresponding action will make recordings incomplete. Please consider adding a matching Action variant (or equivalent instrumentation) and enqueueing it here so we keep the trace consistent.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fbabcc9 and 6abf7b3.

📒 Files selected for processing (3)

• .gitignore (1 hunks)
• lib/kvbm/src/block_manager/vllm/connector/leader.rs (3 hunks)
• lib/kvbm/src/block_manager/vllm/connector/leader/recorder.rs (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: clippy (launch/dynamo-run)
• GitHub Check: tests (.)
• GitHub Check: tests (lib/bindings/python)
• GitHub Check: clippy (.)
• GitHub Check: tests (lib/runtime/examples)
• GitHub Check: tests (launch/dynamo-run)
• GitHub Check: Build and Test - dynamo

🔇 Additional comments (1)

lib/kvbm/src/block_manager/vllm/connector/leader.rs (1)

477-512: Nice cleanup of Finishing slots.

Re-entering the lock, advancing the state, and pruning both the slot and inflight_requests when we finally hit Finished closes the leak path we saw in Finishing-only scenarios. Looks solid.