fix(process): bound Windows exec timeout cleanup (#104234)

Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
mikasa
2026-07-22 00:26:04 +08:00
committed by GitHub
parent 9c621b799a
commit a8a6e2a828
6 changed files with 361 additions and 61 deletions
+39 -14
View File
@@ -4,6 +4,7 @@ import { readFileSync } from "node:fs";
const DEFAULT_GRACE_MS = 3000;
const MAX_GRACE_MS = 60_000;
const TASKKILL_COMPLETION_TIMEOUT_MS = 3000;
export type KillProcessTreeOptions = {
graceMs?: number;
@@ -64,20 +65,22 @@ export function killProcessTree(pid: number, opts?: KillProcessTreeOptions): voi
export function signalProcessTree(
pid: number,
signal: "SIGTERM" | "SIGKILL",
opts?: { detached?: boolean },
opts?: { detached?: boolean; onComplete?: () => void },
): void {
if (!Number.isFinite(pid) || pid <= 0) {
opts?.onComplete?.();
return;
}
if (process.platform === "win32") {
signalProcessTreeWindows(pid, signal);
void signalProcessTreeWindowsAndWait(pid, signal).then(opts?.onComplete);
return;
}
const useGroupKill =
opts?.detached === true || (opts?.detached !== false && isProcessGroupLeader(pid));
signalProcessTreeUnix(pid, signal, useGroupKill);
opts?.onComplete?.();
}
function normalizeGraceMs(value?: number): number {
@@ -166,17 +169,32 @@ function signalProcessTreeUnix(
}
}
function runTaskkill(args: string[]): void {
try {
const child = spawn("taskkill", args, {
stdio: "ignore",
detached: true,
windowsHide: true,
});
child.once("error", () => {});
} catch {
// Ignore taskkill spawn failures.
}
function runTaskkill(args: string[]): Promise<void> {
return new Promise((resolve) => {
let settled = false;
const finish = () => {
if (settled) {
return;
}
settled = true;
clearTimeout(completionTimer);
resolve();
};
const completionTimer = setTimeout(finish, TASKKILL_COMPLETION_TIMEOUT_MS);
completionTimer.unref?.();
try {
const child = spawn("taskkill", args, {
stdio: "ignore",
detached: true,
windowsHide: true,
});
child.once("error", finish);
child.once("close", finish);
} catch {
// Ignore taskkill spawn failures.
finish();
}
});
}
function killProcessTreeWindows(pid: number, graceMs: number): void {
@@ -191,7 +209,14 @@ function killProcessTreeWindows(pid: number, graceMs: number): void {
}
function signalProcessTreeWindows(pid: number, signal: "SIGTERM" | "SIGKILL"): void {
void signalProcessTreeWindowsAndWait(pid, signal);
}
function signalProcessTreeWindowsAndWait(
pid: number,
signal: "SIGTERM" | "SIGKILL",
): Promise<void> {
const args =
signal === "SIGKILL" ? ["/F", "/T", "/PID", String(pid)] : ["/T", "/PID", String(pid)];
runTaskkill(args);
return runTaskkill(args);
}
+35
View File
@@ -317,6 +317,41 @@ describe("killProcessTree", () => {
});
});
it("on Windows exposes taskkill completion", async () => {
const taskkillChild = new EventEmitter();
spawnMock.mockReturnValueOnce(taskkillChild);
await withMockedPlatform("win32", async () => {
const completed = vi.fn();
signalProcessTree(8989, "SIGKILL", { onComplete: completed });
await Promise.resolve();
expect(completed).not.toHaveBeenCalled();
taskkillChild.emit("close", 0);
await Promise.resolve();
expect(completed).toHaveBeenCalledOnce();
expectTaskkillCall(0, ["/F", "/T", "/PID", "8989"]);
});
});
it("on Windows bounds taskkill completion when no event arrives", async () => {
const taskkillChild = new EventEmitter();
spawnMock.mockReturnValueOnce(taskkillChild);
await withMockedPlatform("win32", async () => {
const completed = vi.fn();
signalProcessTree(9090, "SIGKILL", { onComplete: completed });
await vi.advanceTimersByTimeAsync(2_999);
expect(completed).not.toHaveBeenCalled();
await vi.advanceTimersByTimeAsync(1);
expect(completed).toHaveBeenCalledOnce();
expectTaskkillCall(0, ["/F", "/T", "/PID", "9090"]);
});
});
it("on Windows force-kills synchronously without delayed taskkill", async () => {
await withMockedPlatform("win32", async () => {
killProcessTree(9999, { force: true });
+162 -10
View File
@@ -15,7 +15,11 @@ import {
const { spawnWithFallbackMock, signalProcessTreeMock, createWindowsOutputDecoderMock } = vi.hoisted(
() => ({
spawnWithFallbackMock: vi.fn(),
signalProcessTreeMock: vi.fn(),
signalProcessTreeMock: vi.fn(
(_pid: number, _signal: string, opts?: { onComplete?: () => void }) => {
opts?.onComplete?.();
},
),
createWindowsOutputDecoderMock: vi.fn(() => ({
decode: (chunk: Buffer | string) => (Buffer.isBuffer(chunk) ? chunk.toString("utf8") : chunk),
flush: () => "",
@@ -202,13 +206,16 @@ describe("createChildAdapter", () => {
}
adapter.kill();
await Promise.resolve();
// Detachment flag is now passed to signalProcessTree so it knows whether
// it can safely group-kill via -pid. (#71662)
const expectedDetached = process.platform !== "win32" && !process.env.OPENCLAW_SERVICE_MARKER;
expect(signalProcessTreeMock).toHaveBeenCalledWith(4321, "SIGKILL", {
detached: expectedDetached,
});
expect(signalProcessTreeMock).toHaveBeenCalledWith(
4321,
"SIGKILL",
expect.objectContaining({ detached: expectedDetached }),
);
expect(killMock).toHaveBeenCalledWith("SIGKILL");
});
@@ -228,8 +235,13 @@ describe("createChildAdapter", () => {
});
adapter.kill();
await Promise.resolve();
expect(signalProcessTreeMock).toHaveBeenCalledWith(8888, "SIGKILL", { detached: false });
expect(signalProcessTreeMock).toHaveBeenCalledWith(
8888,
"SIGKILL",
expect.objectContaining({ detached: false }),
);
expect(killMock).toHaveBeenCalledWith("SIGKILL");
});
@@ -238,7 +250,12 @@ describe("createChildAdapter", () => {
try {
const { adapter, killMock } = await createAdapterHarness({ pid: 9999 });
adapter.kill();
expect(signalProcessTreeMock).toHaveBeenCalledWith(9999, "SIGKILL", { detached: false });
await Promise.resolve();
expect(signalProcessTreeMock).toHaveBeenCalledWith(
9999,
"SIGKILL",
expect.objectContaining({ detached: false }),
);
expect(killMock).toHaveBeenCalledWith("SIGKILL");
} finally {
delete process.env.OPENCLAW_SERVICE_MARKER;
@@ -375,7 +392,106 @@ describe("createChildAdapter", () => {
expect(killMock).toHaveBeenCalledWith("SIGKILL");
});
it("settles wait from exit state on Windows even when close never arrives", async () => {
it("waits for Windows tree-kill completion before forced stream settlement", async () => {
vi.useFakeTimers();
setPlatform("win32");
let resolveTreeKill: (() => void) | undefined;
signalProcessTreeMock.mockImplementationOnce(
(_pid: number, _signal: string, opts?: { onComplete?: () => void }) => {
resolveTreeKill = opts?.onComplete;
},
);
const stub = createStubChild(9753);
spawnWithFallbackMock.mockResolvedValue({ child: stub.child, usedFallback: false });
const adapter = await createChildAdapter({
argv: ["node", "-e", "setInterval(() => {}, 1000)"],
stdinMode: "pipe-closed",
});
const settled = vi.fn();
void adapter.wait().then(settled);
adapter.kill("SIGKILL");
stub.emitExit(null, "SIGKILL");
await vi.advanceTimersByTimeAsync(1_000);
expect(settled).not.toHaveBeenCalled();
expect(stub.child.stdout?.destroyed).toBe(false);
expect(stub.child.stderr?.destroyed).toBe(false);
resolveTreeKill?.();
await Promise.resolve();
await vi.advanceTimersByTimeAsync(249);
expect(settled).not.toHaveBeenCalled();
await vi.advanceTimersByTimeAsync(1);
expect(settled).toHaveBeenCalledWith({ code: null, signal: "SIGKILL" });
expect(stub.child.stdout?.destroyed).toBe(true);
expect(stub.child.stderr?.destroyed).toBe(true);
});
it("blocks Windows child close until tree-kill completion", async () => {
vi.useFakeTimers();
setPlatform("win32");
let resolveTreeKill: (() => void) | undefined;
signalProcessTreeMock.mockImplementationOnce(
(_pid: number, _signal: string, opts?: { onComplete?: () => void }) => {
resolveTreeKill = opts?.onComplete;
},
);
const stub = createStubChild(9754);
spawnWithFallbackMock.mockResolvedValue({ child: stub.child, usedFallback: false });
const adapter = await createChildAdapter({
argv: ["node", "-e", "setInterval(() => {}, 1000)"],
stdinMode: "pipe-closed",
});
const settled = vi.fn();
void adapter.wait().then(settled);
adapter.kill("SIGKILL");
stub.emitExit(null, "SIGKILL");
stub.emitClose(null, "SIGKILL");
await vi.advanceTimersByTimeAsync(1_000);
expect(settled).not.toHaveBeenCalled();
resolveTreeKill?.();
await Promise.resolve();
await vi.advanceTimersByTimeAsync(250);
expect(settled).toHaveBeenCalledWith({ code: null, signal: "SIGKILL" });
});
it("blocks drained Windows streams until tree-kill completion", async () => {
vi.useFakeTimers();
setPlatform("win32");
let resolveTreeKill: (() => void) | undefined;
signalProcessTreeMock.mockImplementationOnce(
(_pid: number, _signal: string, opts?: { onComplete?: () => void }) => {
resolveTreeKill = opts?.onComplete;
},
);
const stub = createStubChild(9755);
spawnWithFallbackMock.mockResolvedValue({ child: stub.child, usedFallback: false });
const adapter = await createChildAdapter({
argv: ["node", "-e", "setInterval(() => {}, 1000)"],
stdinMode: "pipe-closed",
});
const settled = vi.fn();
void adapter.wait().then(settled);
adapter.kill("SIGKILL");
stub.emitExit(null, "SIGKILL");
stub.child.stdout?.emit("end");
stub.child.stderr?.emit("end");
await Promise.resolve();
expect(settled).not.toHaveBeenCalled();
resolveTreeKill?.();
await vi.advanceTimersByTimeAsync(0);
expect(settled).toHaveBeenCalledWith({ code: null, signal: "SIGKILL" });
});
it("preserves descendant output after ordinary Windows child exit", async () => {
vi.useFakeTimers();
setPlatform("win32");
@@ -391,6 +507,10 @@ describe("createChildAdapter", () => {
});
return { ...stub, adapter: adapterLocal };
})();
const stdout = vi.fn();
const stderr = vi.fn();
adapter.onStdout(stdout);
adapter.onStderr(stderr);
const settled = vi.fn();
void adapter.wait().then((result) => {
@@ -398,13 +518,45 @@ describe("createChildAdapter", () => {
});
emitExit(0, null);
child.stdout?.emit("end");
child.stderr?.emit("end");
await vi.advanceTimersByTimeAsync(300);
await vi.advanceTimersByTimeAsync(1_000);
expect(settled).not.toHaveBeenCalled();
expect(child.stdout?.destroyed).toBe(false);
expect(child.stderr?.destroyed).toBe(false);
const stdoutPipe = child.stdout as PassThrough;
const stderrPipe = child.stderr as PassThrough;
stdoutPipe.write("late stdout");
stderrPipe.write("late stderr");
stdoutPipe.end();
stderrPipe.end();
await vi.runAllTimersAsync();
expect(stdout).toHaveBeenCalledWith("late stdout");
expect(stderr).toHaveBeenCalledWith("late stderr");
expect(settled).toHaveBeenCalledWith({ code: 0, signal: null });
});
it("settles ordinary Windows exit when streams drain before exit and close is missing", async () => {
setPlatform("win32");
const stub = createStubChild(9756);
spawnWithFallbackMock.mockResolvedValue({ child: stub.child, usedFallback: false });
const adapter = await createChildAdapter({
argv: ["node", "-e", "process.exit(0)"],
stdinMode: "pipe-closed",
});
const waitPromise = adapter.wait();
const settled = vi.fn();
void waitPromise.then(settled);
stub.child.stdout?.emit("end");
stub.child.stderr?.emit("end");
await Promise.resolve();
expect(settled).not.toHaveBeenCalled();
stub.emitExit(0, null);
await expect(waitPromise).resolves.toEqual({ code: 0, signal: null });
});
it("disables detached mode in service-managed runtime", async () => {
process.env.OPENCLAW_SERVICE_MARKER = "openclaw";
+73 -31
View File
@@ -19,7 +19,7 @@ import type { ManagedRunStdin, SpawnProcessAdapter } from "../types.js";
import { toStringEnv } from "./env.js";
const FORCE_KILL_WAIT_FALLBACK_MS = 4000;
const WINDOWS_CLOSE_STATE_SETTLE_TIMEOUT_MS = 250;
const FORCED_WINDOWS_CLOSE_SETTLE_MS = 250;
const WINDOWS_PACKAGE_MANAGER_SHIMS = ["npm", "pnpm", "yarn", "npx"] as const;
function resolveChildInvocation(params: {
@@ -252,8 +252,11 @@ export async function createChildAdapter(params: {
let rejectWait: ((reason?: unknown) => void) | null = null;
let waitPromise: Promise<{ code: number | null; signal: NodeJS.Signals | null }> | null = null;
let forceKillWaitFallbackTimer: NodeJS.Timeout | null = null;
let forcedWindowsCloseTimer: NodeJS.Timeout | null = null;
let hardKillRequested = false;
let windowsTreeKillCompleted = false;
let childExitState: { code: number | null; signal: NodeJS.Signals | null } | null = null;
let windowsCloseFallbackTimer: NodeJS.Timeout | null = null;
let childCloseState: { code: number | null; signal: NodeJS.Signals | null } | null = null;
let stdoutDrained = child.stdout == null;
let stderrDrained = child.stderr == null;
@@ -265,12 +268,12 @@ export async function createChildAdapter(params: {
forceKillWaitFallbackTimer = null;
};
const clearWindowsCloseFallbackTimer = () => {
if (!windowsCloseFallbackTimer) {
const clearForcedWindowsCloseTimer = () => {
if (!forcedWindowsCloseTimer) {
return;
}
clearTimeout(windowsCloseFallbackTimer);
windowsCloseFallbackTimer = null;
clearTimeout(forcedWindowsCloseTimer);
forcedWindowsCloseTimer = null;
};
const settleWait = (value: { code: number | null; signal: NodeJS.Signals | null }) => {
@@ -278,7 +281,7 @@ export async function createChildAdapter(params: {
return;
}
clearForceKillWaitFallback();
clearWindowsCloseFallbackTimer();
clearForcedWindowsCloseTimer();
waitResult = value;
if (resolveWait) {
const resolve = resolveWait;
@@ -293,7 +296,7 @@ export async function createChildAdapter(params: {
return;
}
clearForceKillWaitFallback();
clearWindowsCloseFallbackTimer();
clearForcedWindowsCloseTimer();
waitError = error;
if (rejectWait) {
const reject = rejectWait;
@@ -325,9 +328,32 @@ export async function createChildAdapter(params: {
};
};
const scheduleForcedWindowsCloseSettlement = () => {
if (
process.platform !== "win32" ||
!hardKillRequested ||
!windowsTreeKillCompleted ||
childExitState == null ||
forcedWindowsCloseTimer
) {
return;
}
const exitState = childExitState;
forcedWindowsCloseTimer = setTimeout(() => {
child.stdout?.destroy();
child.stderr?.destroy();
settleWait(resolveObservedExitState(exitState));
}, FORCED_WINDOWS_CLOSE_SETTLE_MS);
forcedWindowsCloseTimer.unref?.();
};
const isWindowsHardKillSettlementBlocked = () =>
process.platform === "win32" && hardKillRequested && !windowsTreeKillCompleted;
const maybeSettleAfterWindowsExit = () => {
if (
process.platform !== "win32" ||
isWindowsHardKillSettlementBlocked() ||
childExitState == null ||
!stdoutDrained ||
!stderrDrained
@@ -337,17 +363,6 @@ export async function createChildAdapter(params: {
settleWait(resolveObservedExitState(childExitState));
};
const scheduleWindowsCloseFallback = () => {
if (process.platform !== "win32") {
return;
}
clearWindowsCloseFallbackTimer();
windowsCloseFallbackTimer = setTimeout(() => {
maybeSettleAfterWindowsExit();
}, WINDOWS_CLOSE_STATE_SETTLE_TIMEOUT_MS);
windowsCloseFallbackTimer.unref?.();
};
child.stdout?.once("end", () => {
stdoutDrained = true;
maybeSettleAfterWindowsExit();
@@ -370,10 +385,16 @@ export async function createChildAdapter(params: {
});
child.once("exit", (code, signal) => {
childExitState = { code, signal };
scheduleWindowsCloseFallback();
scheduleForcedWindowsCloseSettlement();
maybeSettleAfterWindowsExit();
});
child.once("close", (code, signal) => {
settleWait(resolveObservedExitState({ code, signal }));
childCloseState = { code, signal };
childExitState ??= childCloseState;
if (isWindowsHardKillSettlementBlocked()) {
return;
}
settleWait(resolveObservedExitState(childCloseState));
});
const wait = async () => {
@@ -416,19 +437,40 @@ export async function createChildAdapter(params: {
const signalProcessTreeForChild = (pid: number, signal: "SIGTERM" | "SIGKILL") => {
signalProcessTree(pid, signal, { detached: childIsDetached });
};
const signalProcessTreeForChildAndWait = (pid: number, signal: "SIGTERM" | "SIGKILL") =>
new Promise<void>((resolve) => {
signalProcessTree(pid, signal, { detached: childIsDetached, onComplete: resolve });
});
const kill = (signal?: NodeJS.Signals) => {
const pid = child.pid ?? undefined;
if (signal === undefined || signal === "SIGKILL") {
hardKillRequested = true;
scheduleForcedWindowsCloseSettlement();
if (pid) {
// Pass through whether the child is actually detached. Without this,
// `signalProcessTree` group-kills via `-pid` and takes out the gateway's
// own process group along with the child. (#71662)
signalProcessTreeForChild(pid, "SIGKILL");
}
try {
child.kill("SIGKILL");
} catch {
// ignore kill errors
// Let the tree owner traverse the live root before directly killing it.
// On Windows, killing the root first can make `taskkill /T` lose the
// descendant relationship. (#71662)
void signalProcessTreeForChildAndWait(pid, "SIGKILL").then(() => {
try {
child.kill("SIGKILL");
} catch {
// ignore kill errors
}
windowsTreeKillCompleted = true;
if (childCloseState) {
settleWait(resolveObservedExitState(childCloseState));
return;
}
maybeSettleAfterWindowsExit();
scheduleForcedWindowsCloseSettlement();
});
} else {
windowsTreeKillCompleted = true;
try {
child.kill("SIGKILL");
} catch {
// ignore kill errors
}
}
scheduleForceKillWaitFallback("SIGKILL");
return;
@@ -446,7 +488,7 @@ export async function createChildAdapter(params: {
const dispose = () => {
clearForceKillWaitFallback();
clearWindowsCloseFallbackTimer();
clearForcedWindowsCloseTimer();
child.removeAllListeners();
};
+38 -4
View File
@@ -1,6 +1,7 @@
// Process supervisor tests cover lifecycle, restart, and termination behavior.
import { performance } from "node:perf_hooks";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { mockProcessPlatform } from "../../test-utils/vitest-spies.js";
import type { SpawnProcessAdapter } from "./types.js";
const { createChildAdapterMock, createPtyAdapterMock } = vi.hoisted(() => ({
@@ -161,14 +162,45 @@ describe("process supervisor", () => {
await vi.advanceTimersByTimeAsync(5);
const exit = await exitPromise;
expect(adapter.killMock).toHaveBeenCalledWith("SIGTERM");
await vi.advanceTimersByTimeAsync(5_000);
expect(adapter.killMock).not.toHaveBeenCalledWith("SIGKILL");
const expectedTimeoutSignal = process.platform === "win32" ? "SIGKILL" : "SIGTERM";
expect(adapter.killMock).toHaveBeenCalledWith(expectedTimeoutSignal);
if (process.platform !== "win32") {
await vi.advanceTimersByTimeAsync(5_000);
expect(adapter.killMock).not.toHaveBeenCalledWith("SIGKILL");
}
expect(exit.reason).toBe("no-output-timeout");
expect(exit.noOutputTimedOut).toBe(true);
expect(exit.timedOut).toBe(true);
});
it("coalesces overlapping Windows deadline cancellation while hard kill is pending", async () => {
vi.useFakeTimers();
mockProcessPlatform("win32");
const adapter = createStubChildAdapter();
createChildAdapterMock.mockResolvedValue(adapter);
const supervisor = createProcessSupervisor();
const run = await spawnChild(supervisor, {
sessionId: "s-windows-timeout-overlap",
argv: createSilentIdleArgv(),
timeoutMs: 20,
noOutputTimeoutMs: 5,
stdinMode: "pipe-closed",
});
const exitPromise = run.wait();
await vi.advanceTimersByTimeAsync(5);
expect(adapter.killMock).toHaveBeenCalledTimes(1);
expect(adapter.killMock).toHaveBeenCalledWith("SIGKILL");
await vi.advanceTimersByTimeAsync(15);
expect(adapter.killMock).toHaveBeenCalledTimes(1);
adapter.settle(null, "SIGKILL");
const exit = await exitPromise;
expect(exit.reason).toBe("no-output-timeout");
});
it("escalates cancellation to SIGKILL when graceful shutdown does not settle", async () => {
vi.useFakeTimers();
const adapter = createStubChildAdapter({
@@ -263,7 +295,9 @@ describe("process supervisor", () => {
await vi.advanceTimersByTimeAsync(1);
const exit = await exitPromise;
expect(adapter.killMock).toHaveBeenCalledWith("SIGTERM");
expect(adapter.killMock).toHaveBeenCalledWith(
process.platform === "win32" ? "SIGKILL" : "SIGTERM",
);
expect(exit.reason).toBe("overall-timeout");
expect(exit.timedOut).toBe(true);
});
+14 -2
View File
@@ -145,6 +145,7 @@ export function createProcessSupervisor(): ProcessSupervisor {
let timeoutTimer: NodeJS.Timeout | null = null;
let noOutputTimer: NodeJS.Timeout | null = null;
let forceKillTimer: NodeJS.Timeout | null = null;
let cancelRequested = false;
const captureOutput = input.captureOutput !== false;
const maxCapturedOutputChars = clampCapturedOutputChars(input.maxCapturedOutputChars);
@@ -227,8 +228,19 @@ export function createProcessSupervisor(): ProcessSupervisor {
}
};
cancelAdapter = (_reason: TerminationReason) => {
if (settled || forceKillTimer) {
cancelAdapter = (reason: TerminationReason) => {
if (settled || cancelRequested) {
return;
}
cancelRequested = true;
// Windows has no catchable SIGTERM equivalent: the adapter implements it
// with asynchronous taskkill, so waiting the cleanup grace only delays an
// already-expired deadline before the same forced tree termination.
if (
process.platform === "win32" &&
(reason === "overall-timeout" || reason === "no-output-timeout")
) {
adapter.kill("SIGKILL");
return;
}
adapter.kill("SIGTERM");