From d198c68b9ebb2ee86ef33ad0cdeeaa790eaa8062 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Tue, 12 May 2026 17:50:31 +0800 Subject: [PATCH] Fail Codex resume requests explicitly (#947) --- .../providers/codex-app-server-agent.test.ts | 106 ++++++++++++++++++ .../agent/providers/codex-app-server-agent.ts | 7 +- .../src/server/daemon-client.e2e.test.ts | 70 +++++++++++- packages/server/src/server/session.ts | 39 ++++++- 4 files changed, 215 insertions(+), 7 deletions(-) diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts index b282e409d..8981cba94 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts @@ -35,6 +35,7 @@ interface CollaborationModeRecord { } interface CodexSessionTestAccess { + ensureThreadLoaded(): Promise; handleToolApprovalRequest(params: unknown): Promise; handleNotification(method: string, params: unknown): void; loadPersistedHistory(): Promise; @@ -378,6 +379,80 @@ describe("Codex app-server provider", () => { }); }); + test("resumeSession does not replace a persisted Codex thread when app-server resume fails", async () => { + const threadRequests: string[] = []; + const appServer = createFakeCodexAppServer({ + "thread/loaded/list": () => { + threadRequests.push("thread/loaded/list"); + return { data: [] }; + }, + "thread/resume": () => { + threadRequests.push("thread/resume"); + return Promise.reject(new Error("no rollout found for thread id archived-thread-id")); + }, + "thread/start": () => { + threadRequests.push("thread/start"); + return { thread: { id: "replacement-empty-thread-id" } }; + }, + "thread/read": () => { + threadRequests.push("thread/read"); + return { thread: { turns: [] } }; + }, + getUserSavedConfig: () => { + threadRequests.push("getUserSavedConfig"); + return { config: {} }; + }, + "config/read": () => { + threadRequests.push("config/read"); + return { config: {} }; + }, + "model/list": () => { + threadRequests.push("model/list"); + return { + data: [{ id: "gpt-5.4", isDefault: true, defaultReasoningEffort: "medium" }], + }; + }, + }); + const provider = new CodexAppServerAgentClient(createTestLogger()); + castInternals<{ goalsEnabledPromise: Promise | null }>(provider).goalsEnabledPromise = + Promise.resolve(false); + castInternals<{ spawnAppServer: () => Promise }>( + provider, + ).spawnAppServer = async () => appServer.child; + + const outcome = await Promise.race([ + provider + .resumeSession({ + sessionId: "archived-thread-id", + metadata: { + cwd: "/tmp/codex-question-test", + modeId: "auto", + model: "gpt-5.4", + }, + }) + .then( + () => "resolved" as const, + (error) => { + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toContain( + "no rollout found for thread id archived-thread-id", + ); + return "rejected" as const; + }, + ), + new Promise<"timed_out">((resolve) => setTimeout(() => resolve("timed_out"), 500)), + ]); + + if (outcome === "timed_out") { + appServer.child.kill("SIGTERM"); + throw new Error(`resumeSession timed out; thread requests: ${threadRequests.join(", ")}`); + } + + expect(threadRequests).toEqual(["thread/loaded/list", "thread/resume"]); + expect(outcome).toBe("rejected"); + appServer.assertNoErrors(); + }); + test("lists repo skills using WorkspaceGitService repo-root resolution", async () => { const tempDir = await mkdtemp(path.join(tmpdir(), "codex-skills-")); const cwd = path.join(tempDir, "repo", "packages", "app"); @@ -1134,6 +1209,37 @@ describe("Codex app-server provider", () => { ]); }); + test("does not replace a persisted Codex thread when app-server resume fails", async () => { + const session = createSession({ thinkingOptionId: "medium" }); + session.currentThreadId = "archived-thread-id"; + const requests: Array<{ method: string; params: unknown }> = []; + session.client = { + request: vi.fn(async (method: string, params: unknown) => { + requests.push({ method, params }); + if (method === "thread/loaded/list") { + return { data: [] }; + } + if (method === "thread/resume") { + throw new Error("no rollout found for thread id archived-thread-id"); + } + if (method === "thread/start") { + return { thread: { id: "replacement-empty-thread-id" } }; + } + return {}; + }), + }; + + await expect(asInternals(session).ensureThreadLoaded()).rejects.toThrow( + "no rollout found for thread id archived-thread-id", + ); + + expect(session.currentThreadId).toBe("archived-thread-id"); + expect(requests).toEqual([ + { method: "thread/loaded/list", params: {} }, + { method: "thread/resume", params: { threadId: "archived-thread-id" } }, + ]); + }); + test("appends blank-line spacing to /goal status messages", async () => { const requests: Array<{ method: string; params: unknown }> = []; const session = createSession({}, { goalsEnabled: true }); diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.ts index 501f84e40..3c30f581c 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.ts @@ -2987,9 +2987,10 @@ class CodexAppServerAgentSession implements AgentSession { } await this.client.request("thread/resume", params); } catch (error) { - this.logger.warn({ error }, "Failed to resume Codex thread, starting new thread"); - this.currentThreadId = null; - await this.ensureThread(); + const threadId = this.currentThreadId; + const message = error instanceof Error ? error.message : String(error); + this.logger.warn({ error, threadId }, "Failed to resume persisted Codex thread"); + throw new Error(`Failed to resume Codex thread ${threadId}: ${message}`, { cause: error }); } } diff --git a/packages/server/src/server/daemon-client.e2e.test.ts b/packages/server/src/server/daemon-client.e2e.test.ts index 6243acdb7..5cba8db89 100644 --- a/packages/server/src/server/daemon-client.e2e.test.ts +++ b/packages/server/src/server/daemon-client.e2e.test.ts @@ -151,7 +151,7 @@ function waitForSignal( class NonPersistentReloadSession implements AgentSession { readonly provider = "claude" as const; - readonly id = null; + readonly id: string | null; readonly capabilities = { supportsStreaming: false, supportsSessionPersistence: true, @@ -161,7 +161,12 @@ class NonPersistentReloadSession implements AgentSession { supportsToolInvocations: false, } as const; - constructor(private readonly onClose: () => void) {} + constructor( + private readonly onClose: () => void, + id: string | null = null, + ) { + this.id = id; + } async run(): Promise { return { @@ -259,6 +264,37 @@ class NonPersistentReloadClient implements AgentClient { } } +class FailingResumeSession extends NonPersistentReloadSession { + constructor(onClose: () => void) { + super(onClose, "failing-resume-session"); + } + + describePersistence(): AgentPersistenceHandle | null { + return { + provider: "claude", + sessionId: this.id, + metadata: { cwd: process.cwd() }, + }; + } +} + +class FailingResumeClient extends NonPersistentReloadClient { + async createSession(_config: AgentSessionConfig): Promise { + this.createSessionCalls += 1; + return new FailingResumeSession(() => { + this.closeCalls += 1; + }); + } + + async resumeSession( + _handle: AgentPersistenceHandle, + _overrides?: Partial, + ): Promise { + this.resumeSessionCalls += 1; + throw new Error("resume exploded"); + } +} + function resolveSpeechConfig() { if (hasLocalSpeech) { return { @@ -482,6 +518,36 @@ test("refresh_agent rebuilds a live agent even when it has no persistence handle } }); +test("refresh_agent rejects when persisted session resume fails", async () => { + const cwd = tmpCwd(); + const client = new FailingResumeClient(); + const localCtx = await createDaemonTestContext({ + agentClients: { + claude: client, + }, + }); + + try { + const created = await localCtx.client.createAgent({ + config: { + provider: "claude", + cwd, + }, + }); + await localCtx.client.archiveAgent(created.id); + + await expect(localCtx.client.refreshAgent(created.id)).rejects.toMatchObject({ + name: "DaemonRpcError", + code: "agent_refresh_failed", + requestType: "refresh_agent_request", + }); + expect(client.resumeSessionCalls).toBe(1); + } finally { + await localCtx.cleanup(); + rmSync(cwd, { recursive: true, force: true }); + } +}); + test("resume_agent auto-unarchives archived agents", async () => { const cwd = tmpCwd(); try { diff --git a/packages/server/src/server/session.ts b/packages/server/src/server/session.ts index 41ab862b7..ae81e9e01 100644 --- a/packages/server/src/server/session.ts +++ b/packages/server/src/server/session.ts @@ -3085,6 +3085,17 @@ export class Session { const { handle, overrides, requestId } = msg; if (!handle) { this.sessionLogger.warn("Resume request missing persistence handle"); + if (requestId) { + this.emit({ + type: "rpc_error", + payload: { + requestId, + requestType: msg.type, + error: "Unable to resume agent: missing persistence handle", + code: "agent_resume_failed", + }, + }); + } this.emit({ type: "activity_log", payload: { @@ -3121,14 +3132,26 @@ export class Session { }); } } catch (error) { + const message = getErrorMessage(error); this.sessionLogger.error({ err: error }, "Failed to resume agent"); + if (requestId) { + this.emit({ + type: "rpc_error", + payload: { + requestId, + requestType: msg.type, + error: message, + code: "agent_resume_failed", + }, + }); + } this.emit({ type: "activity_log", payload: { id: uuidv4(), timestamp: new Date(), type: "error", - content: `Failed to resume agent: ${getErrorMessage(error)}`, + content: `Failed to resume agent: ${message}`, }, }); } @@ -3249,14 +3272,26 @@ export class Session { }); } } catch (error) { + const message = getErrorMessage(error); this.sessionLogger.error({ err: error, agentId }, `Failed to refresh agent ${agentId}`); + if (requestId) { + this.emit({ + type: "rpc_error", + payload: { + requestId, + requestType: msg.type, + error: message, + code: "agent_refresh_failed", + }, + }); + } this.emit({ type: "activity_log", payload: { id: uuidv4(), timestamp: new Date(), type: "error", - content: `Failed to refresh agent: ${getErrorMessage(error)}`, + content: `Failed to refresh agent: ${message}`, }, }); }