diff --git a/packages/agent-runtime/src/runtime-thread-identity.test.ts b/packages/agent-runtime/src/runtime-thread-identity.test.ts index 0771db6da4..c708caef98 100644 --- a/packages/agent-runtime/src/runtime-thread-identity.test.ts +++ b/packages/agent-runtime/src/runtime-thread-identity.test.ts @@ -109,6 +109,33 @@ describe("RuntimeThreadIdentityRegistry", () => { ).toBeUndefined(); }); + it("consumes a thread's pending identity slot when its identity is recorded", () => { + const registry = new RuntimeThreadIdentityRegistry(); + const providerState = registry.createProviderState({ providerId: "codex" }); + for (const threadId of ["thread-1", "thread-2"]) { + registry.registerThreadProvider({ + providerId: "codex", + providerState, + expectsIdentityNotification: true, + threadId, + }); + } + + registry.recordProviderThreadIdentity({ + providerState, + threadId: "thread-1", + providerThreadId: "provider-thread-1", + }); + + expect(providerState.pendingIdentityThreadIds).toEqual(["thread-2"]); + expect(registry.resolvePendingProviderThreadIdentity(providerState)).toBe( + "thread-2", + ); + expect( + registry.resolvePendingProviderThreadIdentity(providerState), + ).toBeUndefined(); + }); + it("stamps projected events with the resolved bb thread id", () => { const event: ThreadEvent = { type: "turn/started", diff --git a/packages/agent-runtime/src/runtime-thread-identity.ts b/packages/agent-runtime/src/runtime-thread-identity.ts index 952d6187eb..8dc39e82f4 100644 --- a/packages/agent-runtime/src/runtime-thread-identity.ts +++ b/packages/agent-runtime/src/runtime-thread-identity.ts @@ -91,6 +91,10 @@ export class RuntimeThreadIdentityRegistry { recordProviderThreadIdentity(args: RecordProviderThreadIdentityArgs): void { this.threadToProviderThread.set(args.threadId, args.providerThreadId); + args.providerState.pendingIdentityThreadIds = + args.providerState.pendingIdentityThreadIds.filter( + (pendingThreadId) => pendingThreadId !== args.threadId, + ); } resolveBbThreadIdForProviderThread( diff --git a/packages/agent-runtime/src/runtime.codex-topology.test.ts b/packages/agent-runtime/src/runtime.codex-topology.test.ts index 83bff6d293..704c55a0ad 100644 --- a/packages/agent-runtime/src/runtime.codex-topology.test.ts +++ b/packages/agent-runtime/src/runtime.codex-topology.test.ts @@ -373,6 +373,93 @@ describe("codex process topology", () => { expect(runtime.hasThread("t1")).toBe(false); expect(runtime.hasThread("t2")).toBe(false); }, 30_000); + it("refuses to resume a provider thread that another hosted thread already owns", async () => { + const topology = createCodexTopologyRuntime(); + const { runtime } = topology; + + const providerThreadId1 = await startCodexThread(runtime, "t1"); + + await expect( + runtime.resumeThread({ + environmentId: "env-1", + projectId: "p1", + providerId: "codex", + providerThreadId: providerThreadId1, + threadId: "t2", + options: fullRuntimeOptions, + }), + ).rejects.toThrow( + `provider thread "${providerThreadId1}" is already hosted by thread "t1"`, + ); + expect(runtime.hasThread("t2")).toBe(false); + expect(runtime.getProviderSession("t1")?.providerThreadId).toBe( + providerThreadId1, + ); + expect(topology.spawned()).toBe(1); + }, 30_000); + + it("keeps a rewound thread's provider identity when another thread starts on the same bridge", async () => { + const topology = createCodexTopologyRuntime(); + const { runtime, events } = topology; + + const originalProviderThreadId1 = await startCodexThread(runtime, "t1"); + const staged = await runtime.prepareThreadRewind({ + environmentId: "env-1", + threadId: "t1", + leaseId: "lease-1", + projectId: "p1", + providerId: "codex", + sourceProviderThreadId: originalProviderThreadId1, + retainThroughProviderCheckpoint: "turn-1", + options: fullRuntimeOptions, + instructionMode: "append", + }); + const { providerThreadId: providerThreadId1 } = await runtime.startThread({ + environmentId: "env-1", + projectId: "p1", + providerId: "codex", + threadId: "t1", + options: fullRuntimeOptions, + fork: { sourceProviderThreadId: staged.providerThreadId }, + }); + await runtime.discardThreadRewind({ leaseId: "lease-1" }); + expect(providerThreadId1).not.toBe(originalProviderThreadId1); + const eventsBeforeSecondStart = events.length; + + const providerThreadId2 = await startCodexThread(runtime, "t2"); + expect(providerThreadId2).not.toBe(providerThreadId1); + expect(runtime.getProviderSession("t1")?.providerThreadId).toBe( + providerThreadId1, + ); + expect(runtime.getProviderSession("t2")?.providerThreadId).toBe( + providerThreadId2, + ); + + await runtime.runTurn({ + clientRequestId: "creq_cdxidnty22", + threadId: "t1", + input: [promptTextInput({ text: "hello" })], + options: fullRuntimeOptions, + }); + await waitForThreadAgentMessageText({ + events, + providerId: "codex", + runtime, + text: "hello from codex turn", + threadId: "t1", + }); + const stampedProviderThreadIds = new Set( + events + .slice(eventsBeforeSecondStart) + .filter((event) => event.threadId === "t1") + .flatMap((event) => + "providerThreadId" in event && event.providerThreadId + ? [event.providerThreadId] + : [], + ), + ); + expect([...stampedProviderThreadIds]).toEqual([providerThreadId1]); + }, 30_000); }); function isAlive(pid: number): boolean { diff --git a/packages/agent-runtime/src/runtime.ts b/packages/agent-runtime/src/runtime.ts index 3d64e4d7c8..d6b9aa4697 100644 --- a/packages/agent-runtime/src/runtime.ts +++ b/packages/agent-runtime/src/runtime.ts @@ -778,6 +778,25 @@ export function createAgentRuntime(options: AgentRuntimeOptions): AgentRuntime { }); } + function resolveProviderIdentityTargetThreadId(args: { + eventThreadId: string; + proc: ProviderProcess; + sourceThreadId: string | undefined; + }): string | undefined { + if ( + args.sourceThreadId !== undefined && + args.proc.identity.threadIds.has(args.sourceThreadId) + ) { + return args.sourceThreadId; + } + if (args.proc.identity.threadIds.has(args.eventThreadId)) { + return args.eventThreadId; + } + return threadIdentityRegistry.resolvePendingProviderThreadIdentity( + args.proc.identity, + ); + } + function forgetThreadRuntimeStateForProviderState( providerState: RuntimeProviderProcess["identity"], threadId: string, @@ -1184,27 +1203,33 @@ export function createAgentRuntime(options: AgentRuntimeOptions): AgentRuntime { if (event.type !== "thread/identity" || !event.providerThreadId) { continue; } - - if (args.proc.identity.threadIds.has(event.threadId)) { - recordProviderThreadIdentity( - args.proc, - event.threadId, - event.providerThreadId, + const targetThreadId = resolveProviderIdentityTargetThreadId({ + eventThreadId: event.threadId, + proc: args.proc, + sourceThreadId: args.sourceThreadId, + }); + if (targetThreadId === undefined) { + options.onStderr?.( + `Dropping thread/identity for provider thread "${event.providerThreadId}" from "${args.proc.providerId}"; no bb thread could be resolved`, ); continue; } - - const bbThreadId = - threadIdentityRegistry.resolvePendingProviderThreadIdentity( - args.proc.identity, - ); - if (bbThreadId) { - recordProviderThreadIdentity( - args.proc, - bbThreadId, - event.providerThreadId, + const ownerThreadId = + threadIdentityRegistry.resolveBbThreadIdForProviderThread({ + providerState: args.proc.identity, + providerThreadId: event.providerThreadId, + }); + if (ownerThreadId !== undefined && ownerThreadId !== targetThreadId) { + options.onStderr?.( + `Ignoring thread/identity that would bind provider thread "${event.providerThreadId}" to bb thread "${targetThreadId}"; bb thread "${ownerThreadId}" already owns it on "${args.proc.providerId}"`, ); + continue; } + recordProviderThreadIdentity( + args.proc, + targetThreadId, + event.providerThreadId, + ); } for (const event of args.events) { @@ -1822,6 +1847,18 @@ export function createAgentRuntime(options: AgentRuntimeOptions): AgentRuntime { options: execOpts, providerId, }); + if (providerThreadId !== undefined) { + const ownerThreadId = + threadIdentityRegistry.resolveBbThreadIdForProviderThread({ + providerState: proc.identity, + providerThreadId, + }); + if (ownerThreadId !== undefined && ownerThreadId !== threadId) { + throw new Error( + `Cannot resume thread "${threadId}" on "${providerId}": provider thread "${providerThreadId}" is already hosted by thread "${ownerThreadId}"`, + ); + } + } const resolvedEnvironment = resolveRuntimeThreadEnvironment({ contributedEnv, environmentId, diff --git a/plugins/provider-codex/src/bridge/bridge-process.test-support.ts b/plugins/provider-codex/src/bridge/bridge-process.test-support.ts new file mode 100644 index 0000000000..30b2467005 --- /dev/null +++ b/plugins/provider-codex/src/bridge/bridge-process.test-support.ts @@ -0,0 +1,130 @@ +import { existsSync, readFileSync, rmSync } from "node:fs"; +import type { experimental_createBridgeJsonRpcTestHarness as createBridgeJsonRpcTestHarness } from "@get-bb/plugin-sdk/provider-bridge/testing"; + +type BridgeJsonRpcTestHarness = ReturnType< + typeof createBridgeJsonRpcTestHarness +>; + +const PROCESS_POLL_INTERVAL_MS = 20; + +function readProcessLog(processLogPath: string): string { + if (!existsSync(processLogPath)) { + return ""; + } + try { + return readFileSync(processLogPath, "utf8"); + } catch (error) { + if (error instanceof Error && Reflect.get(error, "code") === "ENOENT") { + return ""; + } + throw error; + } +} + +export function spawnedAppServerPids(processLogPath: string): number[] { + return readProcessLog(processLogPath) + .split("\n") + .filter((line) => line.startsWith("spawn:")) + .map((line) => Number(line.split(":")[1])) + .filter((pid) => Number.isSafeInteger(pid) && pid > 0); +} + +function processIsAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + if (error instanceof Error && Reflect.get(error, "code") === "ESRCH") { + return false; + } + throw error; + } +} + +export async function waitForAppServerChildrenToExit( + processLogPath: string, + timeoutMs = 8_000, +): Promise { + const childPids = spawnedAppServerPids(processLogPath); + const deadline = Date.now() + timeoutMs; + while (childPids.some(processIsAlive)) { + if (Date.now() > deadline) { + throw new Error( + `Timed out waiting for app-server children to exit: ${JSON.stringify(childPids.filter(processIsAlive))}`, + ); + } + await new Promise((resolveTick) => + setTimeout(resolveTick, PROCESS_POLL_INTERVAL_MS), + ); + } +} + +export async function waitForAppServerProcessStep( + processLogPath: string, + step: string, + timeoutMs = 5_000, +): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (readProcessLog(processLogPath).includes(`${step}:`)) { + return; + } + await new Promise((resolveTick) => + setTimeout(resolveTick, PROCESS_POLL_INTERVAL_MS), + ); + } + throw new Error(`Timed out waiting for app-server process step: ${step}`); +} + +export async function cleanupBridgeProcessTest(args: { + harness: BridgeJsonRpcTestHarness | undefined; + cleanupId: number; + threadId: string; + providerThreadId: string; + processLogPath: string; + workspaceDir: string; + unstubEnvs: () => void; +}): Promise { + const errors: unknown[] = []; + if (args.harness !== undefined) { + try { + args.harness.sendRequest(args.cleanupId, "thread/stop", { + threadId: args.threadId, + providerThreadId: args.providerThreadId, + intent: "release", + activeTurnId: null, + }); + await args.harness.waitForResponse(args.cleanupId).catch(() => undefined); + } catch (error) { + errors.push(error); + } + } + try { + await waitForAppServerChildrenToExit(args.processLogPath); + } catch (error) { + errors.push(error); + } + try { + args.harness?.restore(); + } catch (error) { + errors.push(error); + } + try { + args.unstubEnvs(); + } catch (error) { + errors.push(error); + } + try { + if (args.workspaceDir !== "") { + rmSync(args.workspaceDir, { recursive: true, force: true }); + } + } catch (error) { + errors.push(error); + } + if (errors.length === 1) { + throw errors[0]; + } + if (errors.length > 1) { + throw new AggregateError(errors, "Bridge process test cleanup failed"); + } +} diff --git a/plugins/provider-codex/src/bridge/bridge.archived-rebuild.test.ts b/plugins/provider-codex/src/bridge/bridge.archived-rebuild.test.ts index c09a785a06..8a451e51ed 100644 --- a/plugins/provider-codex/src/bridge/bridge.archived-rebuild.test.ts +++ b/plugins/provider-codex/src/bridge/bridge.archived-rebuild.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; @@ -6,6 +6,10 @@ import { afterEach, beforeEach, expect, it, vi } from "vitest"; import { experimental_createBridgeJsonRpcTestHarness as createBridgeJsonRpcTestHarness } from "@get-bb/plugin-sdk/provider-bridge/testing"; import type { BridgeJsonRpcOutputMessage } from "@get-bb/plugin-sdk/provider-bridge/testing"; import { handleLine } from "./bridge.js"; +import { + cleanupBridgeProcessTest, + spawnedAppServerPids, +} from "./bridge-process.test-support.js"; const THREAD_ID = "thr_archived_rebuild_1"; const PROVIDER_THREAD_ID = "rebuild-rollout-1"; @@ -31,9 +35,9 @@ const changedSessionOptions = { const turnInput = [{ type: "text", text: "hello", mentions: [] }]; let harness: ReturnType; -let workspaceDir: string; -let archiveStatePath: string; -let processLogPath: string; +let workspaceDir = ""; +let archiveStatePath = ""; +let processLogPath = ""; beforeEach(() => { workspaceDir = mkdtempSync(join(tmpdir(), "bb-codex-archived-rebuild-")); @@ -53,52 +57,17 @@ beforeEach(() => { }); afterEach(async () => { - const cleanupId = 993_001; - harness.sendRequest(cleanupId, "thread/stop", { + await cleanupBridgeProcessTest({ + harness, + cleanupId: 993_001, threadId: THREAD_ID, providerThreadId: PROVIDER_THREAD_ID, - intent: "release", - activeTurnId: null, + processLogPath, + workspaceDir, + unstubEnvs: vi.unstubAllEnvs, }); - await harness.waitForResponse(cleanupId).catch(() => undefined); - await waitForAppServerChildrenToExit(); - harness.restore(); - vi.unstubAllEnvs(); - rmSync(workspaceDir, { recursive: true, force: true }); }); -function spawnedAppServerPids(): number[] { - return readFileSync(processLogPath, "utf8") - .split("\n") - .filter((line) => line.startsWith("spawn:")) - .map((line) => Number(line.split(":")[1])); -} - -function processIsAlive(pid: number): boolean { - try { - process.kill(pid, 0); - return true; - } catch (error) { - if (error instanceof Error && Reflect.get(error, "code") === "ESRCH") { - return false; - } - throw error; - } -} - -async function waitForAppServerChildrenToExit(): Promise { - const childPids = spawnedAppServerPids(); - const deadline = Date.now() + 15_000; - while (childPids.some(processIsAlive)) { - if (Date.now() > deadline) { - throw new Error( - `Timed out waiting for app-server children to exit: ${JSON.stringify(childPids.filter(processIsAlive))}`, - ); - } - await new Promise((resolveTick) => setTimeout(resolveTick, 20)); - } -} - async function resumeThread(): Promise { harness.sendRequest(1, "thread/resume", { threadId: THREAD_ID, @@ -197,11 +166,11 @@ it("keeps the thread resumable when a settings-change rebuild hits an externally it("keeps the thread resumable when the rebuild after the child died hits an externally archived rollout", async () => { await resumeThread(); - const spawnLine = readFileSync(processLogPath, "utf8") - .split("\n") - .find((line) => line.startsWith("spawn:")); - const childPid = Number(spawnLine?.split(":")[1]); - expect(Number.isInteger(childPid)).toBe(true); + const [childPid] = spawnedAppServerPids(processLogPath); + expect(childPid).toBeDefined(); + if (childPid === undefined) { + throw new Error("Expected the fake app-server child to have spawned"); + } process.kill(childPid, "SIGKILL"); const deadline = Date.now() + 15_000; while ( diff --git a/plugins/provider-codex/src/bridge/bridge.ts b/plugins/provider-codex/src/bridge/bridge.ts index 6bf3ef14d2..77ef1e05ca 100644 --- a/plugins/provider-codex/src/bridge/bridge.ts +++ b/plugins/provider-codex/src/bridge/bridge.ts @@ -281,6 +281,9 @@ const CHILD_REQUEST_TIMEOUT_MS = 60_000; const INTERRUPT_SETTLEMENT_TIMEOUT_MS = 5_000; const CODEX_ARCHIVED_SESSION_ERROR_PATTERN = /\b(?:session|thread)\s+\S+\s+is archived\b/i; +const CODEX_ACTIVE_WRITER_ERROR_PATTERN = + /\bthread\s+\S+\s+already has an active writer\b/i; +const CODEX_ACTIVE_WRITER_RETRY_DELAYS_MS = [100, 400, 1_000] as const; const CODEX_ALREADY_ARCHIVED_ERROR_PATTERN = /\bno rollout found for thread id\b/i; const CODEX_NOT_ARCHIVED_ERROR_PATTERN = @@ -323,6 +326,12 @@ function archivedSessionHint(message: string): ProviderRecoveryHint | null { : null; } +function withActiveWriterGuidance(message: string): string { + return CODEX_ACTIVE_WRITER_ERROR_PATTERN.test(message) + ? `${message}. Another Codex process still owns this thread. Close any other Codex session using it; if none is open, wait for a previous Codex process to finish shutting down or stop the leftover codex app-server process, then retry.` + : message; +} + async function delay(ms: number): Promise { await new Promise((resolve) => { setTimeout(resolve, ms); @@ -894,6 +903,40 @@ const codexThreadIdentityResultSchema = z .object({ thread: z.object({ id: z.string().min(1) }).passthrough() }) .passthrough(); +async function requestThreadConstructionWithWriterRetry( + connection: CodexAppServerConnection, + method: string, + params: BbThreadStartParams | ThreadResumeParams | BbThreadForkParams, +): Promise> { + const sendOnce = (): Promise< + z.infer + > => + connection.request({ + method, + params, + resultSchema: codexThreadIdentityResultSchema, + timeoutMs: CHILD_REQUEST_TIMEOUT_MS, + }); + for (const [ + retryIndex, + retryDelayMs, + ] of CODEX_ACTIVE_WRITER_RETRY_DELAYS_MS.entries()) { + try { + return await sendOnce(); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + if (!CODEX_ACTIVE_WRITER_ERROR_PATTERN.test(message)) { + throw error; + } + process.stderr.write( + `codex ${method} found an active rollout writer; retrying in ${retryDelayMs}ms (${retryIndex + 1}/${CODEX_ACTIVE_WRITER_RETRY_DELAYS_MS.length}).\n`, + ); + await delay(retryDelayMs); + } + } + return await sendOnce(); +} + type CodexSessionConstructionRequest = | { kind: "start" } | { kind: "resume"; providerThreadId: string } @@ -1061,12 +1104,11 @@ async function constructThreadSession( } } - const result = await connection.request({ + const result = await requestThreadConstructionWithWriterRetry( + connection, method, params, - resultSchema: codexThreadIdentityResultSchema, - timeoutMs: CHILD_REQUEST_TIMEOUT_MS, - }); + ); const codexThreadId = result.thread.id; session.codexThreadId = codexThreadId; translator.activateThreadGitWritableRoots({ @@ -1307,8 +1349,9 @@ function sendConstructionError( error: unknown, resumable: boolean, ): void { - const message = describeCodexLaunchError(error); - const recovery = archivedSessionHint(message); + const providerMessage = describeCodexLaunchError(error); + const recovery = archivedSessionHint(providerMessage); + const message = withActiveWriterGuidance(providerMessage); sendError( id, resumable && recovery !== null @@ -1679,8 +1722,9 @@ async function handleThreadMaintenance( } function rejectWithCodexError(id: string | number, error: unknown): void { - const message = describeCodexLaunchError(error); - const recovery = archivedSessionHint(message); + const providerMessage = describeCodexLaunchError(error); + const recovery = archivedSessionHint(providerMessage); + const message = withActiveWriterGuidance(providerMessage); if (recovery !== null) { sendError(id, BRIDGE_JSON_RPC_ERRORS.BRIDGE_ERROR, message, { recovery }); return; diff --git a/plugins/provider-codex/src/bridge/bridge.writer-lock.test.ts b/plugins/provider-codex/src/bridge/bridge.writer-lock.test.ts new file mode 100644 index 0000000000..febfcd5989 --- /dev/null +++ b/plugins/provider-codex/src/bridge/bridge.writer-lock.test.ts @@ -0,0 +1,219 @@ +import { existsSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { experimental_createBridgeJsonRpcTestHarness as createBridgeJsonRpcTestHarness } from "@get-bb/plugin-sdk/provider-bridge/testing"; +import { handleLine } from "./bridge.js"; +import { + cleanupBridgeProcessTest, + spawnedAppServerPids, + waitForAppServerProcessStep, +} from "./bridge-process.test-support.js"; + +const THREAD_ID = "thr_writer_lock_1"; +const PROVIDER_THREAD_ID = "codex-writer-lock-1"; + +const fakeAppServerPath = fileURLToPath( + new URL("./fake-codex-app-server.mjs", import.meta.url), +); + +const sessionOptions = { + permissionMode: "full", + permissionScope: "full", + approvalReviewer: null, + permissionEscalation: null, + reasoningLevel: "low", +} as const; + +const changedSessionOptions = { + ...sessionOptions, + reasoningLevel: "high", +} as const; + +let harness: ReturnType; +let workspaceDir = ""; +let processLogPath = ""; +let writerLockPath = ""; + +beforeEach(() => { + workspaceDir = mkdtempSync(join(tmpdir(), "bb-codex-writer-lock-")); + processLogPath = join(workspaceDir, "app-server-processes.log"); + writerLockPath = join(workspaceDir, "writer.lock"); + const scriptPath = join(workspaceDir, "fake-codex-script.json"); + writeFileSync( + scriptPath, + JSON.stringify({ + processLogPath, + writerLockPath, + sigtermDelayMs: 500, + }), + ); + vi.stubEnv("BB_CODEX_BRIDGE_APP_SERVER_COMMAND", process.execPath); + vi.stubEnv( + "BB_CODEX_BRIDGE_APP_SERVER_ARGS", + JSON.stringify([fakeAppServerPath, scriptPath]), + ); + harness = createBridgeJsonRpcTestHarness(handleLine); +}); + +afterEach(async () => { + await cleanupBridgeProcessTest({ + harness, + cleanupId: 995_001, + threadId: THREAD_ID, + providerThreadId: PROVIDER_THREAD_ID, + processLogPath, + workspaceDir, + unstubEnvs: vi.unstubAllEnvs, + }); +}); + +async function resumeThread(id: number): Promise { + harness.sendRequest(id, "thread/resume", { + threadId: THREAD_ID, + providerThreadId: PROVIDER_THREAD_ID, + cwd: workspaceDir, + instructionMode: "append", + options: sessionOptions, + }); + expect((await harness.waitForResponse(id)).error).toBeUndefined(); +} + +it("waits for the previous writer before resuming during a settings rebuild", async () => { + await resumeThread(1); + + harness.sendRequest(2, "turn/start", { + threadId: THREAD_ID, + providerThreadId: PROVIDER_THREAD_ID, + clientRequestId: "creq_abcdefghjk", + input: [{ type: "text", text: "hello", mentions: [] }], + options: changedSessionOptions, + }); + const rebuiltTurn = await harness.waitForResponse(2); + + expect(rebuiltTurn.error).toBeUndefined(); + expect(rebuiltTurn.result).toEqual({ threadId: THREAD_ID }); +}, 30_000); + +it("finishes releasing the writer before acknowledging thread stop", async () => { + await resumeThread(1); + + harness.sendRequest(2, "thread/stop", { + threadId: THREAD_ID, + providerThreadId: PROVIDER_THREAD_ID, + intent: "release", + activeTurnId: null, + }); + expect((await harness.waitForResponse(2)).error).toBeUndefined(); + expect(existsSync(writerLockPath)).toBe(false); + + await resumeThread(3); +}, 30_000); + +it("does not install a replacement after a concurrent release is acknowledged", async () => { + await resumeThread(1); + + harness.sendRequest(2, "turn/start", { + threadId: THREAD_ID, + providerThreadId: PROVIDER_THREAD_ID, + clientRequestId: "creq_abcdefghjk", + input: [{ type: "text", text: "hello", mentions: [] }], + options: changedSessionOptions, + }); + await waitForAppServerProcessStep(processLogPath, "sigterm"); + + harness.sendRequest(3, "thread/stop", { + threadId: THREAD_ID, + providerThreadId: PROVIDER_THREAD_ID, + intent: "release", + activeTurnId: null, + }); + expect((await harness.waitForResponse(3)).error).toBeUndefined(); + expect(existsSync(writerLockPath)).toBe(false); + + const rebuiltTurn = await harness.waitForResponse(2); + expect(rebuiltTurn.error).toBeDefined(); + expect( + harness.messages.filter((message) => message.method === "session/replaced"), + ).toHaveLength(0); + expect(spawnedAppServerPids(processLogPath)).toHaveLength(1); +}, 30_000); + +it("does not install a replacement after concurrent discard maintenance settles", async () => { + await resumeThread(1); + + harness.sendRequest(2, "turn/start", { + threadId: THREAD_ID, + providerThreadId: PROVIDER_THREAD_ID, + clientRequestId: "creq_abcdefghjk", + input: [{ type: "text", text: "hello", mentions: [] }], + options: changedSessionOptions, + }); + await waitForAppServerProcessStep(processLogPath, "sigterm"); + + harness.sendRequest(3, "thread/discard", { + threadId: THREAD_ID, + providerThreadId: PROVIDER_THREAD_ID, + }); + expect((await harness.waitForResponse(3)).result).toEqual({ ok: true }); + expect(existsSync(writerLockPath)).toBe(false); + + const rebuiltTurn = await harness.waitForResponse(2); + expect(rebuiltTurn.error).toBeDefined(); + expect( + harness.messages.filter((message) => message.method === "session/replaced"), + ).toHaveLength(0); + expect(spawnedAppServerPids(processLogPath)).toHaveLength(2); +}, 30_000); + +it("retries a resume while another Codex process is releasing the writer", async () => { + writeFileSync(writerLockPath, String(process.pid)); + const stderrWrite = vi + .spyOn(process.stderr, "write") + .mockImplementation(() => true); + + try { + harness.sendRequest(1, "thread/resume", { + threadId: THREAD_ID, + providerThreadId: PROVIDER_THREAD_ID, + cwd: workspaceDir, + instructionMode: "append", + options: sessionOptions, + }); + await waitForAppServerProcessStep(processLogPath, "writer-conflict"); + rmSync(writerLockPath, { force: true }); + const resumed = await harness.waitForResponse(1); + + expect(resumed.error).toBeUndefined(); + expect(resumed.result).toEqual({ + providerThreadId: PROVIDER_THREAD_ID, + sessionRestorable: true, + }); + expect(stderrWrite).toHaveBeenCalledWith( + "codex thread/resume found an active rollout writer; retrying in 100ms (1/3).\n", + ); + } finally { + stderrWrite.mockRestore(); + rmSync(writerLockPath, { force: true }); + } +}, 30_000); + +it("explains persistent writer contention and resumes after the owner closes", async () => { + writeFileSync(writerLockPath, String(process.pid)); + + harness.sendRequest(1, "thread/resume", { + threadId: THREAD_ID, + providerThreadId: PROVIDER_THREAD_ID, + cwd: workspaceDir, + instructionMode: "append", + options: sessionOptions, + }); + const blocked = await harness.waitForResponse(1); + rmSync(writerLockPath, { force: true }); + + expect(blocked.error?.message).toBe( + `thread ${PROVIDER_THREAD_ID} already has an active writer. Another Codex process still owns this thread. Close any other Codex session using it; if none is open, wait for a previous Codex process to finish shutting down or stop the leftover codex app-server process, then retry.`, + ); + await resumeThread(2); +}, 30_000); diff --git a/plugins/provider-codex/src/bridge/fake-codex-app-server.mjs b/plugins/provider-codex/src/bridge/fake-codex-app-server.mjs index d65c3bedf6..6841040082 100644 --- a/plugins/provider-codex/src/bridge/fake-codex-app-server.mjs +++ b/plugins/provider-codex/src/bridge/fake-codex-app-server.mjs @@ -29,6 +29,7 @@ import { existsSync, openSync, readFileSync, + unlinkSync, writeFileSync, } from "node:fs"; import { createInterface } from "node:readline"; @@ -166,7 +167,10 @@ const archivedThreadIds = new Set(); */ const processLogPath = script?.processLogPath ?? null; const stallThreadStart = script?.stallThreadStart ?? false; +const writerLockPath = script?.writerLockPath ?? null; const sigtermDelayMs = script?.sigtermDelayMs ?? 0; +let ownsWriterLock = false; +let servesThread = false; function logProcessStep(step) { if (processLogPath === null) { @@ -175,13 +179,59 @@ function logProcessStep(step) { appendFileSync(processLogPath, `${step}:${process.pid}:${process.ppid}\n`); } +function releaseWriterLock() { + if (!ownsWriterLock || writerLockPath === null) { + return; + } + ownsWriterLock = false; + if ( + existsSync(writerLockPath) && + readFileSync(writerLockPath, "utf8") === String(process.pid) + ) { + unlinkSync(writerLockPath); + } +} + +function acquireWriterLock() { + if (writerLockPath === null || ownsWriterLock) { + return true; + } + try { + writeFileSync(writerLockPath, String(process.pid), { flag: "wx" }); + ownsWriterLock = true; + return true; + } catch (error) { + if (!error || typeof error !== "object" || error.code !== "EEXIST") { + throw error; + } + const ownerPid = Number(readFileSync(writerLockPath, "utf8")); + try { + process.kill(ownerPid, 0); + return false; + } catch (ownerError) { + if ( + !ownerError || + typeof ownerError !== "object" || + ownerError.code !== "ESRCH" + ) { + throw ownerError; + } + unlinkSync(writerLockPath); + return acquireWriterLock(); + } + } +} + function exitCleanly() { + releaseWriterLock(); logProcessStep("exit"); process.exit(0); } +process.on("exit", releaseWriterLock); process.on("SIGTERM", () => { - if (sigtermDelayMs > 0) { + logProcessStep("sigterm"); + if (sigtermDelayMs > 0 && servesThread) { setTimeout(exitCleanly, sigtermDelayMs); return; } @@ -353,6 +403,7 @@ async function handleRequest(message) { respond(id, {}); return; case "thread/start": { + servesThread = true; if (stallThreadStart) { await new Promise(() => undefined); } @@ -363,6 +414,16 @@ async function handleRequest(message) { return; } case "thread/resume": { + servesThread = true; + if (!acquireWriterLock()) { + logProcessStep("writer-conflict"); + respondError( + id, + -32603, + `thread ${params.threadId} already has an active writer`, + ); + return; + } // Scripted archived-session rejection: the real app-server refuses to // resume an archived thread with an error naming the session. Tests use // an `archived-` provider-thread-id prefix to trigger it. @@ -388,6 +449,7 @@ async function handleRequest(message) { return; } case "thread/fork": { + servesThread = true; // The real app-server reads the source rollout; an archived source is // refused with the same wording a resume gets. if (