Skip to content

Commit 745cfc0

Browse files
zhangguiping-xydtsteipete
zhangguiping-xydt
and
steipete
authored
fix(codex): bound shared app-server startup waits (#89442)
* fix(codex): isolated cron reports Codex startup stalls * fix(codex): isolated cron reports Codex startup stalls * fix(codex): bound shared app-server startup waits Co-authored-by: 张贵萍0668001030 <[email protected]> * test(codex): satisfy startup lifecycle checks * test(codex): align deferred auth test type * test(codex): type auth mock as async void --------- Co-authored-by: Peter Steinberger <[email protected]>
1 parent 2d1a8bd commit 745cfc0

4 files changed

Lines changed: 298 additions & 45 deletions

File tree

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

Lines changed: 150 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,10 @@ import { type CodexPluginConfig, resolveCodexAppServerRuntimeOptions } from "./c
1414
import { testCodexAppServerBindingStore } from "./session-binding.test-helpers.js";
1515
import {
1616
clearSharedCodexAppServerClient,
17+
clearSharedCodexAppServerClientAndWait,
1718
getLeasedSharedCodexAppServerClient,
1819
releaseLeasedSharedCodexAppServerClient,
20+
type CodexAppServerClientFactory,
1921
} from "./shared-client.js";
2022
import { createClientHarness, createCodexTestModel } from "./test-support.js";
2123

@@ -85,19 +87,18 @@ function startThreadWithHarness(
8587
signal = new AbortController().signal,
8688
overrides?: {
8789
pluginConfig?: CodexPluginConfig;
88-
attemptClientFactory?: (
89-
harness: ClientHarness,
90-
) => Parameters<typeof startCodexAttemptThread>[0]["attemptClientFactory"];
90+
attemptClientFactory?: (harness: ClientHarness) => CodexAppServerClientFactory;
91+
buildAttemptParams?: () => EmbeddedRunAttemptParams;
9192
harness?: ClientHarness;
9293
paths?: AttemptPaths;
9394
skipStartSpy?: boolean;
9495
},
9596
) {
9697
const harness = overrides?.harness ?? createClientHarness();
9798
const paths = overrides?.paths ?? createAttemptPaths();
98-
if (!overrides?.skipStartSpy) {
99-
vi.spyOn(CodexAppServerClient, "start").mockReturnValue(harness.client);
100-
}
99+
const startSpy = overrides?.skipStartSpy
100+
? undefined
101+
: vi.spyOn(CodexAppServerClient, "start").mockReturnValue(harness.client);
101102
const effectivePluginConfig = overrides?.pluginConfig ?? pluginConfig;
102103

103104
const run = startCodexAttemptThread({
@@ -112,7 +113,7 @@ function startThreadWithHarness(
112113
startupEnvApiKeyCacheKey: undefined,
113114
agentDir: paths.agentDir,
114115
config: undefined,
115-
buildAttemptParams: () => createAttemptParams(paths),
116+
buildAttemptParams: overrides?.buildAttemptParams ?? (() => createAttemptParams(paths)),
116117
sessionAgentId: "agent-1",
117118
effectiveWorkspace: paths.workspaceDir,
118119
effectiveCwd: paths.cwd,
@@ -132,7 +133,7 @@ function startThreadWithHarness(
132133
spawnedBy: undefined,
133134
});
134135

135-
return { harness, run };
136+
return { harness, run, startSpy };
136137
}
137138

138139
async function answerInitialize(harness: ClientHarness): Promise<void> {
@@ -166,6 +167,15 @@ async function waitForThreadStart(harness: ClientHarness): Promise<{ id?: number
166167
return waitForRequest(harness, "thread/start");
167168
}
168169

170+
function isProcessAlive(pid: number): boolean {
171+
try {
172+
process.kill(pid, 0);
173+
return true;
174+
} catch {
175+
return false;
176+
}
177+
}
178+
169179
describe("startCodexAttemptThread", () => {
170180
beforeEach(() => {
171181
vi.useRealTimers();
@@ -291,7 +301,13 @@ describe("startCodexAttemptThread", () => {
291301
});
292302

293303
it("closes the shared app-server when startup times out during initialize", async () => {
294-
const { harness, run } = startThreadWithHarness(500);
304+
const initializeTimeoutPluginConfig = {
305+
...pluginConfig,
306+
appServer: { command: "codex", requestTimeoutMs: 100 },
307+
} satisfies CodexPluginConfig;
308+
const { harness, run } = startThreadWithHarness(500, new AbortController().signal, {
309+
pluginConfig: initializeTimeoutPluginConfig,
310+
});
295311
const runError = run.then(
296312
() => undefined,
297313
(error: unknown) => error,
@@ -302,7 +318,7 @@ describe("startCodexAttemptThread", () => {
302318

303319
const error = await runError;
304320
expect(error).toBeInstanceOf(Error);
305-
expect((error as Error).message).toBe("codex app-server startup timed out");
321+
expect((error as Error).message).toBe("codex app-server initialize timed out");
306322
await vi.waitFor(() => expect(harness.stdinDestroyed).toBe(true), {
307323
interval: 1,
308324
timeout: 1_000,
@@ -312,20 +328,142 @@ describe("startCodexAttemptThread", () => {
312328
).toBe(false);
313329
});
314330

331+
it("does not retire shared startup when this attempt's initialize wait expires", async () => {
332+
const sharedInitializePluginConfig = {
333+
...pluginConfig,
334+
appServer: { command: "codex", requestTimeoutMs: 100 },
335+
} satisfies CodexPluginConfig;
336+
const appServer = resolveCodexAppServerRuntimeOptions({
337+
pluginConfig: sharedInitializePluginConfig,
338+
});
339+
const paths = createAttemptPaths();
340+
const { harness, run, startSpy } = startThreadWithHarness(1_000, new AbortController().signal, {
341+
pluginConfig: sharedInitializePluginConfig,
342+
paths,
343+
});
344+
await waitForRequest(harness, "initialize");
345+
const peerAcquire = getLeasedSharedCodexAppServerClient({
346+
startOptions: appServer.start,
347+
agentDir: paths.agentDir,
348+
timeoutMs: 1_000,
349+
});
350+
351+
await expect(run).rejects.toThrow("codex app-server initialize timed out");
352+
expect(harness.stdinDestroyed).toBe(false);
353+
await answerInitialize(harness);
354+
await expect(peerAcquire).resolves.toBe(harness.client);
355+
await expect(
356+
getLeasedSharedCodexAppServerClient({
357+
startOptions: appServer.start,
358+
agentDir: paths.agentDir,
359+
timeoutMs: 1_000,
360+
}),
361+
).resolves.toBe(harness.client);
362+
expect(startSpy).toHaveBeenCalledTimes(1);
363+
expect(releaseLeasedSharedCodexAppServerClient(harness.client)).toBe(true);
364+
expect(releaseLeasedSharedCodexAppServerClient(harness.client)).toBe(true);
365+
});
366+
367+
it("bounds a real stdio initialize request and cleans up the child", async () => {
368+
const paths = createAttemptPaths();
369+
const root = path.dirname(paths.agentDir);
370+
const fixturePath = path.join(root, "stall-initialize.mjs");
371+
const requestLogPath = path.join(root, "requests.log");
372+
const pidPath = path.join(root, "child.pid");
373+
await fs.mkdir(root, { recursive: true });
374+
await fs.writeFile(
375+
fixturePath,
376+
[
377+
'import fs from "node:fs";',
378+
'import readline from "node:readline";',
379+
"const [requestLogPath, pidPath] = process.argv.slice(2);",
380+
'fs.writeFileSync(pidPath, String(process.pid), "utf8");',
381+
"const lines = readline.createInterface({ input: process.stdin });",
382+
'lines.on("line", (line) => {',
383+
" const message = JSON.parse(line);",
384+
' fs.appendFileSync(requestLogPath, `${String(message.method)}\\n`, "utf8");',
385+
"});",
386+
"setInterval(() => undefined, 1000);",
387+
].join("\n"),
388+
"utf8",
389+
);
390+
const stdioPluginConfig = {
391+
appServer: {
392+
transport: "stdio",
393+
command: process.execPath,
394+
args: [fixturePath, requestLogPath, pidPath],
395+
requestTimeoutMs: 2_000,
396+
},
397+
} satisfies CodexPluginConfig;
398+
let childPid: number | undefined;
399+
400+
try {
401+
const { run } = startThreadWithHarness(5_000, new AbortController().signal, {
402+
pluginConfig: stdioPluginConfig,
403+
paths,
404+
skipStartSpy: true,
405+
});
406+
407+
await expect(run).rejects.toThrow("codex app-server initialize timed out");
408+
409+
const requestMethods = (await fs.readFile(requestLogPath, "utf8")).trim().split(/\r?\n/u);
410+
expect(requestMethods).toEqual(["initialize"]);
411+
childPid = Number.parseInt(await fs.readFile(pidPath, "utf8"), 10);
412+
expect(childPid).toBeGreaterThan(0);
413+
const observedPid = childPid;
414+
await vi.waitFor(() => expect(isProcessAlive(observedPid)).toBe(false), {
415+
interval: 25,
416+
timeout: 3_000,
417+
});
418+
} finally {
419+
await clearSharedCodexAppServerClientAndWait({
420+
exitTimeoutMs: 3_000,
421+
forceKillDelayMs: 100,
422+
});
423+
if (childPid && isProcessAlive(childPid)) {
424+
try {
425+
process.kill(childPid, "SIGKILL");
426+
} catch {
427+
// The child can exit between the liveness probe and fallback kill.
428+
}
429+
}
430+
}
431+
});
432+
433+
it("cleans up a client surfaced by a factory that later rejects", async () => {
434+
const { harness, run } = startThreadWithHarness(5_000, new AbortController().signal, {
435+
attemptClientFactory: (factoryHarness) => async (options) => {
436+
options?.onStartedClient?.(factoryHarness.client);
437+
throw new Error("custom initialize failed");
438+
},
439+
});
440+
441+
await expect(run).rejects.toThrow("custom initialize failed");
442+
expect(harness.stdinDestroyed).toBe(true);
443+
});
444+
315445
it("closes a startup client that arrives after startup timeout", async () => {
316446
let observedFactoryOptions:
317447
| {
318448
onStartedClient?: (client: CodexAppServerClient) => void;
319449
abandonSignal?: AbortSignal;
450+
timeoutMs?: number;
320451
}
321452
| undefined;
453+
let factoryCalls = 0;
322454
let resolveFactoryDone: () => void = () => undefined;
323455
const factoryDone = new Promise<void>((resolve) => {
324456
resolveFactoryDone = resolve;
325457
});
458+
const delayedFactoryPluginConfig = {
459+
...pluginConfig,
460+
appServer: { command: "codex", requestTimeoutMs: 2_500 },
461+
} satisfies CodexPluginConfig;
326462
const { harness, run } = startThreadWithHarness(100, new AbortController().signal, {
463+
pluginConfig: delayedFactoryPluginConfig,
327464
attemptClientFactory: (factoryHarness) => async (options) => {
328465
try {
466+
factoryCalls += 1;
329467
observedFactoryOptions = options;
330468
await new Promise<void>((resolve) => {
331469
setTimeout(resolve, 250);
@@ -350,6 +488,8 @@ describe("startCodexAttemptThread", () => {
350488
).toBe(false);
351489
expect(observedFactoryOptions?.onStartedClient).toBeTypeOf("function");
352490
expect(observedFactoryOptions?.abandonSignal?.aborted).toBe(true);
491+
expect(observedFactoryOptions?.timeoutMs).toBe(2_500);
492+
expect(factoryCalls).toBe(1);
353493
});
354494

355495
it("clears the shared app-server when cancellation abandons an in-flight thread request", async () => {

‎extensions/codex/src/app-server/attempt-startup.ts‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ import {
6060
import type { CodexAppServerBindingStore } from "./session-binding.js";
6161
import {
6262
clearSharedCodexAppServerClientIfCurrent,
63+
clearSharedCodexAppServerClientIfCurrentAndUnclaimed,
6364
releaseLeasedSharedCodexAppServerClient,
6465
type CodexAppServerClientFactory,
6566
} from "./shared-client.js";
@@ -206,6 +207,7 @@ export async function startCodexAttemptThread(params: {
206207
}
207208
},
208209
abandonSignal: startupAbandonController.signal,
210+
timeoutMs: params.appServer.requestTimeoutMs,
209211
});
210212
const activeStartupClient = startupClient;
211213
let startupClientLeaseReleased = false;
@@ -455,6 +457,16 @@ export async function startCodexAttemptThread(params: {
455457
}
456458
} catch (error) {
457459
startupAttemptError = error;
460+
if (!startupAbandoned && !params.signal.aborted && !startupClient) {
461+
const sharedClient = clearSharedCodexAppServerClientIfCurrentAndUnclaimed(
462+
startupClientForAbandonedRequestCleanup,
463+
);
464+
if (sharedClient.found && !sharedClient.closed) {
465+
// Shared acquisition already released this caller. A peer still
466+
// owns the client, so outer cleanup must not retire it.
467+
startupClientForAbandonedRequestCleanup = undefined;
468+
}
469+
}
458470
throw error;
459471
} finally {
460472
if (!startupAttemptSucceeded) {
@@ -491,7 +503,11 @@ export async function startCodexAttemptThread(params: {
491503
try {
492504
return await startupAttempt();
493505
} catch (error) {
494-
if (params.signal.aborted || !isCodexAppServerConnectionClosedError(error)) {
506+
if (
507+
startupAbandoned ||
508+
params.signal.aborted ||
509+
!isCodexAppServerConnectionClosedError(error)
510+
) {
495511
throw error;
496512
}
497513
const failedClient = attemptedClient;

‎extensions/codex/src/app-server/shared-client.test.ts‎

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,11 @@ import { createClientHarness } from "./test-support.js";
77
const mocks = vi.hoisted(() => ({
88
bridgeCodexAppServerStartOptions: vi.fn(async ({ startOptions }) => startOptions),
99
applyCodexAppServerAuthProfile: vi.fn(
10-
async (_params?: { agentDir?: string; authProfileId?: string; config?: unknown }) => undefined,
10+
async (_params?: {
11+
agentDir?: string;
12+
authProfileId?: string;
13+
config?: unknown;
14+
}): Promise<void> => undefined,
1115
),
1216
resolveCodexAppServerAuthProfileIdForAgent: vi.fn(
1317
(params?: { authProfileId?: string }) => params?.authProfileId,
@@ -126,6 +130,15 @@ function clientStartCall(startSpy: unknown) {
126130
};
127131
}
128132

133+
function deferNextAuthProfileApplication(): () => void {
134+
let release: () => void = () => {};
135+
const gate = new Promise<void>((resolve) => {
136+
release = () => resolve();
137+
});
138+
mocks.applyCodexAppServerAuthProfile.mockReturnValueOnce(gate);
139+
return release;
140+
}
141+
129142
describe("shared Codex app-server client", () => {
130143
beforeAll(async () => {
131144
({ listCodexAppServerModels } = await import("./models.js"));
@@ -242,6 +255,54 @@ describe("shared Codex app-server client", () => {
242255
expect(startSpy).toHaveBeenCalledTimes(2);
243256
});
244257

258+
it("keeps shared startup alive for a caller with a longer initialize timeout", async () => {
259+
const harness = createClientHarness();
260+
const startSpy = vi.spyOn(CodexAppServerClient, "start").mockReturnValue(harness.client);
261+
262+
const shortAcquire = getSharedCodexAppServerClient({ timeoutMs: 5 });
263+
const longAcquire = getSharedCodexAppServerClient({ timeoutMs: 1000 });
264+
265+
await expect(shortAcquire).rejects.toThrow("codex app-server initialize timed out");
266+
expect(harness.process.stdin.destroyed).toBe(false);
267+
268+
await sendInitializeResult(harness, "openclaw/0.143.0 (macOS; test)");
269+
270+
await expect(longAcquire).resolves.toBe(harness.client);
271+
expect(startSpy).toHaveBeenCalledTimes(1);
272+
expect(harness.process.stdin.destroyed).toBe(false);
273+
});
274+
275+
it("reports a stalled shared auth phase separately from initialize", async () => {
276+
const harness = createClientHarness();
277+
vi.spyOn(CodexAppServerClient, "start").mockReturnValue(harness.client);
278+
const releaseAuth = deferNextAuthProfileApplication();
279+
280+
const acquire = getSharedCodexAppServerClient({ timeoutMs: 100 });
281+
await sendInitializeResult(harness, "openclaw/0.143.0 (macOS; test)");
282+
283+
await expect(acquire).rejects.toThrow("codex app-server authentication timed out");
284+
expect(harness.process.stdin.destroyed).toBe(true);
285+
releaseAuth();
286+
});
287+
288+
it("keeps shared auth alive for a caller with a longer timeout", async () => {
289+
const harness = createClientHarness();
290+
const startSpy = vi.spyOn(CodexAppServerClient, "start").mockReturnValue(harness.client);
291+
const releaseAuth = deferNextAuthProfileApplication();
292+
293+
const shortAcquire = getSharedCodexAppServerClient({ timeoutMs: 100 });
294+
const longAcquire = getSharedCodexAppServerClient({ timeoutMs: 1000 });
295+
await sendInitializeResult(harness, "openclaw/0.143.0 (macOS; test)");
296+
297+
await expect(shortAcquire).rejects.toThrow("codex app-server authentication timed out");
298+
expect(harness.process.stdin.destroyed).toBe(false);
299+
300+
releaseAuth();
301+
await expect(longAcquire).resolves.toBe(harness.client);
302+
expect(startSpy).toHaveBeenCalledTimes(1);
303+
expect(harness.process.stdin.destroyed).toBe(false);
304+
});
305+
245306
it("keeps a pending shared app-server alive when another acquire still owns startup", async () => {
246307
const harness = createClientHarness();
247308
const abandonController = new AbortController();

0 commit comments

Comments
 (0)