diff --git a/apps/desktop/src/lib/trpc/routers/ports/ports.ts b/apps/desktop/src/lib/trpc/routers/ports/ports.ts index d14c822c991..de7774189d6 100644 --- a/apps/desktop/src/lib/trpc/routers/ports/ports.ts +++ b/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() })) diff --git a/apps/desktop/src/main/lib/terminal/manager.test.ts b/apps/desktop/src/main/lib/terminal/manager.test.ts index c56b533f783..064ad3f13e0 100644 --- a/apps/desktop/src/main/lib/terminal/manager.test.ts +++ b/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; let mockPty: { + pid: number; write: ReturnType; resize: ReturnType; kill: ReturnType; @@ -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); }); }); diff --git a/apps/desktop/src/main/lib/terminal/manager.ts b/apps/desktop/src/main/lib/terminal/manager.ts index fdc12c24ecc..4259c2a5522 100644 --- a/apps/desktop/src/main/lib/terminal/manager.ts +++ b/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((resolve) => { let resolved = false; - let sigtermTimeout: ReturnType | undefined; - let sigkillTimeout: ReturnType | undefined; + let forceCleanupTimeout: ReturnType | 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[] = []; for (const [paneId, session] of this.sessions.entries()) { + session.writeQueue.dispose(); if (session.isAlive) { const exitPromise = new Promise((resolve) => { let timeoutId: ReturnType | 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); + } + }); } } diff --git a/apps/desktop/src/main/lib/terminal/port-manager.ts b/apps/desktop/src/main/lib/terminal/port-manager.ts index d4241ae6aee..fca76c722dd 100644 --- a/apps/desktop/src/main/lib/terminal/port-manager.ts +++ b/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 }); } } diff --git a/apps/desktop/src/main/lib/tree-kill-with-escalation.ts b/apps/desktop/src/main/lib/tree-kill-with-escalation.ts new file mode 100644 index 00000000000..e6f9c1264ec --- /dev/null +++ b/apps/desktop/src/main/lib/tree-kill-with-escalation.ts @@ -0,0 +1,121 @@ +import treeKill from "tree-kill"; + +const DEFAULT_ESCALATION_TIMEOUT_MS = 2000; +const POLL_INTERVAL_MS = 50; + +/** + * Kill a process tree with escalation to SIGKILL if the process survives. + * Sends SIGTERM, polls for exit, escalates to SIGKILL after timeout. + */ +export function treeKillWithEscalation({ + pid, + signal = "SIGTERM", + escalationTimeoutMs = DEFAULT_ESCALATION_TIMEOUT_MS, +}: { + pid: number; + signal?: string; + escalationTimeoutMs?: number; +}): Promise<{ success: boolean; error?: string }> { + return new Promise((resolve) => { + let resolved = false; + let pollTimer: ReturnType | null = null; + let escalationTimer: ReturnType | null = null; + + const clearTimers = () => { + if (pollTimer) { + clearInterval(pollTimer); + pollTimer = null; + } + if (escalationTimer) { + clearTimeout(escalationTimer); + escalationTimer = null; + } + }; + + const doResolve = (result: { success: boolean; error?: string }) => { + if (resolved) return; + resolved = true; + clearTimers(); + resolve(result); + }; + + treeKill(pid, signal, (err) => { + if (resolved) return; + + if (err) { + if (isProcessNotFoundError(err)) { + doResolve({ success: true }); + return; + } + console.error( + `[treeKillWithEscalation] Failed to ${signal} pid ${pid}:`, + err, + ); + } + + if (!isProcessAlive(pid)) { + doResolve({ success: true }); + return; + } + + pollTimer = setInterval(() => { + if (!isProcessAlive(pid)) { + doResolve({ success: true }); + } + }, POLL_INTERVAL_MS); + pollTimer.unref(); + }); + + escalationTimer = setTimeout(() => { + escalationTimer = null; + if (resolved) return; + + if (!isProcessAlive(pid)) { + doResolve({ success: true }); + return; + } + + console.log( + `[treeKillWithEscalation] Process ${pid} still alive after ${signal}, escalating to SIGKILL`, + ); + + treeKill(pid, "SIGKILL", (err) => { + if (resolved) return; + + if (err) { + if (isProcessNotFoundError(err)) { + doResolve({ success: true }); + return; + } + console.error( + `[treeKillWithEscalation] Failed to SIGKILL pid ${pid}:`, + err, + ); + doResolve({ success: false, error: err.message }); + } else { + doResolve({ success: true }); + } + }); + }, escalationTimeoutMs); + escalationTimer.unref(); + }); +} + +/** + * ESRCH = dead, EPERM = alive (process exists but we lack permission) + */ +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (err) { + return (err as NodeJS.ErrnoException).code !== "ESRCH"; + } +} + +function isProcessNotFoundError(err: Error): boolean { + const code = (err as NodeJS.ErrnoException).code; + if (code === "ESRCH") return true; + const message = err.message ?? ""; + return message.includes("ESRCH") || message.includes("No such process"); +} diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsx b/apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsx index 96da16909e5..dfde18f0d0d 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsx +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsx @@ -24,7 +24,7 @@ export function PortsList() { }; return ( -
+