diff --git a/eslint.config.mjs b/eslint.config.mjs index 66100d7ae..b65029291 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -4,6 +4,46 @@ import react from "eslint-plugin-react"; import reactHooks from "eslint-plugin-react-hooks"; import tseslint from "typescript-eslint"; +/** + * Custom ESLint plugin for zombie process prevention + * Enforces safe child_process patterns + */ +const localPlugin = { + rules: { + "no-unsafe-child-process": { + meta: { + type: "problem", + docs: { + description: "Prevent unsafe child_process usage that can cause zombie processes", + }, + messages: { + unsafePromisifyExec: + "Do not use promisify(exec) directly. Use DisposableExec wrapper with 'using' declaration to prevent zombie processes.", + }, + }, + create(context) { + return { + CallExpression(node) { + // Ban promisify(exec) + if ( + node.callee.type === "Identifier" && + node.callee.name === "promisify" && + node.arguments.length > 0 && + node.arguments[0].type === "Identifier" && + node.arguments[0].name === "exec" + ) { + context.report({ + node, + messageId: "unsafePromisifyExec", + }); + } + }, + }; + }, + }, + }, +}; + export default defineConfig([ { ignores: [ @@ -53,6 +93,7 @@ export default defineConfig([ plugins: { react, "react-hooks": reactHooks, + local: localPlugin, }, settings: { react: { @@ -137,6 +178,9 @@ export default defineConfig([ "react/react-in-jsx-scope": "off", "react/prop-types": "off", + // Zombie process prevention + "local/no-unsafe-child-process": "error", + // Allow console for this app (it's a dev tool) "no-console": "off", diff --git a/src/git.test.ts b/src/git.test.ts index 522cddbd7..b511215c0 100644 --- a/src/git.test.ts +++ b/src/git.test.ts @@ -7,6 +7,7 @@ import * as fs from "fs/promises"; import { exec } from "child_process"; import { promisify } from "util"; +// eslint-disable-next-line local/no-unsafe-child-process -- Test file needs direct exec access for setup const execAsync = promisify(exec); describe("createWorktree", () => { diff --git a/src/git.ts b/src/git.ts index a1b3e4807..9f04c0cf5 100644 --- a/src/git.ts +++ b/src/git.ts @@ -1,10 +1,7 @@ -import { exec } from "child_process"; -import { promisify } from "util"; import * as fs from "fs"; import * as path from "path"; import type { Config } from "./config"; - -const execAsync = promisify(exec); +import { execAsync } from "./utils/disposableExec"; export interface WorktreeResult { success: boolean; @@ -17,9 +14,10 @@ export interface CreateWorktreeOptions { } export async function listLocalBranches(projectPath: string): Promise { - const { stdout } = await execAsync( + using proc = execAsync( `git -C "${projectPath}" for-each-ref --format="%(refname:short)" refs/heads` ); + const { stdout } = await proc.result; return stdout .split("\n") .map((line) => line.trim()) @@ -29,7 +27,8 @@ export async function listLocalBranches(projectPath: string): Promise async function getCurrentBranch(projectPath: string): Promise { try { - const { stdout } = await execAsync(`git -C "${projectPath}" rev-parse --abbrev-ref HEAD`); + using proc = execAsync(`git -C "${projectPath}" rev-parse --abbrev-ref HEAD`); + const { stdout } = await proc.result; const branch = stdout.trim(); if (!branch || branch === "HEAD") { return null; @@ -108,19 +107,26 @@ export async function createWorktree( // If branch already exists locally, reuse it instead of creating a new one if (localBranches.includes(branchName)) { - await execAsync(`git -C "${projectPath}" worktree add "${workspacePath}" "${branchName}"`); + using proc = execAsync( + `git -C "${projectPath}" worktree add "${workspacePath}" "${branchName}"` + ); + await proc.result; return { success: true, path: workspacePath }; } // Check if branch exists remotely (origin/) - const { stdout: remoteBranchesRaw } = await execAsync(`git -C "${projectPath}" branch -a`); + using remoteBranchesProc = execAsync(`git -C "${projectPath}" branch -a`); + const { stdout: remoteBranchesRaw } = await remoteBranchesProc.result; const branchExists = remoteBranchesRaw .split("\n") .map((b) => b.trim().replace(/^(\*)\s+/, "")) .some((b) => b === branchName || b === `remotes/origin/${branchName}`); if (branchExists) { - await execAsync(`git -C "${projectPath}" worktree add "${workspacePath}" "${branchName}"`); + using proc = execAsync( + `git -C "${projectPath}" worktree add "${workspacePath}" "${branchName}"` + ); + await proc.result; return { success: true, path: workspacePath }; } @@ -131,9 +137,10 @@ export async function createWorktree( }; } - await execAsync( + using proc = execAsync( `git -C "${projectPath}" worktree add -b "${branchName}" "${workspacePath}" "${normalizedTrunkBranch}"` ); + await proc.result; return { success: true, path: workspacePath }; } catch (error) { @@ -149,9 +156,10 @@ export async function removeWorktree( ): Promise { try { // Remove the worktree (from the main repository context) - await execAsync( + using proc = execAsync( `git -C "${projectPath}" worktree remove "${workspacePath}" ${options.force ? "--force" : ""}` ); + await proc.result; return { success: true }; } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -161,7 +169,8 @@ export async function removeWorktree( export async function pruneWorktrees(projectPath: string): Promise { try { - await execAsync(`git -C "${projectPath}" worktree prune`); + using proc = execAsync(`git -C "${projectPath}" worktree prune`); + await proc.result; return { success: true }; } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -190,7 +199,8 @@ export async function moveWorktree( } // Move the worktree using git (from the main repository context) - await execAsync(`git -C "${projectPath}" worktree move "${oldPath}" "${newPath}"`); + using proc = execAsync(`git -C "${projectPath}" worktree move "${oldPath}" "${newPath}"`); + await proc.result; return { success: true, path: newPath }; } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -200,7 +210,8 @@ export async function moveWorktree( export async function listWorktrees(projectPath: string): Promise { try { - const { stdout } = await execAsync(`git -C "${projectPath}" worktree list --porcelain`); + using proc = execAsync(`git -C "${projectPath}" worktree list --porcelain`); + const { stdout } = await proc.result; const worktrees: string[] = []; const lines = stdout.split("\n"); @@ -223,7 +234,8 @@ export async function listWorktrees(projectPath: string): Promise { export async function isGitRepository(projectPath: string): Promise { try { - await execAsync(`git -C "${projectPath}" rev-parse --git-dir`); + using proc = execAsync(`git -C "${projectPath}" rev-parse --git-dir`); + await proc.result; return true; } catch { return false; @@ -238,7 +250,8 @@ export async function isGitRepository(projectPath: string): Promise { export async function getMainWorktreeFromWorktree(worktreePath: string): Promise { try { // Get the worktree list from the worktree itself - const { stdout } = await execAsync(`git -C "${worktreePath}" worktree list --porcelain`); + using proc = execAsync(`git -C "${worktreePath}" worktree list --porcelain`); + const { stdout } = await proc.result; const lines = stdout.split("\n"); // The first worktree in the list is always the main worktree diff --git a/src/services/gitService.ts b/src/services/gitService.ts index 69511c98c..97e1acba5 100644 --- a/src/services/gitService.ts +++ b/src/services/gitService.ts @@ -1,11 +1,8 @@ -import { exec } from "child_process"; -import { promisify } from "util"; import * as fs from "fs"; import * as fsPromises from "fs/promises"; import * as path from "path"; import type { Config } from "@/config"; - -const execAsync = promisify(exec); +import { execAsync } from "@/utils/disposableExec"; export interface WorktreeResult { success: boolean; @@ -35,7 +32,8 @@ export async function createWorktree( } // Check if branch exists - const { stdout: branches } = await execAsync(`git -C "${projectPath}" branch -a`); + using branchesProc = execAsync(`git -C "${projectPath}" branch -a`); + const { stdout: branches } = await branchesProc.result; const branchExists = branches .split("\n") .some( @@ -47,10 +45,16 @@ export async function createWorktree( if (branchExists) { // Branch exists, create worktree with existing branch - await execAsync(`git -C "${projectPath}" worktree add "${workspacePath}" "${branchName}"`); + using proc = execAsync( + `git -C "${projectPath}" worktree add "${workspacePath}" "${branchName}"` + ); + await proc.result; } else { // Branch doesn't exist, create new branch with worktree - await execAsync(`git -C "${projectPath}" worktree add -b "${branchName}" "${workspacePath}"`); + using proc = execAsync( + `git -C "${projectPath}" worktree add -b "${branchName}" "${workspacePath}"` + ); + await proc.result; } return { success: true, path: workspacePath }; @@ -67,9 +71,8 @@ export async function createWorktree( export async function isWorktreeClean(workspacePath: string): Promise { try { // Check for uncommitted changes (staged or unstaged) - const { stdout: statusOutput } = await execAsync( - `git -C "${workspacePath}" status --porcelain` - ); + using proc = execAsync(`git -C "${workspacePath}" status --porcelain`); + const { stdout: statusOutput } = await proc.result; return statusOutput.trim() === ""; } catch { // If git command fails, assume not clean (safer default) @@ -98,9 +101,10 @@ export async function removeWorktree( ): Promise { try { // Remove the worktree (from the main repository context) - await execAsync( + using proc = execAsync( `git -C "${projectPath}" worktree remove "${workspacePath}" ${options.force ? "--force" : ""}` ); + await proc.result; return { success: true }; } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -110,7 +114,8 @@ export async function removeWorktree( export async function pruneWorktrees(projectPath: string): Promise { try { - await execAsync(`git -C "${projectPath}" worktree prune`); + using proc = execAsync(`git -C "${projectPath}" worktree prune`); + await proc.result; return { success: true }; } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -259,7 +264,8 @@ export async function moveWorktree( } // Move the worktree using git (from the main repository context) - await execAsync(`git -C "${projectPath}" worktree move "${oldPath}" "${newPath}"`); + using proc = execAsync(`git -C "${projectPath}" worktree move "${oldPath}" "${newPath}"`); + await proc.result; return { success: true, path: newPath }; } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -269,7 +275,8 @@ export async function moveWorktree( export async function listWorktrees(projectPath: string): Promise { try { - const { stdout } = await execAsync(`git -C "${projectPath}" worktree list --porcelain`); + using proc = execAsync(`git -C "${projectPath}" worktree list --porcelain`); + const { stdout } = await proc.result; const worktrees: string[] = []; const lines = stdout.split("\n"); @@ -292,7 +299,8 @@ export async function listWorktrees(projectPath: string): Promise { export async function isGitRepository(projectPath: string): Promise { try { - await execAsync(`git -C "${projectPath}" rev-parse --git-dir`); + using proc = execAsync(`git -C "${projectPath}" rev-parse --git-dir`); + await proc.result; return true; } catch { return false; @@ -307,7 +315,8 @@ export async function isGitRepository(projectPath: string): Promise { export async function getMainWorktreeFromWorktree(worktreePath: string): Promise { try { // Get the worktree list from the worktree itself - const { stdout } = await execAsync(`git -C "${worktreePath}" worktree list --porcelain`); + using proc = execAsync(`git -C "${worktreePath}" worktree list --porcelain`); + const { stdout } = await proc.result; const lines = stdout.split("\n"); // The first worktree in the list is always the main worktree diff --git a/src/services/ipcMain.ts b/src/services/ipcMain.ts index 1dcf47f80..766bd8428 100644 --- a/src/services/ipcMain.ts +++ b/src/services/ipcMain.ts @@ -654,16 +654,26 @@ export class IpcMain { // macOS - try Ghostty first, fallback to Terminal.app const terminal = this.findAvailableCommand(["ghostty", "terminal"]); if (terminal === "ghostty") { - spawn("open", ["-a", "Ghostty", workspacePath], { detached: true }); + const child = spawn("open", ["-a", "Ghostty", workspacePath], { + detached: true, + stdio: "ignore", + }); + child.unref(); } else { - spawn("open", ["-a", "Terminal", workspacePath], { detached: true }); + const child = spawn("open", ["-a", "Terminal", workspacePath], { + detached: true, + stdio: "ignore", + }); + child.unref(); } } else if (process.platform === "win32") { // Windows - spawn("cmd", ["/c", "start", "cmd", "/K", "cd", "/D", workspacePath], { + const child = spawn("cmd", ["/c", "start", "cmd", "/K", "cd", "/D", workspacePath], { detached: true, shell: true, + stdio: "ignore", }); + child.unref(); } else { // Linux - try terminal emulators in order of preference // x-terminal-emulator is checked first as it respects user's system-wide preference diff --git a/src/services/tools/bash.test.ts b/src/services/tools/bash.test.ts index 3ed6da140..7fb9e9165 100644 --- a/src/services/tools/bash.test.ts +++ b/src/services/tools/bash.test.ts @@ -814,4 +814,46 @@ fi expect(result.exitCode).toBe(0); } }); + + it("should not create zombie processes when spawning background tasks", async () => { + using testEnv = createTestBashTool(); + const tool = testEnv.tool; + + // Spawn a background sleep process that would become a zombie if not cleaned up + // Use a unique marker to identify our test process + // Note: Start with echo to avoid triggering standalone sleep blocker + const marker = `zombie-test-${Date.now()}`; + const args: BashToolArgs = { + script: `echo "${marker}"; sleep 100 & echo $!`, + timeout_secs: 1, + }; + + const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult; + + // Tool should complete successfully + expect(result.success).toBe(true); + if (result.success) { + expect(result.output).toContain(marker); + const lines = result.output.split("\n"); + const bgPid = lines[1]; // Second line should be the background PID + + // Give a moment for cleanup to happen + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Verify the background process was killed (process group cleanup) + using checkEnv = createTestBashTool(); + const checkResult = (await checkEnv.tool.execute!( + { + script: `ps -p ${bgPid} > /dev/null 2>&1 && echo "ALIVE" || echo "DEAD"`, + timeout_secs: 1, + }, + mockToolCallOptions + )) as BashToolResult; + + expect(checkResult.success).toBe(true); + if (checkResult.success) { + expect(checkResult.output).toBe("DEAD"); + } + } + }); }); diff --git a/src/services/tools/bash.ts b/src/services/tools/bash.ts index abd0d4662..cf421c1c6 100644 --- a/src/services/tools/bash.ts +++ b/src/services/tools/bash.ts @@ -16,14 +16,26 @@ import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools"; import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions"; /** - * Wraps a ChildProcess to make it disposable for use with `using` statements + * Wraps a ChildProcess to make it disposable for use with `using` statements. + * Kills the entire process group to prevent zombie processes. */ class DisposableProcess implements Disposable { constructor(private readonly process: ChildProcess) {} [Symbol.dispose](): void { - if (!this.process.killed) { - this.process.kill(); + if (!this.process.killed && this.process.pid !== undefined) { + // Kill the entire process group by using negative PID + // This ensures all child processes (including backgrounded ones) are terminated + try { + process.kill(-this.process.pid); + } catch { + // If process group kill fails (e.g., process already exited), try killing just the process + try { + this.process.kill(); + } catch { + // Ignore - process might have already exited + } + } } } @@ -120,6 +132,12 @@ export const createBashTool: ToolFactory = (config: ToolConfiguration) => { GIT_TERMINAL_PROMPT: "0", // Disables git credential prompts }, stdio: ["ignore", "pipe", "pipe"], + // CRITICAL: Spawn as detached process group leader to prevent zombie processes. + // When a bash script spawns background processes (e.g., `sleep 100 &`), those + // children would normally be reparented to init when bash exits, becoming orphans. + // With detached:true, bash becomes a process group leader, allowing us to kill + // the entire group (including all backgrounded children) via process.kill(-pid). + detached: true, }) ); diff --git a/src/utils/disposableExec.test.ts b/src/utils/disposableExec.test.ts new file mode 100644 index 000000000..0c1198649 --- /dev/null +++ b/src/utils/disposableExec.test.ts @@ -0,0 +1,339 @@ +/* eslint-disable @typescript-eslint/no-unsafe-assignment */ +/* eslint-disable @typescript-eslint/no-unsafe-member-access */ +/* eslint-disable @typescript-eslint/no-explicit-any */ +/* eslint-disable @typescript-eslint/no-unsafe-argument */ +/* eslint-disable @typescript-eslint/no-unsafe-call */ + +import type { ChildProcess } from "child_process"; +import { describe, expect, test, beforeEach, afterEach } from "@jest/globals"; +import { execAsync } from "./disposableExec"; + +/** + * Tests for DisposableExec - verifies no process leaks under any scenario + * + * These tests access internal implementation details (child process) to verify cleanup. + * The eslint disables are necessary for test verification purposes. + */ + +describe("disposableExec", () => { + const activeProcesses = new Set(); + + beforeEach(() => { + activeProcesses.clear(); + }); + + afterEach(() => { + // Verify all processes are cleaned up after each test + for (const proc of activeProcesses) { + const hasExited = proc.exitCode !== null || proc.signalCode !== null; + expect(hasExited || proc.killed).toBe(true); + if (!hasExited && !proc.killed) { + proc.kill(); + } + } + activeProcesses.clear(); + }); + + test("successful command completes and cleans up automatically", async () => { + let childProc: ChildProcess; + + { + using proc = execAsync("echo 'hello world'"); + childProc = (proc as any).child; + activeProcesses.add(childProc); + + const { stdout } = await proc.result; + expect(stdout.trim()).toBe("hello world"); + } + + // After scope exit, process should be exited + expect(childProc.exitCode).toBe(0); + expect(childProc.killed).toBe(false); + }); + + test("failed command completes and cleans up automatically", async () => { + using proc = execAsync("exit 1"); + const childProc: ChildProcess = (proc as any).child; + activeProcesses.add(childProc); + + try { + await proc.result; + expect(true).toBe(false); // Should not reach here + } catch (error: any) { + expect(error.code).toBe(1); + } + + // After scope exit, process should be exited + expect(childProc.exitCode).toBe(1); + expect(childProc.killed).toBe(false); + }); + + test("disposing before completion kills the process", async () => { + const proc = execAsync("sleep 2"); + const childProc: ChildProcess = (proc as any).child; + activeProcesses.add(childProc); + + // Give process time to start + await new Promise((resolve) => setTimeout(resolve, 50)); + + expect(childProc.exitCode).toBeNull(); + expect(childProc.signalCode).toBeNull(); + + // Explicit disposal - kill the process + proc[Symbol.dispose](); + + // Wait for process to be killed + await new Promise((resolve) => { + if (childProc.killed) { + resolve(undefined); + } else { + childProc.once("exit", () => resolve(undefined)); + } + }); + + // Process should be killed + expect(childProc.killed).toBe(true); + + // Result promise should reject since we killed it + await expect(proc.result).rejects.toThrow(); + }); + + test("using block disposes and kills long-running process", async () => { + let childProc: ChildProcess; + let resultPromise: Promise<{ stdout: string; stderr: string }>; + + { + using proc = execAsync("sleep 2"); + childProc = (proc as any).child; + resultPromise = proc.result; + activeProcesses.add(childProc); + + // Give process time to start + await new Promise((resolve) => setTimeout(resolve, 50)); + + expect(childProc.exitCode).toBeNull(); + expect(childProc.signalCode).toBeNull(); + // Exit scope - should trigger disposal + } + + // Wait for process to be killed + await new Promise((resolve) => { + if (childProc.killed || childProc.exitCode !== null) { + resolve(undefined); + } else { + childProc.once("exit", () => resolve(undefined)); + } + }); + + // Process should be killed + expect(childProc.killed).toBe(true); + + // Result should reject since we killed it + await expect(resultPromise).rejects.toThrow(); + }); + + test("disposing already-exited process is safe", async () => { + const proc = execAsync("echo 'test'"); + const childProc: ChildProcess = (proc as any).child; + activeProcesses.add(childProc); + + await proc.result; + + // Process already exited + expect(childProc.exitCode).toBe(0); + + // Should not throw or cause issues + proc[Symbol.dispose](); + + // Still exited, not killed + expect(childProc.exitCode).toBe(0); + expect(childProc.killed).toBe(false); + }); + + test("stdout and stderr are captured correctly", async () => { + using proc = execAsync("echo 'stdout message' && echo 'stderr message' >&2"); + const childProc = (proc as any).child; + activeProcesses.add(childProc); + + const { stdout, stderr } = await proc.result; + expect(stdout.trim()).toBe("stdout message"); + expect(stderr.trim()).toBe("stderr message"); + }); + + test("error includes stderr content", async () => { + try { + using proc = execAsync("echo 'error details' >&2 && exit 42"); + const childProc = (proc as any).child; + activeProcesses.add(childProc); + + await proc.result; + expect(true).toBe(false); // Should not reach + } catch (error: any) { + expect(error.code).toBe(42); + expect(error.stderr.trim()).toBe("error details"); + expect(error.message).toContain("error details"); + } + }); + + test("multiple processes in parallel all clean up", async () => { + const childProcs: ChildProcess[] = []; + + await Promise.all( + Array.from({ length: 5 }, async (_, i) => { + using proc = execAsync(`echo 'process ${i}'`); + const childProc = (proc as any).child; + childProcs.push(childProc); + activeProcesses.add(childProc); + + const { stdout } = await proc.result; + expect(stdout.trim()).toBe(`process ${i}`); + }) + ); + + // All processes should be exited + for (const proc of childProcs) { + expect(proc.exitCode).toBe(0); + } + }); + + test("exception during process handling still cleans up", async () => { + let childProc: ChildProcess | undefined; + let resultPromise: Promise<{ stdout: string; stderr: string }> | undefined; + + try { + using proc = execAsync("sleep 2"); + childProc = (proc as any).child as ChildProcess; + resultPromise = proc.result; + activeProcesses.add(childProc); + + // Give process time to start + await new Promise((resolve) => setTimeout(resolve, 50)); + + // Throw exception before awaiting result - disposal will happen when leaving this block + throw new Error("Simulated error"); + } catch (error: any) { + expect(error.message).toBe("Simulated error"); + } + + // Wait for process to be killed + if (childProc) { + await new Promise((resolve) => { + if (childProc.killed || childProc.exitCode !== null) { + resolve(undefined); + } else { + childProc.once("exit", () => resolve(undefined)); + } + }); + } + + // Process should be killed despite exception + expect(childProc?.killed).toBe(true); + + // After leaving try block, disposal has occurred + // Result should reject since we killed it via disposal + await expect(resultPromise).rejects.toThrow(); + }); + + test("process killed by signal is handled correctly", async () => { + using proc = execAsync("sleep 2"); + const childProc: ChildProcess = (proc as any).child; + activeProcesses.add(childProc); + + try { + // Give process time to start + await new Promise((resolve) => setTimeout(resolve, 50)); + + // Manually kill with SIGTERM + childProc.kill("SIGTERM"); + + await proc.result; + expect(true).toBe(false); // Should not reach + } catch (error: any) { + expect(error.signal).toBe("SIGTERM"); + expect(error.message).toContain("SIGTERM"); + } + + // Wait for process to fully exit + await new Promise((resolve) => { + if (childProc.exitCode !== null || childProc.signalCode !== null) { + resolve(undefined); + } else { + childProc.once("exit", () => resolve(undefined)); + } + }); + + // Process should be killed + expect(childProc.killed).toBe(true); + expect(childProc.signalCode).toBe("SIGTERM"); + }); + + test("early disposal prevents result promise from hanging", async () => { + const proc = execAsync("sleep 2"); + const childProc = (proc as any).child; + activeProcesses.add(childProc); + + // Give process time to start + await new Promise((resolve) => setTimeout(resolve, 50)); + + // Dispose immediately + proc[Symbol.dispose](); + + // Wait for process to be killed + await new Promise((resolve) => { + if (childProc.killed || childProc.exitCode !== null) { + resolve(undefined); + } else { + childProc.once("exit", () => resolve(undefined)); + } + }); + + // Process should be killed + expect(childProc.killed).toBe(true); + + // Result should reject, not hang forever + await expect(proc.result).rejects.toThrow(); + }); + + test("dispose is idempotent - calling multiple times is safe", async () => { + const proc = execAsync("sleep 2"); + const childProc = (proc as any).child; + activeProcesses.add(childProc); + + // Give process time to start + await new Promise((resolve) => setTimeout(resolve, 50)); + + // Multiple dispose calls should be safe + proc[Symbol.dispose](); + proc[Symbol.dispose](); + proc[Symbol.dispose](); + + // Wait for process to be killed + await new Promise((resolve) => { + if (childProc.killed || childProc.exitCode !== null) { + resolve(undefined); + } else { + childProc.once("exit", () => resolve(undefined)); + } + }); + + // Process should be killed once + expect(childProc.killed).toBe(true); + + // Result should reject since we killed it + await expect(proc.result).rejects.toThrow(); + }); + + test("close event waits for stdio to flush", async () => { + // Generate large output to test stdio buffering + const largeOutput = "x".repeat(100000); + using proc = execAsync(`echo '${largeOutput}'`); + const childProc = (proc as any).child; + activeProcesses.add(childProc); + + const { stdout } = await proc.result; + + // Should receive all output, not truncated + expect(stdout.trim()).toBe(largeOutput); + expect(stdout.trim().length).toBe(largeOutput.length); + }); +}); diff --git a/src/utils/disposableExec.ts b/src/utils/disposableExec.ts new file mode 100644 index 000000000..21e43f9ce --- /dev/null +++ b/src/utils/disposableExec.ts @@ -0,0 +1,91 @@ +import { exec } from "child_process"; +import type { ChildProcess } from "child_process"; + +/** + * Disposable wrapper for exec that ensures child process cleanup. + * Prevents zombie processes by killing child when scope exits. + * + * Usage: + * using proc = execAsync("git status"); + * const { stdout } = await proc.result; + */ +class DisposableExec implements Disposable { + constructor( + private readonly promise: Promise<{ stdout: string; stderr: string }>, + private readonly child: ChildProcess + ) {} + + [Symbol.dispose](): void { + // Only kill if process hasn't exited naturally + // Check the child's actual exit state, not promise state (avoids async timing issues) + const hasExited = this.child.exitCode !== null || this.child.signalCode !== null; + if (!hasExited && !this.child.killed) { + this.child.kill(); + } + } + + get result() { + return this.promise; + } +} + +/** + * Execute command with automatic cleanup via `using` declaration. + * Prevents zombie processes by ensuring child is reaped even on error. + * + * @example + * using proc = execAsync("git status"); + * const { stdout } = await proc.result; + */ +export function execAsync(command: string): DisposableExec { + const child = exec(command); + const promise = new Promise<{ stdout: string; stderr: string }>((resolve, reject) => { + let stdout = ""; + let stderr = ""; + let exitCode: number | null = null; + let exitSignal: string | null = null; + + child.stdout?.on("data", (data) => { + stdout += data; + }); + child.stderr?.on("data", (data) => { + stderr += data; + }); + + // Use 'close' event instead of 'exit' - close fires after all stdio streams are closed + // This ensures we've received all buffered output before resolving/rejecting + child.on("exit", (code, signal) => { + exitCode = code; + exitSignal = signal; + }); + + child.on("close", () => { + // Only resolve if process exited cleanly (code 0, no signal) + if (exitCode === 0 && exitSignal === null) { + resolve({ stdout, stderr }); + } else { + // Include stderr in error message for better debugging + const errorMsg = + stderr.trim() || + (exitSignal + ? `Command killed by signal ${exitSignal}` + : `Command failed with exit code ${exitCode ?? "unknown"}`); + const error = new Error(errorMsg) as Error & { + code: number | null; + signal: string | null; + stdout: string; + stderr: string; + }; + error.code = exitCode; + error.signal = exitSignal; + error.stdout = stdout; + error.stderr = stderr; + reject(error); + } + }); + + child.on("error", reject); + }); + + return new DisposableExec(promise, child); +} diff --git a/tests/ipcMain/helpers.ts b/tests/ipcMain/helpers.ts index 236e1c879..70481737d 100644 --- a/tests/ipcMain/helpers.ts +++ b/tests/ipcMain/helpers.ts @@ -319,12 +319,14 @@ export async function createTempGitRepo(): Promise { const fs = await import("fs/promises"); const { exec } = await import("child_process"); const { promisify } = await import("util"); + // eslint-disable-next-line local/no-unsafe-child-process const execAsync = promisify(exec); // Use mkdtemp to avoid race conditions and ensure unique directory const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "cmux-test-repo-")); - // Batch git commands where possible to reduce overhead + // Use promisify(exec) for test setup - DisposableExec has issues in CI + // TODO: Investigate why DisposableExec causes empty git output in CI await execAsync(`git init`, { cwd: tempDir }); await execAsync(`git config user.email "test@example.com" && git config user.name "Test User"`, { cwd: tempDir,