From e65619dd0c3efe588315c0405e7fb61030b32c09 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Sat, 20 Jun 2026 13:52:28 +0200 Subject: [PATCH] fix(crabbox): clean wrapper child trees on parent signal --- scripts/crabbox-wrapper.mjs | 106 ++++++++++++++-- test/scripts/crabbox-wrapper.test.ts | 183 ++++++++++++++++++++++----- 2 files changed, 243 insertions(+), 46 deletions(-) diff --git a/scripts/crabbox-wrapper.mjs b/scripts/crabbox-wrapper.mjs index 578093be2485..b2d56a8cf8b2 100755 --- a/scripts/crabbox-wrapper.mjs +++ b/scripts/crabbox-wrapper.mjs @@ -2560,23 +2560,23 @@ const childInvocation = spawnInvocation(binary, childArgs, childEnv, process.pla const child = spawn(childInvocation.command, childInvocation.args, { cwd: childCwd, stdio: "inherit", + detached: process.platform !== "win32", env: childEnv, windowsVerbatimArguments: childInvocation.windowsVerbatimArguments, }); +const childKillGraceMs = 5_000; +let childForceKillTimer; +let childTreeShutdownStarted = false; if (fullCheckout) { try { stopFullCheckoutKeepalive = startFullCheckoutKeepalive(fullCheckout, { intervalMs: fullCheckoutKeepaliveIntervalMsValue, onMissing: () => { - if (!child.killed) { - child.kill("SIGTERM"); - } + void exitAfterChildTreeTermination(child, "SIGTERM", 1); }, }); } catch (error) { - if (!child.killed) { - child.kill("SIGTERM"); - } + signalChildProcessTree(child, "SIGTERM"); cleanupOnce(); throw error; } @@ -2588,17 +2588,17 @@ const signalExitCodes = new Map([ ["SIGTERM", 143], ]); for (const signal of signalExitCodes.keys()) { - process.once(signal, () => { - if (!child.killed) { - child.kill(signal); - } - cleanupOnce(); - process.exit(signalExitCodes.get(signal) ?? 1); + process.on(signal, () => { + void exitAfterChildTreeTermination(child, signal, signalExitCodes.get(signal) ?? 1); }); } process.once("exit", cleanupOnce); child.on("exit", (code, signal) => { + clearChildForceKillTimer(); + if (childTreeShutdownStarted) { + return; + } let fullCheckoutAvailable = true; if (fullCheckout) { fullCheckoutAvailable = assertFullCheckoutAvailableBeforeExit(fullCheckout.dir); @@ -2612,6 +2612,10 @@ child.on("exit", (code, signal) => { }); child.on("error", (error) => { + clearChildForceKillTimer(); + if (childTreeShutdownStarted) { + return; + } if (fullCheckout) { assertFullCheckoutAvailableBeforeExit(fullCheckout.dir); } @@ -2619,3 +2623,81 @@ child.on("error", (error) => { console.error(`[crabbox] failed to execute ${displayBinary}: ${error.message}`); process.exit(2); }); + +async function exitAfterChildTreeTermination(childProcess, signal, exitCode) { + if (childTreeShutdownStarted) { + signalChildProcessTree(childProcess, "SIGKILL"); + return; + } + childTreeShutdownStarted = true; + signalChildProcessTree(childProcess, signal); + await waitForChildTreeExit(childProcess, childKillGraceMs); + if (childProcessTreeIsAlive(childProcess)) { + signalChildProcessTree(childProcess, "SIGKILL"); + } + await waitForChildTreeExit(childProcess, childKillGraceMs); + cleanupOnce(); + process.exit(exitCode); +} + +function signalChildProcessTree(childProcess, signal) { + if ( + process.platform === "win32" && + (childProcess.exitCode !== null || childProcess.signalCode !== null) + ) { + return; + } + try { + if (process.platform !== "win32" && typeof childProcess.pid === "number") { + process.kill(-childProcess.pid, signal); + } else { + childProcess.kill(signal); + } + } catch (error) { + if (error?.code !== "ESRCH") { + try { + childProcess.kill(signal); + } catch {} + } + } + if (signal !== "SIGKILL" && !childForceKillTimer) { + childForceKillTimer = setTimeout(() => { + childForceKillTimer = undefined; + signalChildProcessTree(childProcess, "SIGKILL"); + }, childKillGraceMs); + childForceKillTimer.unref?.(); + } +} + +function clearChildForceKillTimer() { + if (childForceKillTimer) { + clearTimeout(childForceKillTimer); + childForceKillTimer = undefined; + } +} + +function childProcessTreeIsAlive(childProcess) { + if (process.platform === "win32" || typeof childProcess.pid !== "number") { + return childProcess.exitCode === null && childProcess.signalCode === null; + } + try { + process.kill(-childProcess.pid, 0); + return true; + } catch (error) { + return error?.code === "EPERM"; + } +} + +async function waitForChildTreeExit(childProcess, timeoutMs) { + const started = Date.now(); + while (Date.now() - started < timeoutMs) { + if (!childProcessTreeIsAlive(childProcess)) { + clearChildForceKillTimer(); + return true; + } + await new Promise((done) => { + setTimeout(done, 50); + }); + } + return !childProcessTreeIsAlive(childProcess); +} diff --git a/test/scripts/crabbox-wrapper.test.ts b/test/scripts/crabbox-wrapper.test.ts index e78613e81c25..70c99aac038d 100644 --- a/test/scripts/crabbox-wrapper.test.ts +++ b/test/scripts/crabbox-wrapper.test.ts @@ -1,9 +1,11 @@ // Crabbox Wrapper tests cover crabbox wrapper script behavior. -import { spawnSync } from "node:child_process"; +import { spawn, spawnSync } from "node:child_process"; import { chmodSync, + existsSync, mkdirSync, mkdtempSync, + readFileSync, readdirSync, rmSync, statSync, @@ -11,6 +13,7 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; const tempDirs: string[] = []; @@ -44,6 +47,12 @@ function writeFakeCrabbox(binDir: string, helpText: string): string { const helperPath = path.join(binDir, "fake-crabbox-json.cjs"); if (process.platform !== "win32") { + const signalIgnoringDescendantScript = [ + "process.on('SIGHUP', () => {});", + "process.on('SIGINT', () => {});", + "process.on('SIGTERM', () => {});", + "setInterval(() => {}, 1000);", + ].join(""); const script = [ "#!/bin/sh", 'if [ "$1" = "--version" ]; then', @@ -116,6 +125,12 @@ function writeFakeCrabbox(binDir: string, helpText: string): string { " fi", ' cd "$deleted_cwd" || exit 1', "fi", + 'if [ -n "${OPENCLAW_FAKE_CRABBOX_DESCENDANT_PID_PATH:-}" ]; then', + ` ${shellSingleQuote(process.execPath)} --input-type=module --eval ${shellSingleQuote(signalIgnoringDescendantScript)} &`, + ' printf "%s" "$!" > "$OPENCLAW_FAKE_CRABBOX_DESCENDANT_PID_PATH"', + ' trap "exit 0" INT TERM HUP', + " while :; do sleep 1; done", + "fi", 'printf "%s\\0" "__OPENCLAW_FAKE_CRABBOX_V1__"', 'printf "%s\\0" "$PWD"', 'printf "%s\\0" "$#"', @@ -280,47 +295,57 @@ function shellArgListCondition(args: string[]): string { return checks.join(" && "); } -function runWrapper( - helpText: string, - args: string[], - options: { - configJson?: Record; - configStatus?: number; - env?: Record; - extraPathEntries?: string[]; - gitResponses?: Record; - input?: string; - } = {}, -) { - const binDir = makeFakeCrabbox(helpText); - const gitResponses = { ...defaultGitResponses, ...options.gitResponses }; - const gitBinDir = makeFakeGit(gitResponses); +function runWrapper(helpText: string, args: string[], options: WrapperOptions = {}) { return spawnSync(process.execPath, ["scripts/crabbox-wrapper.mjs", ...args], { cwd: repoRoot, encoding: "utf8", input: options.input, - env: { - ...process.env, - PATH: [...(options.extraPathEntries ?? []), binDir, gitBinDir, process.env.PATH ?? ""] - .filter(Boolean) - .join(path.delimiter), - CRABBOX_PROVIDER: "", - OPENCLAW_CRABBOX_ALLOW_DIRECT_AWS: "", - OPENCLAW_CRABBOX_SYNC_MIN_FREE_BYTES: "0", - OPENCLAW_CRABBOX_WRAPPER_IGNORE_REPO_BINARY: "1", - ...(options.configJson - ? { OPENCLAW_FAKE_CRABBOX_CONFIG_JSON: JSON.stringify(options.configJson) } - : {}), - ...(options.configStatus - ? { OPENCLAW_FAKE_CRABBOX_CONFIG_STATUS: String(options.configStatus) } - : {}), - ...options.env, - OPENCLAW_FAKE_GIT_RESPONSES: JSON.stringify(gitResponses), - }, + env: wrapperEnv(helpText, options), timeout: 10_000, }); } +type WrapperOptions = { + configJson?: Record; + configStatus?: number; + env?: Record; + extraPathEntries?: string[]; + gitResponses?: Record; + input?: string; +}; + +function spawnWrapper(helpText: string, args: string[], options: WrapperOptions = {}) { + return spawn(process.execPath, ["scripts/crabbox-wrapper.mjs", ...args], { + cwd: repoRoot, + env: wrapperEnv(helpText, options), + stdio: ["ignore", "pipe", "pipe"], + }); +} + +function wrapperEnv(helpText: string, options: WrapperOptions): NodeJS.ProcessEnv { + const binDir = makeFakeCrabbox(helpText); + const gitResponses = { ...defaultGitResponses, ...options.gitResponses }; + const gitBinDir = makeFakeGit(gitResponses); + return { + ...process.env, + PATH: [...(options.extraPathEntries ?? []), binDir, gitBinDir, process.env.PATH ?? ""] + .filter(Boolean) + .join(path.delimiter), + CRABBOX_PROVIDER: "", + OPENCLAW_CRABBOX_ALLOW_DIRECT_AWS: "", + OPENCLAW_CRABBOX_SYNC_MIN_FREE_BYTES: "0", + OPENCLAW_CRABBOX_WRAPPER_IGNORE_REPO_BINARY: "1", + ...(options.configJson + ? { OPENCLAW_FAKE_CRABBOX_CONFIG_JSON: JSON.stringify(options.configJson) } + : {}), + ...(options.configStatus + ? { OPENCLAW_FAKE_CRABBOX_CONFIG_STATUS: String(options.configStatus) } + : {}), + ...options.env, + OPENCLAW_FAKE_GIT_RESPONSES: JSON.stringify(gitResponses), + }; +} + function parseFakeCrabboxOutput(result: ReturnType): { args: string[]; cwd: string; @@ -355,6 +380,76 @@ function normalizeShellLineEndings(value: string): string { return value.replace(/\r\n/g, "\n"); } +async function waitForCondition(predicate: () => boolean, timeoutMs = 8_000): Promise { + const started = Date.now(); + while (Date.now() - started < timeoutMs) { + if (predicate()) { + return; + } + await delay(50); + } + throw new Error("timed out waiting for condition"); +} + +async function waitForProcessExit( + child: ReturnType, + timeoutMs = 12_000, +): Promise<{ status: number | null; signal: NodeJS.Signals | null }> { + return await Promise.race([ + new Promise<{ status: number | null; signal: NodeJS.Signals | null }>((resolve, reject) => { + child.once("error", reject); + child.once("exit", (status, signal) => resolve({ status, signal })); + }), + delay(timeoutMs).then(() => { + throw new Error("timed out waiting for wrapper process exit"); + }), + ]); +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +async function runSignalCleanupProof(sendSignals: (pid: number) => Promise): Promise { + const root = mkdtempSync(path.join(tmpdir(), "openclaw-crabbox-descendant-")); + tempDirs.push(root); + const descendantPidPath = path.join(root, "descendant.pid"); + let descendantPid = 0; + const runner = spawnWrapper( + "provider: hetzner, aws, local-container, blacksmith-testbox, or cloudflare\n", + ["run", "--provider", "aws", "--", "echo ok"], + { + env: { + OPENCLAW_FAKE_CRABBOX_DESCENDANT_PID_PATH: descendantPidPath, + }, + }, + ); + + try { + await waitForCondition(() => existsSync(descendantPidPath)); + descendantPid = Number.parseInt(readFileSync(descendantPidPath, "utf8"), 10); + expect(Number.isInteger(descendantPid)).toBe(true); + expect(isProcessAlive(descendantPid)).toBe(true); + + const runnerExit = waitForProcessExit(runner); + await sendSignals(runner.pid!); + await expect(runnerExit).resolves.toEqual({ status: 143, signal: null }); + await waitForCondition(() => !isProcessAlive(descendantPid)); + } finally { + if (runner.pid && isProcessAlive(runner.pid)) { + runner.kill("SIGKILL"); + } + if (descendantPid && isProcessAlive(descendantPid)) { + process.kill(descendantPid, "SIGKILL"); + } + } +} + function testCrabboxConfigDir(home: string): string { if (process.platform === "darwin") { return path.join(home, "Library", "Application Support", "crabbox"); @@ -3041,6 +3136,26 @@ describe.concurrent("scripts/crabbox-wrapper", () => { } }); + (process.platform === "win32" ? it.skip : it)( + "terminates Crabbox descendants before parent signal exit", + async () => { + await runSignalCleanupProof(async (runnerPid) => { + process.kill(runnerPid, "SIGTERM"); + }); + }, + ); + + (process.platform === "win32" ? it.skip : it)( + "keeps cleanup active after repeated parent signals", + async () => { + await runSignalCleanupProof(async (runnerPid) => { + process.kill(runnerPid, "SIGTERM"); + await delay(50); + process.kill(runnerPid, "SIGTERM"); + }); + }, + ); + (process.platform === "win32" ? it.skip : it)( "terminates when sparse-sync temporary full checkouts disappear while Crabbox is running", () => {