From a3aee1940a85869c93dbbbc1ed0ea8048e2bb3c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Thu, 8 Oct 2026 10:18:28 -0400 Subject: [PATCH] Q58: command timeouts and stops kill the whole process tree Co-Authored-By: Claude Opus 5.5 --- src/main/services/workspace-command.ts | 68 +++++++++++++++++++------- tests/unit/workspace-command.test.ts | 30 +++++++++++- 2 files changed, 78 insertions(+), 20 deletions(-) diff --git a/src/main/services/workspace-command.ts b/src/main/services/workspace-command.ts index af83179b..c9a3cc56 100644 --- a/src/main/services/workspace-command.ts +++ b/src/main/services/workspace-command.ts @@ -1,4 +1,4 @@ -import { spawn } from "node:child_process"; +import { spawn, spawnSync } from "node:child_process"; import { currentRunSignal } from "./run-control"; const COMMAND_TIMEOUT_MS = 120_000; @@ -43,31 +43,61 @@ function ownCopy(view: Buffer): Buffer { return copy; } -export function runCommand(workspaceRoot: string, command: string): Promise { +export function runCommand(workspaceRoot: string, command: string, options: { timeoutMs?: number } = {}): Promise { + const timeoutMs = options.timeoutMs ?? COMMAND_TIMEOUT_MS; return new Promise((resolvePromise) => { - const child = spawn(command, { shell: true, cwd: workspaceRoot, windowsHide: true }); + // Its own process group on POSIX, so the whole tree can be killed at once. + const child = spawn(command, { shell: true, cwd: workspaceRoot, windowsHide: true, detached: process.platform !== "win32" }); const output = new BoundedOutput(); - let killedByTimeout = false; + let settled = false; + const runSignal = currentRunSignal(); + const onStop = (): void => abandon("ERROR: the run was stopped and the command was killed."); + const finish = (text: string): void => { + if (settled) return; + settled = true; + clearTimeout(timer); + runSignal?.removeEventListener("abort", onStop); + resolvePromise(text); + }; + // Children can outlive the shell and keep the pipes open, so stop reading instead of waiting for close. + const abandon = (text: string): void => { + if (settled) return; + const killFailure = killProcessTree(child.pid); + child.stdout.destroy(); + child.stderr.destroy(); + finish(killFailure ? `${text} Killing it failed, so it may still be running: ${killFailure}` : text); + }; const timer = setTimeout(() => { - killedByTimeout = true; - child.kill(); - }, COMMAND_TIMEOUT_MS); - currentRunSignal()?.addEventListener("abort", () => child.kill(), { once: true }); + abandon(`ERROR: command timed out after ${timeoutMs / 1000}s and was killed. Partial output:\n${output.text()}`); + }, timeoutMs); + runSignal?.addEventListener("abort", onStop, { once: true }); const collect = (chunk: Buffer): void => output.push(chunk); child.stdout.on("data", collect); child.stderr.on("data", collect); - child.on("error", (error) => { - clearTimeout(timer); - resolvePromise(`ERROR: ${error.message}`); - }); + child.on("error", (error) => finish(`ERROR: ${error.message}`)); child.on("close", (code) => { - clearTimeout(timer); - const text = output.text(); - if (killedByTimeout) { - resolvePromise(`ERROR: command timed out after ${COMMAND_TIMEOUT_MS / 1000}s and was killed. Partial output:\n${text}`); - return; - } - resolvePromise(`exit code ${code ?? 0}\n${text.trim() || "(no output)"}`); + finish(`exit code ${code ?? 0}\n${output.text().trim() || "(no output)"}`); }); }); } + +/** Kills a process and every process it started; returns why that failed, or null. */ +function killProcessTree(pid: number | undefined): string | null { + if (pid === undefined) return null; + console.warn(`[run_command] killing process tree ${pid}`); + if (process.platform === "win32") { + const result = spawnSync("taskkill", ["/pid", String(pid), "/T", "/F"], { windowsHide: true, encoding: "utf8" }); + if (result.error) return result.error.message; + // 128: the process already exited. + if (result.status !== 0 && result.status !== 128) return (result.stderr || result.stdout || `taskkill exited ${result.status}`).trim(); + return null; + } + try { + process.kill(-pid, "SIGKILL"); + return null; + } catch (error) { + // ESRCH: the group already exited. + if ((error as NodeJS.ErrnoException).code === "ESRCH") return null; + return (error as Error).message; + } +} diff --git a/tests/unit/workspace-command.test.ts b/tests/unit/workspace-command.test.ts index 02798614..31e97af9 100644 --- a/tests/unit/workspace-command.test.ts +++ b/tests/unit/workspace-command.test.ts @@ -1,6 +1,9 @@ +import { mkdtempSync, readFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { describe, expect, test } from "vitest"; +import { join } from "node:path"; +import { describe, expect, test, vi } from "vitest"; import { BoundedOutput, runCommand } from "../../src/main/services/workspace-command"; +import { stopRun, withRunControl } from "../../src/main/services/run-control"; describe("bounded command output", () => { test("holds at most head and tail while counting what it drops", () => { @@ -37,4 +40,29 @@ describe("bounded command output", () => { expect(result).toContain("bytes of output dropped"); expect(result.length).toBeLessThan(16_200); }); + + test("a timeout kills the whole process tree, including children that hold the output pipes", async () => { + const dir = mkdtempSync(join(tmpdir(), "ac-tree-")); + const pidFile = join(dir, "grandchild.pid").replaceAll("\\", "/"); + const script = `const c=require('child_process').spawn(process.execPath,['-e','setInterval(()=>{},1000)'],{stdio:'inherit'});require('fs').writeFileSync('${pidFile}',String(c.pid));setInterval(()=>{},1000)`; + const started = Date.now(); + const result = await runCommand(dir, `node -e "${script}"`, { timeoutMs: 1_500 }); + expect(result).toMatch(/^ERROR: command timed out after 1\.5s and was killed/); + expect(Date.now() - started).toBeLessThan(10_000); + const grandchild = Number(readFileSync(pidFile, "utf8")); + await new Promise((resolve) => setTimeout(resolve, 500)); + expect(() => process.kill(grandchild, 0)).toThrow(); + }, 20_000); +}); + +describe("run_command and run stops", () => { + test("a finished command is not killed again when its run is stopped later", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + await withRunControl("run_finished_cmd", async () => { + expect(await runCommand(tmpdir(), "node -e \"process.stdout.write('ok')\"")).toBe("exit code 0\nok"); + stopRun("run_finished_cmd"); + }); + expect(warn.mock.calls.filter((call) => String(call[0]).startsWith("[run_command] killing"))).toEqual([]); + warn.mockRestore(); + }); });