diff --git a/src/fleet/containers.runtime.test.ts b/src/fleet/containers.runtime.test.ts index f7b1b35f0605..3affb5578357 100644 --- a/src/fleet/containers.runtime.test.ts +++ b/src/fleet/containers.runtime.test.ts @@ -1,6 +1,14 @@ +import type { ChildProcess } from "node:child_process"; +import { EventEmitter } from "node:events"; import fs from "node:fs/promises"; import { beforeEach, describe, expect, it, vi } from "vitest"; +const spawnMock = vi.hoisted(() => vi.fn()); + +vi.mock("node:child_process", () => ({ + spawn: spawnMock, +})); + const profileMocks = vi.hoisted(() => ({ buildCellRunArgs: vi.fn((_profile: unknown, options: { environmentFile: string }) => [ "run", @@ -34,6 +42,23 @@ function successfulExecutor() { })); } +function createStreamChild(pid?: number): ChildProcess { + const stdout = Object.assign(new EventEmitter(), { + pause: vi.fn(), + resume: vi.fn(), + }); + const stderr = Object.assign(new EventEmitter(), { + pause: vi.fn(), + resume: vi.fn(), + }); + return Object.assign(new EventEmitter(), { + pid, + stdout, + stderr, + kill: vi.fn(() => true), + }) as unknown as ChildProcess; +} + describe("fleet container runtime", () => { beforeEach(() => { vi.clearAllMocks(); @@ -464,6 +489,39 @@ describe("fleet container runtime", () => { ).rejects.toThrow(/podman logs failed with signal SIGSEGV/iu); }); + it("rejects a log stream spawn error when the child has no pid", async () => { + const child = createStreamChild(); + spawnMock.mockReturnValue(child); + const runtime = createFleetContainerRuntime(successfulExecutor()); + + const logs = runtime.logs("podman", "cell-acme", { follow: true, redactValues: [] }); + child.emit("error", new Error("spawn failed")); + + await expect(logs).rejects.toThrow("spawn failed"); + child.emit("close", -2, null); + }); + + it("waits for log stream close across repeated operational errors", async () => { + const child = createStreamChild(4242); + spawnMock.mockReturnValue(child); + const runtime = createFleetContainerRuntime(successfulExecutor()); + let settled = false; + + const logs = runtime + .logs("podman", "cell-acme", { follow: true, redactValues: [] }) + .finally(() => { + settled = true; + }); + child.emit("error", new Error("first signal delivery failed")); + child.emit("error", new Error("second signal delivery failed")); + await Promise.resolve(); + + expect(settled).toBe(false); + + child.emit("close", 0, null); + await expect(logs).resolves.toBeUndefined(); + }); + it("creates and removes a labeled per-cell network", async () => { const executor = successfulExecutor(); const runtime = createFleetContainerRuntime(executor); diff --git a/src/fleet/containers.runtime.ts b/src/fleet/containers.runtime.ts index bdc0464deaea..d46cb7a9ffad 100644 --- a/src/fleet/containers.runtime.ts +++ b/src/fleet/containers.runtime.ts @@ -521,7 +521,11 @@ const defaultFleetContainerStreamExecutor: FleetContainerStreamExecutor = ( }; pipeWithBackpressure(child.stdout, process.stdout, stdout); pipeWithBackpressure(child.stderr, process.stderr, stderr); - child.once("error", reject); + child.on("error", (error) => { + if (child.pid === undefined) { + reject(error); + } + }); child.once("close", (code, signal) => { stdout.flush(); stderr.flush(); diff --git a/src/process/child-process-bridge.test.ts b/src/process/child-process-bridge.test.ts new file mode 100644 index 000000000000..fa1e6234381b --- /dev/null +++ b/src/process/child-process-bridge.test.ts @@ -0,0 +1,39 @@ +import type { ChildProcess } from "node:child_process"; +import { EventEmitter } from "node:events"; +import { describe, expect, it, vi } from "vitest"; +import { attachChildProcessBridge } from "./child-process-bridge.js"; + +describe("attachChildProcessBridge", () => { + it.each(["exit", "close"] as const)( + "keeps forwarding after operational errors until child %s", + (terminalEvent) => { + const signal: NodeJS.Signals = "SIGTERM"; + const existingListeners = new Set(process.listeners(signal)); + const kill = vi.fn(() => true); + const child = Object.assign(new EventEmitter(), { + pid: 4242, + kill, + }) as unknown as ChildProcess; + child.on("error", () => {}); + + const { detach } = attachChildProcessBridge(child, { signals: [signal] }); + const signalListener = process + .listeners(signal) + .find((listener) => !existingListeners.has(listener)); + + try { + expect(signalListener).toBeDefined(); + child.emit("error", new Error("signal delivery failed")); + expect(process.listeners(signal)).toContain(signalListener); + + signalListener?.(signal); + expect(kill).toHaveBeenCalledWith(signal); + + child.emit(terminalEvent, 0, null); + expect(process.listeners(signal)).not.toContain(signalListener); + } finally { + detach(); + } + }, + ); +}); diff --git a/src/process/child-process-bridge.ts b/src/process/child-process-bridge.ts index 8aa5c699a732..80effd659e7c 100644 --- a/src/process/child-process-bridge.ts +++ b/src/process/child-process-bridge.ts @@ -13,7 +13,7 @@ const defaultSignals: NodeJS.Signals[] = ? ["SIGTERM", "SIGINT", "SIGBREAK"] : ["SIGTERM", "SIGINT", "SIGHUP", "SIGQUIT"]; -/** Forwards process termination signals to a child and detaches on child exit/error. */ +/** Forwards process termination signals to a child and detaches on terminal lifecycle events. */ export function attachChildProcessBridge( child: ChildProcess, { signals = defaultSignals, onSignal }: ChildProcessBridgeOptions = {}, @@ -43,8 +43,10 @@ export function attachChildProcessBridge( listeners.clear(); }; + // Child errors can report failed signal/IPC operations while the PID stays live. + // Keep forwarding until exit, with close covering failed spawn and final handle cleanup. child.once("exit", detach); - child.once("error", detach); + child.once("close", detach); return { detach }; } diff --git a/src/process/respawn-child-runner.test.ts b/src/process/respawn-child-runner.test.ts index 2f6618107817..e7401b2c0669 100644 --- a/src/process/respawn-child-runner.test.ts +++ b/src/process/respawn-child-runner.test.ts @@ -205,4 +205,68 @@ describe("runRespawnChildWithSignalBridge", () => { }, ); }); + + it("settles a spawn error when the child has no pid", () => { + const { child } = createChild(); + const onError = vi.fn(); + const exit = vi.fn(); + + runRespawnChildWithSignalBridge({ + command: "missing-command", + args: [], + env: {}, + runtime: { + spawn: vi.fn(() => child) as unknown as typeof spawn, + attachChildProcessBridge: vi.fn(), + exit: exit as unknown as (code?: number) => never, + }, + onError, + }); + + const error = new Error("spawn failed"); + child.emit("error", error); + + expect(onError).toHaveBeenCalledWith(error); + expect(exit).toHaveBeenCalledWith(1); + }); + + it("keeps escalation active across repeated operational errors", () => { + vi.useFakeTimers(); + const { child, kill } = createChild(5678); + const onError = vi.fn(); + const exit = vi.fn(); + let onSignal: ((signal: NodeJS.Signals) => void) | undefined; + + try { + runRespawnChildWithSignalBridge({ + command: "/usr/bin/node", + args: ["/repo/openclaw/dist/entry.js"], + env: {}, + runtime: { + spawn: vi.fn(() => child) as unknown as typeof spawn, + attachChildProcessBridge: vi.fn((_child, options) => { + onSignal = options?.onSignal; + return { detach: vi.fn() }; + }), + exit: exit as unknown as (code?: number) => never, + }, + onError, + }); + + onSignal?.("SIGTERM"); + child.emit("error", new Error("first signal delivery failed")); + child.emit("error", new Error("second signal delivery failed")); + vi.advanceTimersByTime(2_000); + + expect(onError).not.toHaveBeenCalled(); + expect(exit).not.toHaveBeenCalled(); + expect(kill).toHaveBeenNthCalledWith(1, "SIGTERM"); + expect(kill).toHaveBeenNthCalledWith(2, process.platform === "win32" ? "SIGTERM" : "SIGKILL"); + + child.emit("exit", null, "SIGKILL"); + expect(exit).toHaveBeenCalledWith(1); + } finally { + vi.useRealTimers(); + } + }); }); diff --git a/src/process/respawn-child-runner.ts b/src/process/respawn-child-runner.ts index dc634be2f25f..ae0d108df83a 100644 --- a/src/process/respawn-child-runner.ts +++ b/src/process/respawn-child-runner.ts @@ -120,7 +120,10 @@ export function runRespawnChildWithSignalBridge(params: { runtime.exit(code ?? 1); }); - child.once("error", (error) => { + child.on("error", (error) => { + if (child.pid !== undefined) { + return; + } clearSignalTimers(); onError(error); runtime.exit(1); diff --git a/src/tui/tui-launch.test.ts b/src/tui/tui-launch.test.ts index 6bd2bbd92d70..7d15f2ca9a06 100644 --- a/src/tui/tui-launch.test.ts +++ b/src/tui/tui-launch.test.ts @@ -21,8 +21,8 @@ import { launchTuiCli } from "./tui-launch.js"; const originalArgv = [...process.argv]; const originalExecArgv = [...process.execArgv]; -function createChildProcess(): ChildProcess { - return new EventEmitter() as ChildProcess; +function createChildProcess(pid?: number): ChildProcess { + return Object.assign(new EventEmitter(), { pid }) as ChildProcess; } function expectSpawned(expectedArgs: string[]): SpawnOptions { @@ -187,4 +187,37 @@ describe("launchTuiCli", () => { ]); expect(options.env).toBe(process.env); }); + + it("rejects a spawn error when the child has no pid", async () => { + const child = createChildProcess(); + spawnMock.mockImplementation(() => { + queueMicrotask(() => child.emit("error", new Error("spawn failed"))); + return child; + }); + + await expect(launchTuiCli({ deliver: false })).rejects.toThrow( + "failed to launch TUI: spawn failed", + ); + expect(detachMock).toHaveBeenCalledOnce(); + }); + + it("waits for terminal exit across repeated operational errors", async () => { + const child = createChildProcess(4242); + spawnMock.mockReturnValue(child); + let settled = false; + + const launched = launchTuiCli({ deliver: false }).finally(() => { + settled = true; + }); + child.emit("error", new Error("first signal delivery failed")); + child.emit("error", new Error("second signal delivery failed")); + await Promise.resolve(); + + expect(settled).toBe(false); + expect(detachMock).not.toHaveBeenCalled(); + + child.emit("exit", 0, null); + await expect(launched).resolves.toBeUndefined(); + expect(detachMock).toHaveBeenCalledOnce(); + }); }); diff --git a/src/tui/tui-launch.ts b/src/tui/tui-launch.ts index 1e59f72177aa..f4fbf67ae8d5 100644 --- a/src/tui/tui-launch.ts +++ b/src/tui/tui-launch.ts @@ -59,7 +59,10 @@ export async function launchTuiCli(opts: TuiOptions): Promise { }); const { detach } = attachChildProcessBridge(child); - child.once("error", (error) => { + child.on("error", (error) => { + if (child.pid !== undefined) { + return; + } detach(); reject(new Error(`failed to launch TUI: ${formatErrorMessage(error)}`)); });