From acb37a2b99fe72c10ddee43caf564000f75c00aa Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Mon, 3 Aug 2026 23:25:14 -0700 Subject: [PATCH] fix(codex): stop a surviving descendant wedging the Codex home process lock forever (#12410) * 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. * fix(codex): preserve home lock exclusivity --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> (cherry picked from commit 1ef83986a131e544294b14a6c47a969594a5b17d) --- .../codex-cli/codex-home-process-lock.test.ts | 26 +++++++- .../commit-message-text-generation.test.ts | 65 +++++++++++++++++++ .../commit-message-text-generation.ts | 18 +++-- 3 files changed, 103 insertions(+), 6 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 7287d011b42..e5a7eee7976 100644 --- a/src/main/codex-cli/codex-home-process-lock.test.ts +++ b/src/main/codex-cli/codex-home-process-lock.test.ts @@ -1,6 +1,6 @@ import { join } from 'node:path' import { homedir } from 'node:os' -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import { resolveCodexHomeProcessLockKey, resolveCodexHomeProcessLockKeyForSpawnEnv, @@ -70,6 +70,30 @@ describe('withCodexHomeProcessLock', () => { await expect(withCodexHomeProcessLock('home-a', async () => 'after')).resolves.toBe('after') }) + 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-long-running', async () => { + events.push('first:start') + await gate.promise + }) + const second = withCodexHomeProcessLock('home-long-running', async () => { + events.push('second:start') + }) + + await vi.advanceTimersByTimeAsync(60 * 60_000) + expect(events).toEqual(['first:start']) + + gate.resolve() + await Promise.all([first, second]) + expect(events).toEqual(['first:start', 'second: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/text-generation/commit-message-text-generation.test.ts b/src/main/text-generation/commit-message-text-generation.test.ts index 5c21ffa231e..3d435ee9477 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 8410231dc40..98314f6975b 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