fix(desktop): use tree-kill to ensure subprocesses are killed when closing ports

apps/desktop/src/lib/trpc/routers/ports/ports.ts

@@ -50,9 +50,11 @@ export const createPortsRouter = () => {
 					port: z.number().int().positive(),
 				}),
 			)
-			.mutation(({ input }): { success: boolean; error?: string } => {
-				return portManager.killPort(input);
-			}),
+			.mutation(
+				async ({ input }): Promise<{ success: boolean; error?: string }> => {
+					return portManager.killPort(input);
+				},
+			),
 
 		hasStaticConfig: publicProcedure
 			.input(z.object({ workspaceId: z.string() }))

apps/desktop/src/main/lib/terminal/manager.test.ts

@@ -11,12 +11,21 @@ mock.module("main/lib/analytics", () => ({
 	track: mock(() => {}),
 }));
 
+// Mock treeKillWithEscalation to avoid actual process killing in tests
+const mockTreeKillWithEscalation = mock(() =>
+	Promise.resolve({ success: true }),
+);
+mock.module("../tree-kill-with-escalation", () => ({
+	treeKillWithEscalation: mockTreeKillWithEscalation,
+}));
+
 // Import manager after mocks are set up
 const { TerminalManager } = await import("./manager");
 
 describe("TerminalManager", () => {
 	let manager: InstanceType<typeof TerminalManager>;
 	let mockPty: {
+		pid: number;
 		write: ReturnType<typeof mock>;
 		resize: ReturnType<typeof mock>;
 		kill: ReturnType<typeof mock>;
@@ -27,22 +36,27 @@ describe("TerminalManager", () => {
 	beforeEach(() => {
 		manager = new TerminalManager();
 
+		// Reset the treeKillWithEscalation mock and make it trigger onExit
+		mockTreeKillWithEscalation.mockReset();
+		mockTreeKillWithEscalation.mockImplementation(() => {
+			// Trigger onExit when tree-kill is called to simulate process death
+			const onExitCallback =
+				mockPty.onExit.mock.calls[mockPty.onExit.mock.calls.length - 1]?.[0];
+			if (onExitCallback) {
+				setImmediate(async () => {
+					await onExitCallback({ exitCode: 0, signal: undefined });
+				});
+			}
+			return Promise.resolve({ success: true });
+		});
+
 		// Setup mock pty
 		mockPty = {
+			pid: 12345, // Mock PID for treeKillWithEscalation
 			write: mock(() => {}),
 			resize: mock(() => {}),
-			// biome-ignore lint/suspicious/noExplicitAny: Mock requires this binding for proper context
-			kill: mock(function (this: any, _signal?: string) {
-				// Automatically trigger onExit when kill is called to avoid timeouts in cleanup
-				const onExitCallback =
-					mockPty.onExit.mock.calls[mockPty.onExit.mock.calls.length - 1]?.[0];
-				if (onExitCallback) {
-					// Use setImmediate to avoid blocking
-					setImmediate(async () => {
-						await onExitCallback({ exitCode: 0, signal: undefined });
-					});
-				}
-			}),
+			// kill is still used by signal() method
+			kill: mock(() => {}),
 			onData: mock((callback: (data: string) => void) => {
 				// Store callback for testing
 				mockPty.onData.mockImplementation(() => callback);
@@ -238,7 +252,7 @@ describe("TerminalManager", () => {
 	});
 
 	describe("kill", () => {
-		it("should kill the terminal session", async () => {
+		it("should kill the terminal session using tree-kill", async () => {
 			await manager.createOrAttach({
 				paneId: "pane-1",
 				tabId: "tab-1",
@@ -251,14 +265,12 @@ describe("TerminalManager", () => {
 
 			await manager.kill({ paneId: "pane-1" });
 
-			expect(mockPty.kill).toHaveBeenCalled();
-
-			const onExitCallback =
-				mockPty.onExit.mock.calls[mockPty.onExit.mock.calls.length - 1]?.[0];
-			if (onExitCallback) {
-				await onExitCallback({ exitCode: 0, signal: undefined });
-			}
+			// Should use treeKillWithEscalation to kill the process tree
+			expect(mockTreeKillWithEscalation).toHaveBeenCalledWith({
+				pid: mockPty.pid,
+			});
 
+			// The mock triggers onExit automatically, wait for it
 			await exitPromise;
 		});
 	});
@@ -304,7 +316,7 @@ describe("TerminalManager", () => {
 	});
 
 	describe("cleanup", () => {
-		it("should kill all sessions and wait for exit handlers", async () => {
+		it("should kill all sessions using tree-kill and wait for exit handlers", async () => {
 			await manager.createOrAttach({
 				paneId: "pane-1",
 				tabId: "tab-1",
@@ -317,21 +329,11 @@ describe("TerminalManager", () => {
 				workspaceId: "workspace-1",
 			});
 
-			const cleanupPromise = manager.cleanup();
-
-			const onExitCallback1 = mockPty.onExit.mock.calls[0]?.[0];
-			const onExitCallback2 = mockPty.onExit.mock.calls[1]?.[0];
-
-			if (onExitCallback1) {
-				await onExitCallback1({ exitCode: 0, signal: undefined });
-			}
-			if (onExitCallback2) {
-				await onExitCallback2({ exitCode: 0, signal: undefined });
-			}
-
-			await cleanupPromise;
+			// The mock triggers onExit automatically when treeKillWithEscalation is called
+			await manager.cleanup();
 
-			expect(mockPty.kill).toHaveBeenCalledTimes(2);
+			// Should use treeKillWithEscalation for each session
+			expect(mockTreeKillWithEscalation).toHaveBeenCalledTimes(2);
 		});
 	});
 

apps/desktop/src/main/lib/terminal/manager.ts

@@ -1,6 +1,7 @@
 import { EventEmitter } from "node:events";
 import { track } from "main/lib/analytics";
 import { ensureAgentHooks } from "../agent-setup/ensure-agent-hooks";
+import { treeKillWithEscalation } from "../tree-kill-with-escalation";
 import { FALLBACK_SHELL, SHELL_CRASH_THRESHOLD_MS } from "./env";
 import { portManager } from "./port-manager";
 import {
@@ -258,7 +259,8 @@ export class TerminalManager extends EventEmitter {
 		}
 
 		if (session.isAlive) {
-			session.pty.kill();
+			// Kill the entire process tree, not just the shell
+			await treeKillWithEscalation({ pid: session.pty.pid });
 		} else {
 			this.sessions.delete(paneId);
 		}
@@ -348,56 +350,40 @@ export class TerminalManager extends EventEmitter {
 
 		return new Promise<boolean>((resolve) => {
 			let resolved = false;
-			let sigtermTimeout: ReturnType<typeof setTimeout> | undefined;
-			let sigkillTimeout: ReturnType<typeof setTimeout> | undefined;
+			let forceCleanupTimeout: ReturnType<typeof setTimeout> | undefined;
 
-			const cleanup = (success: boolean) => {
+			const finish = (success: boolean) => {
 				if (resolved) return;
 				resolved = true;
-				this.off(`exit:${paneId}`, exitHandler);
-				if (sigtermTimeout) clearTimeout(sigtermTimeout);
-				if (sigkillTimeout) clearTimeout(sigkillTimeout);
+				this.off(`exit:${paneId}`, onExit);
+				if (forceCleanupTimeout) clearTimeout(forceCleanupTimeout);
 				resolve(success);
 			};
 
-			const exitHandler = () => cleanup(true);
-			this.once(`exit:${paneId}`, exitHandler);
+			const onExit = () => finish(true);
+			this.once(`exit:${paneId}`, onExit);
 
-			// Escalate to SIGKILL after 2s
-			sigtermTimeout = setTimeout(() => {
-				if (resolved || !session.isAlive) return;
+			treeKillWithEscalation({ pid: session.pty.pid }).then((result) => {
+				if (resolved) return;
 
-				try {
-					session.pty.kill("SIGKILL");
-				} catch (error) {
-					console.error(`Failed to send SIGKILL to terminal ${paneId}:`, error);
+				if (!result.success) {
+					console.error(`Terminal ${paneId} tree-kill failed:`, result.error);
 				}
 
-				// Force cleanup after another 500ms
-				sigkillTimeout = setTimeout(() => {
+				// node-pty's onExit may not fire reliably; force cleanup after delay
+				forceCleanupTimeout = setTimeout(() => {
 					if (resolved) return;
 					if (session.isAlive) {
 						console.error(
-							`Terminal ${paneId} did not exit after SIGKILL, forcing cleanup`,
+							`Terminal ${paneId} did not emit exit after kill, forcing cleanup`,
 						);
 						session.isAlive = false;
 						this.sessions.delete(paneId);
 					}
-					cleanup(false);
+					finish(!!result.success);
 				}, 500);
-				sigkillTimeout.unref();
-			}, 2000);
-			sigtermTimeout.unref();
-
-			// Send SIGTERM
-			try {
-				session.pty.kill();
-			} catch (error) {
-				console.error(`Failed to send SIGTERM to terminal ${paneId}:`, error);
-				session.isAlive = false;
-				this.sessions.delete(paneId);
-				cleanup(false);
-			}
+				forceCleanupTimeout.unref();
+			});
 		});
 	}
 
@@ -445,30 +431,34 @@ export class TerminalManager extends EventEmitter {
 		const exitPromises: Promise<void>[] = [];
 
 		for (const [paneId, session] of this.sessions.entries()) {
+			session.writeQueue.dispose();
 			if (session.isAlive) {
 				const exitPromise = new Promise<void>((resolve) => {
 					let timeoutId: ReturnType<typeof setTimeout> | undefined;
-					const exitHandler = () => {
-						this.off(`exit:${paneId}`, exitHandler);
+					const onExit = () => {
+						this.off(`exit:${paneId}`, onExit);
 						if (timeoutId !== undefined) {
 							clearTimeout(timeoutId);
 						}
 						resolve();
 					};
-					this.once(`exit:${paneId}`, exitHandler);
+					this.once(`exit:${paneId}`, onExit);
 
+					// 2.5s allows for tree-kill escalation (2s) + buffer
 					timeoutId = setTimeout(() => {
-						this.off(`exit:${paneId}`, exitHandler);
+						this.off(`exit:${paneId}`, onExit);
 						resolve();
-					}, 2000);
+					}, 2500);
 					timeoutId.unref();
 				});
 
 				exitPromises.push(exitPromise);
-				session.writeQueue.dispose();
-				session.pty.kill();
-			} else {
-				session.writeQueue.dispose();
+
+				treeKillWithEscalation({ pid: session.pty.pid }).then((result) => {
+					if (!result.success) {
+						console.error(`Terminal ${paneId} cleanup failed:`, result.error);
+					}
+				});
 			}
 		}
 

apps/desktop/src/main/lib/terminal/port-manager.ts

@@ -1,6 +1,6 @@
 import { EventEmitter } from "node:events";
-import process from "node:process";
 import type { DetectedPort } from "shared/types";
+import { treeKillWithEscalation } from "../tree-kill-with-escalation";
 import { getListeningPortsForPids, getProcessTree } from "./port-scanner";
 import type { TerminalSession } from "./types";
 
@@ -404,45 +404,35 @@ class PortManager extends EventEmitter {
 	}
 
 	/**
-	 * Safely kill a process listening on a tracked port.
-	 * Only kills if:
-	 * - The port is tracked by us
-	 * - The PID is not the terminal's shell PID (only child processes)
+	 * Kill a process tree listening on a tracked port.
+	 * Refuses to kill the terminal's shell process itself.
 	 */
-	killPort({ paneId, port }: { paneId: string; port: number }): {
+	killPort({ paneId, port }: { paneId: string; port: number }): Promise<{
 		success: boolean;
 		error?: string;
-	} {
+	}> {
 		const key = this.makeKey(paneId, port);
 		const detectedPort = this.ports.get(key);
 
 		if (!detectedPort) {
-			return { success: false, error: "Port not found in tracked ports" };
+			return Promise.resolve({
+				success: false,
+				error: "Port not found in tracked ports",
+			});
 		}
 
-		// Get the terminal's shell PID to ensure we don't kill it
 		const session = this.sessions.get(paneId);
 		const daemonSession = this.daemonSessions.get(paneId);
 		const shellPid = session?.session.pty.pid ?? daemonSession?.pid;
 
 		if (shellPid != null && detectedPort.pid === shellPid) {
-			return {
+			return Promise.resolve({
 				success: false,
 				error: "Cannot kill the terminal shell process",
-			};
+			});
 		}
 
-		try {
-			process.kill(detectedPort.pid, "SIGTERM");
-			return { success: true };
-		} catch (error) {
-			const message = error instanceof Error ? error.message : "Unknown error";
-			console.error(
-				`[PortManager] Failed to kill process ${detectedPort.pid}:`,
-				message,
-			);
-			return { success: false, error: message };
-		}
+		return treeKillWithEscalation({ pid: detectedPort.pid });
 	}
 }