fix(desktop): tree-kill all child processes on daemon restart#1504
Conversation
Previously, daemon restart only called subprocess.kill("SIGKILL") which
killed the immediate subprocess but left PTY shells and their children
(git, npm, etc.) as orphans. The SIGKILL fallback timer was also unreffed
and abandoned when the daemon called process.exit(0).
Now session.dispose() uses tree-kill on both the subprocess PID and PTY
PID, and the shutdown path awaits completion before exiting. Also extracts
shared state-reset logic and consolidates tree-kill utilities into a
single module.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughConverts terminal process teardown to async flows: adds a Promise-based Changes
sequenceDiagram
participant TH as TerminalHost
participant S as Session
participant TK as treeKillAsync
participant OS as Operating System
TH->>S: dispose() (await)
S->>S: collectProcessPids()
S->>TK: treeKillAsync(pid, signal)
TK->>OS: send kill to pid tree
OS-->>TK: callback/ack
TK-->>S: resolve Promise
S->>S: resetProcessState(), dispose emulator, clear clients
S-->>TH: dispose resolved
TH->>TH: await all sessions / timeout, continue shutdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (2)
apps/desktop/src/main/terminal-host/session.ts (2)
847-866: Good consolidation of reset logic.One observation:
ptyPidis not cleared here, socollectProcessPids()would still return it on a hypothetical second call. This is safe becausedispose()guards withthis.disposedandhandleSubprocessExitdoesn't callcollectProcessPids(), but ifresetProcessStateis intended to fully reset process-related state, consider also clearingptyPidfor consistency.
806-808: Minor style nit:dispose()returnsPromise<void>but isn'tasync.The method uses explicit
Promise.resolve()and.then()instead ofasync/await. Since sibling files (terminal-host.ts,index.ts) useasync dispose(), making thisasynctoo would improve consistency and simplify the early returns to plainreturn;.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…oating promises, trim comments
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| this.disposed = true; | ||
|
|
||
| const pidsToKill = this.collectProcessPids(); | ||
|
|
There was a problem hiding this comment.
Dispose frame never sent due to early disposed flag
Medium Severity
this.disposed is set to true before sendDisposeToSubprocess() is called. But sendFrameToSubprocess (which sendDisposeToSubprocess delegates to) has an early return if (this.disposed) return false, so the dispose frame is silently dropped and never reaches the subprocess. The graceful shutdown notification intended by the code never actually fires.
Additional Locations (1)
| await Promise.race([ | ||
| Promise.all(sessions.map((s) => s.dispose())), | ||
| new Promise<void>((resolve) => setTimeout(resolve, 5000)), | ||
| ]); |
There was a problem hiding this comment.
Leaked timeout in TerminalHost.dispose() Promise.race
Low Severity
The 5-second setTimeout in Promise.race is never cleared when Promise.all resolves first. Unlike promiseWithTimeout in the same file which properly clears its timer, this timeout leaks and keeps the event loop alive unnecessarily. In production process.exit() masks this, but it could affect tests or other callers.
1 participant
Summary
process.exit(0), so it never actually firedsession.dispose()tree-kills both the subprocess PID and PTY PID, and the shutdown path awaits completion before exitingChanges
dispose()now usestreeKillAsyncon both subprocess and PTY PIDs instead ofsubprocess.kill("SIGKILL")with an unreffed timer. ExtractedresetProcessState()to deduplicate state-reset logic shared withhandleSubprocessExit. ExtractedcollectProcessPids()for clarity.dispose()is now async — awaits all session tree-kills with a 5s hard timeout to prevent hanging.stopServer()awaitsterminalHost.dispose()before closing the server and exiting, ensuringps/pgrepchild enumeration completes beforeprocess.exit(0).treeKillAsync(promisifiedtree-killwrapper). Updated import inport-manager.ts.Test Plan
sleep 999), verify it gets killedSummary by CodeRabbit
Chores
Bug Fixes
Note
Medium Risk
Changes shutdown and process-termination behavior in the terminal daemon; mistakes could cause hangs (mitigated by a 5s timeout) or over/under-killing processes.
Overview
Fixes daemon restart/shutdown leaving orphaned terminal processes by making session/host disposal async and awaited during
stopServer().Session.dispose()now collects both subprocess and PTY PIDs and uses a new promisifiedtreeKillAsync(SIGKILL) to tree-kill descendants, replacing the previous unreffed timeout-based kill; teardown state-reset logic is centralized viaresetProcessState().TerminalHost.dispose()awaits all session disposals with a 5s cap, and callers updated to fire-and-forgetdispose()where waiting isn’t needed. Also updates the port manager to importtreeKillWithEscalationfrom the consolidatedtree-killmodule.Written by Cursor Bugbot for commit 25ebf1f. This will update automatically on new commits. Configure here.