fix(service): preserve supervision across restart - #1160
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesService lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/service.ts`:
- Around line 2654-2657: Update the manager shutdown logic around ops.status(),
isServiceInstalled(), and stopTrackedProxyForServiceCommand() to catch only a
confirmed not-installed condition; report other status or stop failures and
return before invoking ops.install() or reporting installation success. Add a
regression test proving installation is skipped when manager shutdown fails.
- Line 2655: Update the cleanup around the ops.stop() call to use
diagnoseService().backend, rather than only isServiceInstalled(), when selecting
the service manager to stop; ensure the previously recorded backend is stopped
before switching to the newly requested parsed.backend, including stopping both
Windows backends when they differ.
In `@tests/grok-lifecycle.test.ts`:
- Around line 83-99: Strengthen the restart assertions in the test named
“handleStop returns its outcome and restart preserves its supervision mode” to
verify branch association, not just source order: assert that
serviceCommand("start") is inside the serviceWasInstalled=true path and
handleEnsure() is the false/standalone path. Match the conditional block
explicitly while preserving the existing ordering checks.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f0364115-3ac4-4c8c-83f0-9f9747f1a9f6
📒 Files selected for processing (5)
docs-site/src/content/docs/reference/cli/lifecycle.mdsrc/cli/index.tssrc/service.tstests/grok-lifecycle.test.tstests/service.test.ts
| try { | ||
| if (ops.status() !== null || isServiceInstalled()) ops.stop(); | ||
| } catch { /* absent or already stopped */ } | ||
| await stopTrackedProxyForServiceCommand(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail closed when manager shutdown fails.
The empty catch swallows permission errors, ownership errors, command failures, and real stop failures. The code then calls ops.install() while the old manager may still be running. This can leave a respawning proxy on the configured port and make reportServiceServing("installed") report a false success. Catch only a confirmed not-installed condition; otherwise report the error and return before installing assets. Add a regression test that verifies installation does not run after a stop failure.
🤖 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/service.ts` around lines 2654 - 2657, Update the manager shutdown logic
around ops.status(), isServiceInstalled(), and
stopTrackedProxyForServiceCommand() to catch only a confirmed not-installed
condition; report other status or stop failures and return before invoking
ops.install() or reporting installation success. Add a regression test proving
installation is skipped when manager shutdown fails.
| // restart-loops on EADDRINUSE, and the old standalone process makes the install | ||
| // verification report a false success. | ||
| try { | ||
| if (ops.status() !== null || isServiceInstalled()) ops.stop(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Stop the recorded service backend before switching backends.
ops is built from parsed.backend, which is the backend requested for the new install. isServiceInstalled() calls diagnoseService().installed but discards diagnoseService().backend. If a Windows service was installed with the other backend, this condition can be true while ops.stop() targets only the new backend. The old manager can remain registered and respawn a proxy while the new backend is installed. Use the recorded backend from diagnoseService().backend for cleanup, or stop both Windows backends when the requested backend differs.
🤖 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/service.ts` at line 2655, Update the cleanup around the ops.stop() call
to use diagnoseService().backend, rather than only isServiceInstalled(), when
selecting the service manager to stop; ensure the previously recorded backend is
stopped before switching to the newly requested parsed.backend, including
stopping both Windows backends when they differ.
| test("handleStop returns its outcome and restart preserves its supervision mode", () => { | ||
| const stopFn = sliceFn(CLI_SOURCE, "async function handleStop(", "async function handleUninstall("); | ||
| // process.exit() inside handleStop would strand runTrayProxyRestart's start() half. | ||
| expect(stopFn).toContain("process.exitCode = 1"); | ||
| expect(stopFn).toContain("return !stopFailed"); | ||
| expect(stopFn).not.toContain("process.exit(1)"); | ||
|
|
||
| const restartCase = sliceFn(CLI_SOURCE, 'case "restart"', 'case "health"'); | ||
| expect(restartCase).toContain("if (await handleStop()) await handleEnsure()"); | ||
| const diagnoseAt = restartCase.indexOf("diagnoseService().installed"); | ||
| const stopAt = restartCase.indexOf("await handleStop()"); | ||
| const serviceStartAt = restartCase.indexOf('await serviceCommand("start")'); | ||
| const standaloneStartAt = restartCase.indexOf("await handleEnsure()"); | ||
| expect(diagnoseAt).toBeGreaterThan(-1); | ||
| expect(diagnoseAt).toBeLessThan(stopAt); | ||
| expect(serviceStartAt).toBeGreaterThan(stopAt); | ||
| expect(standaloneStartAt).toBeGreaterThan(serviceStartAt); | ||
| expect(restartCase).toContain("if (serviceWasInstalled)"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert branch association, not only source order.
The assertions prove that both startup calls occur after handleStop(), but they do not prove that serviceCommand("start") runs only when serviceWasInstalled is true or that handleEnsure() is the false branch. An unconditional service start followed by handleEnsure() would still pass these checks and would break standalone restart. Match the conditional block explicitly or execute both supervision paths.
🤖 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/grok-lifecycle.test.ts` around lines 83 - 99, Strengthen the restart
assertions in the test named “handleStop returns its outcome and restart
preserves its supervision mode” to verify branch association, not just source
order: assert that serviceCommand("start") is inside the
serviceWasInstalled=true path and handleEnsure() is the false/standalone path.
Match the conditional block explicitly while preserving the existing ordering
checks.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea78087a3f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| break; | ||
| } | ||
| case "restart": { | ||
| const serviceWasInstalled = diagnoseService().installed; |
There was a problem hiding this comment.
When a stale, conflicting, or otherwise non-startable service registration exists, diagnoseService().installed is still true even though src/service.ts explicitly separates that from startable/viable; in that case ocx restart now stops the currently working proxy and then attempts ocx service start, which can fail or time out and leave no proxy running. This regresses the previous standalone restart behavior for broken service registrations, so the service path should be gated on a startable/viable diagnostic or fall back to handleEnsure() if service start does not actually recover.
Useful? React with 👍 / 👎.
| Stop and restart the proxy while preserving its supervision mode. If a background service was | ||
| installed, `restart` starts that service again so login and crash protection remain active. Without | ||
| an installed service, it starts the standalone background proxy through `ensure`. Both paths sync | ||
| the live port back into Codex. |
There was a problem hiding this comment.
This changes the canonical ocx restart behavior to preserve service supervision, but the Korean and Simplified Chinese lifecycle pages still describe it as stop followed by ensure, so the localized docs now contradict the English source for the same user workflow. Please update those directly affected pages along with this English change.
AGENTS.md reference: docs-site/AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| try { | ||
| if (ops.status() !== null || isServiceInstalled()) ops.stop(); | ||
| } catch { /* absent or already stopped */ } | ||
| await stopTrackedProxyForServiceCommand(); |
There was a problem hiding this comment.
If the tracked standalone proxy cannot be stopped here (for example the management stop times out or is refused), stopTrackedProxyForServiceCommand() only logs and returns "none", but install still proceeds and reportServiceServing() can accept the old standalone /healthz listener as proof that the newly registered service is serving. That leaves the same false-success/service-restart-loop scenario this install cleanup is meant to prevent, so failed cleanup should block the install or the verification must prove the listener is the new supervised child.
Useful? React with 👍 / 👎.
| // Preserve the installed supervision boundary. Starting via handleEnsure here | ||
| // creates a detached standalone proxy after handleStop unloads the service, | ||
| // so every routine restart silently loses login/crash protection. | ||
| if (serviceWasInstalled) await serviceCommand("start"); |
There was a problem hiding this comment.
For a healthy installed service, this path now returns after serviceCommand("start") only verifies /healthz; that endpoint is available immediately after the service child binds, before handleStart completes its startup syncCodexOnStartIfEnabled injection. Previously restart used handleEnsure(), whose parent process synced the live port before returning, so a user who runs codex immediately after ocx restart can still be on the restored native/stale config until the service child finishes its async sync. Please wait for readiness or explicitly sync after service start.
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
Thank you @jonathanli12 — I’ve carried this PR’s unique service-install cleanup and its regression coverage into #1206. The alternate stop/start Closing this source PR as superseded by #1206. Your contribution is explicitly credited in the consolidation PR. 🙏 |
2 participants
Summary
ocx restartValidation
No live service restart was used for validation.
Summary by CodeRabbit
New Features
ocx restartto preserve the current supervision mode.Bug Fixes
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.