From 5c266aa7a0f5f8ba6330290c0c7f28bc7ec51271 Mon Sep 17 00:00:00 2001 From: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:39:36 -0400 Subject: [PATCH 1/2] fix(codex): stop a surviving descendant wedging the Codex home process lock The per-Codex-home process lock was released only by the child's 'close' event. 'close' fires when the child AND every process that inherited its stdio have exited, so a grandchild that outlives codex (an MCP server, a helper, an `sh -c` wrapper) keeps the pipe open and 'close' never arrives. killProcessTree only SIGKILLs the direct child on POSIX, so Stop and the 60s generation timeout both leave the lock held. Every later AI commit message, PR field, branch name, and quota probe for that home then queued forever with no error and no recovery until the app restarted. Release the hold on 'exit' as well as 'close': once the codex process itself is gone it can no longer rotate that home's auth.json, which is the race the lock exists to prevent, so waiting on descendants bought nothing. As a backstop for a child that never reports exit at all (a kill that does not land), bound the hold: while another run is queued behind it, a hold that outlives CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS releases the queue. The cap is armed only when someone is waiting and only counts time the entry actually holds the lock, so uncontended runs arm no timer and queueing behind a slow predecessor never shortens a run's own budget. --- .../codex-cli/codex-home-process-lock.test.ts | 54 ++++++++++++- src/main/codex-cli/codex-home-process-lock.ts | 78 ++++++++++++++++--- .../commit-message-text-generation.test.ts | 65 ++++++++++++++++ .../commit-message-text-generation.ts | 18 +++-- 4 files changed, 200 insertions(+), 15 deletions(-) diff --git a/src/main/codex-cli/codex-home-process-lock.test.ts b/src/main/codex-cli/codex-home-process-lock.test.ts index 7287d011b42f..edb27bc83b33 100644 --- a/src/main/codex-cli/codex-home-process-lock.test.ts +++ b/src/main/codex-cli/codex-home-process-lock.test.ts @@ -1,7 +1,8 @@ import { join } from 'node:path' import { homedir } from 'node:os' -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import { + CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS, resolveCodexHomeProcessLockKey, resolveCodexHomeProcessLockKeyForSpawnEnv, withCodexHomeProcessLock @@ -70,6 +71,57 @@ describe('withCodexHomeProcessLock', () => { await expect(withCodexHomeProcessLock('home-a', async () => 'after')).resolves.toBe('after') }) + it('releases the lock when a hold never settles', async () => { + vi.useFakeTimers() + try { + const events: string[] = [] + // A codex child whose surviving descendant keeps its stdio open never + // reports completion, so this hold never settles on its own. + void withCodexHomeProcessLock('home-wedged', () => new Promise(() => {})) + const queued = withCodexHomeProcessLock('home-wedged', async () => { + events.push('queued:start') + }) + + await vi.advanceTimersByTimeAsync(CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS - 1) + expect(events).toEqual([]) + + await vi.advanceTimersByTimeAsync(1) + await queued + expect(events).toEqual(['queued:start']) + } finally { + vi.useRealTimers() + } + }) + + it('measures the release cap from when a run starts, not when it is queued', async () => { + vi.useFakeTimers() + try { + const events: string[] = [] + const gate = deferred() + const first = withCodexHomeProcessLock('home-queued', async () => { + events.push('first:start') + await gate.promise + }) + void withCodexHomeProcessLock('home-queued', () => new Promise(() => {})) + const third = withCodexHomeProcessLock('home-queued', async () => { + events.push('third:start') + }) + + // Time spent waiting behind a healthy run must not burn the wedged run's cap. + await vi.advanceTimersByTimeAsync(CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS - 1_000) + gate.resolve() + await first + await vi.advanceTimersByTimeAsync(CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS - 1) + expect(events).toEqual(['first:start']) + + await vi.advanceTimersByTimeAsync(1) + await third + expect(events).toEqual(['first:start', 'third:start']) + } finally { + vi.useRealTimers() + } + }) + it('keys explicit and default host homes consistently', () => { const previousCodexHome = process.env.CODEX_HOME delete process.env.CODEX_HOME diff --git a/src/main/codex-cli/codex-home-process-lock.ts b/src/main/codex-cli/codex-home-process-lock.ts index ef91a5705d90..0d1f1224f8b7 100644 --- a/src/main/codex-cli/codex-home-process-lock.ts +++ b/src/main/codex-cli/codex-home-process-lock.ts @@ -8,7 +8,13 @@ import { normalizeRuntimePathForComparison } from '../../shared/cross-platform-p // so Orca's own spawns (quota probes, commit-message runs) serialize per home. // User terminal panes are intentionally not serialized here. -const lockTails = new Map>() +type LockTail = { + settled: Promise + /** Bounds the running hold; only needed once someone is queued behind it. */ + armReleaseCap: () => void +} + +const lockTails = new Map() export function resolveCodexHomeProcessLockKey(codexHomePath?: string | null): string { const home = codexHomePath ?? process.env.CODEX_HOME ?? join(homedir(), '.codex') @@ -37,19 +43,73 @@ export function resolveCodexHomeProcessLockKeyForSpawnEnv( return normalizeRuntimePathForComparison(codexHome ?? join(homedir(), '.codex')) } +// Why: a hold that never settles (a killed codex child whose descendant keeps +// the inherited stdio open never reports close) would kill this home for the +// rest of the session. Far above the 60s generation timeout so healthy runs and +// their process teardown never trip it. +export const CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS = 5 * 60_000 + export function withCodexHomeProcessLock(lockKey: string, fn: () => Promise): Promise { - const prior = lockTails.get(lockKey) ?? Promise.resolve() - const run = prior.then(fn) - // Why: keep the queue alive past a failed run so later entrants still start. - const tail = run.then( - () => undefined, - () => undefined - ) + const prior = lockTails.get(lockKey) + prior?.armReleaseCap() + const started = prior?.settled ?? Promise.resolve() + const run = started.then(fn) + const tail = createLockTail(started, run, lockKey) lockTails.set(lockKey, tail) - void tail.then(() => { + void tail.settled.then(() => { if (lockTails.get(lockKey) === tail) { lockTails.delete(lockKey) } }) return run } + +function createLockTail( + started: Promise, + run: Promise, + lockKey: string +): LockTail { + let capTimer: ReturnType | null = null + let running = false + let queuedBehind = false + let release!: () => void + const settled = new Promise((resolve) => { + release = () => { + running = false + if (capTimer) { + clearTimeout(capTimer) + capTimer = null + } + resolve() + } + }) + // The cap only counts time this entry actually holds the lock, so waiting + // behind a slow predecessor never shortens a run's own budget. + const armIfHolding = (): void => { + if (!running || !queuedBehind || capTimer) { + return + } + capTimer = setTimeout(() => { + console.warn( + `[codex-home-lock] releasing ${lockKey} after ${CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS}ms without completion` + ) + release() + }, CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS) + if (typeof capTimer === 'object' && 'unref' in capTimer) { + capTimer.unref() + } + } + void started.then(() => { + running = true + armIfHolding() + }) + // Why: keep the queue alive past a failed run so later entrants still start. + void run.then(release, release) + return { + settled, + armReleaseCap: () => { + queuedBehind = true + armIfHolding() + } + } +} diff --git a/src/main/text-generation/commit-message-text-generation.test.ts b/src/main/text-generation/commit-message-text-generation.test.ts index 5c21ffa231e2..3d435ee94773 100644 --- a/src/main/text-generation/commit-message-text-generation.test.ts +++ b/src/main/text-generation/commit-message-text-generation.test.ts @@ -540,6 +540,38 @@ describe('discoverCommitMessageModelsLocal', () => { } }) + it('releases the Codex home after a discovery timeout once the child exits', async () => { + vi.useFakeTimers() + const firstChild = createMockDiscoveryChild() + const secondChild = createMockDiscoveryChild() + spawnMock.mockReturnValueOnce(firstChild as never).mockReturnValueOnce(secondChild as never) + const env = { CODEX_HOME: '/managed/codex-discovery-descendant-home' } + + try { + const first = discoverCommitMessageModelsLocal('codex', env) + await vi.advanceTimersByTimeAsync(0) + const second = discoverCommitMessageModelsLocal('codex', env) + await vi.advanceTimersByTimeAsync(60_000) + await expect(first).resolves.toMatchObject({ success: false }) + expect(spawnMock).toHaveBeenCalledTimes(1) + + // A grandchild kept the inherited stdout open, so the killed child reports + // 'exit' and 'close' never arrives. + firstChild.emit('exit', null, 'SIGKILL') + await vi.advanceTimersByTimeAsync(0) + expect(spawnMock).toHaveBeenCalledTimes(2) + + secondChild.stdout.emit( + 'data', + Buffer.from(JSON.stringify({ models: [{ slug: 'gpt-5.5', display_name: 'GPT-5.5' }] })) + ) + secondChild.emit('close', 0) + await expect(second).resolves.toMatchObject({ success: true, defaultModelId: 'gpt-5.5' }) + } finally { + vi.useRealTimers() + } + }) + it('settles and detaches model discovery when output exceeds the limit', async () => { const child = createMockDiscoveryChild() spawnMock.mockReturnValue(child as never) @@ -1692,6 +1724,39 @@ describe('generateCommitMessageFromContext', () => { await expect(second).resolves.toMatchObject({ success: true, message: 'Update README' }) }) + it('releases the Codex home lock when the child exits with a descendant holding its stdio', async () => { + const firstChild = createMockDiscoveryChild() + const secondChild = createMockDiscoveryChild() + spawnMock.mockReturnValueOnce(firstChild as never).mockReturnValueOnce(secondChild as never) + const env = { CODEX_HOME: '/managed/codex-descendant-home' } + const context = { branch: 'main', stagedSummary: 'M\tREADME.md', stagedPatch: '+hello' } + const params = { agentId: 'codex' as const, model: 'gpt-5.5' } + + const first = generateCommitMessageFromContext(context, params, { + kind: 'local', + cwd: '/descendant-repo', + env + }) + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(1)) + cancelGenerateCommitMessageLocal('/descendant-repo') + await expect(first).resolves.toMatchObject({ canceled: true }) + expectChildTerminated(firstChild) + + // SIGKILL reaches the codex process but not a grandchild that inherited its + // stdout, so 'exit' arrives and 'close' never does. + firstChild.emit('exit', null, 'SIGKILL') + + const second = generateCommitMessageFromContext(context, params, { + kind: 'local', + cwd: '/descendant-repo-2', + env + }) + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2)) + secondChild.stdout.emit('data', Buffer.from('Update README\n')) + secondChild.emit('close', 0) + await expect(second).resolves.toMatchObject({ success: true, message: 'Update README' }) + }) + it('holds the Codex home lock until Windows tree termination and wrapper close', async () => { const originalPlatform = process.platform Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) diff --git a/src/main/text-generation/commit-message-text-generation.ts b/src/main/text-generation/commit-message-text-generation.ts index 8410231dc409..98314f6975b1 100644 --- a/src/main/text-generation/commit-message-text-generation.ts +++ b/src/main/text-generation/commit-message-text-generation.ts @@ -452,6 +452,10 @@ export async function discoverCommitMessageModelsLocal( if (agentId === 'codex') { // Result publication stays prompt while the home lock follows the // process lifetime after asynchronous timeout/output-limit kills. + // Why: 'close' also waits on descendants that inherited this child's + // stdio, so a surviving MCP helper would hold the home forever; at + // 'exit' the codex process is gone and can no longer rotate auth.json. + child.once('exit', markClosedAfterTermination) child.once('close', markClosedAfterTermination) } child.on('error', onError) @@ -604,7 +608,7 @@ function runLocalPlan( emptyResultName = 'message', operation: TextGenerationOperation = 'commit-message', wslDistro?: string, - holdHomeLockUntilClose = false + holdHomeLockUntilExit = false ): LocalProcessExecution { const { binary, args, stdinPayload, label } = plan let markProcessClosed!: () => void @@ -685,7 +689,7 @@ function runLocalPlan( if (cancelToken && cancelTokensByLane.get(laneKey) === cancelToken) { cancelTokensByLane.delete(laneKey) } - if (!holdHomeLockUntilClose) { + if (!holdHomeLockUntilExit) { markProcessClosed() } resolve(result) @@ -769,7 +773,11 @@ function runLocalPlan( } child.stdout?.on('data', onStdoutData) child.stderr?.on('data', onStderrData) - if (holdHomeLockUntilClose) { + if (holdHomeLockUntilExit) { + // Why: 'close' also waits on descendants that inherited this child's + // stdio, so a surviving MCP helper would hold the home forever; at 'exit' + // the codex process is gone and can no longer rotate auth.json. + child.once('exit', markClosedAfterTermination) child.once('close', markClosedAfterTermination) } child.on('error', onError) @@ -801,7 +809,7 @@ function runLocalPlanForAgent( operation: TextGenerationOperation ): Promise { const start = ( - holdHomeLockUntilClose = false + holdHomeLockUntilExit = false ): LocalProcessExecution => runLocalPlan( plan, @@ -810,7 +818,7 @@ function runLocalPlanForAgent( emptyResultName, operation, target.wslDistro, - holdHomeLockUntilClose + holdHomeLockUntilExit ) if (agentId !== 'codex') { // Why: no extra promise hops here — cancellation timing for non-codex From 5aee22e980cd0c53abe3d998b50ce9a46f353d3a Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:06:02 -0700 Subject: [PATCH 2/2] fix(codex): preserve home lock exclusivity --- .../codex-cli/codex-home-process-lock.test.ts | 44 ++--------- src/main/codex-cli/codex-home-process-lock.ts | 78 +++---------------- 2 files changed, 17 insertions(+), 105 deletions(-) diff --git a/src/main/codex-cli/codex-home-process-lock.test.ts b/src/main/codex-cli/codex-home-process-lock.test.ts index edb27bc83b33..e5a7eee7976d 100644 --- a/src/main/codex-cli/codex-home-process-lock.test.ts +++ b/src/main/codex-cli/codex-home-process-lock.test.ts @@ -2,7 +2,6 @@ import { join } from 'node:path' import { homedir } from 'node:os' import { describe, expect, it, vi } from 'vitest' import { - CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS, resolveCodexHomeProcessLockKey, resolveCodexHomeProcessLockKeyForSpawnEnv, withCodexHomeProcessLock @@ -71,52 +70,25 @@ describe('withCodexHomeProcessLock', () => { await expect(withCodexHomeProcessLock('home-a', async () => 'after')).resolves.toBe('after') }) - it('releases the lock when a hold never settles', async () => { - vi.useFakeTimers() - try { - const events: string[] = [] - // A codex child whose surviving descendant keeps its stdio open never - // reports completion, so this hold never settles on its own. - void withCodexHomeProcessLock('home-wedged', () => new Promise(() => {})) - const queued = withCodexHomeProcessLock('home-wedged', async () => { - events.push('queued:start') - }) - - await vi.advanceTimersByTimeAsync(CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS - 1) - expect(events).toEqual([]) - - await vi.advanceTimersByTimeAsync(1) - await queued - expect(events).toEqual(['queued:start']) - } finally { - vi.useRealTimers() - } - }) - - it('measures the release cap from when a run starts, not when it is queued', async () => { + it('does not release a running lock based on elapsed time', async () => { vi.useFakeTimers() try { const events: string[] = [] const gate = deferred() - const first = withCodexHomeProcessLock('home-queued', async () => { + const first = withCodexHomeProcessLock('home-long-running', async () => { events.push('first:start') await gate.promise }) - void withCodexHomeProcessLock('home-queued', () => new Promise(() => {})) - const third = withCodexHomeProcessLock('home-queued', async () => { - events.push('third:start') + const second = withCodexHomeProcessLock('home-long-running', async () => { + events.push('second:start') }) - // Time spent waiting behind a healthy run must not burn the wedged run's cap. - await vi.advanceTimersByTimeAsync(CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS - 1_000) - gate.resolve() - await first - await vi.advanceTimersByTimeAsync(CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS - 1) + await vi.advanceTimersByTimeAsync(60 * 60_000) expect(events).toEqual(['first:start']) - await vi.advanceTimersByTimeAsync(1) - await third - expect(events).toEqual(['first:start', 'third:start']) + gate.resolve() + await Promise.all([first, second]) + expect(events).toEqual(['first:start', 'second:start']) } finally { vi.useRealTimers() } diff --git a/src/main/codex-cli/codex-home-process-lock.ts b/src/main/codex-cli/codex-home-process-lock.ts index 0d1f1224f8b7..ef91a5705d90 100644 --- a/src/main/codex-cli/codex-home-process-lock.ts +++ b/src/main/codex-cli/codex-home-process-lock.ts @@ -8,13 +8,7 @@ import { normalizeRuntimePathForComparison } from '../../shared/cross-platform-p // so Orca's own spawns (quota probes, commit-message runs) serialize per home. // User terminal panes are intentionally not serialized here. -type LockTail = { - settled: Promise - /** Bounds the running hold; only needed once someone is queued behind it. */ - armReleaseCap: () => void -} - -const lockTails = new Map() +const lockTails = new Map>() export function resolveCodexHomeProcessLockKey(codexHomePath?: string | null): string { const home = codexHomePath ?? process.env.CODEX_HOME ?? join(homedir(), '.codex') @@ -43,73 +37,19 @@ export function resolveCodexHomeProcessLockKeyForSpawnEnv( return normalizeRuntimePathForComparison(codexHome ?? join(homedir(), '.codex')) } -// Why: a hold that never settles (a killed codex child whose descendant keeps -// the inherited stdio open never reports close) would kill this home for the -// rest of the session. Far above the 60s generation timeout so healthy runs and -// their process teardown never trip it. -export const CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS = 5 * 60_000 - export function withCodexHomeProcessLock(lockKey: string, fn: () => Promise): Promise { - const prior = lockTails.get(lockKey) - prior?.armReleaseCap() - const started = prior?.settled ?? Promise.resolve() - const run = started.then(fn) - const tail = createLockTail(started, run, lockKey) + const prior = lockTails.get(lockKey) ?? Promise.resolve() + const run = prior.then(fn) + // Why: keep the queue alive past a failed run so later entrants still start. + const tail = run.then( + () => undefined, + () => undefined + ) lockTails.set(lockKey, tail) - void tail.settled.then(() => { + void tail.then(() => { if (lockTails.get(lockKey) === tail) { lockTails.delete(lockKey) } }) return run } - -function createLockTail( - started: Promise, - run: Promise, - lockKey: string -): LockTail { - let capTimer: ReturnType | null = null - let running = false - let queuedBehind = false - let release!: () => void - const settled = new Promise((resolve) => { - release = () => { - running = false - if (capTimer) { - clearTimeout(capTimer) - capTimer = null - } - resolve() - } - }) - // The cap only counts time this entry actually holds the lock, so waiting - // behind a slow predecessor never shortens a run's own budget. - const armIfHolding = (): void => { - if (!running || !queuedBehind || capTimer) { - return - } - capTimer = setTimeout(() => { - console.warn( - `[codex-home-lock] releasing ${lockKey} after ${CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS}ms without completion` - ) - release() - }, CODEX_HOME_PROCESS_LOCK_MAX_HOLD_MS) - if (typeof capTimer === 'object' && 'unref' in capTimer) { - capTimer.unref() - } - } - void started.then(() => { - running = true - armIfHolding() - }) - // Why: keep the queue alive past a failed run so later entrants still start. - void run.then(release, release) - return { - settled, - armReleaseCap: () => { - queuedBehind = true - armIfHolding() - } - } -}