-
Notifications
You must be signed in to change notification settings - Fork 905
fix(desktop): tree-kill all child processes on daemon restart #1504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b4e3958
1a095fa
16433fa
25ebf1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import type { | |
| TerminalExitEvent, | ||
| TerminalSnapshot, | ||
| } from "../lib/terminal-host/types"; | ||
| import { treeKillAsync } from "../lib/tree-kill"; | ||
| import { | ||
| createFrameHeader, | ||
| PtySubprocessFrameDecoder, | ||
|
|
@@ -340,24 +341,7 @@ export class Session { | |
| this.ptyReadyResolve = null; | ||
| } | ||
|
|
||
| this.subprocess = null; | ||
| this.subprocessReady = false; | ||
| this.subprocessDecoder = null; | ||
| this.subprocessStdinQueue = []; | ||
| this.subprocessStdinQueuedBytes = 0; | ||
| this.subprocessStdinDrainArmed = false; | ||
| this.subprocessStdoutPaused = false; | ||
|
|
||
| this.emulatorWriteQueue = []; | ||
| this.emulatorWriteQueuedBytes = 0; | ||
| this.emulatorWriteScheduled = false; | ||
| this.snapshotBoundaryIndex = null; | ||
| const waiters = this.emulatorFlushWaiters; | ||
| this.emulatorFlushWaiters = []; | ||
| for (const resolve of waiters) resolve(); | ||
| const boundaryWaiters = this.snapshotBoundaryWaiters; | ||
| this.snapshotBoundaryWaiters = []; | ||
| for (const resolve of boundaryWaiters) resolve(); | ||
| this.resetProcessState(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -814,33 +798,46 @@ export class Session { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Dispose of the session | ||
| */ | ||
| dispose(): void { | ||
| if (this.disposed) return; | ||
| /** Callers that don't need to wait can fire-and-forget. */ | ||
| dispose(): Promise<void> { | ||
| if (this.disposed) return Promise.resolve(); | ||
| this.disposed = true; | ||
|
|
||
| const pidsToKill = this.collectProcessPids(); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dispose frame never sent due to early
|
||
| if (this.subprocess) { | ||
| // Capture reference before nullifying - the timeout needs it | ||
| const subprocess = this.subprocess; | ||
| this.sendDisposeToSubprocess(); | ||
| // Force kill after timeout if dispose frame didn't terminate it | ||
| const killTimer = setTimeout(() => { | ||
| try { | ||
| subprocess.kill("SIGKILL"); | ||
| } catch { | ||
| // Process may already be dead | ||
| } | ||
| }, 1000); | ||
| killTimer.unref(); // Don't keep daemon alive for this timer | ||
| this.subprocess = null; | ||
| } | ||
|
|
||
| this.resetProcessState(); | ||
| this.emulator.dispose(); | ||
| this.attachedClients.clear(); | ||
| this.clientSocketsWaitingForDrain.clear(); | ||
|
|
||
| if (pidsToKill.length === 0) return Promise.resolve(); | ||
|
|
||
| // Must await: treeKill enumerates descendants via ps/pgrep before signaling | ||
| return Promise.all( | ||
| pidsToKill.map((pid) => treeKillAsync(pid, "SIGKILL")), | ||
| ).then(() => {}); | ||
| } | ||
|
|
||
| /** Includes PTY PID as safety net in case the shell was reparented after subprocess exit. */ | ||
| private collectProcessPids(): number[] { | ||
| const pids: number[] = []; | ||
| if (this.subprocess?.pid) pids.push(this.subprocess.pid); | ||
| if (this.ptyPid) pids.push(this.ptyPid); | ||
| return pids; | ||
| } | ||
|
|
||
| private resetProcessState(): void { | ||
| this.subprocess = null; | ||
| this.subprocessReady = false; | ||
| this.subprocessDecoder = null; | ||
| this.subprocessStdinQueue = []; | ||
| this.subprocessStdinQueuedBytes = 0; | ||
| this.subprocessStdinDrainArmed = false; | ||
| this.subprocessStdoutPaused = false; | ||
|
|
||
| this.emulatorWriteQueue = []; | ||
| this.emulatorWriteQueuedBytes = 0; | ||
|
|
@@ -852,11 +849,6 @@ export class Session { | |
| const boundaryWaiters = this.snapshotBoundaryWaiters; | ||
| this.snapshotBoundaryWaiters = []; | ||
| for (const resolve of boundaryWaiters) resolve(); | ||
|
|
||
| this.emulator.dispose(); | ||
| this.attachedClients.clear(); | ||
| this.clientSocketsWaitingForDrain.clear(); | ||
| this.subprocessStdoutPaused = false; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,14 +90,14 @@ export class TerminalHost { | |
|
|
||
| // Force-dispose terminating sessions to prevent race conditions | ||
| if (session?.isTerminating) { | ||
| session.dispose(); | ||
| void session.dispose(); | ||
| this.sessions.delete(sessionId); | ||
| this.clearKillTimer(sessionId); | ||
| session = undefined; | ||
| } | ||
|
|
||
| if (session && !session.isAlive) { | ||
| session.dispose(); | ||
| void session.dispose(); | ||
| this.sessions.delete(sessionId); | ||
| session = undefined; | ||
| } | ||
|
|
@@ -137,7 +137,7 @@ export class TerminalHost { | |
| } | ||
|
|
||
| if (!session.isAlive) { | ||
| session.dispose(); | ||
| void session.dispose(); | ||
| throw new Error("Session spawn failed: PTY process exited immediately"); | ||
| } | ||
|
|
||
|
|
@@ -207,7 +207,7 @@ export class TerminalHost { | |
| if (session) { | ||
| session.detach(socket); | ||
| if (!session.isAlive && session.clientCount === 0) { | ||
| session.dispose(); | ||
| void session.dispose(); | ||
| this.sessions.delete(request.sessionId); | ||
| } | ||
| } | ||
|
|
@@ -253,7 +253,7 @@ export class TerminalHost { | |
| console.warn( | ||
| `[TerminalHost] Force disposing stuck session ${sessionId} after ${KILL_TIMEOUT_MS}ms`, | ||
| ); | ||
| s.dispose(); | ||
| void s.dispose(); | ||
| this.sessions.delete(sessionId); | ||
| } | ||
| this.killTimers.delete(sessionId); | ||
|
|
@@ -318,22 +318,27 @@ export class TerminalHost { | |
| session.detach(socket); | ||
| // Clean up dead sessions when last client detaches | ||
| if (!session.isAlive && session.clientCount === 0) { | ||
| session.dispose(); | ||
| void session.dispose(); | ||
| this.sessions.delete(sessionId); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| dispose(): void { | ||
| async dispose(): Promise<void> { | ||
| for (const timer of this.killTimers.values()) { | ||
| clearTimeout(timer); | ||
| } | ||
| this.killTimers.clear(); | ||
|
|
||
| for (const session of this.sessions.values()) { | ||
| session.dispose(); | ||
| } | ||
| const sessions = [...this.sessions.values()]; | ||
| this.sessions.clear(); | ||
|
|
||
| if (sessions.length === 0) return; | ||
|
|
||
| await Promise.race([ | ||
| Promise.all(sessions.map((s) => s.dispose())), | ||
| new Promise<void>((resolve) => setTimeout(resolve, 5000)), | ||
| ]); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaked timeout in
|
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -393,7 +398,7 @@ export class TerminalHost { | |
| } | ||
|
|
||
| if (session.clientCount === 0) { | ||
| session.dispose(); | ||
| void session.dispose(); | ||
| this.sessions.delete(sessionId); | ||
| } else { | ||
| this.scheduleSessionCleanup(sessionId); | ||
|
|
||


Uh oh!
There was an error while loading. Please reload this page.