From 56ed8bca5d46e936fb7ac589783884869a1b9cb1 Mon Sep 17 00:00:00 2001 From: James Meyers Date: Sun, 2 Aug 2026 19:57:48 -0700 Subject: [PATCH] Exit when the Codex process dies --- .../e2e/acp-e2e-backend-death.test.ts | 62 +++++++++++++++++++ .../CodexACPAgent/e2e/acp-e2e-test-utils.ts | 5 ++ .../e2e/spawned-agent-fixture.ts | 12 ++++ src/index.ts | 41 ++++++++++++ 4 files changed, 120 insertions(+) create mode 100644 src/__tests__/CodexACPAgent/e2e/acp-e2e-backend-death.test.ts diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-backend-death.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-backend-death.test.ts new file mode 100644 index 00000000..82fe7f8b --- /dev/null +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-backend-death.test.ts @@ -0,0 +1,62 @@ +import {execSync} from "node:child_process"; +import {afterEach, expect, it} from "vitest"; +import {createUnauthenticatedFixture, describeE2E, type SpawnedAgentFixture} from "./acp-e2e-test-utils"; + +// The process walk uses `ps`, so the spec is POSIX-only; the behaviour under +// test is platform-independent (the exit hook listens for both child exit and +// stdout EOF). +describeE2E("E2E backend death", () => { + let fixture: SpawnedAgentFixture; + + afterEach(async () => { + await fixture?.dispose(); + }); + + it.skipIf(process.platform === "win32")( + "exits promptly when the codex backend dies", + async () => { + fixture = await createUnauthenticatedFixture(); + const agentPid = fixture.agentPid; + expect(agentPid).toBeDefined(); + + const backend = descendantsOf(agentPid as number).find( + (p) => /app-server/.test(p.command) && !/codex\.js/.test(p.command), + ); + expect(backend, "codex app-server process not found under the agent").toBeDefined(); + + process.kill((backend as ProcessRow).pid, "SIGKILL"); + + const exited = await fixture.waitForAgentExit(5_000); + expect(exited, "agent did not exit within 5s of its backend dying").toBe(true); + }, + ); +}); + +interface ProcessRow { + pid: number; + ppid: number; + command: string; +} + +function descendantsOf(rootPid: number): ProcessRow[] { + const out = execSync("ps -axo pid=,ppid=,command=", {encoding: "utf8"}); + const rows = out + .trim() + .split("\n") + .map((line): ProcessRow | null => { + const m = line.trim().match(/^(\d+)\s+(\d+)\s+(.*)$/); + return m ? {pid: Number(m[1]), ppid: Number(m[2]), command: m[3] ?? ""} : null; + }) + .filter((row): row is ProcessRow => row !== null); + const result: ProcessRow[] = []; + const walk = (parent: number): void => { + for (const row of rows) { + if (row.ppid === parent) { + result.push(row); + walk(row.pid); + } + } + }; + walk(rootPid); + return result; +} diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-test-utils.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-test-utils.ts index 35bb5f3a..e8753cbe 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-test-utils.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-test-utils.ts @@ -71,6 +71,11 @@ export async function createAuthenticatedFixture(initialMode?: AgentMode, mcpSer }, extraEnv, mcpServers); } +/** A fixture that only initializes — for tests that need no authentication. */ +export async function createUnauthenticatedFixture(): Promise { + return await createSpawnedFixture(async () => {}); +} + export async function createGatewayFixture( baseUrl: string, headers: Record, diff --git a/src/__tests__/CodexACPAgent/e2e/spawned-agent-fixture.ts b/src/__tests__/CodexACPAgent/e2e/spawned-agent-fixture.ts index d4edc609..f13b2ac8 100644 --- a/src/__tests__/CodexACPAgent/e2e/spawned-agent-fixture.ts +++ b/src/__tests__/CodexACPAgent/e2e/spawned-agent-fixture.ts @@ -21,6 +21,10 @@ export interface TestSkill { export interface SpawnedAgentFixture { readonly connection: acp.ClientSideConnection; readonly workspaceDir: string; + /** Pid of the spawned agent process, for tests that manage its process tree. */ + readonly agentPid: number | undefined; + /** Resolves true if the agent process exits within the timeout. */ + waitForAgentExit(timeoutMs: number): Promise; createSession(mcpServers?: acp.McpServer[]): Promise; restart(): Promise; writeSkill(skill: TestSkill, rootDir?: string): void; @@ -178,6 +182,14 @@ class SpawnedAgentFixtureImpl implements SpawnedAgentFixture { return this.paths.workspaceDir; } + get agentPid(): number | undefined { + return this.agentProcess.pid; + } + + async waitForAgentExit(timeoutMs: number): Promise { + return await waitForProcessExit(this.agentProcess, timeoutMs); + } + async createSession(mcpServers: acp.McpServer[] = []): Promise { return await this.connection.newSession({ cwd: this.workspaceDir, diff --git a/src/index.ts b/src/index.ts index 014801ff..5dd77d89 100644 --- a/src/index.ts +++ b/src/index.ts @@ -89,7 +89,9 @@ function startAcpServer() { stderr = (stderr + data.toString()).slice(-maxStderrTailChars); }); + let clientInitiatedShutdown = false; process.stdin.on("close", () => { + clientInitiatedShutdown = true; codexConnection.process.stdin.end(); // Kill the codex process if it doesn't exit naturally setTimeout(() => { @@ -100,6 +102,45 @@ function startAcpServer() { }, 2000); }); + // If the codex process dies, exit instead of leaving the client a + // connection that can never answer again; exiting surfaces the death as + // stdio EOF and rejects in-flight client requests at the transport layer. + // stdout EOF is watched too: the win32 spawn goes through a shell, so the + // child handle can outlive the real codex process. The diagnostic goes to + // stderr because clients surface an exited agent's stderr to the user. + let backendLossHandled = false; + const reportBackendLossAndExit = (reason: string) => { + const exitCode = codexConnection.process.exitCode; + const hint = exitCode === 3221225781 ? " — VC++ redistributable should be installed" : ""; + const stderrTail = stderr.trim(); + process.stderr.write( + `codex-acp: codex process died (${reason}, exit code ${exitCode ?? "unknown"})${hint}\n` + + (stderrTail ? stderrTail + "\n" : "") + ); + logger.log("Codex process lost; exiting", {reason: reason, exitCode: exitCode, stderrTail: stderr}); + // Grace period so stderr and any queued stdout responses flush. + setTimeout(() => process.exit(1), 50); + }; + const exitOnBackendLoss = (reason: string) => { + if (backendLossHandled || clientInitiatedShutdown) { + return; + } + backendLossHandled = true; + if (codexConnection.process.exitCode === null) { + // stdout EOF usually precedes the exit event that carries the exit + // code (and with it the VC++ hint) — give it a moment to arrive. + const fallback = setTimeout(() => reportBackendLossAndExit(reason), 150); + codexConnection.process.once("exit", () => { + clearTimeout(fallback); + reportBackendLossAndExit(reason); + }); + } else { + reportBackendLossAndExit(reason); + } + }; + codexConnection.process.on("exit", () => exitOnBackendLoss("process-exit")); + codexConnection.process.stdout.on("end", () => exitOnBackendLoss("stdout-eof")); + const acpJsonStream = createJsonStream(process.stdin, process.stdout); function createAgent(connection: acp.AgentContext): CodexAcpServer {