[cli] fix: guard service-stop against late sessions - #244
Conversation
|
Warning Review limit reached
Next review available in: 11 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)
✨ 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 tightens the non-force shutdown sequence for the KeepGPU daemon by introducing a final status check after stopping sessions to ensure no new active sessions have appeared before signaling the process. The reviewer recommends wrapping the subsequent RPC calls in a try-except block to gracefully handle potential ServiceUnreachableError exceptions and maintain consistent, user-friendly error messages.
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.
| _require_no_active_jobs_for_service_stop(status) | ||
| stop_result = _rpc_call("stop_keep", {}, host, port, timeout=45.0) | ||
| stop_result = _validate_stop_keep_result(stop_result) | ||
| _require_clean_stop_keep_for_service_stop(stop_result) | ||
| status = _rpc_call("status", {}, host, port) | ||
| status = _validate_status_result(status, single_job=False) | ||
| _require_no_active_jobs_for_service_stop(status) |
There was a problem hiding this comment.
If the service becomes unreachable during the stop_keep call or the final status recheck, a ServiceUnreachableError will be raised. Since these calls are outside the initial try...except ServiceUnreachableError block, the error will propagate to the outer handler as a generic exception, missing the helpful context that explains why the non-force shutdown failed and suggesting the use of --force.
Consider wrapping the entire RPC sequence (including stop_keep and the final status check) in a try...except ServiceUnreachableError block to ensure consistent and user-friendly error messages if the daemon becomes unresponsive mid-shutdown.
| _require_no_active_jobs_for_service_stop(status) | |
| stop_result = _rpc_call("stop_keep", {}, host, port, timeout=45.0) | |
| stop_result = _validate_stop_keep_result(stop_result) | |
| _require_clean_stop_keep_for_service_stop(stop_result) | |
| status = _rpc_call("status", {}, host, port) | |
| status = _validate_status_result(status, single_job=False) | |
| _require_no_active_jobs_for_service_stop(status) | |
| _require_no_active_jobs_for_service_stop(status) | |
| try: | |
| stop_result = _rpc_call("stop_keep", {}, host, port, timeout=45.0) | |
| stop_result = _validate_stop_keep_result(stop_result) | |
| _require_clean_stop_keep_for_service_stop(stop_result) | |
| status = _rpc_call("status", {}, host, port) | |
| status = _validate_status_result(status, single_job=False) | |
| _require_no_active_jobs_for_service_stop(status) | |
| except ServiceUnreachableError as exc: | |
| raise RuntimeError( | |
| f"KeepGPU service is unavailable at {host}:{port}. Non-force service-stop must verify no tracked keep sessions before stopping the daemon. " | |
| "For an unresponsive auto-started daemon, run `keep-gpu service-stop --force`." | |
| ) from exc |
1 participant
Summary
keep-gpu service-stopso a non-emptystopped,timed_out, orfailedstop_keepresult blocks daemon signalingstatusrecheck before_stop_service_process()to catch sessions that appear afterstop_keepreturns cleanlyVerification
PYTHONPATH=src pytest tests/test_cli_service_commands.py::test_service_stop_rejects_newly_stopped_session_before_stopping_daemon -q(RED before fix, then pass)PYTHONPATH=src pytest tests/test_cli_service_commands.py::test_service_stop_rechecks_status_after_clean_stop_keep_before_stopping_daemon -q(RED before fix, then pass)PYTHONPATH=src pytest tests/test_cli_service_commands.py -q->226 passedPYTHONPATH=src pytest tests -q->921 passed, 11 skippedmkdocs build --strictpre-commit run --all-files --show-diff-on-failureREADME/Zenodo badge untouched.