feat(dashboard): drain-and-restart from memory observability card (#563) - #580
Conversation
Expose a longer 60s informed recycle on the memory card (POST /api/system/restart) that reuses drainAndShutdown and respawns via ensure/service without Codex teardown (lidge-jun#563).
|
@codex review |
|
@coderabbitai review |
|
Warning Review limit reached
Next review available in: 10 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe memory observability card now supports a confirmed 60-second drain-and-restart workflow. Server routes expose active-turn state and restart acceptance, while the UI handles draining, reconnecting, errors, translations, tests, and documentation. ChangesMemory drain-and-restart flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MemoryCard
participant SystemRoutes
participant SystemRestart
participant ProcessSupervisor
User->>MemoryCard: Confirm Drain & restart
MemoryCard->>SystemRoutes: POST /api/system/restart
SystemRoutes->>SystemRestart: acceptSystemRestart()
SystemRestart->>ProcessSupervisor: drain, ensure, and exit
ProcessSupervisor-->>MemoryCard: /healthz becomes available
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85ff4611d1
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!serviceInstalled) { | ||
| (io.spawnEnsure ?? spawnDetachedEnsure)(); | ||
| } | ||
| (io.exitProcess ?? ((code: number) => { process.exit(code); }))(0); |
There was a problem hiding this comment.
For the Linux systemd service and both Windows backends inspected, an installed service only restarts failed processes: src/service.ts configures Restart=on-failure, the Task Scheduler wrapper loops only for nonzero %ERRORLEVEL%, and WinSW uses <onfailure>. This branch skips ocx ensure whenever the service is merely installed and then exits with code 0, so Drain & restart leaves the proxy permanently stopped on those standard service installations. Invoke the supervisor's restart operation, use an intentional failure status for these backends, and add coverage against the actual supervisor policies rather than a mock that assumes every installed service respawns.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1e20955: supervised service children now exit(1); non-supervised path spawns ocx start.
| restartAccepted = true; | ||
| schedule(async () => { | ||
| const drain = io.drainAndShutdown ?? drainAndShutdown; | ||
| await drain(undefined, MEMORY_DRAIN_RESTART_MS); |
There was a problem hiding this comment.
When an admitted /v1/responses request is still awaiting upstream headers, it has not yet reached registerTurn()—for example, the passthrough SSE path registers only after upstreamResponse exists in src/server/responses/core.ts. If Drain & restart is clicked during that phase, activeTurns can be empty, so this call completes immediately and the process cuts the request instead of allowing the advertised 60-second drain. Track the turn from request admission through response completion and add a regression test with an upstream whose headers are delayed.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — early registration before upstream headers is a pre-existing gap shared with POST /api/stop drain. Out of scope for this recycle-path PR; happy to follow up as a dedicated drain-tracking issue if maintainers want it.
| const serviceInstalled = (io.isServiceInstalled ?? isServiceInstalled)(); | ||
| // Service supervisors respawn on exit; spawning ensure would race a second start. | ||
| if (!serviceInstalled) { | ||
| (io.spawnEnsure ?? spawnDetachedEnsure)(); |
There was a problem hiding this comment.
When no service is installed and codexAutoStart is false, this launches ocx ensure, but handleEnsure() explicitly returns without starting anything for that supported configuration. The old process then exits, so the confirm-gated action always shuts the proxy down rather than restarting it; the troubleshooting page's claim that this action restarts via ocx ensure is therefore incorrect for these users. An explicit restart should launch start directly or provide an ensure mode that bypasses the autostart preference.
AGENTS.md reference: AGENTS.md:L96-L97
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1e20955: non-supervised path now spawns detached ocx start --port (bypasses ensure's codexAutoStart gate).
| setData(json); | ||
| setUnavailable(false); | ||
| setSupportsRestart(typeof json.activeTurnCount === "number"); | ||
| if (json.isDraining && restartPhase === "idle") setRestartPhase("draining"); |
There was a problem hiding this comment.
When a zero-turn restart finishes between the 5-second memory polls, the immediate poll triggered by entering draining can still succeed against the old process and the next poll can already succeed against the new one. Because only a failed poll transitions to reconnecting and a successful non-draining payload never leaves draining, the card then remains disabled and displays “Draining” indefinitely even though the restart completed. Capture the original health pid and poll until it changes, or otherwise recognize a post-acceptance healthy replacement without requiring an observed connection failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1e20955: capture pre-restart pid and clear draining/reconnect once memory/healthz reports a different pid (or observed outage + healthy replacement).
| if (!serviceInstalled) { | ||
| (io.spawnEnsure ?? spawnDetachedEnsure)(); | ||
| } | ||
| (io.exitProcess ?? ((code: number) => { process.exit(code); }))(0); |
There was a problem hiding this comment.
For a proxy launched interactively without OCX_SERVICE, this process.exit(0) still runs the process.on("exit", syncCleanup) handler in src/cli/index.ts, which restores native Codex, strips the Grok fence, and reverts the injected environment. Since the detached ensure child is started immediately before that synchronous cleanup, the old cleanup races the replacement's injection and can leave a healthy restarted proxy whose clients no longer route to it, contrary to this module's stated no-teardown contract. Mark the current process as recycling before exit or complete cleanup before spawning the replacement, and cover preservation of the injected files in an integration test.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1e20955: markRecyclingForExit() skips restoreNativeCodex / stripGrokConfig / revertSystemEnv in syncCleanup so the recycle keeps injection intact.
| } | ||
|
|
||
| function spawnDetachedEnsure(): void { | ||
| const child = spawn(process.execPath, [process.argv[1], "ensure"], { |
There was a problem hiding this comment.
When the current proxy was started with an explicit --port or selected a fallback because config.port was occupied, the detached command receives no current-port argument and handleEnsure() instead hard-pins config.port ?? 10100. The restart can therefore fail while waiting on the still-occupied preferred port, or come back on a different port so the dashboard at the original apiBase never reconnects. Capture the live server/runtime port and launch the replacement on that same target, as the update restart flow already does.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1e20955: spawn pins the live listen port from the Bun server / runtime record.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs-site/src/content/docs/troubleshooting/windows-memory.md`:
- Around line 55-58: Update the documented proxy restart flow around the
60-second active-turn drain in the Windows memory troubleshooting guidance to
explicitly state that any remaining active turns are aborted when the timeout
expires before restarting. Keep the existing wait-limit and restart behavior
unchanged.
- Around line 53-55: Update the dashboard description near the “Memory
observability” card and “Drain & restart” action to call the displayed value the
current active-turn count, not the in-flight request count. Keep the wording
consistent with the management API’s activeTurnCount semantics; do not imply
broader HTTP request accounting unless a separate request-count field is
actually exposed.
In `@gui/src/components/MemoryObservabilityCard.tsx`:
- Around line 338-368: Update the lifecycle status spans rendered by
MemoryObservabilityCard around restartPhase to use polite live-region semantics,
such as aria-live="polite" or role="status", so draining, reconnecting, and
error transitions are announced to screen readers. Preserve the existing
conditional rendering, text, and styling.
- Around line 217-243: Update the reconnect polling effect in
MemoryObservabilityCard’s useEffect to mirror the sibling polling guard: track
an inFlight state, skip each scheduled tick while a previous request is active,
and always clear the guard after completion. Pass an AbortSignal.timeout(10_000)
to the /healthz fetch so hung requests are bounded, while preserving the
existing cancellation, retry, and give-up behavior.
In `@src/server/management/system-restart.ts`:
- Around line 61-72: Update the restart acceptance path around the scheduled
callback to invoke a lifecycle begin-drain operation synchronously before the
200ms delay. Modify drainAndShutdown to reuse the already-entered draining state
without resetting it, so request rejection and /api/system/memory reporting take
effect immediately. Add a regression test covering a data-plane request being
rejected after restart acceptance but before the scheduled callback executes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7b52943e-4c14-445c-a1c2-aa0c42536581
📒 Files selected for processing (13)
docs-site/src/content/docs/troubleshooting/windows-memory.mdgui/src/components/MemoryObservabilityCard.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/tests/memory-observability-card.test.tsxsrc/server/management/system-restart.tssrc/server/management/system-routes.tstests/memory-watchdog.test.tstests/system-restart.test.ts
Address Codex review: use failure-exit for supervised service children, spawn start with the live port (not ensure), suppress injection teardown on recycle, and clear the GUI draining state on pid change.
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Set draining before the 200ms flush delay, bound reconnect /healthz polls, announce restart status to assistive tech, and clarify docs around active-turn drain timeout abort.
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Issue author here — thanks for picking this up so quickly, and for implementing One minor edge: if the detached Happy to see this land — the card finally closes the observe→act gap from #427. |
|
Follow-up for the spawn-failure edge you flagged: #594 — wait for |
…-up) (#594) * fix(server): exit cleanly when drain-and-restart spawn fails Follow-up to #580: wait for detached start spawn success, and on sync throw or pre-start error exit(1) without markRecycling so we never leave a drained process latched or exit(0) with no replacement. * fix(server): sanitize restart spawn errors and restore fences Address Codex review on #594: log only errno codes (no pathful messages), and clear inherited OCX_SERVICE on spawn failure so ensure/tray daemons still run syncCleanup restore.
Summary
Closes #563
Test plan
Summary by CodeRabbit
New Features
Documentation