From 92442e743517cb4f1c304967bd79e2b9401a6ba6 Mon Sep 17 00:00:00 2001 From: Ivan Malison Date: Thu, 3 Sep 2026 15:50:26 -0700 Subject: [PATCH] fix(server): reject pending Codex app-server requests on dispose (#4283) The provider refresh deadline aborts a Codex catalog probe by disposing the app-server client, but dispose killed the child without settling in-flight requests. With the request fallback timeout at 14 days, an unanswered `initialize` left `fetchCatalog` pending forever, so the workspace snapshot entry stayed "loading" and the model picker showed "Loading" for every Codex profile with no timeout ever surfacing. Co-authored-by: Claude Fable 5.1 --- ...codex-app-server-agent.spawn-error.test.ts | 29 ++++++++++++++++++- .../codex/app-server-transport.test.ts | 10 +++++++ .../providers/codex/app-server-transport.ts | 15 ++++++---- 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.spawn-error.test.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.spawn-error.test.ts index bd1e0580f7..bff992e2ff 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.spawn-error.test.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.spawn-error.test.ts @@ -1,7 +1,9 @@ -import { describe, expect, test } from "vitest"; +import { describe, expect, test, vi } from "vitest"; import { CodexAppServerAgentClient } from "./codex-app-server-agent.js"; +import { runProviderRefreshWithDeadline } from "../provider-refresh-deadline.js"; import { createTestLogger } from "../../../test-utils/test-logger.js"; +import { createCodexAppServerChildProcess } from "./codex/test-utils/fake-app-server.js"; describe("CodexAppServerAgentClient spawn error handling", () => { const logger = createTestLogger(); @@ -32,6 +34,31 @@ describe("CodexAppServerAgentClient spawn error handling", () => { } }); + test("fetchCatalog settles when the refresh deadline aborts an unanswered initialize", async () => { + const client = new CodexAppServerAgentClient(logger, { + command: { + mode: "replace", + argv: ["/nonexistent/codex-binary-that-does-not-exist"], + }, + }); + const child = createCodexAppServerChildProcess(); + const kill = vi.spyOn(child, "kill"); + Object.assign(client, { spawnAppServer: async () => child }); + + await expect( + runProviderRefreshWithDeadline({ + label: "Codex", + timeoutMs: 50, + operation: (context) => + client.fetchCatalog( + { scope: "workspace", cwd: "/tmp/codex-models", force: false }, + context, + ), + }), + ).rejects.toThrow("Timed out refreshing Codex after 50ms"); + expect(kill).toHaveBeenCalled(); + }); + test("listImportableSessions rejects gracefully when the codex binary does not exist", async () => { const client = new CodexAppServerAgentClient(logger, { command: { diff --git a/packages/server/src/server/agent/providers/codex/app-server-transport.test.ts b/packages/server/src/server/agent/providers/codex/app-server-transport.test.ts index a6c5d2a8b2..1d7417510b 100644 --- a/packages/server/src/server/agent/providers/codex/app-server-transport.test.ts +++ b/packages/server/src/server/agent/providers/codex/app-server-transport.test.ts @@ -22,6 +22,16 @@ describe("Codex app-server transport", () => { child.stdin.end(); }); + test("dispose rejects pending requests instead of leaving them hanging", async () => { + const child = createCodexAppServerChildProcess(); + const client = new CodexAppServerClient(child, createTestLogger()); + + const request = client.request("initialize", {}); + await client.dispose(); + + await expect(request).rejects.toThrow("Codex app-server client is closed"); + }); + test.each([ "item/commandExecution/requestApproval", "item/fileChange/requestApproval", diff --git a/packages/server/src/server/agent/providers/codex/app-server-transport.ts b/packages/server/src/server/agent/providers/codex/app-server-transport.ts index e143cd6863..32152e7ea5 100644 --- a/packages/server/src/server/agent/providers/codex/app-server-transport.ts +++ b/packages/server/src/server/agent/providers/codex/app-server-transport.ts @@ -261,6 +261,7 @@ export class CodexAppServerClient { this.disposed = true; this.unexpectedTerminationHandler = null; this.rl.close(); + this.rejectPending(new Error("Codex app-server client is closed")); try { this.child.stdin.end(); } catch { @@ -290,11 +291,7 @@ export class CodexAppServerClient { } this.disposed = true; this.rl.close(); - for (const pending of this.pending.values()) { - clearTimeout(pending.timer); - pending.reject(error); - } - this.pending.clear(); + this.rejectPending(error); const handler = this.unexpectedTerminationHandler; this.unexpectedTerminationHandler = null; if (!handler) { @@ -307,6 +304,14 @@ export class CodexAppServerClient { } } + private rejectPending(error: Error): void { + for (const pending of this.pending.values()) { + clearTimeout(pending.timer); + pending.reject(error); + } + this.pending.clear(); + } + private writeJsonRpcResponse(response: JsonRpcResponse): void { if (this.disposed || this.child.stdin.destroyed || !this.child.stdin.writable) { return;