diff --git a/extensions/codex/src/app-server/sandbox-exec-server.lifecycle.test.ts b/extensions/codex/src/app-server/sandbox-exec-server.lifecycle.test.ts new file mode 100644 index 000000000000..bfc297cbf07a --- /dev/null +++ b/extensions/codex/src/app-server/sandbox-exec-server.lifecycle.test.ts @@ -0,0 +1,248 @@ +// Codex tests cover sandbox exec-server child and backend lease lifecycle ordering. +import type { ChildProcessWithoutNullStreams } from "node:child_process"; +import { EventEmitter } from "node:events"; +import { PassThrough } from "node:stream"; +import type { SandboxContext } from "openclaw/plugin-sdk/sandbox"; +import type { WebSocket } from "ws"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +const spawnMock = vi.hoisted(() => vi.fn()); +vi.mock("node:child_process", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawn: (...args: Parameters) => spawnMock(...args), + }; +}); + +import { createSandboxContext } from "./sandbox-exec-server.test-helpers.js"; +import { httpRequest } from "./sandbox-exec-server/http.js"; +import { startProcess } from "./sandbox-exec-server/processes.js"; +import type { ManagedProcess, OpenClawExecServer } from "./sandbox-exec-server/types.js"; + +type FakeSocket = WebSocket & { send: ReturnType }; + +function createFakeChild(): ChildProcessWithoutNullStreams { + return Object.assign(new EventEmitter(), { + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + pid: 42_424, + kill: vi.fn(() => true), + }) as unknown as ChildProcessWithoutNullStreams; +} + +function createFakeSocket(): FakeSocket { + return Object.assign(new EventEmitter(), { + readyState: 1, + send: vi.fn(), + }) as unknown as FakeSocket; +} + +function createExecServer(sandbox: SandboxContext): OpenClawExecServer { + return { sandbox } as OpenClawExecServer; +} + +function processStartParams(processId: string) { + return { + processId, + argv: ["sh", "-lc", "true"], + cwd: "/workspace", + env: {}, + tty: false, + pipeStdin: false, + arg0: null, + }; +} + +function streamingHttpParams(requestId: string) { + return { + requestId, + method: "GET", + url: "https://example.test/sse", + streamResponse: true, + }; +} + +afterEach(() => { + spawnMock.mockReset(); +}); + +describe("Codex sandbox exec-server lifecycle", () => { + it("retains the process backend lease after child error until close", async () => { + const child = createFakeChild(); + spawnMock.mockReturnValue(child); + const finalizeExec = vi.fn(async () => undefined); + const sandbox = createSandboxContext({ + buildExecSpec: async () => ({ + argv: ["sandbox-child"], + env: {}, + finalizeToken: "process-token", + stdinMode: "pipe-closed", + }), + finalizeExec, + }); + const socket = createFakeSocket(); + const processes = new Map(); + + await startProcess( + createExecServer(sandbox), + processes, + socket, + processStartParams("process-error"), + ); + child.emit("error", new Error("child transport failed")); + + expect(child.pid).toBe(42_424); + expect(processes.get("process-error")).toMatchObject({ + closed: false, + exited: false, + failure: "child transport failed", + }); + expect(finalizeExec).not.toHaveBeenCalled(); + expect(socket.send).not.toHaveBeenCalled(); + + child.emit("close", 23, null); + await vi.waitFor(() => expect(finalizeExec).toHaveBeenCalledOnce()); + + expect(processes.get("process-error")).toMatchObject({ + closed: true, + exited: true, + exitCode: 23, + }); + expect(finalizeExec).toHaveBeenCalledWith({ + status: "failed", + exitCode: 23, + timedOut: false, + token: "process-token", + }); + expect( + socket.send.mock.calls.map(([payload]) => JSON.parse(String(payload)).method), + ).toEqual(["process/exited", "process/closed"]); + }); + + it.each([ + { label: "an empty exec spec", argv: [] as string[], spawnError: null }, + { label: "a synchronous spawn failure", argv: ["sandbox-child"], spawnError: "spawn failed" }, + ])("finalizes process tokens after $label", async ({ argv, spawnError }) => { + if (spawnError) { + spawnMock.mockImplementationOnce(() => { + throw new Error(spawnError); + }); + } + const finalizeExec = vi.fn(async () => undefined); + const sandbox = createSandboxContext({ + buildExecSpec: async () => ({ + argv, + env: {}, + finalizeToken: "process-start-token", + stdinMode: "pipe-closed", + }), + finalizeExec, + }); + + await expect( + startProcess( + createExecServer(sandbox), + new Map(), + createFakeSocket(), + processStartParams("process-start-failure"), + ), + ).rejects.toThrow(spawnError ?? "did not provide a command"); + expect(finalizeExec).toHaveBeenCalledOnce(); + expect(finalizeExec).toHaveBeenCalledWith({ + status: "failed", + exitCode: null, + timedOut: false, + token: "process-start-token", + }); + }); + + it("retains the streaming HTTP backend lease after child error until close", async () => { + const child = createFakeChild(); + spawnMock.mockReturnValue(child); + const finalizeExec = vi.fn(async () => undefined); + const sandbox = createSandboxContext({ + buildExecSpec: async () => ({ + argv: ["sandbox-http-child"], + env: {}, + finalizeToken: "http-token", + stdinMode: "pipe-closed", + }), + finalizeExec, + }); + const request = httpRequest( + createExecServer(sandbox), + createFakeSocket(), + streamingHttpParams("http-error"), + ); + let settled = false; + void request.then( + () => { + settled = true; + }, + () => { + settled = true; + }, + ); + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledOnce()); + + child.emit("error", new Error("HTTP child transport failed")); + await Promise.resolve(); + + expect(child.pid).toBe(42_424); + expect(settled).toBe(false); + expect(finalizeExec).not.toHaveBeenCalled(); + + const rejection = expect(request).rejects.toThrow("HTTP child transport failed"); + child.emit("close", 29, null); + await rejection; + await vi.waitFor(() => expect(finalizeExec).toHaveBeenCalledOnce()); + expect(finalizeExec).toHaveBeenCalledWith({ + status: "failed", + exitCode: 29, + timedOut: false, + token: "http-token", + }); + }); + + it.each([ + { label: "an empty exec spec", argv: [] as string[], spawnError: null }, + { + label: "a synchronous spawn failure", + argv: ["sandbox-http-child"], + spawnError: "HTTP spawn failed", + }, + ])("finalizes streaming HTTP tokens after $label", async ({ argv, spawnError }) => { + if (spawnError) { + spawnMock.mockImplementationOnce(() => { + throw new Error(spawnError); + }); + } + const finalizeExec = vi.fn(async () => undefined); + const sandbox = createSandboxContext({ + buildExecSpec: async () => ({ + argv, + env: {}, + finalizeToken: "http-start-token", + stdinMode: "pipe-closed", + }), + finalizeExec, + }); + + await expect( + httpRequest( + createExecServer(sandbox), + createFakeSocket(), + streamingHttpParams("http-start-failure"), + ), + ).rejects.toThrow(spawnError ?? "did not provide a command"); + expect(finalizeExec).toHaveBeenCalledOnce(); + expect(finalizeExec).toHaveBeenCalledWith({ + status: "failed", + exitCode: null, + timedOut: false, + token: "http-start-token", + }); + }); +}); diff --git a/extensions/codex/src/app-server/sandbox-exec-server/http.ts b/extensions/codex/src/app-server/sandbox-exec-server/http.ts index 7020209533c0..802ab571ccd3 100644 --- a/extensions/codex/src/app-server/sandbox-exec-server/http.ts +++ b/extensions/codex/src/app-server/sandbox-exec-server/http.ts @@ -116,15 +116,31 @@ async function runStreamingSandboxHttpRequest( env: {}, usePty: false, }); - const [command, ...args] = execSpec.argv; - if (!command) { - throw new Error("OpenClaw sandbox HTTP exec spec did not provide a command."); + let child: ChildProcessWithoutNullStreams; + try { + const [command, ...args] = execSpec.argv; + if (!command) { + throw new Error("OpenClaw sandbox HTTP exec spec did not provide a command."); + } + child = spawn(command, args, { + env: execSpec.env, + stdio: ["pipe", "pipe", "pipe"], + }); + } catch (error) { + try { + await backend.finalizeExec?.({ + status: "failed", + exitCode: null, + timedOut: false, + token: execSpec.finalizeToken, + }); + } catch (finalizeError) { + embeddedAgentLog.warn("codex sandbox http/request finalize after start failure failed", { + error: finalizeError, + }); + } + throw error; } - - const child = spawn(command, args, { - env: execSpec.env, - stdio: ["pipe", "pipe", "pipe"], - }); const abortOnSocketClose = () => child.kill("SIGTERM"); socket.once("close", abortOnSocketClose); child.once("close", () => { @@ -156,6 +172,7 @@ function readStreamingSandboxHttpResponse(params: { return new Promise((resolve, reject) => { let headerResolved = false; let failed = false; + let childFailure: string | null = null; let lastBodySeq = 0; let stdoutBuffer = ""; let stderr = ""; @@ -232,12 +249,20 @@ function readStreamingSandboxHttpResponse(params: { params.child.stderr.on("data", (chunk: Buffer) => { stderr = `${stderr}${chunk.toString("utf8")}`.slice(-4096); }); - params.child.once("error", (error) => fail(error.message, null)); + params.child.once("error", (error) => { + // ChildProcess error can precede close while the helper is still alive. + // Keep its backend lease until close provides the terminal exit state. + childFailure ??= error.message; + }); params.child.once("close", (code) => { const exitCode = code ?? 1; if (failed) { return; } + if (childFailure) { + fail(childFailure, exitCode); + return; + } if (exitCode === 0) { void finalize("completed", exitCode).catch((error: unknown) => { embeddedAgentLog.warn("codex sandbox http/request finalize failed", { error }); diff --git a/extensions/codex/src/app-server/sandbox-exec-server/processes.ts b/extensions/codex/src/app-server/sandbox-exec-server/processes.ts index 81824dbd6e20..f84ff06d1e83 100644 --- a/extensions/codex/src/app-server/sandbox-exec-server/processes.ts +++ b/extensions/codex/src/app-server/sandbox-exec-server/processes.ts @@ -2,7 +2,7 @@ * Manages subprocess lifecycle, streaming output buffers, stdin writes, and * termination for Codex sandbox exec-server process RPCs. */ -import { spawn } from "node:child_process"; +import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process"; import { embeddedAgentLog } from "openclaw/plugin-sdk/agent-harness-runtime"; import type { WebSocket } from "ws"; import type { JsonObject, JsonValue } from "../protocol.js"; @@ -99,19 +99,29 @@ async function runProcess( }); managed.finalizeToken = execSpec.finalizeToken; managed.finalizeExec = backend.finalizeExec; - if (managed.abortController.signal.aborted) { - managed.failure = "process start cancelled"; - await finalizeProcess(managed); - throw new Error("process start cancelled"); + let child: ChildProcessWithoutNullStreams; + try { + if (managed.abortController.signal.aborted) { + throw new Error("process start cancelled"); + } + const [command, ...args] = execSpec.argv; + if (!command) { + throw new Error("OpenClaw sandbox exec spec did not provide a command."); + } + child = spawn(command, args, { + env: execSpec.env, + stdio: ["pipe", "pipe", "pipe"], + }); + } catch (error) { + managed.failure = error instanceof Error ? error.message : String(error); + await finalizeProcess(managed).catch((finalizeError: unknown) => { + embeddedAgentLog.warn("codex sandbox exec-server finalize after start failure failed", { + processId: managed.processId, + error: finalizeError instanceof Error ? finalizeError.message : String(finalizeError), + }); + }); + throw error; } - const [command, ...args] = execSpec.argv; - if (!command) { - throw new Error("OpenClaw sandbox exec spec did not provide a command."); - } - const child = spawn(command, args, { - env: execSpec.env, - stdio: ["pipe", "pipe", "pipe"], - }); managed.child = child; const abortListener = () => child.kill("SIGTERM"); managed.abortController.signal.addEventListener("abort", abortListener, { once: true }); @@ -120,8 +130,10 @@ async function runProcess( ); child.stderr.on("data", (chunk: Buffer) => appendProcessChunk(managed, "stderr", chunk)); child.once("error", (error) => { - managed.failure = error.message; - emitProcessClosed(managed, null); + // Node can report an abort or transport error before the child exits. The + // backend lease and Codex terminal notifications stay owned until close. + managed.failure ??= error.message; + notifyProcessWaiters(managed); }); child.once("close", (code) => { managed.abortController.signal.removeEventListener("abort", abortListener);