From ecefe1c39c979c659ae60e8bf7686aa3f5f8fa38 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Thu, 23 Apr 2026 08:44:22 -0700 Subject: [PATCH] fix(scripts): reap child check processes --- scripts/check-changed.mjs | 39 +++---- scripts/lib/managed-child-process.mjs | 117 +++++++++++++++++++++ scripts/run-oxlint.mjs | 36 +++---- test/scripts/managed-child-process.test.ts | 103 ++++++++++++++++++ 4 files changed, 249 insertions(+), 46 deletions(-) create mode 100644 scripts/lib/managed-child-process.mjs create mode 100644 test/scripts/managed-child-process.test.ts diff --git a/scripts/check-changed.mjs b/scripts/check-changed.mjs index 09eeb844696..6bfdebfb0cb 100644 --- a/scripts/check-changed.mjs +++ b/scripts/check-changed.mjs @@ -1,4 +1,3 @@ -import { spawn } from "node:child_process"; import { performance } from "node:perf_hooks"; import { detectChangedLanes, @@ -8,6 +7,7 @@ import { } from "./changed-lanes.mjs"; import { booleanFlag, parseFlagArgs, stringFlag } from "./lib/arg-utils.mjs"; import { printTimingSummary } from "./lib/check-timing-summary.mjs"; +import { runManagedCommand } from "./lib/managed-child-process.mjs"; import { resolveChangedTestTargetPlan } from "./test-projects.test-support.mjs"; export function createChangedCheckPlan(result, options = {}) { @@ -216,31 +216,22 @@ async function runNode(command, timings) { async function runCommand(command, timings) { const startedAt = performance.now(); console.error(`\n[check:changed] ${command.name}`); - const child = spawn(command.bin, command.args, { - stdio: "inherit", - shell: process.platform === "win32", - }); + let status = 1; + try { + status = await runManagedCommand({ + bin: command.bin, + args: command.args, + }); + } catch (error) { + console.error(error); + } - return await new Promise((resolve) => { - child.once("error", (error) => { - console.error(error); - timings.push({ - name: command.name, - durationMs: performance.now() - startedAt, - status: 1, - }); - resolve(1); - }); - child.once("close", (status) => { - const resolvedStatus = status ?? 1; - timings.push({ - name: command.name, - durationMs: performance.now() - startedAt, - status: resolvedStatus, - }); - resolve(resolvedStatus); - }); + timings.push({ + name: command.name, + durationMs: performance.now() - startedAt, + status, }); + return status; } function printSummary(timings, options) { diff --git a/scripts/lib/managed-child-process.mjs b/scripts/lib/managed-child-process.mjs new file mode 100644 index 00000000000..9bbf8e3896e --- /dev/null +++ b/scripts/lib/managed-child-process.mjs @@ -0,0 +1,117 @@ +import { spawn } from "node:child_process"; + +const FORWARDED_SIGNALS = ["SIGINT", "SIGTERM", "SIGHUP"]; +const FORCE_KILL_DELAY_MS = 5_000; + +/** + * @param {NodeJS.Signals} signal + * @returns {number} + */ +export function signalExitCode(signal) { + const signalNumber = signalNumberFor(signal); + return signalNumber ? 128 + signalNumber : 1; +} + +/** + * @param {import("node:child_process").ChildProcess} child + * @param {NodeJS.Signals} [signal] + */ +export function terminateManagedChild(child, signal = "SIGTERM") { + if (!child.pid) { + return; + } + + try { + if (process.platform !== "win32") { + process.kill(-child.pid, signal); + return; + } + } catch (error) { + if (!isMissingProcessError(error)) { + try { + child.kill(signal); + } catch { + // The process may have already exited between the group kill and fallback kill. + } + } + return; + } + + child.kill(signal); +} + +/** + * @param {{ + * bin: string; + * args?: string[]; + * cwd?: string; + * env?: NodeJS.ProcessEnv; + * stdio?: import("node:child_process").StdioOptions; + * shell?: boolean; + * }} options + * @returns {Promise} + */ +export async function runManagedCommand({ + bin, + args = [], + cwd, + env, + stdio = "inherit", + shell = process.platform === "win32", +}) { + const child = spawn(bin, args, { + cwd, + env, + stdio, + shell, + detached: process.platform !== "win32", + }); + + let receivedSignal = null; + let forceKillTimer = null; + + const forwardSignal = (signal) => { + receivedSignal ??= signal; + terminateManagedChild(child, signal); + forceKillTimer ??= setTimeout(() => { + terminateManagedChild(child, "SIGKILL"); + }, FORCE_KILL_DELAY_MS); + }; + + for (const signal of FORWARDED_SIGNALS) { + process.once(signal, forwardSignal); + } + + try { + return await new Promise((resolve, reject) => { + child.once("error", reject); + child.once("close", (status) => { + if (forceKillTimer) { + clearTimeout(forceKillTimer); + } + resolve(receivedSignal ? signalExitCode(receivedSignal) : (status ?? 1)); + }); + }); + } finally { + for (const signal of FORWARDED_SIGNALS) { + process.off(signal, forwardSignal); + } + } +} + +function signalNumberFor(signal) { + switch (signal) { + case "SIGHUP": + return 1; + case "SIGINT": + return 2; + case "SIGTERM": + return 15; + default: + return 0; + } +} + +function isMissingProcessError(error) { + return Boolean(error && typeof error === "object" && "code" in error && error.code === "ESRCH"); +} diff --git a/scripts/run-oxlint.mjs b/scripts/run-oxlint.mjs index d82914b366d..cfbdf9cdc02 100644 --- a/scripts/run-oxlint.mjs +++ b/scripts/run-oxlint.mjs @@ -1,10 +1,10 @@ -import { spawnSync } from "node:child_process"; import path from "node:path"; import { acquireLocalHeavyCheckLockSync, applyLocalOxlintPolicy, shouldAcquireLocalHeavyCheckLockForOxlint, } from "./lib/local-heavy-check-runtime.mjs"; +import { runManagedCommand } from "./lib/managed-child-process.mjs"; const oxlintPath = path.resolve("node_modules", ".bin", "oxlint"); const PREPARE_EXTENSION_BOUNDARY_ARGS = [ @@ -24,7 +24,7 @@ export function shouldPrepareExtensionPackageBoundaryArtifacts(args) { return !args.some((arg) => OXLINT_PREPARE_SKIP_FLAGS.has(arg)); } -function prepareExtensionPackageBoundaryArtifacts(env) { +async function prepareExtensionPackageBoundaryArtifacts(env) { const releaseArtifactsLock = acquireLocalHeavyCheckLockSync({ cwd: process.cwd(), env, @@ -33,18 +33,15 @@ function prepareExtensionPackageBoundaryArtifacts(env) { }); try { - const result = spawnSync(process.execPath, PREPARE_EXTENSION_BOUNDARY_ARGS, { - stdio: "inherit", + const status = await runManagedCommand({ + bin: process.execPath, + args: PREPARE_EXTENSION_BOUNDARY_ARGS, env, }); - if (result.error) { - throw result.error; - } - - if ((result.status ?? 1) !== 0) { + if (status !== 0) { throw new Error( - `prepare-extension-package-boundary-artifacts failed with exit code ${result.status ?? 1}`, + `prepare-extension-package-boundary-artifacts failed with exit code ${status}`, ); } } finally { @@ -52,7 +49,7 @@ function prepareExtensionPackageBoundaryArtifacts(env) { } } -export function main(argv = process.argv.slice(2), runtimeEnv = process.env) { +export async function main(argv = process.argv.slice(2), runtimeEnv = process.env) { const { args: finalArgs, env } = applyLocalOxlintPolicy(argv, runtimeEnv); const releaseLock = env.OPENCLAW_OXLINT_SKIP_LOCK === "1" @@ -73,25 +70,20 @@ export function main(argv = process.argv.slice(2), runtimeEnv = process.env) { env.OPENCLAW_OXLINT_SKIP_PREPARE !== "1" && shouldPrepareExtensionPackageBoundaryArtifacts(finalArgs) ) { - prepareExtensionPackageBoundaryArtifacts(env); + await prepareExtensionPackageBoundaryArtifacts(env); } - const result = spawnSync(oxlintPath, finalArgs, { - stdio: "inherit", + const status = await runManagedCommand({ + bin: oxlintPath, + args: finalArgs, env, - shell: process.platform === "win32", }); - - if (result.error) { - throw result.error; - } - - process.exitCode = result.status ?? 1; + process.exitCode = status; } finally { releaseLock(); } } if (import.meta.main) { - main(); + await main(); } diff --git a/test/scripts/managed-child-process.test.ts b/test/scripts/managed-child-process.test.ts new file mode 100644 index 00000000000..0f1702c12f5 --- /dev/null +++ b/test/scripts/managed-child-process.test.ts @@ -0,0 +1,103 @@ +import { spawn } from "node:child_process"; +import fs from "node:fs"; +import path from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; +import { pathToFileURL } from "node:url"; +import { describe, expect, it } from "vitest"; +import { signalExitCode } from "../../scripts/lib/managed-child-process.mjs"; +import { createScriptTestHarness } from "./test-helpers.js"; + +const { createTempDir } = createScriptTestHarness(); + +describe("managed-child-process", () => { + it("maps forwarded signals to shell-compatible exit codes", () => { + expect(signalExitCode("SIGHUP")).toBe(129); + expect(signalExitCode("SIGINT")).toBe(130); + expect(signalExitCode("SIGTERM")).toBe(143); + }); + + it("kills the managed child process group when the runner is terminated", async () => { + const dir = createTempDir("openclaw-managed-child-"); + const childPath = path.join(dir, "child.mjs"); + const runnerPath = path.join(dir, "runner.mjs"); + const childPidPath = path.join(dir, "child.pid"); + const helperUrl = pathToFileURL(path.resolve("scripts/lib/managed-child-process.mjs")).href; + + fs.writeFileSync( + childPath, + ` +import fs from "node:fs"; + +fs.writeFileSync(process.argv[2], String(process.pid)); +for (const signal of ["SIGHUP", "SIGINT", "SIGTERM"]) { + process.on(signal, () => process.exit(0)); +} +setInterval(() => {}, 1_000); +`, + "utf8", + ); + fs.writeFileSync( + runnerPath, + ` +import { runManagedCommand } from ${JSON.stringify(helperUrl)}; + +process.exitCode = await runManagedCommand({ + bin: process.execPath, + args: [${JSON.stringify(childPath)}, ${JSON.stringify(childPidPath)}], + stdio: "ignore", +}); +`, + "utf8", + ); + + const runner = spawn(process.execPath, [runnerPath], { + stdio: "ignore", + }); + let childPid = 0; + + try { + await waitFor(() => fs.existsSync(childPidPath)); + childPid = Number(fs.readFileSync(childPidPath, "utf8")); + expect(Number.isInteger(childPid)).toBe(true); + expect(isProcessAlive(childPid)).toBe(true); + + process.kill(runner.pid!, "SIGTERM"); + const result = await waitForClose(runner); + + expect(result).toEqual({ code: 143, signal: null }); + await waitFor(() => !isProcessAlive(childPid)); + } finally { + if (runner.pid && isProcessAlive(runner.pid)) { + process.kill(runner.pid, "SIGKILL"); + } + if (childPid && isProcessAlive(childPid)) { + process.kill(childPid, "SIGKILL"); + } + } + }); +}); + +async function waitFor(condition: () => boolean, timeoutMs = 3_000) { + const startedAt = Date.now(); + while (!condition()) { + if (Date.now() - startedAt > timeoutMs) { + throw new Error("timed out waiting for condition"); + } + await delay(25); + } +} + +async function waitForClose(child: ReturnType) { + return await new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => { + child.once("close", (code, signal) => resolve({ code, signal })); + }); +} + +function isProcessAlive(pid: number) { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +}