Skip to content

[controllers] fix: surface keep worker startup failures - #101

Merged
Wangmerlyn merged 2 commits into
mainfrom
codex/controller-startup-failures
Jun 28, 2026
Merged

[controllers] fix: surface keep worker startup failures#101
Wangmerlyn merged 2 commits into
mainfrom
codex/controller-startup-failures

Conversation

@Wangmerlyn

@Wangmerlyn Wangmerlyn commented Jun 28, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add CUDA/ROCm startup handshakes so keep() only reports success after fatal backend setup succeeds.
  • Prevent false active service sessions when worker startup fails, and keep failed/stopping startup retries from returning success.
  • Ensure ROCm SMI shutdown runs when startup fails before a service session owns cleanup.
  • Update AGENTS.md, Python/API/architecture docs, and the implementation plan.

Review loop

  • Local spec review found ROCm SMI cleanup gap; fixed and re-reviewed clean.
  • Local code-quality review found stale startup-thread retry false-success gap plus the SMI cleanup gap; fixed and re-reviewed clean.

Verification

  • RED focused startup tests failed before implementation: keep() did not raise and service returned a job id.
  • Review-regression tests after fixes -> 3 passed
  • Original focused startup tests after fixes -> 3 passed
  • PYTHONPATH=$PWD/src pytest tests/cuda_controller tests/rocm_controller tests/mcp/test_server.py -q -> 100 passed, 5 skipped
  • PYTHONPATH=$PWD/src pytest tests -q -> 312 passed, 11 skipped
  • PYTHONPATH=$PWD/src mkdocs build -> exit 0, known MkDocs/Material and unnav'd plan notices
  • pre-commit run --all-files -> passed

Summary by CodeRabbit

  • Bug Fixes

    • Improved GPU session startup so failures are reported immediately instead of appearing as successful starts.
    • Prevented duplicate start attempts while a previous startup is still shutting down.
    • Ensured GPU shutdown cleanup happens reliably after startup failures.
  • Documentation

    • Clarified manual start/stop behavior, startup timing, and how runtime versus startup errors are handled.
    • Added guidance for startup failure handling and verification steps.

@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@Wangmerlyn, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 53 minutes and 52 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 credits.

🚦 How do rate 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 see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1c17c569-60ae-40fc-ab23-3cfb7ddce57a

📥 Commits

Reviewing files that changed from the base of the PR and between e3bac6d and abe3560.

📒 Files selected for processing (2)
  • src/keep_gpu/single_gpu_controller/cuda_gpu_controller.py
  • src/keep_gpu/single_gpu_controller/rocm_gpu_controller.py
📝 Walkthrough

Walkthrough

Adds a synchronous startup handshake to CudaGPUController.keep() and RocmGPUController.keep() using startup_evt/startup_errors shared with _keep_loop. Fatal backend startup failures now propagate before keep() returns. Includes controller and JSON-RPC tests, a release() fix for ROCm shutdown, and documentation updates across four docs files plus a new implementation plan.

Changes

Synchronous keep() startup handshake

Layer / File(s) Summary
CudaGPUController startup handshake
src/keep_gpu/single_gpu_controller/cuda_gpu_controller.py
keep() inspects _stop_evt for prior failures, creates startup_evt/startup_errors, waits up to 5s for worker signal, and raises on timeout or captured error. _keep_loop now accepts those parameters and signals startup success or failure via startup_evt before the workload loop.
RocmGPUController startup handshake and release fix
src/keep_gpu/single_gpu_controller/rocm_gpu_controller.py
keep() rejects alive-thread/stop-event conflicts, coordinates startup with bounded timeout, and cleans up ROCm SMI on failure. _keep_loop signals startup completion. release() moves ROCm shutdown into finally.
Controller and JSON-RPC startup failure tests
tests/cuda_controller/test_keep_and_release.py, tests/rocm_controller/test_rocm_backoff.py, tests/mcp/test_server.py
Tests verify keep() propagates startup exceptions, leaves no live thread, rejects retries during incomplete startup, and that JSON-RPC returns JSONRPC_INTERNAL_ERROR with no active session when CUDA worker startup fails.
Documentation and guidelines
AGENTS.md, docs/plans/controller-startup-failures.md, docs/concepts/architecture.md, docs/guides/python.md, docs/reference/api.md
Updates guidelines, architecture, Python guide, and API reference to describe the startup handshake invariant. Adds a new implementation plan documenting the handshake design and test requirements.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Wangmerlyn/KeepGPU#56: Introduced RocmGPUController and its keep/release behavior, which this PR directly extends with the startup handshake.
  • Wangmerlyn/KeepGPU#59: Added MCP JSON-RPC server handling; this PR adds a regression test against that same server for CUDA startup failure propagation.
  • Wangmerlyn/KeepGPU#62: Also modifies AGENTS.md guidelines, which this PR further extends with startup/error propagation requirements.

Poem

🐇 A thread woke up and tried to run,
But startup failed before it'd begun.
No false "success!" would slip on through—
The event was set, the error flew.
Now keep() waits, then speaks the truth,
And sessions stay clean—hop, that's the proof!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.54% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: surfacing controller keep worker startup failures.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/controller-startup-failures

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

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements a startup handshake mechanism for both CUDA and ROCm single-GPU controllers to ensure that keep() does not report success if backend startup setup fails. It introduces synchronous propagation of fatal startup errors (such as device-selection errors) to prevent the service layer from registering false active sessions. The changes include updates to documentation, implementation of the handshake using threading events in the controllers, and comprehensive unit and integration tests to verify the new behavior. There are no review comments, so I have no 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 coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@src/keep_gpu/single_gpu_controller/cuda_gpu_controller.py`:
- Line 204: The startup exception handler in CUDA_GPUController is intentionally
broad, but Ruff now flags it with BLE001. Add a targeted suppression comment on
the except Exception as exc block in the startup path, and include a short
rationale that this catch is deliberate for startup fallback/error handling.
Keep the suppression local to the specific handler so pre-commit stops reporting
the warning without masking other broad exceptions.

In `@src/keep_gpu/single_gpu_controller/rocm_gpu_controller.py`:
- Line 113: Add local Ruff suppressions for the intentional broad exception
handlers in the ROCm GPU controller: the best-effort cleanup catch in the
cleanup path and the backend startup failure propagation catch should each be
marked with the appropriate BLE001 noqa comment. Update the affected `except
Exception as exc` blocks in `RocmGpuController` so the intent is explicit and
Ruff no longer flags these deliberate broad catches.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 26dca3f2-d398-4faf-b787-5ea6d266c324

📥 Commits

Reviewing files that changed from the base of the PR and between fab5a03 and e3bac6d.

📒 Files selected for processing (10)
  • AGENTS.md
  • docs/concepts/architecture.md
  • docs/guides/python.md
  • docs/plans/controller-startup-failures.md
  • docs/reference/api.md
  • src/keep_gpu/single_gpu_controller/cuda_gpu_controller.py
  • src/keep_gpu/single_gpu_controller/rocm_gpu_controller.py
  • tests/cuda_controller/test_keep_and_release.py
  • tests/mcp/test_server.py
  • tests/rocm_controller/test_rocm_backoff.py

Comment thread src/keep_gpu/single_gpu_controller/cuda_gpu_controller.py Outdated
Comment thread src/keep_gpu/single_gpu_controller/rocm_gpu_controller.py Outdated
@Wangmerlyn
Wangmerlyn merged commit 14b0255 into main Jun 28, 2026
5 checks passed
@Wangmerlyn
Wangmerlyn deleted the codex/controller-startup-failures branch June 28, 2026 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant