fix(codex): keep failed probes under home lock

This commit is contained in:
Brennan Benson
2026-08-02 17:41:36 -07:00
parent c9422edf4f
commit 765d117d9b
4 changed files with 136 additions and 7 deletions
@@ -265,4 +265,74 @@ describe('fetchCodexRateLimits probe shutdown', () => {
Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform })
}
})
it.each([
{ failAt: 1, phase: 'initial request' },
{ failAt: 2, phase: 'initialized notification' },
{ failAt: 3, phase: 'rate-limit request' }
])('keeps the home lock when the $phase stdin write throws', async ({ failAt, phase }) => {
const originalPlatform = process.platform
Object.defineProperty(process, 'platform', { configurable: true, value: 'darwin' })
const firstChild = makeRpcChild()
let writeCount = 0
firstChild.stdin.write.mockImplementation((line: string) => {
writeCount += 1
if (writeCount === failAt) {
throw new Error(`${phase} stdin failed`)
}
const message = JSON.parse(line) as { id?: number; method?: string }
if (message.method === 'initialize') {
setTimeout(() => {
firstChild.stdout.emit(
'data',
Buffer.from(`${JSON.stringify({ jsonrpc: '2.0', id: message.id, result: {} })}\n`)
)
}, 0)
}
})
firstChild.stdin.end = vi.fn()
firstChild.kill = vi.fn((signal?: string) => {
if (signal === 'SIGKILL') {
firstChild.exitCode = 1
firstChild.emit('exit', 1, 'SIGKILL')
}
return true
})
const secondChild = makeRpcChild()
childSpawnMock.mockReturnValueOnce(firstChild).mockReturnValueOnce(secondChild)
respondToRpcRateLimitRead(secondChild, {
primary: { usedPercent: 3 },
secondary: { usedPercent: 4 }
})
const home = `/managed/home-stdin-error-${failAt}`
try {
const first = fetchCodexRateLimits({
allowPtyFallback: false,
codexHomePath: home
})
await vi.advanceTimersByTimeAsync(0)
expect(firstChild.kill).toHaveBeenCalledWith('SIGTERM')
const second = fetchCodexRateLimits({
allowPtyFallback: false,
codexHomePath: home
})
await vi.advanceTimersByTimeAsync(4_999)
expect(childSpawnMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(1)
expect(firstChild.kill).toHaveBeenCalledWith('SIGKILL')
await vi.advanceTimersByTimeAsync(1)
expect(childSpawnMock).toHaveBeenCalledTimes(2)
await vi.advanceTimersByTimeAsync(1)
await expect(first).resolves.toMatchObject({
status: 'error',
error: `${phase} stdin failed`
})
await expect(second).resolves.toMatchObject({ status: 'ok' })
} finally {
Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform })
}
})
})
+18 -6
View File
@@ -722,9 +722,17 @@ async function fetchViaRpc(options?: FetchCodexRateLimitsOptions): Promise<Provi
// Why: send initialized after initialize or the server rejects methods.
let rateLimitsId: number | null = null
const initId = sendRpc('initialize', {
clientInfo: { name: 'orca', version: '1.0.0' }
})
let initId: number
try {
initId = sendRpc('initialize', {
clientInfo: { name: 'orca', version: '1.0.0' }
})
} catch (error) {
// A child already exists, so a synchronous pipe failure must follow the
// same reaping path as an asynchronous ChildProcess error.
onError(error instanceof Error ? error : new Error(String(error)))
return
}
function sendNotification(method: string): void {
child.stdin.write(`${JSON.stringify({ jsonrpc: '2.0', method, params: {} })}\n`)
@@ -753,9 +761,13 @@ async function fetchViaRpc(options?: FetchCodexRateLimitsOptions): Promise<Provi
if (msg.id === initId) {
// Why: the boot/auth-refresh budget ends here; the read gets its own deadline.
armRpcDeadline(rpcTimeoutMs)
// Initialize succeeded — send `initialized`, then request rate limits.
sendNotification('initialized')
rateLimitsId = sendRpc('account/rateLimits/read')
try {
// Initialize succeeded — send `initialized`, then request rate limits.
sendNotification('initialized')
rateLimitsId = sendRpc('account/rateLimits/read')
} catch (error) {
onError(error instanceof Error ? error : new Error(String(error)))
}
continue
}
@@ -5,6 +5,7 @@ import { CODEX_PROBE_SHUTDOWN_DRAIN_MS, terminateCodexProbeChild } from './codex
function makeFakeChild() {
const emitter = new EventEmitter()
const child = {
pid: 4321,
exitCode: null as number | null,
signalCode: null as NodeJS.Signals | null,
stdin: { end: vi.fn() },
@@ -89,12 +90,14 @@ describe('terminateCodexProbeChild', () => {
it('sends no signal during the drain window on Windows where kill is always forceful', async () => {
const child = makeFakeChild()
const done = terminateCodexProbeChild(child, { platform: 'win32' })
const killWindowsProcessTree = vi.fn(async () => {})
const done = terminateCodexProbeChild(child, { platform: 'win32', killWindowsProcessTree })
expect(child.stdin.end).toHaveBeenCalledTimes(1)
expect(child.kill).not.toHaveBeenCalled()
await vi.advanceTimersByTimeAsync(CODEX_PROBE_SHUTDOWN_DRAIN_MS)
expect(killWindowsProcessTree).toHaveBeenCalledWith(child.pid)
expect(child.kill).toHaveBeenCalledTimes(1)
expect(child.kill).toHaveBeenCalledWith()
@@ -102,6 +105,34 @@ describe('terminateCodexProbeChild', () => {
await done
})
it('waits for the Windows descendant tree kill before releasing the probe', async () => {
const child = makeFakeChild()
let finishTreeKill!: () => void
const treeKill = new Promise<void>((resolve) => {
finishTreeKill = resolve
})
const killWindowsProcessTree = vi.fn(() => treeKill)
let settled = false
const done = terminateCodexProbeChild(child, {
platform: 'win32',
drainMs: 0,
killWindowsProcessTree
}).then(() => {
settled = true
})
await vi.advanceTimersByTimeAsync(0)
expect(killWindowsProcessTree).toHaveBeenCalledWith(child.pid)
child.exit()
await Promise.resolve()
expect(settled).toBe(false)
finishTreeKill()
await done
expect(settled).toBe(true)
expect(child.kill).not.toHaveBeenCalled()
})
it('does nothing for a child that already exited', async () => {
const child = makeFakeChild()
child.exit()
@@ -1,3 +1,5 @@
import { terminateWindowsProcessTree, type WindowsTreeKiller } from '../windows-process-tree-kill'
// Why: Codex OAuth uses rotating refresh tokens. Hard-killing a probe's
// app-server mid-refresh can strand auth.json between rotations and permanently
// invalidate the stored credential, so termination always requests shutdown
@@ -7,6 +9,7 @@ export const CODEX_PROBE_SHUTDOWN_DRAIN_MS = 5_000
const HARD_KILL_EXIT_WAIT_MS = 1_000
export type TerminatableProbeChild = {
pid?: number
exitCode: number | null
signalCode?: NodeJS.Signals | null
kill: (signal?: NodeJS.Signals) => boolean
@@ -21,6 +24,7 @@ export type TerminateCodexProbeOptions = {
hardKillWaitMs?: number
// Why: injectable so the Windows branch is testable from POSIX CI hosts.
platform?: NodeJS.Platform
killWindowsProcessTree?: WindowsTreeKiller
}
function hasExited(child: TerminatableProbeChild): boolean {
@@ -82,6 +86,18 @@ export async function terminateCodexProbeChild(
if (await waitForExit(child, options?.drainMs ?? CODEX_PROBE_SHUTDOWN_DRAIN_MS)) {
return
}
if (platform === 'win32' && child.pid) {
try {
// npm-installed Codex runs beneath cmd.exe; killing only that wrapper can
// leave app-server alive after the credential-home lock is released.
await (options?.killWindowsProcessTree ?? terminateWindowsProcessTree)(child.pid)
} catch {
// The direct-child fallback still applies if an injected killer rejects.
}
}
if (hasExited(child)) {
return
}
try {
if (platform === 'win32') {
child.kill()