From 53200287f8ef8977268e16e5c6d103d1e8d082dd Mon Sep 17 00:00:00 2001 From: Shakker <165377636+shakkernerd@users.noreply.github.com> Date: Fri, 11 Sep 2026 12:05:59 +0100 Subject: [PATCH] fix: preserve child signal termination during CLI respawn (#144838) Preserve actual Unix child termination signals through startup respawns so supervisors stop interrupted runtimes without entering watcher recovery. Keep explicit numeric exits and Windows termination behavior intact. Fixes #144200. --- docs/cli/agent.md | 2 +- node-runtime-recovery.mjs | 4 + src/entry.compile-cache.test.ts | 86 +++++++++----- src/process/respawn-child-runner.test.ts | 62 ++++++++-- src/process/respawn-child-runner.ts | 4 + test/openclaw-launcher.e2e.test.ts | 40 +++++-- test/scripts/run-node-lifecycle.test.ts | 140 ++++++++++++++++++++++- 7 files changed, 290 insertions(+), 48 deletions(-) diff --git a/docs/cli/agent.md b/docs/cli/agent.md index 7d8ed7e4b729..435b71b32ef6 100644 --- a/docs/cli/agent.md +++ b/docs/cli/agent.md @@ -219,7 +219,7 @@ openclaw agent --agent ops --message "Run locally" --local - `--session-key` selects an explicit session key. Agent-prefixed keys must use `agent::`, and `--agent` must match the key's agent id when both are given. Bare non-sentinel keys scope to `--agent` when supplied, or to the configured default agent otherwise; for example `--agent ops --session-key incident-42` routes to `agent:ops:incident-42`. The literal keys `global` and `unknown` stay unscoped only when no `--agent` is supplied. - `--json` reserves stdout for the JSON response; Gateway, plugin, and `--local` diagnostics go to stderr so scripts can parse stdout directly. - After transient handshake retries are exhausted, a Gateway timeout or closed connection fails the command; the CLI never silently reruns the turn embedded. Transport loss is ambiguous — the Gateway may have accepted and may still finish the turn — so the stderr hint says to check `openclaw gateway status` and the session transcript before retrying or rerunning with `--local`, to avoid executing the turn twice. When the Gateway accepted the run before the transport error, the hint names the accepted run ID, and `--json` failures keep the canonical `ok: false` envelope with `runId` and `origin: "gateway"` fields alongside `error.type`/`error.message`. -- `SIGTERM`/`SIGINT` interrupt a waiting Gateway-backed request; if the Gateway already accepted the run, the CLI also sends `chat.abort` for that run id before exiting. `--local` runs receive the same signal but do not send `chat.abort`. A launcher child that terminates from the first forwarded `SIGINT` or `SIGTERM` exits with status 130 or 143, respectively. If the internal run-dedup key already has an active run for this session, the response reports `status: "in_flight"` and the non-JSON CLI prints a stderr diagnostic instead of an empty reply. For external cron/systemd wrappers, keep a hard-kill backstop such as `timeout -k 60 600 openclaw agent ...` so the supervisor can reap the process if shutdown cannot drain. +- `SIGTERM`/`SIGINT` interrupt a waiting Gateway-backed request; if the Gateway already accepted the run, the CLI also sends `chat.abort` for that run id before exiting. `--local` runs receive the same signal but do not send `chat.abort`. On Unix, startup wrappers preserve the runtime child's actual termination signal, including `SIGKILL` after shutdown escalation; shells report `SIGINT` and `SIGTERM` as statuses 130 and 143. Explicit numeric returns stay numeric, including a handled shutdown returning `0`. Windows retains its numeric termination behavior. If the internal run-dedup key already has an active run for this session, the response reports `status: "in_flight"` and the non-JSON CLI prints a stderr diagnostic instead of an empty reply. For external cron/systemd wrappers, keep a hard-kill backstop such as `timeout -k 60 600 openclaw agent ...` so the supervisor can reap the process if shutdown cannot drain. - When this command triggers `models.json` regeneration, SecretRef-managed provider credentials are persisted as non-secret markers (for example env var names, `secretref-env:ENV_VAR_NAME`, or `secretref-managed`), never resolved secret plaintext. Marker writes come from the active source config snapshot, not from resolved runtime secret values. ## JSON failures diff --git a/node-runtime-recovery.mjs b/node-runtime-recovery.mjs index f3bf978de225..fd9a037dfca7 100644 --- a/node-runtime-recovery.mjs +++ b/node-runtime-recovery.mjs @@ -151,6 +151,10 @@ export const runRespawnedChild = (command, args, env) => { child.once("exit", (code, signal) => { detach(); if (signal) { + if (process.platform !== "win32") { + process.kill(process.pid, signal); + return; + } const forwardedSignalExitCode = !hardKillBackstopStarted && signal === firstForwardedSignal ? signal === "SIGINT" diff --git a/src/entry.compile-cache.test.ts b/src/entry.compile-cache.test.ts index ebcadf459538..b9f80ffaa0ed 100644 --- a/src/entry.compile-cache.test.ts +++ b/src/entry.compile-cache.test.ts @@ -67,6 +67,7 @@ describe("entry compile cache", () => { let argv: string[]; let child: ChildProcess; let kill: Mock; + let processKill: MockInstance; let exit: MockInstance; let writeStderr: MockInstance; let envSnapshot: ReturnType; @@ -93,6 +94,7 @@ describe("entry compile cache", () => { spawn.mockReset().mockReturnValue(child); vi.spyOn(process, "argv", "get").mockImplementation(() => argv); vi.spyOn(process, "execArgv", "get").mockReturnValue(["--no-warnings"]); + processKill = vi.spyOn(process, "kill").mockReturnValue(true); exit = vi.spyOn(process, "exit").mockImplementation(vi.fn()); writeStderr = vi.spyOn(process.stderr, "write").mockReturnValue(true); }); @@ -258,36 +260,62 @@ describe("entry compile cache", () => { expect(writeStderr).not.toHaveBeenCalled(); }); - it("marks signal-terminated compile-cache respawn children as failed without forcing another exit", async () => { - await markSourceCheckout(); - await respawnWithoutOpenClawCompileCacheIfNeeded({ currentFile: entryFile, installRoot: root }); - child.emit("exit", null, "SIGTERM"); - expect(exit).toHaveBeenCalledExactlyOnceWith(1); - }); - - it("waits for a signaled compile-cache respawn child after force-killing it", async () => { - await markSourceCheckout(); - argv = [process.execPath, entryFile, "tui"]; - vi.useFakeTimers(); - try { - await respawnWithoutOpenClawCompileCacheIfNeeded({ - currentFile: entryFile, - installRoot: root, + it.each(["linux", "win32"] as const)( + "preserves compile-cache respawn child signal termination on %s", + async (platform) => { + await markSourceCheckout(); + await withMockedPlatform(platform, async () => { + await respawnWithoutOpenClawCompileCacheIfNeeded({ + currentFile: entryFile, + installRoot: root, + }); + child.emit("exit", null, "SIGTERM"); + if (platform === "win32") { + expect(exit).toHaveBeenCalledExactlyOnceWith(1); + expect(processKill).not.toHaveBeenCalled(); + } else { + expect(processKill).toHaveBeenCalledExactlyOnceWith(process.pid, "SIGTERM"); + expect(exit).not.toHaveBeenCalled(); + } }); - const [, options] = expectDefined(attachChildProcessBridge.mock.calls[0], "bridge call"); - expectDefined(options?.onSignal, "signal handler")("SIGTERM"); - vi.advanceTimersByTime(1_000); - expect(kill).toHaveBeenCalledWith("SIGTERM"); - expect(exit).not.toHaveBeenCalled(); - vi.advanceTimersByTime(1_000); - expect(kill).toHaveBeenCalledWith(process.platform === "win32" ? "SIGTERM" : "SIGKILL"); - expect(exit).not.toHaveBeenCalled(); - child.emit("exit", null, "SIGKILL"); - expect(exit).toHaveBeenCalledExactlyOnceWith(1); - } finally { - vi.useRealTimers(); - } - }); + }, + ); + + it.each(["linux", "win32"] as const)( + "waits for a signaled compile-cache respawn child after force-killing it on %s", + async (platform) => { + await markSourceCheckout(); + argv = [process.execPath, entryFile, "tui"]; + vi.useFakeTimers(); + try { + await withMockedPlatform(platform, async () => { + await respawnWithoutOpenClawCompileCacheIfNeeded({ + currentFile: entryFile, + installRoot: root, + }); + const [, options] = expectDefined(attachChildProcessBridge.mock.calls[0], "bridge call"); + expectDefined(options?.onSignal, "signal handler")("SIGTERM"); + vi.advanceTimersByTime(1_000); + expect(kill).toHaveBeenCalledWith("SIGTERM"); + expect(exit).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1_000); + expect(kill).toHaveBeenCalledWith(platform === "win32" ? "SIGTERM" : "SIGKILL"); + expect(exit).not.toHaveBeenCalled(); + expect(processKill).not.toHaveBeenCalled(); + child.emit("exit", null, "SIGKILL"); + if (platform === "win32") { + expect(exit).toHaveBeenCalledExactlyOnceWith(1); + expect(processKill).not.toHaveBeenCalled(); + } else { + expect(processKill).toHaveBeenCalledExactlyOnceWith(process.pid, "SIGKILL"); + expect(exit).not.toHaveBeenCalled(); + } + }); + } finally { + vi.useRealTimers(); + } + }, + ); it("respawns when Node already enabled its cache without an inherited cache path", async () => { await markSourceCheckout(); diff --git a/src/process/respawn-child-runner.test.ts b/src/process/respawn-child-runner.test.ts index 073587f596a3..8de718b9aaee 100644 --- a/src/process/respawn-child-runner.test.ts +++ b/src/process/respawn-child-runner.test.ts @@ -1,10 +1,11 @@ // Respawn child runner tests cover signal forwarding and process-tree cleanup. import type { ChildProcess, spawn } from "node:child_process"; import { EventEmitter } from "node:events"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { createDeferredCore } from "../shared/deferred.js"; const signalProcessTreeMock = vi.hoisted(() => vi.fn()); +const processKillMock = vi.fn(() => true); vi.mock("./kill-tree.js", () => ({ signalProcessTree: signalProcessTreeMock, @@ -27,6 +28,12 @@ function createChild(pid?: number): { child: ChildProcess; kill: ReturnType { beforeEach(() => { signalProcessTreeMock.mockReset(); + processKillMock.mockClear(); + vi.spyOn(process, "kill").mockImplementation(processKillMock); + }); + + afterEach(() => { + vi.restoreAllMocks(); }); it("spawns POSIX respawn children detached for process-group cleanup", () => { @@ -55,9 +62,26 @@ describe("runRespawnChildWithSignalBridge", () => { }); it.each([ - { signal: "SIGINT" as const, laterSignal: "SIGTERM" as const, exitCode: 130 }, - { signal: "SIGTERM" as const, laterSignal: "SIGINT" as const, exitCode: 143 }, - ])("exits $exitCode when the child exits by forwarded $signal", (testCase) => { + { + signal: "SIGINT" as const, + firstSignal: "SIGINT" as const, + laterSignal: "SIGTERM" as const, + exitCode: 130, + }, + { + signal: "SIGTERM" as const, + firstSignal: "SIGTERM" as const, + laterSignal: "SIGINT" as const, + exitCode: 143, + }, + { + signal: "SIGTERM" as const, + firstSignal: "SIGINT" as const, + laterSignal: undefined, + exitCode: 1, + }, + { signal: "SIGKILL" as const, firstSignal: undefined, laterSignal: undefined, exitCode: 1 }, + ])("preserves child $signal termination after first signal $firstSignal", (testCase) => { const { child } = createChild(2345); const exit = vi.fn(); let onSignal: ((signal: NodeJS.Signals) => void) | undefined; @@ -77,11 +101,21 @@ describe("runRespawnChildWithSignalBridge", () => { onError: vi.fn(), }); - onSignal?.(testCase.signal); - onSignal?.(testCase.laterSignal); + if (testCase.firstSignal) { + onSignal?.(testCase.firstSignal); + } + if (testCase.laterSignal) { + onSignal?.(testCase.laterSignal); + } child.emit("exit", null, testCase.signal); - expect(exit).toHaveBeenCalledWith(testCase.exitCode); + if (process.platform === "win32") { + expect(exit).toHaveBeenCalledWith(testCase.exitCode); + expect(processKillMock).not.toHaveBeenCalled(); + } else { + expect(processKillMock).toHaveBeenCalledWith(process.pid, testCase.signal); + expect(exit).not.toHaveBeenCalled(); + } }); it("signals detached respawn process groups after forwarded signal grace", () => { @@ -133,7 +167,12 @@ describe("runRespawnChildWithSignalBridge", () => { } child.emit("exit", null, "SIGKILL"); - expect(exit).toHaveBeenCalledWith(1); + if (process.platform === "win32") { + expect(exit).toHaveBeenCalledWith(1); + } else { + expect(processKillMock).toHaveBeenCalledWith(process.pid, "SIGKILL"); + expect(exit).not.toHaveBeenCalled(); + } } finally { vi.useRealTimers(); } @@ -327,7 +366,12 @@ describe("runRespawnChildWithSignalBridge", () => { expect(kill).toHaveBeenNthCalledWith(2, process.platform === "win32" ? "SIGTERM" : "SIGKILL"); child.emit("exit", null, "SIGKILL"); - expect(exit).toHaveBeenCalledWith(1); + if (process.platform === "win32") { + expect(exit).toHaveBeenCalledWith(1); + } else { + expect(processKillMock).toHaveBeenCalledWith(process.pid, "SIGKILL"); + expect(exit).not.toHaveBeenCalled(); + } } finally { vi.useRealTimers(); } diff --git a/src/process/respawn-child-runner.ts b/src/process/respawn-child-runner.ts index 0ebae5c9a9c0..403223036469 100644 --- a/src/process/respawn-child-runner.ts +++ b/src/process/respawn-child-runner.ts @@ -106,6 +106,10 @@ export function runRespawnChildWithSignalBridge(params: { } clearSignalTimers(); if (signal) { + if (process.platform !== "win32") { + process.kill(process.pid, signal); + return; + } const forwardedSignalExitCode = !hardKillBackstopStarted && signal === firstForwardedSignal ? signal === "SIGINT" diff --git a/test/openclaw-launcher.e2e.test.ts b/test/openclaw-launcher.e2e.test.ts index d9da10fa0ce0..8f72071f35e5 100644 --- a/test/openclaw-launcher.e2e.test.ts +++ b/test/openclaw-launcher.e2e.test.ts @@ -1328,9 +1328,10 @@ describe("openclaw launcher", () => { ); it.runIf(process.platform !== "win32").each([ - { signal: "SIGINT" as const, exitCode: 130 }, - { signal: "SIGTERM" as const, exitCode: 143 }, - ])("exits $exitCode when the respawn child terminates from $signal", async (testCase) => { + { signal: "SIGINT" as const, target: "launcher" }, + { signal: "SIGTERM" as const, target: "launcher" }, + { signal: "SIGKILL" as const, target: "child" }, + ])("preserves $signal when the respawn $target is signaled", async (testCase) => { const fixtureRoot = await makeLauncherFixture(fixtureRoots); await addGitMarker(fixtureRoot); const childInfoPath = path.join(fixtureRoot, "child-info.json"); @@ -1359,11 +1360,15 @@ describe("openclaw launcher", () => { const childInfo = await waitForJsonFile<{ pid: number }>(childInfoPath, 5000); respawnChildPid = childInfo.pid; - launcher.kill(testCase.signal); + if (testCase.target === "launcher") { + launcher.kill(testCase.signal); + } else { + process.kill(respawnChildPid, testCase.signal); + } await expect(waitForProcessExit(launcher, "launcher", 5000)).resolves.toEqual({ - code: testCase.exitCode, - signal: null, + code: null, + signal: testCase.signal, }); expect(isProcessAlive(respawnChildPid)).toBe(false); } finally { @@ -1376,6 +1381,25 @@ describe("openclaw launcher", () => { } }); + it("preserves an explicit exit 143 from a compile-cache respawn child", async () => { + const fixtureRoot = await makeLauncherFixture(fixtureRoots); + await addGitMarker(fixtureRoot); + await fs.writeFile( + path.join(fixtureRoot, "dist", "entry.js"), + 'process.stdout.write(process.env.OPENCLAW_COMPILE_CACHE_DISABLED_RESPAWNED ?? "0", () => process.exit(143));\n', + ); + + const result = spawnSync(process.execPath, [path.join(fixtureRoot, "openclaw.mjs")], { + cwd: fixtureRoot, + env: launcherEnv({ NODE_COMPILE_CACHE: path.join(fixtureRoot, ".node-compile-cache") }), + encoding: "utf8", + }); + + expect(result.stdout).toBe("1"); + expect(result.status).toBe(143); + expect(result.signal).toBeNull(); + }); + it.runIf(process.platform !== "win32")( "exits after SIGTERM when the respawn child ignores the forwarded signal", async () => { @@ -1411,8 +1435,8 @@ describe("openclaw launcher", () => { launcher.kill("SIGTERM"); await expect(waitForProcessExit(launcher, "launcher", 5000)).resolves.toEqual({ - code: 1, - signal: null, + code: null, + signal: "SIGKILL", }); expect(isProcessAlive(launcher.pid)).toBe(false); expect(isProcessAlive(respawnChildPid)).toBe(false); diff --git a/test/scripts/run-node-lifecycle.test.ts b/test/scripts/run-node-lifecycle.test.ts index ddc93868c8cd..d8b38f87577f 100644 --- a/test/scripts/run-node-lifecycle.test.ts +++ b/test/scripts/run-node-lifecycle.test.ts @@ -1,14 +1,152 @@ -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + copyFileSync, + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; import { createRequire } from "node:module"; import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; import { expect, it } from "vitest"; +import { toErrorObject } from "../../scripts/lib/error-format.mts"; +import { hasUnjoinedWork } from "../../scripts/lib/managed-child-process.mts"; import { isProcessAlive, waitForDead, waitForPidFile } from "../helpers/process-wait.js"; import { runQaGatewayFixture } from "../helpers/qa-gateway-cleanup.js"; import { runNodeScript } from "../helpers/run-node-script.js"; import { formatShimResult, withShimFixture } from "./direct-run-entrypoints.test-support.js"; +it.runIf(process.platform !== "win32")( + "stops gateway watch when a compile-cache respawn child dies from a signal", + async () => { + await withShimFixture("scripts/run-node.mjs", async (fixture) => { + const { checkoutRoot, fixtureRoot, implementationPath } = fixture; + const childPidPath = path.join(fixtureRoot, "child.pid"); + const launcherPidPath = path.join(fixtureRoot, "launcher.pid"); + const invocationsPath = path.join(fixtureRoot, "invocations.jsonl"); + const releasePath = path.join(fixtureRoot, "release"); + for (const filename of [ + "openclaw.mjs", + "node-version.mjs", + "node-runtime-update.mjs", + "node-runtime-recovery.mjs", + "node-sqlite.mjs", + ]) { + copyFileSync(filename, path.join(checkoutRoot, filename)); + } + mkdirSync(path.join(checkoutRoot, "src")); + mkdirSync(path.join(checkoutRoot, "dist")); + mkdirSync(path.join(fixtureRoot, "home")); + writeFileSync(path.join(checkoutRoot, "package.json"), '{"type":"module"}'); + writeFileSync(path.join(checkoutRoot, "src/entry.ts"), "export {};\n"); + writeFileSync( + path.join(checkoutRoot, "dist/entry.js"), + `import fs from "node:fs"; +if (fs.existsSync(${JSON.stringify(releasePath)})) process.exit(0); +fs.writeFileSync(${JSON.stringify(childPidPath)}, String(process.pid)); +setInterval(() => { + if (fs.existsSync(${JSON.stringify(releasePath)})) process.exit(0); +}, 20); +`, + ); + const runnerUrl = pathToFileURL(path.resolve("scripts/run-node.mts")).href; + writeFileSync( + implementationPath, + `import fs from "node:fs"; +import { spawn } from "node:child_process"; +import { runNodeMain } from ${JSON.stringify(runnerUrl)}; +fs.appendFileSync(${JSON.stringify(invocationsPath)}, JSON.stringify(process.argv.slice(2)) + "\\n"); +// Let a regressed watcher finish after recording its doctor or restart invocation. +if (fs.existsSync(${JSON.stringify(childPidPath)})) process.exit(0); +const outcome = await runNodeMain({ + spawn: (command, args, options) => { + if (!args.includes("openclaw.mjs")) return spawn(process.execPath, ["--eval", ""], options); + const child = spawn(command, args, options); + fs.writeFileSync(${JSON.stringify(launcherPidPath)}, String(child.pid)); + return child; + }, +}); +if (typeof outcome === "string") process.kill(process.pid, outcome); +else process.exit(outcome); +`, + ); + const watchWrapper = path.join(checkoutRoot, "scripts/watch-node.mjs"); + copyFileSync("scripts/watch-node.mjs", watchWrapper); + const watcherUrl = pathToFileURL(path.resolve("scripts/watch-node.mts")).href; + writeFileSync( + path.join(checkoutRoot, "scripts/watch-node.mts"), + `import { runWatchMain } from ${JSON.stringify(watcherUrl)}; +const outcome = await runWatchMain({ + createWatcher: () => ({ on() {}, close() {} }), +}); +if (typeof outcome === "string") process.kill(process.pid, outcome); +else process.exit(outcome); +`, + ); + const env: NodeJS.ProcessEnv = { + ...process.env, + HOME: path.join(fixtureRoot, "home"), + OPENCLAW_HOME: path.join(fixtureRoot, "home"), + OPENCLAW_STATE_DIR: path.join(fixtureRoot, "state"), + OPENCLAW_CONFIG_PATH: path.join(fixtureRoot, "state/openclaw.json"), + OPENCLAW_FORCE_BUILD: "1", + OPENCLAW_RUNNER_LOG: "0", + OPENCLAW_GATEWAY_WATCH_AUTO_DOCTOR: "1", + NODE_COMPILE_CACHE: path.join(fixtureRoot, "compile-cache"), + PNPM_CONFIG_MODULES_DIR: path.dirname( + path.dirname(createRequire(import.meta.url).resolve("tsx/package.json")), + ), + }; + delete env.NODE_OPTIONS; + delete env.NODE_DISABLE_COMPILE_CACHE; + delete env.OPENCLAW_COMPILE_CACHE_DISABLED_RESPAWNED; + let observedExit: { code: number | null; signal: NodeJS.Signals | null } | undefined; + const command = runNodeScript([watchWrapper, "gateway"], env, 10_000, { + cwd: checkoutRoot, + requireProcessTreeExit: true, + onReady(child) { + child.once("exit", (code, signal) => { + observedExit = { code, signal }; + }); + }, + }); + await runQaGatewayFixture( + async () => { + const childPid = await waitForPidFile(childPidPath, 5_000); + const launcherPid = await waitForPidFile(launcherPidPath, 5_000); + expect(childPid, "the launcher must respawn before the signal is sent").not.toBe( + launcherPid, + ); + process.kill(childPid, "SIGKILL"); + const result = await command; + expect(result.error, formatShimResult(result)).toBeUndefined(); + expect(observedExit, formatShimResult(result)).toEqual({ code: null, signal: "SIGKILL" }); + expect(result.status).toBe(137); + expect(readFileSync(invocationsPath, "utf8")).toBe('["gateway"]\n'); + }, + async () => { + // Release even a late-starting child, then join the inherited pipes and outer group. + writeFileSync(releasePath, "release"); + const result = await command; + if (result.error) { + throw toErrorObject(result.error, "Gateway watch command failed"); + } + }, + ).catch((error: unknown) => { + const failure = toErrorObject(error, "Gateway watch fixture failed"); + if (hasUnjoinedWork(failure)) { + // The shim fixture needs this marker at the top level to retain unjoined inputs. + Object.assign(failure, { processTreeState: "indeterminate" }); + } + throw failure; + }); + }); + }, +); + it.runIf(process.platform !== "win32").each(["runner", "watch"] as const)( "preserves native %s signal loss while a private-pipe worker survives", async (mode) => {