fix(dev): clean teardown of all bun dev child processes#3982
Conversation
Three sources of zombie procs after Ctrl-C / kill: - next/wrangler orphaned by `sh -c '<cmd>'` not exec-ing into the real binary; sh exits on subtask SIGTERM and leaves them bound to ports. - host-service spawned `detached: true` + `unref()` even in dev, so it reparents to init when Electron dies. - terminal-host daemon does the same. Fix: - prepend `exec ` in api/web/electric-proxy `sh -c` dev scripts. - gate `detached`/`unref` on `!app.isPackaged` for both host-service and terminal-host. Packaged production builds keep detach + manifest adoption (PTYs survive Electron restart) — only dev changes. Verified: `bun run dev` → SIGINT → 1s later all ports released and all spawned processes (turbo, electron-vite, Electron, host-service ×2, terminal-host, next/wrangler/caddy) are gone.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDev npm scripts now run under ChangesDev Process Cleanup
Sequence DiagramsequenceDiagram
participant Shell as Dev Shell
participant DevRunner as Bun/Turbo Dev
participant Electron as Electron (main)
participant Host as Host-service / PTY daemon
Shell->>DevRunner: start dev scripts (`sh -c 'exec ...'`)
DevRunner->>Electron: start desktop dev
Electron->>Host: spawn child (detached: !isDev)
alt Development
Host-->>Electron: child attached (dies with Electron)
Electron->>Electron: runDevQuitCleanup() on quit/signals
Electron->>Host: stop child (kill sessions) and dispose client
else Production
Host-->>Electron: child detached + unref (survives)
Electron->>Host: releaseAll() on quit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 2/8 reviews remaining, refill in 42 minutes and 32 seconds.Comment |
Greptile SummaryThis PR fixes three categories of orphaned child processes left behind after killing The audit notes that live end-to-end verification of the host-service and terminal-host code paths (requiring a logged-in org and an active terminal session) was not performed in the headless test run — the test plan reviewer checklist covers this gap. Confidence Score: 4/5Safe to merge — all changes are dev-only by construction and production paths are structurally unchanged. Only P2 findings (stale comment). The core logic is sound: apps/desktop/src/main/lib/host-service-coordinator.ts and apps/desktop/src/main/lib/terminal-host/client.ts — both need live verification with a logged-in org and active terminal session per the test plan.
|
| Filename | Overview |
|---|---|
| apps/api/package.json | Prepends exec to the sh -c dev script so SIGTERM lands on next directly instead of the shell wrapper; prod script unchanged. |
| apps/web/package.json | Same exec fix as api/package.json for the Next.js web dev server. |
| apps/electric-proxy/package.json | Prepends exec to the wrangler dev script; deploy script unchanged. |
| apps/desktop/src/main/lib/host-service-coordinator.ts | Gates detached+unref on !app.isPackaged; dev spawns are now attached so Electron exit propagates SIGTERM. Minor stale comment on windowsHide. |
| apps/desktop/src/main/lib/terminal-host/client.ts | Same detached/unref gating as host-service-coordinator, correctly using !app.isPackaged; production PTY-survival path untouched. |
| plans/20260502-bun-dev-server-cleanup.md | Thorough audit doc; explicitly notes host-service and terminal-host were not live-exercised in the headless verification run (no logged-in org, no UI terminal). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["bun run dev (SIGINT)"] --> B[turbo sends SIGTERM to subtasks]
B --> C["api/web/electric-proxy\ndotenv -- sh -c '...'"]
C -->|"OLD: sh exits, next/wrangler orphaned"| D1["❌ next/wrangler still bound to port"]
C -->|"NEW: exec replaces sh\nsignal lands on next/wrangler"| D2["✅ next/wrangler exits cleanly"]
B --> E[electron-vite → Electron main]
E -->|"OLD: detached:true + unref()"| F1["❌ host-service orphaned (PPID=1)"]
E -->|"NEW: detached:false in dev\n(!app.isPackaged)"| F2["✅ host-service dies with Electron"]
F2 --> G2["✅ pty-daemon cascades via serve.ts handler"]
E -->|"OLD: detached:true + unref()"| H1["❌ terminal-host orphaned (PPID=1)"]
E -->|"NEW: detached:false in dev\n(!app.isPackaged)"| H2["✅ terminal-host dies with Electron"]
subgraph PROD ["Production (app.isPackaged = true)"]
P1["detached:true + unref() — unchanged\nManifest-based adoption preserved"]
end
Comments Outside Diff (1)
-
apps/desktop/src/main/lib/host-service-coordinator.ts, line 436-437 (link)Stale comment after detach flag change
The inline comment
"Avoid a flashing CMD window on Windows for the detached child"still says "detached child", but in dev mode the child is now spawned withdetached: false. The option itself is harmless regardless of detach state, but the comment is now misleading.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/main/lib/host-service-coordinator.ts Line: 436-437 Comment: **Stale comment after detach flag change** The inline comment `"Avoid a flashing CMD window on Windows for the detached child"` still says "detached child", but in dev mode the child is now spawned with `detached: false`. The option itself is harmless regardless of detach state, but the comment is now misleading. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/main/lib/host-service-coordinator.ts:436-437
**Stale comment after detach flag change**
The inline comment `"Avoid a flashing CMD window on Windows for the detached child"` still says "detached child", but in dev mode the child is now spawned with `detached: false`. The option itself is harmless regardless of detach state, but the comment is now misleading.
```suggestion
// Avoid a flashing CMD window on Windows.
windowsHide: true,
```
Reviews (1): Last reviewed commit: "fix(dev): clean teardown of all bun dev ..." | Re-trigger Greptile
Per Greptile review on #3982 — `detached` is now gated on `!app.isPackaged`, so referring to "the detached child" is misleading. `windowsHide` applies either way.
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/host-service-coordinator.ts (1)
428-477:⚠️ Potential issue | 🟠 MajorApp-quit path needs stopAll() and terminal-host disposal for dev children.
The before-quit handler calls
releaseAll()unconditionally, butreleaseAll()only stops liveness checks and clears the instance map—it does not kill processes. In dev, host-service children are spawned non-detached (detached: !isDev) and remain ref'd (child.unref()only in production), so they will hang on app exit waiting for an explicit SIGTERM. Similarly, the terminal-host client spawns with the same lifecycle pattern and lacks disposal in the before-quit path. The app-quit flow should callstopAll()instead ofreleaseAll()for dev, and add explicit terminal-host cleanup, to ensure attached dev children are terminated during normal quit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 428 - 477, The app quit handler currently calls releaseAll(), which only stops liveness checks and leaves non-detached dev child processes alive; change the before-quit flow in HostServiceCoordinator to call stopAll() instead of releaseAll() when isDev so attached dev children are terminated, and also ensure any terminal-host client is explicitly cleaned up (e.g., call terminalHost.dispose() or terminalHost.stop() / terminalHostClient.close() depending on the terminal-host API) in that same quit path; reference the existing methods releaseAll(), stopAll(), and the terminal-host instance/variable used by the coordinator to locate where to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 428-477: The app quit handler currently calls releaseAll(), which
only stops liveness checks and leaves non-detached dev child processes alive;
change the before-quit flow in HostServiceCoordinator to call stopAll() instead
of releaseAll() when isDev so attached dev children are terminated, and also
ensure any terminal-host client is explicitly cleaned up (e.g., call
terminalHost.dispose() or terminalHost.stop() / terminalHostClient.close()
depending on the terminal-host API) in that same quit path; reference the
existing methods releaseAll(), stopAll(), and the terminal-host
instance/variable used by the coordinator to locate where to make the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0827a418-8ae1-4f05-82d5-f4f834db4f06
📒 Files selected for processing (6)
apps/api/package.jsonapps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/main/lib/terminal-host/client.tsapps/electric-proxy/package.jsonapps/web/package.jsonplans/20260502-bun-dev-server-cleanup.md
Audit found four more `sh -c '<cmd>'` dev scripts missing `exec` (admin,
docs, marketing, packages/cli — all reachable via `bun dev:all` /
`bun dev:docs` / `bun dev:marketing`), and one defense-in-depth gap in
the pty-daemon supervisor.
- apps/{admin,docs,marketing}/package.json — add `exec ` inside `sh -c`.
- packages/cli/package.json — same; uses `exec env VAR=… cli-framework
dev "$@"` to preserve the inline env var assignment.
- packages/host-service/src/daemon/DaemonSupervisor.ts — gate
`detached`/`unref` on `isDev` (NODE_ENV-derived). The serve.ts dev
shutdown handler already calls `supervisor.stop()` on the happy path;
this protects against host-service crashes that bypass that handler.
Per CodeRabbit on #3982: with `detached: !isDev` + `if (!isDev) child.unref()`, dev children are now attached & ref'd, so the existing `releaseAll()` / `app.exit(0)` paths in main/index.ts left them as init-orphans on graceful quit (Cmd+Q) and on the dev SIGTERM/SIGINT handler. - before-quit: in dev, call `coordinator.stopAll()` and dispose terminal-host via `shutdownIfRunning({ killSessions: true })` + `disposeTerminalHostClient`. Production keeps `releaseAll()` (manifest adoption is the survival contract). - handleTerminationSignal + parent-pid fallback: run the same dev cleanup before `app.exit(0)`, with a re-entry guard. Verified: SIGTERM to Electron-only (simulating Cmd+Q) takes down host-service ×2 and terminal-host while leaving the rest of `bun dev` intact. Full `bun dev` Ctrl-C still produces zero residual processes.
Remove call-site comments that restated what the function name + docstring already say; trim the spawn-site comments to a two-line prod/dev summary each. The unref() lines are obvious from the surrounding `!isDev` branch.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/index.ts (1)
190-232:⚠️ Potential issue | 🟠 MajorPrevent quit before async cleanup completes.
The
before-quithandler performs awaited cleanup, butevent.preventDefault()is only called conditionally inside the dialog block. In Electron, async event handlers are not awaited by default—the app will proceed with the quit process immediately after the handler returns, causing cleanup operations to race with process exit. Moveevent.preventDefault()to the top of the handler (after theisQuittingguard) to ensure the quit flow is blocked until all cleanup completes.Proposed fix
app.on("before-quit", async (event) => { if (isQuitting) return; + event.preventDefault(); const isDev = process.env.NODE_ENV === "development"; if (!skipQuitConfirmation && !isDev && getConfirmOnQuitSetting()) { - event.preventDefault(); - try { const { response } = await dialog.showMessageBox({ type: "question", buttons: ["Quit", "Cancel"], defaultId: 0, cancelId: 1, title: "Quit Superset", message: "Are you sure you want to quit?", }); if (response === 1) { return; } } catch (error) { console.error("[main] Quit confirmation dialog failed:", error); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/index.ts` around lines 190 - 232, The before-quit handler may return before async cleanup finishes because event.preventDefault() is only called inside the dialog branch; move event.preventDefault() to immediately after the isQuitting guard in the app.on("before-quit") handler so the quit is always blocked until cleanup completes, then conditionally show the dialog (using skipQuitConfirmation and getConfirmOnQuitSetting()) and perform awaited cleanup via runDevQuitCleanup() or getHostServiceCoordinator().releaseAll(), followed by shutdownTanstackDbPersistence() and disposeTray(), and finally call app.exit(0) once those awaits complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/main/index.ts`:
- Around line 190-232: The before-quit handler may return before async cleanup
finishes because event.preventDefault() is only called inside the dialog branch;
move event.preventDefault() to immediately after the isQuitting guard in the
app.on("before-quit") handler so the quit is always blocked until cleanup
completes, then conditionally show the dialog (using skipQuitConfirmation and
getConfirmOnQuitSetting()) and perform awaited cleanup via runDevQuitCleanup()
or getHostServiceCoordinator().releaseAll(), followed by
shutdownTanstackDbPersistence() and disposeTray(), and finally call app.exit(0)
once those awaits complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddebed56-9896-4468-b2ae-156b8593f54c
📒 Files selected for processing (1)
apps/desktop/src/main/index.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/host-service-coordinator.ts (1)
406-409: ⚡ Quick winUpdate stale lifecycle comment to match new dev behavior.
Line 406 says host-service “must survive Electron restarts in every environment,” but Line 431 now intentionally keeps it attached in dev. Please align the comment with the new dev/prod split to avoid future confusion.
Suggested comment-only diff
- // Host-service owns v2 PTYs, so it must survive Electron restarts in - // every environment. This mirrors the terminal-host daemon: detach the - // child and back stdio with real files so parent teardown cannot close - // pipes and take the service down with the app. + // Host-service owns v2 PTYs. In production we detach so it survives + // Electron restarts; in dev we keep it attached so `bun dev` shutdown + // can terminate it cleanly.Also applies to: 427-431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 406 - 409, Update the stale lifecycle comment that currently states "host-service owns v2 PTYs, so it must survive Electron restarts in every environment" to reflect the new dev/prod split: note that in production the host-service is detached and stdio is backed by files so it survives Electron restarts, whereas in development it is intentionally kept attached to aid debugging. Edit the comment near the "host-service owns v2 PTYs" block and the subsequent lines that discuss detaching/backing stdio (the same comment region referenced around the host-service lifecycle handling) to clearly document the differing behavior in dev vs prod.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 406-409: Update the stale lifecycle comment that currently states
"host-service owns v2 PTYs, so it must survive Electron restarts in every
environment" to reflect the new dev/prod split: note that in production the
host-service is detached and stdio is backed by files so it survives Electron
restarts, whereas in development it is intentionally kept attached to aid
debugging. Edit the comment near the "host-service owns v2 PTYs" block and the
subsequent lines that discuss detaching/backing stdio (the same comment region
referenced around the host-service lifecycle handling) to clearly document the
differing behavior in dev vs prod.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fba029f-5fd6-4792-9a25-8cc5e97aa9c0
📒 Files selected for processing (5)
apps/desktop/src/main/index.tsapps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/main/lib/terminal-host/client.tspackages/host-service/src/daemon/DaemonSupervisor.tsplans/20260502-bun-dev-server-cleanup.md
✅ Files skipped from review due to trivial changes (1)
- packages/host-service/src/daemon/DaemonSupervisor.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/main/lib/terminal-host/client.ts
- apps/desktop/src/main/index.ts
1 participant
Summary
After Ctrl-C'ing
bun run dev, three categories of child processes were leaking and surviving as orphans (PPID=1):next dev(api, web) andwrangler dev(electric-proxy) —sh -c '<cmd>'doesn'texecthe real binary, so on subtask SIGTERM the shell exits without forwarding the signal and the dev server is left bound to its port.host-service(apps/desktop/src/main/lib/host-service-coordinator.ts) — spawneddetached: true+child.unref()unconditionally; onbun devkill nostopAll()runs, so it reparents to init.terminal-hostdaemon (apps/desktop/src/main/lib/terminal-host/client.ts) — same pattern, same outcome. Used by both v1 and v2 codepaths (settings, tasksOpenInWorkspace/RunInWorkspacePopover, agent hooks).Changes
apps/{api,web,electric-proxy}/package.json— prependexecinside thesh -cso signals land onnext/wranglerdirectly.apps/desktop/src/main/lib/host-service-coordinator.ts—detached: !isDev+if (!isDev) child.unref(), gated on!app.isPackaged.apps/desktop/src/main/lib/terminal-host/client.ts— same shape.plans/20260502-bun-dev-server-cleanup.md— audit + verification notes.Prod safety
All changes are dev-only by construction:
execonly edits"dev"scripts; prod usesnext start/wrangler deploy.detachedflips for both daemons are gated on!app.isPackaged. Packaged production builds keepdetached: true+unref()exactly as before, preserving manifest-based adoption and PTY survival across Electron restarts (apps/desktop/HOST_SERVICE_LIFECYCLE.md).Verification
Result within 1s of SIGINT — completely clean:
For comparison, sibling worktrees on older code in the same
pssnapshot still hadhost-service.js,terminal-host.js,pty-daemon.jsreparented to init — the exact leak this PR fixes.apps/desktoptypecheck clean.Test plan
bun run dev, sign in to an org, open a v2 workspace + spawn a terminal, then Ctrl-C and confirmpgrep -lf 'host-service|terminal-host|pty-daemon|next dev|wrangler|caddy'is empty for your worktree.Summary by cubic
Fixes leaking dev processes so stopping
bun run devor quitting Electron in dev cleanly shuts down all child servers and daemons, freeing ports with no orphans. Production behavior is unchanged.Bug Fixes
apps/{api,web,electric-proxy,admin,docs,marketing}andpackages/cli: addedexecinsidesh -cfornext dev/wrangler dev/cli-framework devso signals reach the real process.apps/desktop: spawnhost-serviceandterminal-hostwithdetached: !isDevand onlyunref()in production so they exit with Electron in dev.packages/host-service:DaemonSupervisornow spawns the PTY daemon withdetached: !isDevand onlyunref()in production, providing defense-in-depth for crash cases.apps/desktop/src/main/index.ts: dev quit/SIGTERM/parent-exit runs cleanup to stophost-service, shut downterminal-host(kills sessions), dispose the client, then exit; adds a re-entry guard to avoid double handling.Refactors
plans/20260502-bun-dev-server-cleanup.md: tightened to reflect shipped state.Written for commit 6d2de88. Summary will update on new commits.
Summary by CodeRabbit