fix(codex): defer sandbox cleanup until process close (#103617)

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<typeof import("node:child_process")>();
+  return {
+    ...actual,
+    spawn: (...args: Parameters<typeof actual.spawn>) => 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<typeof vi.fn> };
+
+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<string, ManagedProcess>();
+
+    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",
+    });
+  });
+});

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 });

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);