Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion src/main/codex-cli/codex-home-process-lock.test.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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
Expand Down
65 changes: 65 additions & 0 deletions src/main/text-generation/commit-message-text-generation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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' })
Expand Down
18 changes: 13 additions & 5 deletions src/main/text-generation/commit-message-text-generation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -604,7 +608,7 @@ function runLocalPlan(
emptyResultName = 'message',
operation: TextGenerationOperation = 'commit-message',
wslDistro?: string,
holdHomeLockUntilClose = false
holdHomeLockUntilExit = false
): LocalProcessExecution<InternalTextGenerationResult> {
const { binary, args, stdinPayload, label } = plan
let markProcessClosed!: () => void
Expand Down Expand Up @@ -685,7 +689,7 @@ function runLocalPlan(
if (cancelToken && cancelTokensByLane.get(laneKey) === cancelToken) {
cancelTokensByLane.delete(laneKey)
}
if (!holdHomeLockUntilClose) {
if (!holdHomeLockUntilExit) {
markProcessClosed()
}
resolve(result)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -801,7 +809,7 @@ function runLocalPlanForAgent(
operation: TextGenerationOperation
): Promise<InternalTextGenerationResult> {
const start = (
holdHomeLockUntilClose = false
holdHomeLockUntilExit = false
): LocalProcessExecution<InternalTextGenerationResult> =>
runLocalPlan(
plan,
Expand All @@ -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
Expand Down