diff --git a/scripts/run-additional-boundary-checks.mjs b/scripts/run-additional-boundary-checks.mjs index 81c1ce95eb24..f83b31e3f972 100644 --- a/scripts/run-additional-boundary-checks.mjs +++ b/scripts/run-additional-boundary-checks.mjs @@ -254,6 +254,16 @@ async function waitForProcessGroupExit(child, timeoutMs) { return !processGroupAlive(child); } +async function finishTerminatedProcessTree(child, timeoutKillGraceMs = TIMEOUT_KILL_GRACE_MS) { + if (processGroupAlive(child)) { + await waitForProcessGroupExit(child, timeoutKillGraceMs); + } + if (processGroupAlive(child)) { + terminateChild(child, "SIGKILL"); + await waitForProcessGroupExit(child, POST_FORCE_KILL_WAIT_MS); + } +} + function terminateActiveChildren(activeChildren, signal) { for (const child of activeChildren) { terminateChild(child, signal); @@ -262,37 +272,74 @@ function terminateActiveChildren(activeChildren, signal) { function installActiveChildCleanup(activeChildren) { let active = true; + let shutdownChildren = []; + let shutdownPromise = null; + let shutdownForceKillTimer = null; + let resolveShutdownForceKill = null; const removeHandlers = () => { for (const [signal, handler] of signalHandlers) { process.off(signal, handler); } process.off("exit", exitHandler); }; - const cleanup = (signal) => { + const forceKillShutdownChildren = () => { + if (shutdownForceKillTimer) { + clearTimeout(shutdownForceKillTimer); + shutdownForceKillTimer = null; + } + terminateActiveChildren(shutdownChildren, "SIGKILL"); + resolveShutdownForceKill?.(); + }; + const cleanup = (signal, { waitForExit = false } = {}) => { if (!active) { - return; + return shutdownPromise ?? Promise.resolve(); } active = false; - terminateActiveChildren(activeChildren, signal); + shutdownChildren = [...activeChildren]; + terminateActiveChildren(shutdownChildren, signal); + if (!waitForExit) { + return Promise.resolve(); + } + shutdownPromise = new Promise((resolveForceKill) => { + resolveShutdownForceKill = resolveForceKill; + // Keep this timer ref'ed: once the leader exits, group liveness can look + // gone while descendants are still running and still need the force kill. + shutdownForceKillTimer = setTimeout(forceKillShutdownChildren, TIMEOUT_KILL_GRACE_MS); + }) + .then(() => + Promise.all( + shutdownChildren.map((child) => waitForProcessGroupExit(child, POST_FORCE_KILL_WAIT_MS)), + ), + ) + .then(() => undefined); + return shutdownPromise; }; const signalHandlers = new Map(); const signals = process.platform === "win32" ? ["SIGINT", "SIGTERM"] : ["SIGINT", "SIGTERM", "SIGHUP"]; for (const signal of signals) { const handler = () => { - cleanup(signal); - removeHandlers(); - process.kill(process.pid, signal); + if (shutdownPromise) { + forceKillShutdownChildren(); + return; + } + void cleanup(signal, { waitForExit: true }).finally(() => { + removeHandlers(); + process.kill(process.pid, signal); + }); }; signalHandlers.set(signal, handler); - process.once(signal, handler); + process.on(signal, handler); } const exitHandler = () => { - cleanup("SIGTERM"); + void cleanup("SIGTERM"); }; process.once("exit", exitHandler); return () => { + if (shutdownPromise) { + return; + } active = false; removeHandlers(); }; @@ -345,13 +392,7 @@ export function runSingleCheck( }); }; const finishAfterTimeoutTeardown = async (code, signal) => { - if (processGroupAlive(child)) { - await waitForProcessGroupExit(child, TIMEOUT_KILL_GRACE_MS); - } - if (processGroupAlive(child)) { - terminateChild(child, "SIGKILL"); - await waitForProcessGroupExit(child, POST_FORCE_KILL_WAIT_MS); - } + await finishTerminatedProcessTree(child, TIMEOUT_KILL_GRACE_MS); finish(code, signal); }; const timeout = setTimeout(() => { diff --git a/test/scripts/run-additional-boundary-checks.test.ts b/test/scripts/run-additional-boundary-checks.test.ts index b67c9c4e54ff..fb596b9a919a 100644 --- a/test/scripts/run-additional-boundary-checks.test.ts +++ b/test/scripts/run-additional-boundary-checks.test.ts @@ -1,5 +1,5 @@ // Run Additional Boundary Checks tests cover run additional boundary checks script behavior. -import { spawnSync } from "node:child_process"; +import { spawn, spawnSync } from "node:child_process"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; @@ -47,6 +47,13 @@ function isProcessAlive(pid: number): boolean { } } +function isProcessZombie(pid: number): boolean { + const result = spawnSync("ps", ["-o", "stat=", "-p", String(pid)], { + encoding: "utf8", + }); + return result.status === 0 && result.stdout.trim().startsWith("Z"); +} + async function sleep(ms: number): Promise { await new Promise((resolve) => { setTimeout(resolve, ms); @@ -75,6 +82,32 @@ async function waitForDead(pid: number, timeoutMs: number): Promise { throw new Error(`process still alive: ${pid}`); } +async function waitForNotRunning(pid: number, timeoutMs: number): Promise { + const deadlineAt = Date.now() + timeoutMs; + while (Date.now() < deadlineAt) { + if (!isProcessAlive(pid) || isProcessZombie(pid)) { + return; + } + await sleep(25); + } + throw new Error(`process still running: ${pid}`); +} + +async function waitForChildClose( + child: ReturnType, + timeoutMs: number, +): Promise<{ code: number | null; signal: NodeJS.Signals | null }> { + return await new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error("child did not close before timeout")); + }, timeoutMs); + child.once("close", (code, signal) => { + clearTimeout(timeout); + resolve({ code, signal }); + }); + }); +} + describe("run-additional-boundary-checks", () => { it("runs prompt snapshot drift checks in CI", () => { expect(BOUNDARY_CHECKS[0]).toEqual({ @@ -284,4 +317,84 @@ describe("run-additional-boundary-checks", () => { } }, ); + + it.skipIf(process.platform === "win32")( + "cleans active check descendants on parent signal", + async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-boundary-signal-")); + const readyPath = path.join(tempDir, "ready"); + const childPidPath = path.join(tempDir, "child.pid"); + let childPid = 0; + let runner: ReturnType | undefined; + try { + const childScript = [ + "process.on('SIGTERM', () => {});", + "setInterval(() => {}, 1000);", + ].join(""); + const parentScript = [ + "const { spawn } = require('node:child_process');", + "const fs = require('node:fs');", + `const child = spawn(process.execPath, ['-e', ${JSON.stringify(childScript)}], { stdio: 'ignore' });`, + "fs.writeFileSync(process.env.OPENCLAW_TEST_CHILD_PID, String(child.pid));", + "fs.writeFileSync(process.env.OPENCLAW_TEST_READY, 'ready');", + "process.on('SIGTERM', () => process.exit(0));", + "setInterval(() => {}, 1000);", + ].join(""); + const runnerScript = ` +import { runChecks } from ${JSON.stringify( + new URL("../../scripts/run-additional-boundary-checks.mjs", import.meta.url).href, + )}; + +await runChecks( + [{ + label: "parent-signal", + command: process.execPath, + args: ["-e", ${JSON.stringify(parentScript)}], + }], + { + checkTimeoutMs: 30000, + concurrency: 1, + cwd: process.cwd(), + env: process.env, + output: { write() { return true; } }, + outputMaxBytes: 4096, + }, +); +`; + + runner = spawn(process.execPath, ["--input-type=module", "--eval", runnerScript], { + cwd: process.cwd(), + env: { + ...process.env, + OPENCLAW_TEST_CHILD_PID: childPidPath, + OPENCLAW_TEST_READY: readyPath, + }, + stdio: ["ignore", "ignore", "pipe"], + }); + + await waitForFile(readyPath, 2000); + childPid = Number(fs.readFileSync(childPidPath, "utf8")); + expect(Number.isInteger(childPid)).toBe(true); + expect(isProcessAlive(childPid)).toBe(true); + + runner.kill("SIGTERM"); + await sleep(50); + runner.kill("SIGTERM"); + + await expect(waitForChildClose(runner, 10_000)).resolves.toEqual({ + code: null, + signal: "SIGTERM", + }); + await waitForNotRunning(childPid, 2000); + } finally { + if (childPid && isProcessAlive(childPid)) { + process.kill(childPid, "SIGKILL"); + } + if (runner?.pid && isProcessAlive(runner.pid)) { + runner.kill("SIGKILL"); + } + fs.rmSync(tempDir, { force: true, recursive: true }); + } + }, + ); });