Move restart through core daemon#299
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 26 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 (9)
📝 WalkthroughWalkthroughThis PR adds a control-plane restart command and HTTP text endpoint to the daemon, a CLI restart client that falls back to local bootstrap restart on build mismatch, shell shim restart fast-path support, and refactors runtime-restart to use lazy default hooks instead of eager imports. ChangesControl-plane restart flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant restartControlPlaneFromCli
participant Daemon
participant restartAimuxControlPlane
CLI->>restartControlPlaneFromCli: restartControlPlaneFromCli(projectRoot)
restartControlPlaneFromCli->>Daemon: core.restart command
alt daemon healthy
Daemon->>restartAimuxControlPlane: perform restart
restartAimuxControlPlane-->>Daemon: restart result
Daemon-->>restartControlPlaneFromCli: {restart, text} source daemon
else build mismatch error
restartControlPlaneFromCli->>restartAimuxControlPlane: local bootstrap restart
restartAimuxControlPlane-->>restartControlPlaneFromCli: local result, source local-bootstrap
end
restartControlPlaneFromCli-->>CLI: CliControlPlaneRestartResult
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/installed-aimux-shim.sh (2)
70-89: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTemp file not cleaned up if interrupted mid-request.
If the shell process is killed/interrupted during the up-to-300s
curlcall,body_filecreated bymktempis left behind since there's no trap for cleanup. Low impact (single leaked temp file in/tmp) but easy to harden.🧹 Optional cleanup via trap
aimux_try_restart() { port="$(aimux_matching_daemon_port)" || return 1 body_file="$(mktemp "${TMPDIR:-/tmp}/aimux-restart.XXXXXX")" || return 1 + trap 'rm -f "$body_file"' EXIT INT TERM status="$(🤖 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 `@scripts/installed-aimux-shim.sh` around lines 70 - 89, The aimux_try_restart helper leaves the mktemp-created body_file behind if the shell is interrupted during the long curl request. Harden this by adding cleanup around aimux_try_restart so body_file is always removed on exit or interruption, using a trap scoped to the function and ensuring it is cleared after normal completion; keep the change localized to aimux_try_restart and its existing mktemp/curl/cat flow.
64-68: 🚀 Performance & Scalability | 🔵 TrivialDuplicate
/healthfetch introduced by the refactor.
aimux_try_daemon_ensure()callsaimux_matching_daemon_port(), which already performs an internalaimux_health_jsoncall via command substitution (a subshell), then makes a second, separateaimux_health_jsoncall here to get JSON foraimux_print_daemon_ensure. Since the first call runs in a subshell, itsjsonvariable can't be reused, so this doubles the localhost network round-trip and introduces a small window where the daemon's health could change between the two checks.Consider having
aimux_matching_daemon_port()emit both the port and enough info (e.g., printportandpidon separate lines, or reuse the already-fetched JSON) so callers avoid the redundant fetch.
[recommended_refactor_effort_low_reward_medium]🤖 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 `@scripts/installed-aimux-shim.sh` around lines 64 - 68, aimux_try_daemon_ensure() currently triggers two /health fetches because aimux_matching_daemon_port() already calls aimux_health_json internally and then aimux_try_daemon_ensure() calls aimux_health_json again for aimux_print_daemon_ensure. Update the flow so the first lookup reuses or returns the fetched JSON (or returns both port and needed metadata) and have aimux_try_daemon_ensure() pass that through, removing the redundant localhost round-trip while keeping aimux_print_daemon_ensure fed with the same data.src/daemon.ts (1)
426-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type to
restartControlPlane.This is the sole assembly point for
CoreRestartResult({ restart, text }), consumed by both the command route and the text route. An explicit annotation would catch future drift against thecore-command-contract.tscontract at compile time.🛠️ Proposed fix
- private async restartControlPlane(issuedAt: string, projectRoot?: string) { + private async restartControlPlane(issuedAt: string, projectRoot?: string): Promise<CoreRestartResult> {(Import
CoreRestartResultfrom./core-command-contract.jsalongside the existing imports.)🤖 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 `@src/daemon.ts` around lines 426 - 439, Add an explicit return type to restartControlPlane so it is locked to CoreRestartResult. This method is the single place that builds the { restart, text } object used by both routes, so update restartControlPlane in src/daemon.ts to import CoreRestartResult from core-command-contract and annotate the method return type to match that contract, keeping the existing restartAimuxControlPlane and renderRuntimeRestartResult flow unchanged.src/control-plane-restart-client.test.ts (1)
51-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood coverage of daemon success and fallback paths.
Both the direct daemon-forwarding tests and the local-bootstrap fallback test correctly assert the returned
{ restart, text, source }shape and the underlying mock call arguments.One minor gap: there's no test exercising the fallback path with no
projectRoot(i.e.,restartControlPlaneFromCli()triggeringneedsLocalBootstrapand callingrestartAimuxControlPlane({ projectRoot: undefined })). Not blocking, but would round out coverage of theundefinedbranch already exercised for the daemon-success case at Line 64.🤖 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 `@src/control-plane-restart-client.test.ts` around lines 51 - 95, The restartControlPlaneFromCli test suite is missing coverage for the no-project-root fallback branch. Add a test that calls restartControlPlaneFromCli() with no argument, forces the daemon-core request to reject with the local-build mismatch so needsLocalBootstrap is exercised, and asserts restartAimuxControlPlane is called with { projectRoot: undefined } and the returned result still includes the local-bootstrap source. Use the existing restartControlPlaneFromCli, needsLocalBootstrap, and restartAimuxControlPlane symbols to keep the new test aligned with the current flow.
🤖 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/daemon.ts`:
- Around line 600-609: The `/restartText` handler in `src/daemon.ts` should
preserve its text/plain contract even when `restartControlPlane` fails. Update
the POST branch that uses `restartControlPlane(issuedAt)` so that errors like
the lock-contention case are caught locally and returned as a text/plain
response, rather than bubbling to `start()`’s generic JSON `catch`; keep the
response shape consistent with the existing `restartText` route and use the
route’s existing symbols (`restartControlPlane`, `CORE_API_ROUTES.restartText`,
and the `start()` error path) to locate the fix.
---
Nitpick comments:
In `@scripts/installed-aimux-shim.sh`:
- Around line 70-89: The aimux_try_restart helper leaves the mktemp-created
body_file behind if the shell is interrupted during the long curl request.
Harden this by adding cleanup around aimux_try_restart so body_file is always
removed on exit or interruption, using a trap scoped to the function and
ensuring it is cleared after normal completion; keep the change localized to
aimux_try_restart and its existing mktemp/curl/cat flow.
- Around line 64-68: aimux_try_daemon_ensure() currently triggers two /health
fetches because aimux_matching_daemon_port() already calls aimux_health_json
internally and then aimux_try_daemon_ensure() calls aimux_health_json again for
aimux_print_daemon_ensure. Update the flow so the first lookup reuses or returns
the fetched JSON (or returns both port and needed metadata) and have
aimux_try_daemon_ensure() pass that through, removing the redundant localhost
round-trip while keeping aimux_print_daemon_ensure fed with the same data.
In `@src/control-plane-restart-client.test.ts`:
- Around line 51-95: The restartControlPlaneFromCli test suite is missing
coverage for the no-project-root fallback branch. Add a test that calls
restartControlPlaneFromCli() with no argument, forces the daemon-core request to
reject with the local-build mismatch so needsLocalBootstrap is exercised, and
asserts restartAimuxControlPlane is called with { projectRoot: undefined } and
the returned result still includes the local-bootstrap source. Use the existing
restartControlPlaneFromCli, needsLocalBootstrap, and restartAimuxControlPlane
symbols to keep the new test aligned with the current flow.
In `@src/daemon.ts`:
- Around line 426-439: Add an explicit return type to restartControlPlane so it
is locked to CoreRestartResult. This method is the single place that builds the
{ restart, text } object used by both routes, so update restartControlPlane in
src/daemon.ts to import CoreRestartResult from core-command-contract and
annotate the method return type to match that contract, keeping the existing
restartAimuxControlPlane and renderRuntimeRestartResult flow unchanged.
🪄 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: ace2d275-3213-4b43-836e-038d56dd4d44
📒 Files selected for processing (9)
scripts/installed-aimux-shim.shsrc/control-plane-restart-client.test.tssrc/control-plane-restart-client.tssrc/core-command-contract.tssrc/daemon.test.tssrc/daemon.tssrc/installed-shim.test.tssrc/main.tssrc/runtime-restart.ts
|
Sub-agent review follow-up: fixed the stale-daemon bootstrap fallback by preserving daemon info/state before command-bus probing, and restored verified legacy project-service cleanup for daemon-owned restarts. Covered by |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Final sub-agent review follow-up: fixed retained-daemon restart reporting in 9c1ab91. Daemon-owned restarts now set |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
aimux restartwhen the matching daemon is already aliveVerification
yarn vitest run src/installed-shim.test.ts src/daemon.test.ts src/core-command-contract.test.ts src/control-plane-restart-client.test.tsyarn vitest run src/one-shot-node-inventory.test.ts src/core-sidecar-boundary.test.tsyarn typecheckyarn lintyarn vitest runyarn buildAIMUX_RELEASE_VERSION=local-core-sidecar-next-23 yarn release:assetSummary by CodeRabbit
New Features
Bug Fixes
Tests