[mcp] fix: retain cleanup debt after failed start - #292
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a controller-startup cleanup retention contract to KeepGPUServer.start_keep(): sessions failing after controller work begins are retained as "stopping" (cleanup timeout) or "stop_failed" (cleanup error) instead of being silently discarded, preserving the original startup error. Includes new tests and documentation. ChangesStartup Cleanup Retention
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StartKeep as start_keep()
participant Retain as _retain_failed_start_for_cleanup()
participant Controller
Client->>StartKeep: request start
StartKeep->>Controller: keep()
Controller-->>StartKeep: raises exception
alt controller is None
StartKeep->>StartKeep: discard starting/pending-stop tracking
else controller exists
StartKeep->>Retain: retain session
Retain->>Retain: set state=stopping, remove from starting/pending-stop
Retain->>Controller: _release_with_timeout()
alt cleanup succeeds
Controller-->>Retain: success
Retain->>Retain: remove retained session
else cleanup times out or errors
Controller-->>Retain: timeout/TimeoutError
Retain->>Retain: mark state=stopping/stop_failed, log
end
end
StartKeep-->>Client: return original startup error
Possibly related PRs
Poem A job that stumbled, mid-controller stride, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a mechanism to retain a visible session when a GPU controller fails startup after construction and cleanup cannot be proven complete immediately. Specifically, it updates KeepGPUServer.start_keep() to transition the session to stopping or stop_failed states depending on whether cleanup times out or raises an error, while still propagating the original startup exception to the caller. The changes also include updated documentation, a detailed implementation plan, and comprehensive unit and HTTP API tests to verify this retention behavior. There are no review comments to address, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/mcp/test_server.py (1)
2184-2211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
FailsAfterWorkControllerdefinition.An almost identical
FailsAfterWorkControlleris defined inline in the previous test (Line 2143) and again here, and a third variant appears intests/mcp/test_http_api.py. Consider hoisting a shared helper/fixture to reduce duplication, otherwise the logic itself is correct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mcp/test_server.py` around lines 2184 - 2211, The test logic is fine, but FailsAfterWorkController is duplicated across multiple tests, including this KeepGPUServer cleanup-error case and the earlier inline variant. Hoist the shared controller behavior into a reusable helper or fixture in the test module, and update the tests to reference that shared symbol so the duplicated class definition is removed while preserving the current assertions in test_failed_start_with_cleanup_error_remains_visible_as_stop_failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/mcp/test_server.py`:
- Around line 2184-2211: The test logic is fine, but FailsAfterWorkController is
duplicated across multiple tests, including this KeepGPUServer cleanup-error
case and the earlier inline variant. Hoist the shared controller behavior into a
reusable helper or fixture in the test module, and update the tests to reference
that shared symbol so the duplicated class definition is removed while
preserving the current assertions in
test_failed_start_with_cleanup_error_remains_visible_as_stop_failed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25ffbd44-90a6-4ffa-b02f-5b320ea12fbc
📒 Files selected for processing (7)
AGENTS.mddocs/concepts/architecture.mddocs/guides/mcp.mddocs/plans/2026-07-02-startup-cleanup-retention.mdsrc/keep_gpu/mcp/server.pytests/mcp/test_http_api.pytests/mcp/test_server.py
e7dd2bc to
1601658
Compare
1601658 to
d7fc4f7
Compare
|
Resolved the remaining CodeRabbit nit in d7fc4f7 by replacing the duplicated test-local failing controller classes in tests/mcp/test_server.py with a small _failing_after_work_factory helper. Re-verified with targeted cleanup-debt tests, pre-commit, and the full MCP/HTTP suite (347 passed). |
1 participant
Summary
stoppingand cleanup errors asstop_failed.docs/plans/.Verification
PYTHONPATH=src pytest tests/mcp/test_server.py::test_failed_start_with_cleanup_timeout_remains_visible_as_stopping tests/mcp/test_server.py::test_failed_start_with_cleanup_error_remains_visible_as_stop_failed -qfailed before the service fix because status returned inactive.PYTHONPATH=src pytest tests/mcp/test_server.py::test_failed_start_with_cleanup_timeout_remains_visible_as_stopping tests/mcp/test_server.py::test_failed_start_with_cleanup_error_remains_visible_as_stop_failed tests/mcp/test_http_api.py::test_http_start_failure_cleanup_timeout_keeps_session_visible -q-> 3 passed.PYTHONPATH=src pytest tests/mcp/test_server.py tests/mcp/test_http_api.py -q-> 347 passed.mkdocs build --strict-> passed, with the known Material for MkDocs 2.0 warning.pre-commit run --all-files --show-diff-on-failure-> passed.Summary by CodeRabbit
New Features
stoppingduring delayed cleanup, orstop_failedif cleanup encounters an error.Bug Fixes