Skip to content

Commit 5bc2fee

Browse files
steipetesteipete
committed
fix(codex): bound app-server timeout fallout
1 parent 1d972af commit 5bc2fee

17 files changed

Lines changed: 745 additions & 278 deletions

‎extensions/codex/src/app-server/client-factory.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,13 @@ export const defaultCodexAppServerClientFactory: CodexAppServerClientFactory = (
2222
import("./shared-client.js").then(({ getSharedCodexAppServerClient }) =>
2323
getSharedCodexAppServerClient({ startOptions, authProfileId, agentDir, config }),
2424
);
25+
26+
export const defaultLeasedCodexAppServerClientFactory: CodexAppServerClientFactory = (
27+
startOptions,
28+
authProfileId,
29+
agentDir,
30+
config,
31+
) =>
32+
import("./shared-client.js").then(({ getLeasedSharedCodexAppServerClient }) =>
33+
getLeasedSharedCodexAppServerClient({ startOptions, authProfileId, agentDir, config }),
34+
);

‎extensions/codex/src/app-server/models.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,13 @@ async function withCodexAppServerModelClient<T>(
7474
): Promise<T> {
7575
const timeoutMs = options.timeoutMs ?? 2500;
7676
const useSharedClient = options.sharedClient !== false;
77-
const { createIsolatedCodexAppServerClient, getSharedCodexAppServerClient } =
78-
await import("./shared-client.js");
77+
const {
78+
createIsolatedCodexAppServerClient,
79+
getLeasedSharedCodexAppServerClient,
80+
releaseLeasedSharedCodexAppServerClient,
81+
} = await import("./shared-client.js");
7982
const client = useSharedClient
80-
? await getSharedCodexAppServerClient({
83+
? await getLeasedSharedCodexAppServerClient({
8184
startOptions: options.startOptions,
8285
timeoutMs,
8386
authProfileId: options.authProfileId,
@@ -94,7 +97,9 @@ async function withCodexAppServerModelClient<T>(
9497
try {
9598
return await run({ client, timeoutMs });
9699
} finally {
97-
if (!useSharedClient) {
100+
if (useSharedClient) {
101+
releaseLeasedSharedCodexAppServerClient(client);
102+
} else {
98103
client.close();
99104
}
100105
}

‎extensions/codex/src/app-server/request.test.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@ const sharedClientMocks = vi.hoisted(() => ({
55
getSharedCodexAppServerClient: vi.fn(),
66
}));
77

8-
vi.mock("./shared-client.js", () => sharedClientMocks);
8+
vi.mock("./shared-client.js", () => ({
9+
...sharedClientMocks,
10+
getLeasedSharedCodexAppServerClient: sharedClientMocks.getSharedCodexAppServerClient,
11+
releaseLeasedSharedCodexAppServerClient: vi.fn(),
12+
}));
913

1014
const { requestCodexAppServerJson } = await import("./request.js");
1115

‎extensions/codex/src/app-server/request.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@ import type {
99
import { resolveCodexAppServerDirectSandboxBypassBlock } from "./sandbox-guard.js";
1010
import {
1111
createIsolatedCodexAppServerClient,
12-
getSharedCodexAppServerClient,
12+
getLeasedSharedCodexAppServerClient,
13+
releaseLeasedSharedCodexAppServerClient,
1314
} from "./shared-client.js";
1415
import { withTimeout } from "./timeout.js";
1516

@@ -63,7 +64,7 @@ export async function requestCodexAppServerJson<T = JsonValue | undefined>(param
6364
return await withTimeout(
6465
(async () => {
6566
const client = await (
66-
params.isolated ? createIsolatedCodexAppServerClient : getSharedCodexAppServerClient
67+
params.isolated ? createIsolatedCodexAppServerClient : getLeasedSharedCodexAppServerClient
6768
)({
6869
startOptions: params.startOptions,
6970
timeoutMs,
@@ -81,6 +82,8 @@ export async function requestCodexAppServerJson<T = JsonValue | undefined>(param
8182
// underlying codex binary, so the unref'd close() path can leave
8283
// the child running and keep the parent's event loop alive.
8384
await client.closeAndWait({ exitTimeoutMs: 2_000, forceKillDelayMs: 250 });
85+
} else {
86+
releaseLeasedSharedCodexAppServerClient(client);
8487
}
8588
}
8689
})(),

‎extensions/codex/src/app-server/run-attempt.test.ts‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4160,7 +4160,7 @@ describe("runCodexAppServerAttempt", () => {
41604160
});
41614161
});
41624162

4163-
it("interrupts but keeps the app-server client alive when a turn timeout fires", async () => {
4163+
it("unsubscribes and closes the app-server client when the active turn goes idle past the attempt timeout", async () => {
41644164
const close = vi.fn();
41654165
const request = vi.fn(async (method: string) => {
41664166
if (method === "thread/start") {
@@ -4204,7 +4204,14 @@ describe("runCodexAppServerAttempt", () => {
42044204
},
42054205
{ timeoutMs: 5_000 },
42064206
);
4207-
expect(close).not.toHaveBeenCalled();
4207+
expect(request).toHaveBeenCalledWith(
4208+
"thread/unsubscribe",
4209+
{
4210+
threadId: "thread-1",
4211+
},
4212+
{ timeoutMs: 5_000 },
4213+
);
4214+
expect(close).toHaveBeenCalledTimes(1);
42084215
expect(queueActiveRunMessageForTest("session-1", "after timeout")).toBe(false);
42094216
});
42104217

@@ -5562,9 +5569,13 @@ describe("runCodexAppServerAttempt", () => {
55625569
),
55635570
{ interval: 1 },
55645571
);
5565-
expect(warn).not.toHaveBeenCalledWith(
5572+
expect(warn).toHaveBeenCalledWith(
55665573
"codex app-server client retired after timed-out turn",
5567-
expect.anything(),
5574+
expect.objectContaining({
5575+
reason: "turn_completion_idle_timeout",
5576+
threadId: "thread-1",
5577+
turnId: "turn-1",
5578+
}),
55685579
);
55695580
});
55705581

0 commit comments

Comments
 (0)