From 34c90a8cb3fe32a657c6812d1b4087fba6c988b0 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 29 Jul 2026 00:26:32 -0400 Subject: [PATCH] fix(process): prevent orphaned Windows child process trees (#115535) * test(process): reproduce Windows taskkill process-tree leak * fix(process): stop leaked Windows child process trees Escalate only when Windows taskkill reports that graceful process-tree termination failed. Preserve awaited taskkill completion, grace-period fallback, one-shot signaling, and PID-reuse protection. Closes #110789 Supersedes #112202 Co-authored-by: Mohammed Alkindi --------- Co-authored-by: Mohammed Alkindi --- .../agent-core/src/harness/env/kill-tree.ts | 56 +++++++++--- src/process/exec.windows.integration.test.ts | 57 +++++++++++- src/process/kill-tree.test.ts | 88 ++++++++++++++++++- 3 files changed, 185 insertions(+), 16 deletions(-) diff --git a/packages/agent-core/src/harness/env/kill-tree.ts b/packages/agent-core/src/harness/env/kill-tree.ts index 11a87f6f5539..1e21d9da0bff 100644 --- a/packages/agent-core/src/harness/env/kill-tree.ts +++ b/packages/agent-core/src/harness/env/kill-tree.ts @@ -15,7 +15,8 @@ export type KillProcessTreeOptions = { /** * Best-effort process-tree termination with graceful shutdown. * - Windows: use taskkill /T to include descendants. Sends SIGTERM-equivalent - * first (without /F), then force-kills if process survives. + * first (without /F), then force-kills if taskkill refuses or the process + * survives the grace period. * - Unix: send SIGTERM to process group first, wait grace period, then SIGKILL. * * Group kill (`process.kill(-pid, ...)`) is only used when the PID is verified @@ -169,18 +170,19 @@ function signalProcessTreeUnix( } } -function runTaskkill(args: string[]): Promise { +function runTaskkill(args: string[], onExit?: (code: number | null) => void): Promise { return new Promise((resolve) => { let settled = false; - const finish = () => { + const finish = (code: number | null) => { if (settled) { return; } settled = true; clearTimeout(completionTimer); + onExit?.(code); resolve(); }; - const completionTimer = setTimeout(finish, TASKKILL_COMPLETION_TIMEOUT_MS); + const completionTimer = setTimeout(() => finish(null), TASKKILL_COMPLETION_TIMEOUT_MS); completionTimer.unref?.(); try { const child = spawn("taskkill", args, { @@ -188,35 +190,61 @@ function runTaskkill(args: string[]): Promise { detached: true, windowsHide: true, }); - child.once("error", finish); - child.once("close", finish); + // A failed spawn emits error before a close with a negative errno. Only + // taskkill's first actual outcome may authorize immediate escalation. + child.once("error", () => finish(null)); + child.once("close", (code) => finish(code)); } catch { // Ignore taskkill spawn failures. - finish(); + finish(null); } }); } function killProcessTreeWindows(pid: number, graceMs: number): void { - signalProcessTreeWindows(pid, "SIGTERM"); - - setTimeout(() => { + let forced = false; + let graceTimer: ReturnType | undefined; + const forceKill = () => { + if (forced) { + return; + } + // Latch before probing: a later live PID could belong to a reused, + // unrelated Windows process tree. + forced = true; + if (graceTimer !== undefined) { + clearTimeout(graceTimer); + graceTimer = undefined; + } if (!isProcessAlive(pid)) { return; } signalProcessTreeWindows(pid, "SIGKILL"); - }, graceMs).unref(); + }; + + signalProcessTreeWindows(pid, "SIGTERM", (code) => { + if (code !== null && code !== 0) { + forceKill(); + } + }); + + graceTimer = setTimeout(forceKill, graceMs); + graceTimer.unref(); } -function signalProcessTreeWindows(pid: number, signal: "SIGTERM" | "SIGKILL"): void { - void signalProcessTreeWindowsAndWait(pid, signal); +function signalProcessTreeWindows( + pid: number, + signal: "SIGTERM" | "SIGKILL", + onExit?: (code: number | null) => void, +): void { + void signalProcessTreeWindowsAndWait(pid, signal, onExit); } function signalProcessTreeWindowsAndWait( pid: number, signal: "SIGTERM" | "SIGKILL", + onExit?: (code: number | null) => void, ): Promise { const args = signal === "SIGKILL" ? ["/F", "/T", "/PID", String(pid)] : ["/T", "/PID", String(pid)]; - return runTaskkill(args); + return runTaskkill(args, onExit); } diff --git a/src/process/exec.windows.integration.test.ts b/src/process/exec.windows.integration.test.ts index 63a1c37bd244..4321196a5a3f 100644 --- a/src/process/exec.windows.integration.test.ts +++ b/src/process/exec.windows.integration.test.ts @@ -1,6 +1,9 @@ +import { spawn, spawnSync } from "node:child_process"; +import { once } from "node:events"; import process from "node:process"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { runUtf8CommandWithTimeout } from "./exec.js"; +import { killProcessTree } from "./kill-tree.js"; describe("runUtf8CommandWithTimeout Windows integration", () => { it.runIf(process.platform === "win32")( @@ -21,4 +24,56 @@ describe("runUtf8CommandWithTimeout Windows integration", () => { expect(result.stderrTruncatedBytes).toBe(5); }, ); + + it.runIf(process.platform === "win32")( + "force-kills a real Windows process tree when graceful taskkill refuses it", + async () => { + const program = [ + 'const { spawn } = require("node:child_process");', + 'const child = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore", windowsHide: true });', + 'child.once("spawn", () => process.stdout.write(String(child.pid) + "\\n"));', + 'child.once("error", () => process.exit(1));', + "setInterval(() => {}, 1000);", + ].join("\n"); + const parent = spawn(process.execPath, ["-e", program], { + stdio: ["ignore", "pipe", "ignore"], + windowsHide: true, + }); + const parentPid = parent.pid; + const parentStdout = parent.stdout; + + if (parentPid === undefined || parentStdout === null) { + parent.kill(); + throw new Error("Could not start the Windows process tree"); + } + + try { + const [output] = await once(parentStdout, "data"); + const childPid = Number.parseInt(String(output).trim(), 10); + expect(Number.isSafeInteger(childPid)).toBe(true); + expect(() => process.kill(parentPid, 0)).not.toThrow(); + expect(() => process.kill(childPid, 0)).not.toThrow(); + + // An unforced taskkill refuses Node console processes. Cleanup must not + // depend on this unref'd timer surviving an application shutdown. + killProcessTree(parentPid, { graceMs: 30_000 }); + + await vi.waitFor( + () => { + expect(() => process.kill(parentPid, 0)).toThrow(); + expect(() => process.kill(childPid, 0)).toThrow(); + }, + { timeout: 5_000, interval: 50 }, + ); + } finally { + spawnSync("taskkill", ["/F", "/T", "/PID", String(parentPid)], { + stdio: "ignore", + timeout: 5_000, + windowsHide: true, + }); + parentStdout.destroy(); + } + }, + 15_000, + ); }); diff --git a/src/process/kill-tree.test.ts b/src/process/kill-tree.test.ts index 8508a3a12732..12d47091dbca 100644 --- a/src/process/kill-tree.test.ts +++ b/src/process/kill-tree.test.ts @@ -63,7 +63,7 @@ describe("killProcessTree", () => { readFileSyncMock.mockImplementation(() => { throw new Error("proc unavailable"); }); - spawnMock.mockClear(); + spawnMock.mockReset(); spawnSyncMock.mockClear(); killSpy = vi.spyOn(process, "kill"); vi.useFakeTimers(); @@ -113,6 +113,92 @@ describe("killProcessTree", () => { }); }); + it("on Windows force-kills immediately when graceful taskkill refuses a live process tree", async () => { + const gracefulTaskkill = new EventEmitter(); + spawnMock.mockReturnValueOnce(gracefulTaskkill); + killSpy.mockImplementation(() => true); + + await withMockedPlatform("win32", async () => { + killProcessTree(4711, { graceMs: 30_000 }); + + expectTaskkillCall(0, ["/T", "/PID", "4711"]); + gracefulTaskkill.emit("close", 128); + + expect(spawnMock).toHaveBeenCalledTimes(2); + expectTaskkillCall(1, ["/F", "/T", "/PID", "4711"]); + }); + }); + + it("on Windows does not force-kill a disappeared or reused PID after taskkill fails", async () => { + const gracefulTaskkill = new EventEmitter(); + spawnMock.mockReturnValueOnce(gracefulTaskkill); + let processWasReused = false; + killSpy.mockImplementation(((pid: number, signal?: NodeJS.Signals | number) => { + if (pid === 4712 && signal === 0 && !processWasReused) { + throw new Error("ESRCH"); + } + return true; + }) as typeof process.kill); + + await withMockedPlatform("win32", async () => { + killProcessTree(4712, { graceMs: 25 }); + gracefulTaskkill.emit("close", 128); + expect(spawnMock).toHaveBeenCalledTimes(1); + + processWasReused = true; + await vi.advanceTimersByTimeAsync(25); + + expect(spawnMock).toHaveBeenCalledTimes(1); + }); + }); + + it("on Windows force-kills only once when taskkill failure races the grace timer", async () => { + const gracefulTaskkill = new EventEmitter(); + spawnMock.mockReturnValueOnce(gracefulTaskkill); + killSpy.mockImplementation(() => true); + + await withMockedPlatform("win32", async () => { + killProcessTree(4713, { graceMs: 20 }); + gracefulTaskkill.emit("close", 128); + await vi.advanceTimersByTimeAsync(20); + + expect(spawnMock).toHaveBeenCalledTimes(2); + expectTaskkillCall(1, ["/F", "/T", "/PID", "4713"]); + }); + }); + + it("on Windows waits for the grace timer when graceful taskkill cannot start", async () => { + const gracefulTaskkill = new EventEmitter(); + spawnMock.mockReturnValueOnce(gracefulTaskkill); + killSpy.mockImplementation(() => true); + + await withMockedPlatform("win32", async () => { + killProcessTree(4714, { graceMs: 15 }); + expect(() => gracefulTaskkill.emit("error", new Error("spawn ENOENT"))).not.toThrow(); + gracefulTaskkill.emit("close", -4058); + expect(spawnMock).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(15); + + expect(spawnMock).toHaveBeenCalledTimes(2); + expectTaskkillCall(1, ["/F", "/T", "/PID", "4714"]); + }); + }); + + it("on Windows keeps an explicitly requested failed tree signal single-shot", async () => { + const gracefulTaskkill = new EventEmitter(); + spawnMock.mockReturnValueOnce(gracefulTaskkill); + + await withMockedPlatform("win32", async () => { + signalProcessTree(4715, "SIGTERM"); + gracefulTaskkill.emit("close", 128); + await vi.advanceTimersByTimeAsync(60_000); + + expect(spawnMock).toHaveBeenCalledTimes(1); + expectTaskkillCall(0, ["/T", "/PID", "4715"]); + }); + }); + it("on Unix sends SIGTERM first and skips SIGKILL when process exits", async () => { killSpy.mockImplementation(((pid: number, signal?: NodeJS.Signals | number) => { if (pid === -3333 && signal === 0) {