From e1551ad16d89a1ad8dc8e093837f2118797cc1b0 Mon Sep 17 00:00:00 2001 From: Chase McDougall Date: Wed, 21 Jan 2026 23:47:24 -0500 Subject: [PATCH 1/4] fix(desktop): kill all terminal child processes on close using TTY Interactive shells with job control give foreground jobs their own process groups, so killing the shell's process group doesn't kill child processes like claude or npm dev servers. Fix by using pkill -t to kill all processes sharing the same controlling terminal, which includes all descendants regardless of their process group. --- .../src/main/terminal-host/pty-subprocess.ts | 64 +++++++++++++++---- 1 file changed, 53 insertions(+), 11 deletions(-) diff --git a/apps/desktop/src/main/terminal-host/pty-subprocess.ts b/apps/desktop/src/main/terminal-host/pty-subprocess.ts index e01c44a8d2b..71b3ccff8b4 100644 --- a/apps/desktop/src/main/terminal-host/pty-subprocess.ts +++ b/apps/desktop/src/main/terminal-host/pty-subprocess.ts @@ -8,6 +8,7 @@ * to avoid JSON escaping overhead on escape-sequence-heavy PTY output. */ +import { execSync } from "node:child_process"; import { write as fsWrite } from "node:fs"; import type { IPty } from "node-pty"; import * as pty from "node-pty"; @@ -367,25 +368,55 @@ function handleKill(payload: Buffer): void { const pid = ptyProcess.pid; - // Step 1: Send the requested signal (usually SIGTERM for graceful shutdown) + // Step 1: Get the TTY associated with the shell process + // All processes in the terminal share this TTY, even if they have different + // process groups (which happens with job control in interactive shells). + let tty: string | null = null; try { - ptyProcess.kill(signal); + tty = execSync(`ps -o tty= -p ${pid}`, { encoding: "utf8" }).trim(); } catch { - // Process may already be dead + // Process may already be dead, fall back to process group kill } - // Step 2: Escalate to SIGKILL if still alive after 2 seconds + // Step 2: Kill all processes on the TTY (shell + all descendants) + // This works because foreground jobs share the controlling terminal, + // even though zsh gives them their own process groups for job control. + if (tty) { + try { + const pkillSignal = signal.replace(/^SIG/, ""); + execSync(`pkill -${pkillSignal} -t ${tty}`, { stdio: "ignore" }); + } catch { + // pkill returns non-zero if no processes matched - that's fine + } + } else { + // Fallback: try process group kill (may not kill all children) + try { + process.kill(-pid, signal); + } catch { + // Process group already dead + } + } + + // Step 3: Escalate to SIGKILL if still alive after 2 seconds // node-pty's onExit callback may not fire reliably after pty.kill() const escalationTimer = setTimeout(() => { if (!ptyProcess) return; // Already exited via onExit - try { - ptyProcess.kill("SIGKILL"); - } catch { - // Process may already be dead + if (tty) { + try { + execSync(`pkill -KILL -t ${tty}`, { stdio: "ignore" }); + } catch { + // Process may already be dead + } + } else { + try { + process.kill(-pid, "SIGKILL"); + } catch { + // Process group already dead + } } - // Step 3: Force completion if onExit still hasn't fired after another 1 second + // Step 4: Force completion if onExit still hasn't fired after another 1 second // This ensures the subprocess exits even if node-pty never emits onExit const forceExitTimer = setTimeout(() => { if (!ptyProcess) return; // Finally exited via onExit @@ -440,10 +471,21 @@ function handleDispose(): void { ptyFd = null; if (ptyProcess) { + const pid = ptyProcess.pid; + + // Get TTY and kill all processes on it (handles job control process groups) try { - ptyProcess.kill("SIGKILL"); + const tty = execSync(`ps -o tty= -p ${pid}`, { encoding: "utf8" }).trim(); + if (tty) { + execSync(`pkill -KILL -t ${tty}`, { stdio: "ignore" }); + } } catch { - // Ignore + // Fallback to process group kill + try { + process.kill(-pid, "SIGKILL"); + } catch { + // Process may already be dead + } } ptyProcess = null; } From 427e342e94f1dfd75b74563538e9316b12823dcb Mon Sep 17 00:00:00 2001 From: Chase McDougall Date: Thu, 22 Jan 2026 00:09:42 -0500 Subject: [PATCH 2/4] refactor(desktop): use execFileSync to avoid shell interpolation Addresses PR feedback about shell injection risk by using execFileSync with array arguments instead of execSync with string interpolation. --- .../desktop/src/main/terminal-host/pty-subprocess.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/desktop/src/main/terminal-host/pty-subprocess.ts b/apps/desktop/src/main/terminal-host/pty-subprocess.ts index 71b3ccff8b4..e7c4ab52342 100644 --- a/apps/desktop/src/main/terminal-host/pty-subprocess.ts +++ b/apps/desktop/src/main/terminal-host/pty-subprocess.ts @@ -8,7 +8,7 @@ * to avoid JSON escaping overhead on escape-sequence-heavy PTY output. */ -import { execSync } from "node:child_process"; +import { execFileSync } from "node:child_process"; import { write as fsWrite } from "node:fs"; import type { IPty } from "node-pty"; import * as pty from "node-pty"; @@ -373,7 +373,7 @@ function handleKill(payload: Buffer): void { // process groups (which happens with job control in interactive shells). let tty: string | null = null; try { - tty = execSync(`ps -o tty= -p ${pid}`, { encoding: "utf8" }).trim(); + tty = execFileSync("ps", ["-o", "tty=", "-p", String(pid)], { encoding: "utf8" }).trim(); } catch { // Process may already be dead, fall back to process group kill } @@ -384,7 +384,7 @@ function handleKill(payload: Buffer): void { if (tty) { try { const pkillSignal = signal.replace(/^SIG/, ""); - execSync(`pkill -${pkillSignal} -t ${tty}`, { stdio: "ignore" }); + execFileSync("pkill", [`-${pkillSignal}`, "-t", tty], { stdio: "ignore" }); } catch { // pkill returns non-zero if no processes matched - that's fine } @@ -404,7 +404,7 @@ function handleKill(payload: Buffer): void { if (tty) { try { - execSync(`pkill -KILL -t ${tty}`, { stdio: "ignore" }); + execFileSync("pkill", ["-KILL", "-t", tty], { stdio: "ignore" }); } catch { // Process may already be dead } @@ -475,9 +475,9 @@ function handleDispose(): void { // Get TTY and kill all processes on it (handles job control process groups) try { - const tty = execSync(`ps -o tty= -p ${pid}`, { encoding: "utf8" }).trim(); + const tty = execFileSync("ps", ["-o", "tty=", "-p", String(pid)], { encoding: "utf8" }).trim(); if (tty) { - execSync(`pkill -KILL -t ${tty}`, { stdio: "ignore" }); + execFileSync("pkill", ["-KILL", "-t", tty], { stdio: "ignore" }); } } catch { // Fallback to process group kill From a10385fdfc567c304f9a0f9d91e0da58d4f51990 Mon Sep 17 00:00:00 2001 From: Chase McDougall Date: Thu, 22 Jan 2026 01:29:25 -0500 Subject: [PATCH 3/4] refactor(desktop): use tree-kill instead of TTY-based process cleanup Replace custom TTY-based approach (ps + pkill -t) with tree-kill library for killing terminal child processes. tree-kill traverses by PPID which handles job control process groups just as well, with simpler code and no shell command dependencies. --- apps/desktop/package.json | 1 + .../src/main/terminal-host/pty-subprocess.ts | 69 ++++--------------- bun.lock | 5 +- 3 files changed, 19 insertions(+), 56 deletions(-) diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 429c8d08680..0ad7eeb914d 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -125,6 +125,7 @@ "strip-ansi": "^7.1.2", "superjson": "^2.2.5", "tailwind-merge": "^3.4.0", + "tree-kill": "^1.2.2", "trpc-electron": "^0.1.2", "tw-animate-css": "^1.4.0", "zod": "^4.3.5", diff --git a/apps/desktop/src/main/terminal-host/pty-subprocess.ts b/apps/desktop/src/main/terminal-host/pty-subprocess.ts index e7c4ab52342..a0855df1e0e 100644 --- a/apps/desktop/src/main/terminal-host/pty-subprocess.ts +++ b/apps/desktop/src/main/terminal-host/pty-subprocess.ts @@ -8,10 +8,10 @@ * to avoid JSON escaping overhead on escape-sequence-heavy PTY output. */ -import { execFileSync } from "node:child_process"; import { write as fsWrite } from "node:fs"; import type { IPty } from "node-pty"; import * as pty from "node-pty"; +import treeKill from "tree-kill"; import { PtySubprocessFrameDecoder, PtySubprocessIpcType, @@ -368,55 +368,26 @@ function handleKill(payload: Buffer): void { const pid = ptyProcess.pid; - // Step 1: Get the TTY associated with the shell process - // All processes in the terminal share this TTY, even if they have different - // process groups (which happens with job control in interactive shells). - let tty: string | null = null; - try { - tty = execFileSync("ps", ["-o", "tty=", "-p", String(pid)], { encoding: "utf8" }).trim(); - } catch { - // Process may already be dead, fall back to process group kill - } - - // Step 2: Kill all processes on the TTY (shell + all descendants) - // This works because foreground jobs share the controlling terminal, - // even though zsh gives them their own process groups for job control. - if (tty) { - try { - const pkillSignal = signal.replace(/^SIG/, ""); - execFileSync("pkill", [`-${pkillSignal}`, "-t", tty], { stdio: "ignore" }); - } catch { - // pkill returns non-zero if no processes matched - that's fine + // Step 1: Kill the process tree using tree-kill + // tree-kill traverses by PPID to find all descendants + treeKill(pid, signal, (err) => { + if (err) { + // Process may already be dead, that's fine } - } else { - // Fallback: try process group kill (may not kill all children) - try { - process.kill(-pid, signal); - } catch { - // Process group already dead - } - } + }); - // Step 3: Escalate to SIGKILL if still alive after 2 seconds + // Step 2: Escalate to SIGKILL if still alive after 2 seconds // node-pty's onExit callback may not fire reliably after pty.kill() const escalationTimer = setTimeout(() => { if (!ptyProcess) return; // Already exited via onExit - if (tty) { - try { - execFileSync("pkill", ["-KILL", "-t", tty], { stdio: "ignore" }); - } catch { + treeKill(pid, "SIGKILL", (err) => { + if (err) { // Process may already be dead } - } else { - try { - process.kill(-pid, "SIGKILL"); - } catch { - // Process group already dead - } - } + }); - // Step 4: Force completion if onExit still hasn't fired after another 1 second + // Step 3: Force completion if onExit still hasn't fired after another 1 second // This ensures the subprocess exits even if node-pty never emits onExit const forceExitTimer = setTimeout(() => { if (!ptyProcess) return; // Finally exited via onExit @@ -473,20 +444,8 @@ function handleDispose(): void { if (ptyProcess) { const pid = ptyProcess.pid; - // Get TTY and kill all processes on it (handles job control process groups) - try { - const tty = execFileSync("ps", ["-o", "tty=", "-p", String(pid)], { encoding: "utf8" }).trim(); - if (tty) { - execFileSync("pkill", ["-KILL", "-t", tty], { stdio: "ignore" }); - } - } catch { - // Fallback to process group kill - try { - process.kill(-pid, "SIGKILL"); - } catch { - // Process may already be dead - } - } + // Kill the process tree - fire and forget since we're exiting + treeKill(pid, "SIGKILL"); ptyProcess = null; } diff --git a/bun.lock b/bun.lock index 8726b9ed627..3c3f7b83cb4 100644 --- a/bun.lock +++ b/bun.lock @@ -125,7 +125,7 @@ }, "apps/desktop": { "name": "@superset/desktop", - "version": "0.0.58", + "version": "0.0.59", "dependencies": { "@dnd-kit/core": "^6.3.1", "@dnd-kit/sortable": "^10.0.0", @@ -219,6 +219,7 @@ "strip-ansi": "^7.1.2", "superjson": "^2.2.5", "tailwind-merge": "^3.4.0", + "tree-kill": "^1.2.2", "trpc-electron": "^0.1.2", "tw-animate-css": "^1.4.0", "zod": "^4.3.5", @@ -4409,6 +4410,8 @@ "tr46": ["tr46@0.0.3", "", {}, "sha512-N3WMsuqV66lT30CrXNbEjx4GEwlow3v6rr4mCcv6prnfwhS01rkgyFdjPNBYd9br7LpXV1+Emh01fHnq2Gdgrw=="], + "tree-kill": ["tree-kill@1.2.2", "", { "bin": { "tree-kill": "cli.js" } }, "sha512-L0Orpi8qGpRG//Nd+H90vFB+3iHnue1zSSGmNOOCh1GLJ7rUKVwV2HvijphGQS2UmhUZewS9VgvxYIdgr+fG1A=="], + "trim-lines": ["trim-lines@3.0.1", "", {}, "sha512-kRj8B+YHZCc9kQYdWfJB2/oUl9rA99qbowYYBtr4ui4mZyAQ2JpvVBd/6U2YloATfqBhBTSMhTpgBHtU0Mf3Rg=="], "trim-newlines": ["trim-newlines@4.1.1", "", {}, "sha512-jRKj0n0jXWo6kh62nA5TEh3+4igKDXLvzBJcPpiizP7oOolUrYIxmVBG9TOtHYFHoddUk6YvAkGeGoSVTXfQXQ=="], From d386714a2763a634779f26572e6573e918eb751e Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Wed, 21 Jan 2026 22:59:13 -0800 Subject: [PATCH 4/4] fix(desktop): add error logging to tree-kill callbacks tree-kill throws synchronously when no callback is provided and an error occurs. Add callbacks with error logging to all three call sites to prevent unhandled exceptions and improve debuggability. --- apps/desktop/src/main/terminal-host/pty-subprocess.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/apps/desktop/src/main/terminal-host/pty-subprocess.ts b/apps/desktop/src/main/terminal-host/pty-subprocess.ts index a0855df1e0e..2a9a8dd3fa3 100644 --- a/apps/desktop/src/main/terminal-host/pty-subprocess.ts +++ b/apps/desktop/src/main/terminal-host/pty-subprocess.ts @@ -372,7 +372,7 @@ function handleKill(payload: Buffer): void { // tree-kill traverses by PPID to find all descendants treeKill(pid, signal, (err) => { if (err) { - // Process may already be dead, that's fine + console.error("[pty-subprocess] Failed to kill process tree:", err); } }); @@ -383,7 +383,7 @@ function handleKill(payload: Buffer): void { treeKill(pid, "SIGKILL", (err) => { if (err) { - // Process may already be dead + console.error("[pty-subprocess] Failed to SIGKILL process tree:", err); } }); @@ -445,7 +445,11 @@ function handleDispose(): void { const pid = ptyProcess.pid; // Kill the process tree - fire and forget since we're exiting - treeKill(pid, "SIGKILL"); + treeKill(pid, "SIGKILL", (err) => { + if (err) { + console.error("[pty-subprocess] Failed to kill process tree:", err); + } + }); ptyProcess = null; }