diff --git a/src/main/codex-accounts/codex-credential-absence-grace.test.ts b/src/main/codex-accounts/codex-credential-absence-grace.test.ts new file mode 100644 index 00000000000..6672bb7676a --- /dev/null +++ b/src/main/codex-accounts/codex-credential-absence-grace.test.ts @@ -0,0 +1,65 @@ +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { + CODEX_CREDENTIAL_ABSENCE_GRACE_MS, + CodexCredentialAbsenceGrace +} from './codex-credential-absence-grace' + +const VALID_AUTH = JSON.stringify({ + auth_mode: 'chatgpt', + tokens: { access_token: 'a', id_token: 'b', refresh_token: 'c' } +}) + +describe('CodexCredentialAbsenceGrace', () => { + let dir = '' + let authPath = '' + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'codex-absence-grace-')) + authPath = join(dir, 'auth.json') + }) + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }) + }) + + it('treats a torn mid-write read as transient until it outlives the grace window', () => { + const grace = new CodexCredentialAbsenceGrace() + writeFileSync(authPath, '{"tokens":{"acc', 'utf-8') + + expect(grace.assess(authPath, 1_000)).toEqual({ state: 'unreadable', durable: false }) + expect(grace.assess(authPath, 1_000 + CODEX_CREDENTIAL_ABSENCE_GRACE_MS - 1)).toEqual({ + state: 'unreadable', + durable: false + }) + expect(grace.assess(authPath, 1_000 + CODEX_CREDENTIAL_ABSENCE_GRACE_MS)).toEqual({ + state: 'unreadable', + durable: true + }) + }) + + it('clears the absence clock when a transient failure heals', () => { + const grace = new CodexCredentialAbsenceGrace() + writeFileSync(authPath, '{"tokens":{"acc', 'utf-8') + expect(grace.assess(authPath, 1_000).durable).toBe(false) + + writeFileSync(authPath, VALID_AUTH, 'utf-8') + expect(grace.assess(authPath, 2_000)).toEqual({ state: 'present', durable: true }) + + // A later absence starts a fresh grace window instead of inheriting the old clock. + rmSync(authPath) + expect(grace.assess(authPath, 100_000)).toEqual({ state: 'missing', durable: false }) + expect(grace.assess(authPath, 100_000 + CODEX_CREDENTIAL_ABSENCE_GRACE_MS)).toEqual({ + state: 'missing', + durable: true + }) + }) + + it('reports settled credential-free JSON as durable immediately', () => { + const grace = new CodexCredentialAbsenceGrace() + writeFileSync(authPath, '{}', 'utf-8') + expect(grace.assess(authPath, 1_000)).toEqual({ state: 'no-credential', durable: true }) + }) +}) diff --git a/src/main/codex-accounts/codex-credential-absence-grace.ts b/src/main/codex-accounts/codex-credential-absence-grace.ts new file mode 100644 index 00000000000..c3492d41ffd --- /dev/null +++ b/src/main/codex-accounts/codex-credential-absence-grace.ts @@ -0,0 +1,46 @@ +import { existsSync } from 'node:fs' +import { dirname } from 'node:path' +import { normalizeRuntimePathForComparison } from '../../shared/cross-platform-path' +import { + readStoredCodexCredentialState, + type StoredCodexCredentialState +} from './managed-codex-auth-readiness' + +export const CODEX_CREDENTIAL_ABSENCE_GRACE_MS = 5_000 + +export type CodexCredentialAbsenceVerdict = { + state: StoredCodexCredentialState + /** For 'missing'/'unreadable': true once the absence outlived the grace window. */ + durable: boolean +} + +// Why: codex rotates auth.json in place, so one missing/unreadable read can be +// a write in progress. Deselecting the managed account on that snapshot logs +// the user out over a race; absence must persist across a grace window first. +// Valid JSON (with or without a credential) is a settled file and needs none. +export class CodexCredentialAbsenceGrace { + private readonly firstAbsenceAtByPath = new Map() + + constructor(private readonly graceMs = CODEX_CREDENTIAL_ABSENCE_GRACE_MS) {} + + assess(authPath: string, now = Date.now()): CodexCredentialAbsenceVerdict { + const state = readStoredCodexCredentialState(authPath) + const key = normalizeRuntimePathForComparison(authPath) + if (state === 'present' || state === 'no-credential') { + this.firstAbsenceAtByPath.delete(key) + return { state, durable: true } + } + // Why: rotation replaces auth.json inside an existing home; the home + // directory itself being gone is structural, not a write in flight. + if (state === 'missing' && !existsSync(dirname(authPath))) { + this.firstAbsenceAtByPath.delete(key) + return { state, durable: true } + } + const firstAbsenceAt = this.firstAbsenceAtByPath.get(key) + if (firstAbsenceAt === undefined) { + this.firstAbsenceAtByPath.set(key, now) + return { state, durable: false } + } + return { state, durable: now - firstAbsenceAt >= this.graceMs } + } +} diff --git a/src/main/codex-accounts/managed-codex-auth-readiness.ts b/src/main/codex-accounts/managed-codex-auth-readiness.ts index 4c9f28eec70..ce35e56652a 100644 --- a/src/main/codex-accounts/managed-codex-auth-readiness.ts +++ b/src/main/codex-accounts/managed-codex-auth-readiness.ts @@ -85,19 +85,40 @@ function isManagedHostCodexHome( ) } -export function hasStoredCodexCredential(authPath: string): boolean { +export type StoredCodexCredentialState = 'present' | 'missing' | 'unreadable' | 'no-credential' + +// Why: callers deciding whether to deselect an account must tell a settled +// logout ('no-credential') apart from a rotation in progress ('unreadable') or +// an absent file ('missing') — collapsing them to false logs users out on races. +export function readStoredCodexCredentialState(authPath: string): StoredCodexCredentialState { + let raw: string try { - const parsed: unknown = JSON.parse(readFileSync(authPath, 'utf8')) - if (!isRecord(parsed) || Object.keys(parsed).length === 0) { - return false - } - const auth = parsed as StoredCodexAuth - return auth.auth_mode == null + raw = readFileSync(authPath, 'utf8') + } catch (error) { + const code = (error as NodeJS.ErrnoException).code + // Why: Windows auth.json rotation can surface transient EPERM/EBUSY reads. + return code === 'ENOENT' || code === 'ENOTDIR' ? 'missing' : 'unreadable' + } + let parsed: unknown + try { + parsed = JSON.parse(raw) + } catch { + // Torn JSON means a write is in flight, not that the credential is gone. + return 'unreadable' + } + if (!isRecord(parsed) || Object.keys(parsed).length === 0) { + return 'no-credential' + } + const auth = parsed as StoredCodexAuth + const hasCredential = + auth.auth_mode == null ? hasCredentialWithoutDeclaredMode(auth) : hasCredentialForDeclaredMode(auth) - } catch { - return false - } + return hasCredential ? 'present' : 'no-credential' +} + +export function hasStoredCodexCredential(authPath: string): boolean { + return readStoredCodexCredentialState(authPath) === 'present' } function hasCredentialForDeclaredMode(auth: StoredCodexAuth): boolean { diff --git a/src/main/codex-accounts/runtime-home-service.test.ts b/src/main/codex-accounts/runtime-home-service.test.ts index d1a8b1bed0d..eaafafc736e 100644 --- a/src/main/codex-accounts/runtime-home-service.test.ts +++ b/src/main/codex-accounts/runtime-home-service.test.ts @@ -2389,11 +2389,27 @@ describe('CodexRuntimeHomeService', () => { expect(store.updateSettings).not.toHaveBeenCalled() rmSync(join(managedHomePath1, 'auth.json'), { force: true }) + // Why: the first missing read may be a rotation in flight — the selection + // survives the grace window and the launch still targets the account home. + const absenceObservedAt = Date.now() expect( service.prepareForCodexLaunch(undefined, undefined, { unavailableManagedHomePath: managedHomePath1 }) - ).toBeNull() + ).toBe(managedHomePath1) + expect(store.updateSettings).not.toHaveBeenCalled() + + // Once the absence outlives the grace window it is durable and deselects. + const nowSpy = vi.spyOn(Date, 'now').mockImplementation(() => absenceObservedAt + 6_000) + try { + expect( + service.prepareForCodexLaunch(undefined, undefined, { + unavailableManagedHomePath: managedHomePath1 + }) + ).toBeNull() + } finally { + nowSpy.mockRestore() + } expect(store.getSettings().activeCodexManagedAccountId).toBeNull() expect(store.getSettings().activeCodexManagedAccountIdsByRuntime).toEqual({ host: null, @@ -2408,6 +2424,66 @@ describe('CodexRuntimeHomeService', () => { ) }) + it('does not deselect the account when a transient unreadable auth.json heals', async () => { + const systemAuth = createCodexAuthJson('system@example.com', 'acct-system', 'system') + writeFileSync(getSystemCodexAuthPath(), systemAuth, 'utf-8') + const accountAuth = createCodexAuthJson('user@example.com', 'acct-user', 'managed') + const managedHomePath = createManagedAuth(testState.userDataDir, 'account-1', accountAuth) + const store = createStore( + createSettings({ + codexSystemDefaultRealHomeEnabled: true, + codexManagedAccounts: [ + { + id: 'account-1', + email: 'user@example.com', + managedHomePath, + providerAccountId: 'acct-user', + workspaceLabel: null, + workspaceAccountId: 'acct-user', + createdAt: 1, + updatedAt: 1, + lastAuthenticatedAt: 1 + } + ], + activeCodexManagedAccountId: 'account-1', + activeCodexManagedAccountIdsByRuntime: { host: 'account-1', wsl: {} } + }) + ) + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const { CodexRuntimeHomeService } = await import('./runtime-home-service') + const service = new CodexRuntimeHomeService(store as never) + + // A mid-write read observes torn JSON: no deselect, and the launch still + // targets the account home so codex re-reads the settled file itself. + writeFileSync(join(managedHomePath, 'auth.json'), '{"tokens":{"acc', 'utf-8') + expect( + service.prepareForCodexLaunch(undefined, undefined, { + unavailableManagedHomePath: managedHomePath + }) + ).toBe(managedHomePath) + expect(store.getSettings().activeCodexManagedAccountId).toBe('account-1') + expect(store.updateSettings).not.toHaveBeenCalled() + + // The write completes; even past the grace window the selection is intact. + writeFileSync(join(managedHomePath, 'auth.json'), accountAuth, 'utf-8') + const healedAt = Date.now() + const nowSpy = vi.spyOn(Date, 'now').mockImplementation(() => healedAt + 60_000) + try { + expect( + service.prepareForCodexLaunch(undefined, undefined, { + unavailableManagedHomePath: managedHomePath + }) + ).toBe(managedHomePath) + } finally { + nowSpy.mockRestore() + } + expect(store.getSettings().activeCodexManagedAccountId).toBe('account-1') + expect(store.updateSettings).not.toHaveBeenCalled() + expect(warnSpy).not.toHaveBeenCalledWith( + '[codex-runtime-home] Active managed account credential remained unavailable, clearing selection' + ) + }) + it('ignores mismatched runtime auth when the active managed auth is missing', async () => { const runtimeAuthPath = getRuntimeCodexAuthPath() const systemAuth = createCodexAuthJson('system@example.com', 'acct-system', 'system') diff --git a/src/main/codex-accounts/runtime-home-service.ts b/src/main/codex-accounts/runtime-home-service.ts index e9256145b77..2bece79fd9f 100644 --- a/src/main/codex-accounts/runtime-home-service.ts +++ b/src/main/codex-accounts/runtime-home-service.ts @@ -77,7 +77,7 @@ import { codexAuthMatchesSystemDefaultIdentity } from './codex-auth-identity' import { migrateLegacySharedAuthToPerAccountHome } from './legacy-shared-auth-migration' -import { hasStoredCodexCredential } from './managed-codex-auth-readiness' +import { CodexCredentialAbsenceGrace } from './codex-credential-absence-grace' import { syncLegacySharedCodexConfigForRetainedPanes } from './legacy-shared-config-compatibility' import { hasRecordedLegacySharedCodexPane, @@ -179,6 +179,8 @@ export class CodexRuntimeHomeService { // provenance so a later deselect/rollback never adopts stale shared bytes. private lastHostAccountUsedSelfContainedHome = false private sharedAuthRefreshBlockedByManagedTransition = false + // Why: transient auth.json read/parse failures must not deselect an account. + private readonly credentialAbsenceGrace = new CodexCredentialAbsenceGrace() constructor(private readonly store: Store) { this.safeRecoverInterruptedRuntimeAuthOperation() @@ -307,11 +309,15 @@ export class CodexRuntimeHomeService { if ( unavailableManagedHomePath && normalizeRuntimePathForComparison(unavailableManagedHomePath) === - normalizeRuntimePathForComparison(perAccountHome) && - !hasStoredCodexCredential(join(perAccountHome, 'auth.json')) + normalizeRuntimePathForComparison(perAccountHome) ) { - this.clearSelfContainedManagedSelection(account, 'credential remained unavailable') - return null + const absence = this.credentialAbsenceGrace.assess(join(perAccountHome, 'auth.json')) + if (absence.state !== 'present' && absence.durable) { + this.clearSelfContainedManagedSelection(account, 'credential remained unavailable') + return null + } + // Why: a transient missing/unreadable auth.json is usually codex rotating + // it; keep the selection and launch — the CLI re-reads the settled file. } // Why: link the user's real ~/.codex resources and mirror config into THIS // home (never symlinking into or mutating ~/.codex), so the per-account home @@ -732,7 +738,16 @@ export class CodexRuntimeHomeService { } const activeAuthPath = join(activeAccount.managedHomePath, 'auth.json') - if (!existsSync(activeAuthPath)) { + const authAbsence = this.credentialAbsenceGrace.assess(activeAuthPath) + if (authAbsence.state === 'missing' || authAbsence.state === 'unreadable') { + if (authAbsence.state === 'unreadable' || !authAbsence.durable) { + // Why: mid-rotation reads look missing/unreadable for a moment; skip + // this sync without deselecting and let a settled read decide later. + console.warn( + '[codex-runtime-home] Active managed account auth.json unavailable, keeping selection through grace window' + ) + return + } console.warn( '[codex-runtime-home] Active managed account is missing auth.json, restoring system default' ) diff --git a/src/main/codex-cli/codex-home-process-lock.test.ts b/src/main/codex-cli/codex-home-process-lock.test.ts new file mode 100644 index 00000000000..3393b6f097d --- /dev/null +++ b/src/main/codex-cli/codex-home-process-lock.test.ts @@ -0,0 +1,96 @@ +import { join } from 'node:path' +import { homedir } from 'node:os' +import { describe, expect, it } from 'vitest' +import { + resolveCodexHomeProcessLockKey, + resolveCodexHomeProcessLockKeyForSpawnEnv, + withCodexHomeProcessLock +} from './codex-home-process-lock' + +function deferred() { + let resolve!: (value: T) => void + let reject!: (error: unknown) => void + const promise = new Promise((res, rej) => { + resolve = res + reject = rej + }) + return { promise, resolve, reject } +} + +describe('withCodexHomeProcessLock', () => { + it('serializes runs that share a lock key', async () => { + const events: string[] = [] + const firstGate = deferred() + + const first = withCodexHomeProcessLock('home-a', async () => { + events.push('first:start') + await firstGate.promise + events.push('first:end') + return 1 + }) + const second = withCodexHomeProcessLock('home-a', async () => { + events.push('second:start') + return 2 + }) + + await Promise.resolve() + expect(events).toEqual(['first:start']) + + firstGate.resolve() + await expect(first).resolves.toBe(1) + await expect(second).resolves.toBe(2) + expect(events).toEqual(['first:start', 'first:end', 'second:start']) + }) + + it('runs different lock keys concurrently', async () => { + const events: string[] = [] + const firstGate = deferred() + + const first = withCodexHomeProcessLock('home-a', async () => { + events.push('a:start') + await firstGate.promise + }) + const second = withCodexHomeProcessLock('home-b', async () => { + events.push('b:start') + }) + + await second + expect(events).toEqual(['a:start', 'b:start']) + firstGate.resolve() + await first + }) + + it('releases the lock after a rejected run', async () => { + await expect( + withCodexHomeProcessLock('home-a', async () => { + throw new Error('boom') + }) + ).rejects.toThrow('boom') + + await expect(withCodexHomeProcessLock('home-a', async () => 'after')).resolves.toBe('after') + }) + + it('keys explicit and default host homes consistently', () => { + const previousCodexHome = process.env.CODEX_HOME + delete process.env.CODEX_HOME + try { + const defaultKey = resolveCodexHomeProcessLockKey(null) + expect(resolveCodexHomeProcessLockKey(join(homedir(), '.codex'))).toBe(defaultKey) + expect(resolveCodexHomeProcessLockKeyForSpawnEnv(undefined)).toBe(defaultKey) + expect(resolveCodexHomeProcessLockKey('/somewhere/else')).not.toBe(defaultKey) + } finally { + if (previousCodexHome !== undefined) { + process.env.CODEX_HOME = previousCodexHome + } + } + }) + + it('keys a WSL UNC probe home and a WSL spawn env to the same lock', () => { + const probeKey = resolveCodexHomeProcessLockKey('\\\\wsl$\\Ubuntu\\home\\user\\.codex') + const spawnKey = resolveCodexHomeProcessLockKeyForSpawnEnv( + { CODEX_HOME: '/home/user/.codex' }, + 'Ubuntu' + ) + expect(spawnKey).toBe(probeKey) + }) +}) diff --git a/src/main/codex-cli/codex-home-process-lock.ts b/src/main/codex-cli/codex-home-process-lock.ts new file mode 100644 index 00000000000..0472eca622f --- /dev/null +++ b/src/main/codex-cli/codex-home-process-lock.ts @@ -0,0 +1,50 @@ +import { homedir } from 'node:os' +import { join } from 'node:path' +import { normalizeRuntimePathForComparison } from '../../shared/cross-platform-path' + +// Why: Codex OAuth uses rotating refresh tokens stored in each home's auth.json. +// Two Orca-spawned codex processes refreshing the same home concurrently can +// consume one rotation twice and permanently invalidate the stored credential, +// 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>() + +export function resolveCodexHomeProcessLockKey(codexHomePath?: string | null): string { + const home = codexHomePath ?? process.env.CODEX_HOME ?? join(homedir(), '.codex') + return normalizeRuntimePathForComparison(home) +} + +export function resolveCodexHomeProcessLockKeyForSpawnEnv( + env: NodeJS.ProcessEnv | undefined, + wslDistro?: string | null +): string { + const codexHome = env?.CODEX_HOME ?? null + if (wslDistro) { + // Why: WSL spawns carry a Linux CODEX_HOME; key it through the same UNC + // normalization the probe's \\wsl$ home path uses so both lanes collide. + // Without an explicit home the distro default is unknowable from the host; + // a sentinel still serializes same-distro default spawns with each other. + return normalizeRuntimePathForComparison( + `//wsl$/${wslDistro}${codexHome ?? '/.orca-default-codex-home'}` + ) + } + return resolveCodexHomeProcessLockKey(codexHome) +} + +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 + ) + lockTails.set(lockKey, tail) + void tail.then(() => { + if (lockTails.get(lockKey) === tail) { + lockTails.delete(lockKey) + } + }) + return run +} diff --git a/src/main/rate-limits/codex-fetcher-auth-errors.test.ts b/src/main/rate-limits/codex-fetcher-auth-errors.test.ts index 5f5944191c0..9cdf2acc405 100644 --- a/src/main/rate-limits/codex-fetcher-auth-errors.test.ts +++ b/src/main/rate-limits/codex-fetcher-auth-errors.test.ts @@ -34,13 +34,24 @@ function makeRpcChild() { const child = new EventEmitter() as EventEmitter & { stdout: EventEmitter stderr: EventEmitter - stdin: { write: ReturnType } + stdin: { write: ReturnType; end: ReturnType } kill: ReturnType + exitCode: number | null } child.stdout = new EventEmitter() child.stderr = new EventEmitter() - child.stdin = { write: vi.fn() } - child.kill = vi.fn() + // Why: like the real app-server, the fake dies on stdin EOF or a signal — + // the graceful shutdown path resolves only once the child reports exit. + const exitNow = (): void => { + child.exitCode = 0 + child.emit('exit', 0, null) + } + child.stdin = { write: vi.fn(), end: vi.fn(exitNow) } + child.exitCode = null + child.kill = vi.fn(() => { + exitNow() + return true + }) return child } diff --git a/src/main/rate-limits/codex-fetcher-probe-shutdown.test.ts b/src/main/rate-limits/codex-fetcher-probe-shutdown.test.ts new file mode 100644 index 00000000000..fdc7106fbfe --- /dev/null +++ b/src/main/rate-limits/codex-fetcher-probe-shutdown.test.ts @@ -0,0 +1,222 @@ +import { EventEmitter } from 'node:events' +import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest' + +const { childSpawnMock, readFileMock, resolveCodexCommandMock, ptySpawnMock } = vi.hoisted(() => ({ + childSpawnMock: vi.fn(), + readFileMock: vi.fn(), + resolveCodexCommandMock: vi.fn(), + ptySpawnMock: vi.fn() +})) + +vi.mock('node:child_process', () => ({ + spawn: childSpawnMock +})) + +vi.mock('node:fs/promises', () => ({ + readFile: readFileMock +})) + +vi.mock('../codex-cli/command', () => ({ + resolveCodexCommand: resolveCodexCommandMock +})) + +vi.mock('node-pty', () => ({ + spawn: ptySpawnMock +})) + +// Auth gate is covered by codex-auth-presence.test.ts; these tests assume a signed-in Codex. +vi.mock('./codex-auth-presence', () => ({ + probeCodexAuthPresence: vi.fn(() => 'present') +})) + +import { fetchCodexRateLimits } from './codex-fetcher' +import { probeCodexAuthPresence } from './codex-auth-presence' + +function makeRpcChild() { + const child = new EventEmitter() as EventEmitter & { + stdout: EventEmitter + stderr: EventEmitter + stdin: { write: ReturnType; end: ReturnType } + kill: ReturnType + exitCode: number | null + } + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + // Why: like the real app-server, the fake dies on stdin EOF or a signal — + // the graceful shutdown path resolves only once the child reports exit. + const exitNow = (): void => { + child.exitCode = 0 + child.emit('exit', 0, null) + } + child.stdin = { write: vi.fn(), end: vi.fn(exitNow) } + child.exitCode = null + child.kill = vi.fn(() => { + exitNow() + return true + }) + return child +} + +function respondToRpcRateLimitRead( + rpcChild: ReturnType, + rateLimits: unknown +): void { + rpcChild.stdin.write.mockImplementation((line: string) => { + const msg = JSON.parse(line) as { id?: number; method?: string } + if (msg.method === 'initialize') { + setTimeout(() => { + rpcChild.stdout.emit( + 'data', + Buffer.from(`${JSON.stringify({ jsonrpc: '2.0', id: msg.id, result: {} })}\n`) + ) + }, 0) + } + if (msg.method === 'account/rateLimits/read') { + setTimeout(() => { + rpcChild.stdout.emit( + 'data', + Buffer.from(`${JSON.stringify({ jsonrpc: '2.0', id: msg.id, result: { rateLimits } })}\n`) + ) + }, 0) + } + }) +} + +describe('fetchCodexRateLimits probe shutdown', () => { + beforeEach(() => { + vi.useFakeTimers() + vi.clearAllMocks() + resolveCodexCommandMock.mockReturnValue('codex') + vi.mocked(probeCodexAuthPresence).mockResolvedValue('present') + readFileMock.mockRejectedValue(new Error('no auth fixture')) + vi.stubGlobal('fetch', vi.fn()) + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('gives a cold app-server the full boot budget before any deadline fires', async () => { + const rpcChild = makeRpcChild() + childSpawnMock.mockReturnValue(rpcChild) + rpcChild.stdin.write.mockImplementation((line: string) => { + const msg = JSON.parse(line) as { id?: number; method?: string } + if (msg.method === 'initialize') { + // Why: cold starts (auth refresh included) are documented at 10-25s; + // the old 10s deadline killed them mid-auth. + setTimeout(() => { + rpcChild.stdout.emit( + 'data', + Buffer.from(`${JSON.stringify({ jsonrpc: '2.0', id: msg.id, result: {} })}\n`) + ) + }, 20_000) + } + if (msg.method === 'account/rateLimits/read') { + setTimeout(() => { + rpcChild.stdout.emit( + 'data', + Buffer.from( + `${JSON.stringify({ + jsonrpc: '2.0', + id: msg.id, + result: { + rateLimits: { primary: { usedPercent: 3 }, secondary: { usedPercent: 4 } } + } + })}\n` + ) + ) + }, 0) + } + }) + + const resultPromise = fetchCodexRateLimits({ allowPtyFallback: false }) + await vi.advanceTimersByTimeAsync(19_999) + expect(rpcChild.kill).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(2) + await expect(resultPromise).resolves.toMatchObject({ + status: 'ok', + session: { usedPercent: 3 }, + weekly: { usedPercent: 4 } + }) + }) + + it('arms the read deadline after initialize and drains SIGTERM before SIGKILL', async () => { + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: 'darwin' }) + const rpcChild = makeRpcChild() + // Stubborn child: it ignores stdin EOF, survives SIGTERM, dies on SIGKILL. + rpcChild.stdin.end = vi.fn() + rpcChild.kill = vi.fn((signal?: string) => { + if (signal === 'SIGKILL') { + rpcChild.exitCode = 0 + rpcChild.emit('exit', 0, null) + } + return true + }) + childSpawnMock.mockReturnValue(rpcChild) + rpcChild.stdin.write.mockImplementation((line: string) => { + const msg = JSON.parse(line) as { id?: number; method?: string } + if (msg.method === 'initialize') { + setTimeout(() => { + rpcChild.stdout.emit( + 'data', + Buffer.from(`${JSON.stringify({ jsonrpc: '2.0', id: msg.id, result: {} })}\n`) + ) + }, 0) + } + // The rate-limits read never answers, so the request deadline expires. + }) + + try { + const resultPromise = fetchCodexRateLimits({ allowPtyFallback: false }) + await vi.advanceTimersByTimeAsync(0) + await vi.advanceTimersByTimeAsync(9_999) + expect(rpcChild.kill).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(1) + expect(rpcChild.stdin.end).toHaveBeenCalledTimes(1) + expect(rpcChild.kill).toHaveBeenCalledWith('SIGTERM') + expect(rpcChild.kill).not.toHaveBeenCalledWith('SIGKILL') + + await vi.advanceTimersByTimeAsync(5_000) + expect(rpcChild.kill).toHaveBeenCalledWith('SIGKILL') + await expect(resultPromise).resolves.toMatchObject({ + status: 'error', + error: 'RPC timeout' + }) + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + } + }) + + it('does not spawn a second probe for the same home while one is still running', async () => { + const firstChild = makeRpcChild() + const secondChild = makeRpcChild() + childSpawnMock.mockReturnValueOnce(firstChild).mockReturnValueOnce(secondChild) + respondToRpcRateLimitRead(secondChild, { + primary: { usedPercent: 3 }, + secondary: { usedPercent: 4 } + }) + + const first = fetchCodexRateLimits({ + allowPtyFallback: false, + codexHomePath: '/managed/home-a' + }) + await vi.advanceTimersByTimeAsync(0) + const second = fetchCodexRateLimits({ + allowPtyFallback: false, + codexHomePath: '/managed/home-a' + }) + await vi.advanceTimersByTimeAsync(0) + expect(childSpawnMock).toHaveBeenCalledTimes(1) + + firstChild.emit('close') + await vi.advanceTimersByTimeAsync(1) + await vi.advanceTimersByTimeAsync(1) + await vi.advanceTimersByTimeAsync(1) + expect(childSpawnMock).toHaveBeenCalledTimes(2) + await expect(second).resolves.toMatchObject({ status: 'ok' }) + await first + }) +}) diff --git a/src/main/rate-limits/codex-fetcher-session-supplement.test.ts b/src/main/rate-limits/codex-fetcher-session-supplement.test.ts index 9bef59e96bd..40fc6b34b54 100644 --- a/src/main/rate-limits/codex-fetcher-session-supplement.test.ts +++ b/src/main/rate-limits/codex-fetcher-session-supplement.test.ts @@ -20,13 +20,25 @@ function makeRpcChild(rateLimitResetCredits?: unknown) { const child = new EventEmitter() as EventEmitter & { stdout: EventEmitter stderr: EventEmitter - stdin: { write: ReturnType } + stdin: { write: ReturnType; end: ReturnType } kill: ReturnType + exitCode: number | null } child.stdout = new EventEmitter() child.stderr = new EventEmitter() - child.kill = vi.fn() + child.exitCode = null + // Why: like the real app-server, the fake dies on stdin EOF or a signal — + // the graceful shutdown path resolves only once the child reports exit. + const exitNow = (): void => { + child.exitCode = 0 + child.emit('exit', 0, null) + } + child.kill = vi.fn(() => { + exitNow() + return true + }) child.stdin = { + end: vi.fn(exitNow), write: vi.fn((line: string) => { const message = JSON.parse(line) as { id?: number; method?: string } if (message.method === 'initialize') { diff --git a/src/main/rate-limits/codex-fetcher.test.ts b/src/main/rate-limits/codex-fetcher.test.ts index 11057a1fc81..4c086b4919f 100644 --- a/src/main/rate-limits/codex-fetcher.test.ts +++ b/src/main/rate-limits/codex-fetcher.test.ts @@ -43,13 +43,24 @@ function makeRpcChild() { const child = new EventEmitter() as EventEmitter & { stdout: EventEmitter stderr: EventEmitter - stdin: { write: ReturnType } + stdin: { write: ReturnType; end: ReturnType } kill: ReturnType + exitCode: number | null } child.stdout = new EventEmitter() child.stderr = new EventEmitter() - child.stdin = { write: vi.fn() } - child.kill = vi.fn() + // Why: like the real app-server, the fake dies on stdin EOF or a signal — + // the graceful shutdown path resolves only once the child reports exit. + const exitNow = (): void => { + child.exitCode = 0 + child.emit('exit', 0, null) + } + child.stdin = { write: vi.fn(), end: vi.fn(exitNow) } + child.exitCode = null + child.kill = vi.fn(() => { + exitNow() + return true + }) return child } @@ -333,7 +344,8 @@ describe('fetchCodexRateLimits', () => { childSpawnMock.mockReturnValue(rpcChild) const resultPromise = fetchCodexRateLimits({ allowPtyFallback: false }) - await vi.advanceTimersByTimeAsync(10_000) + // Why: without an initialize response only the 30s boot deadline fires. + await vi.advanceTimersByTimeAsync(30_000) await expect(resultPromise).resolves.toMatchObject({ provider: 'codex', diff --git a/src/main/rate-limits/codex-fetcher.ts b/src/main/rate-limits/codex-fetcher.ts index 9173d192038..7976a07b0c8 100644 --- a/src/main/rate-limits/codex-fetcher.ts +++ b/src/main/rate-limits/codex-fetcher.ts @@ -36,9 +36,19 @@ import { createAuthFilesystemOperation, type SharedAuthFilesystemOperation } from './auth-filesystem-operation' +import { terminateCodexProbeChild } from './codex-probe-termination' +import { + resolveCodexHomeProcessLockKey, + withCodexHomeProcessLock +} from '../codex-cli/codex-home-process-lock' const RPC_TIMEOUT_MS = 10_000 const WSL_RPC_TIMEOUT_MS = 25_000 +// Why: cold app-server starts (fresh token refresh included) are documented at +// 10-25s; the request deadline is armed only after initialize responds, so +// these boot budgets are what protect a slow cold start from a mid-auth kill. +const RPC_INIT_TIMEOUT_MS = 30_000 +const WSL_RPC_INIT_TIMEOUT_MS = 40_000 const PTY_TIMEOUT_MS = 15_000 // Why: codex ≥0.145 renders a '›' composer with placeholder text after it, so a // prompt-anchored send can never fire; nudge /status after a short boot grace. @@ -619,7 +629,8 @@ async function fetchViaRpc(options?: FetchCodexRateLimitsOptions): Promise resolve(result), + () => resolve(result) + ) + return } resolve(result) } @@ -676,19 +693,25 @@ async function fetchViaRpc(options?: FetchCodexRateLimitsOptions): Promise { - settle( - { - provider: 'codex', - session: null, - weekly: null, - updatedAt: Date.now(), - error: 'RPC timeout', - status: 'error' - }, - { kill: true } - ) - }, rpcTimeoutMs) + function armRpcDeadline(deadlineMs: number): void { + if (timeout) { + clearTimeout(timeout) + } + timeout = setTimeout(() => { + settle( + { + provider: 'codex', + session: null, + weekly: null, + updatedAt: Date.now(), + error: 'RPC timeout', + status: 'error' + }, + { kill: true } + ) + }, deadlineMs) + } + armRpcDeadline(initTimeoutMs) function sendRpc(method: string, params?: unknown): number { const id = ++rpcId @@ -728,6 +751,8 @@ async function fetchViaRpc(options?: FetchCodexRateLimitsOptions): Promise fetchViaRpc(options)) if (options?.signal?.aborted) { return abortedCodexRateLimitResult() } @@ -1238,7 +1268,7 @@ export async function fetchCodexRateLimits( if (options?.signal?.aborted) { return abortedCodexRateLimitResult() } - const ptyResult = await fetchViaPty(options) + const ptyResult = await withCodexHomeProcessLock(homeLockKey, () => fetchViaPty(options)) if (options?.signal?.aborted) { return abortedCodexRateLimitResult() } diff --git a/src/main/rate-limits/codex-probe-termination.test.ts b/src/main/rate-limits/codex-probe-termination.test.ts new file mode 100644 index 00000000000..ec810440306 --- /dev/null +++ b/src/main/rate-limits/codex-probe-termination.test.ts @@ -0,0 +1,94 @@ +import { EventEmitter } from 'node:events' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { CODEX_PROBE_SHUTDOWN_DRAIN_MS, terminateCodexProbeChild } from './codex-probe-termination' + +function makeFakeChild() { + const emitter = new EventEmitter() + const child = { + exitCode: null as number | null, + signalCode: null as NodeJS.Signals | null, + stdin: { end: vi.fn() }, + kill: vi.fn((_signal?: NodeJS.Signals) => true), + once: emitter.once.bind(emitter), + off: emitter.off.bind(emitter), + exit(code = 0) { + child.exitCode = code + emitter.emit('exit') + } + } + return child +} + +describe('terminateCodexProbeChild', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('requests shutdown and never hard-kills a child that drains in time', async () => { + const child = makeFakeChild() + const done = terminateCodexProbeChild(child, { platform: 'darwin' }) + + expect(child.stdin.end).toHaveBeenCalledTimes(1) + expect(child.kill).toHaveBeenCalledWith('SIGTERM') + + await vi.advanceTimersByTimeAsync(CODEX_PROBE_SHUTDOWN_DRAIN_MS - 1) + child.exit() + await done + + expect(child.kill).toHaveBeenCalledTimes(1) + expect(child.kill).not.toHaveBeenCalledWith('SIGKILL') + }) + + it('escalates to SIGKILL only after the drain window elapses', async () => { + const child = makeFakeChild() + const done = terminateCodexProbeChild(child, { platform: 'linux' }) + + await vi.advanceTimersByTimeAsync(CODEX_PROBE_SHUTDOWN_DRAIN_MS) + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + + child.exit() + await done + expect(child.kill.mock.calls.map((call) => call[0])).toEqual(['SIGTERM', 'SIGKILL']) + }) + + it('resolves within the hard-kill bound even if the child never reports exit', async () => { + const child = makeFakeChild() + let settled = false + const done = terminateCodexProbeChild(child, { platform: 'linux' }).then(() => { + settled = true + }) + + await vi.advanceTimersByTimeAsync(CODEX_PROBE_SHUTDOWN_DRAIN_MS + 1_000) + await done + expect(settled).toBe(true) + }) + + 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' }) + + expect(child.stdin.end).toHaveBeenCalledTimes(1) + expect(child.kill).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(CODEX_PROBE_SHUTDOWN_DRAIN_MS) + expect(child.kill).toHaveBeenCalledTimes(1) + expect(child.kill).toHaveBeenCalledWith() + + child.exit() + await done + }) + + it('does nothing for a child that already exited', async () => { + const child = makeFakeChild() + child.exit() + + await terminateCodexProbeChild(child, { platform: 'darwin' }) + + expect(child.stdin.end).not.toHaveBeenCalled() + expect(child.kill).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/rate-limits/codex-probe-termination.ts b/src/main/rate-limits/codex-probe-termination.ts new file mode 100644 index 00000000000..2b8cf4f6ec0 --- /dev/null +++ b/src/main/rate-limits/codex-probe-termination.ts @@ -0,0 +1,90 @@ +// 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 +// first and only escalates after a bounded drain window. + +export const CODEX_PROBE_SHUTDOWN_DRAIN_MS = 5_000 +const HARD_KILL_EXIT_WAIT_MS = 1_000 + +export type TerminatableProbeChild = { + exitCode: number | null + signalCode?: NodeJS.Signals | null + kill: (signal?: NodeJS.Signals) => boolean + once: (event: 'exit' | 'close' | 'error', listener: () => void) => unknown + off: (event: 'exit' | 'close' | 'error', listener: () => void) => unknown + stdin?: { end: () => void } | null +} + +export type TerminateCodexProbeOptions = { + drainMs?: number + hardKillWaitMs?: number + // Why: injectable so the Windows branch is testable from POSIX CI hosts. + platform?: NodeJS.Platform +} + +function hasExited(child: TerminatableProbeChild): boolean { + return child.exitCode != null || child.signalCode != null +} + +function waitForExit(child: TerminatableProbeChild, timeoutMs: number): Promise { + if (hasExited(child)) { + return Promise.resolve(true) + } + return new Promise((resolve) => { + let timer: ReturnType | null = null + const settle = (exited: boolean): void => { + if (timer) { + clearTimeout(timer) + timer = null + } + child.off('exit', onGone) + child.off('close', onGone) + child.off('error', onGone) + resolve(exited) + } + const onGone = (): void => settle(true) + // Why: 'error' covers children that never spawned and so never emit 'exit'. + child.once('exit', onGone) + child.once('close', onGone) + child.once('error', onGone) + timer = setTimeout(() => settle(false), timeoutMs) + }) +} + +export async function terminateCodexProbeChild( + child: TerminatableProbeChild, + options?: TerminateCodexProbeOptions +): Promise { + if (hasExited(child)) { + return + } + const platform = options?.platform ?? process.platform + // Why: stdin EOF is the graceful stop for a stdio JSON-RPC server and the + // only non-forceful request Windows has; SIGTERM backs it up where signals + // are real (Node's kill() on Windows is always TerminateProcess). + try { + child.stdin?.end() + } catch { + // stdin may already be destroyed; the signal/kill path still applies. + } + if (platform !== 'win32') { + try { + child.kill('SIGTERM') + } catch { + // Already-exited children can race the kill; the exit wait handles it. + } + } + if (await waitForExit(child, options?.drainMs ?? CODEX_PROBE_SHUTDOWN_DRAIN_MS)) { + return + } + try { + if (platform === 'win32') { + child.kill() + } else { + child.kill('SIGKILL') + } + } catch { + // Already-exited children can race the kill; the exit wait handles it. + } + await waitForExit(child, options?.hardKillWaitMs ?? HARD_KILL_EXIT_WAIT_MS) +} diff --git a/src/main/rate-limits/service.test.ts b/src/main/rate-limits/service.test.ts index 245ebed42d2..dafa9a30ed0 100644 --- a/src/main/rate-limits/service.test.ts +++ b/src/main/rate-limits/service.test.ts @@ -1987,6 +1987,51 @@ describe('RateLimitService', () => { expect(service.getState().inactiveCodexAccounts).toEqual([]) }) + it('keeps the inactive Codex debounce across an account switch instead of re-probing', async () => { + const service = new RateLimitService() + service.setInactiveCodexAccountsResolver(() => [ + { id: 'account-b', managedHomePath: '/tmp/account-b/home' } + ]) + vi.mocked(fetchCodexRateLimits).mockImplementation(async () => okProvider('codex', 10)) + + await service.fetchInactiveCodexAccountsOnOpen() + expect(fetchCodexRateLimits).toHaveBeenCalledTimes(1) + + // The switch triggers exactly one fetch: the newly active account's. + await service.refreshForCodexAccountChange('account-a') + expect(fetchCodexRateLimits).toHaveBeenCalledTimes(2) + + // Why: re-opening the switcher inside the debounce window must not spawn + // codex in every inactive credential home again. + await service.fetchInactiveCodexAccountsOnOpen() + expect(fetchCodexRateLimits).toHaveBeenCalledTimes(2) + }) + + it('staggers inactive Codex probes instead of bursting every account at once', async () => { + vi.useFakeTimers() + try { + const service = new RateLimitService() + service.setInactiveCodexAccountsResolver(() => [ + { id: 'account-a', managedHomePath: '/tmp/account-a/home' }, + { id: 'account-b', managedHomePath: '/tmp/account-b/home' } + ]) + vi.mocked(fetchCodexRateLimits).mockImplementation(async () => okProvider('codex', 5)) + + const fetchOnOpen = service.fetchInactiveCodexAccountsOnOpen() + await vi.advanceTimersByTimeAsync(0) + expect(fetchCodexRateLimits).toHaveBeenCalledTimes(1) + + await vi.advanceTimersByTimeAsync(1_999) + expect(fetchCodexRateLimits).toHaveBeenCalledTimes(1) + + await vi.advanceTimersByTimeAsync(1) + expect(fetchCodexRateLimits).toHaveBeenCalledTimes(2) + await fetchOnOpen + } finally { + vi.useRealTimers() + } + }) + it('preserves Gemini buckets through getState after fetch', async () => { const service = new RateLimitService() diff --git a/src/main/rate-limits/service.ts b/src/main/rate-limits/service.ts index 312aa2d4e9c..7cdb1e7bef9 100644 --- a/src/main/rate-limits/service.ts +++ b/src/main/rate-limits/service.ts @@ -90,6 +90,10 @@ const RATE_LIMITED_STALE_THRESHOLD_MS = 24 * 60 * 60 * 1000 // Why: statusline posts arrive on every turn; skip renderer pushes for identical windows so streaming sessions don't spam state updates. const LIVE_CLAUDE_INGEST_DEDUPE_MS = 30 * 1000 const INACTIVE_FETCH_DEBOUNCE_MS = 60 * 1000 // 60 seconds — debounce fetch-on-open +// Why: each inactive Codex probe spawns a real codex process inside that +// account's live credential home; pace them out instead of bursting every +// account the moment the switcher opens. +const INACTIVE_CODEX_PROBE_STAGGER_MS = 2_000 const DEFERRED_STARTUP_ACTIVE_REFRESH_MS = 1000 // Why: inactive account arrays are derived from provider caches on demand in getState()/pushToRenderer(). @@ -132,6 +136,23 @@ function normalizeClaudeConfigDir(dir: string | null | undefined): string | null return trimmed || null } +function delayUnlessAborted(ms: number, signal: AbortSignal): Promise { + if (signal.aborted) { + return Promise.resolve() + } + return new Promise((resolve) => { + const onAbort = (): void => { + clearTimeout(timer) + resolve() + } + const timer = setTimeout(() => { + signal.removeEventListener('abort', onAbort) + resolve() + }, ms) + signal.addEventListener('abort', onAbort, { once: true }) + }) +} + function isSameUsageWindow( a: ProviderRateLimits['session'], b: ProviderRateLimits['session'] @@ -393,7 +414,10 @@ export class RateLimitService { this.activeFailureStreakByProvider.codex = 0 this.inactiveCodexAccountsGeneration += 1 this.pruneInactiveCodexState() - this.lastInactiveCodexFetchAt = 0 + // Why: the switch must NOT reset the inactive-fetch debounce — re-probing + // every inactive account per switch spawns codex in each credential home + // and endangers rotating refresh tokens; the switcher shows the cached + // snapshot (seeded above for the outgoing account) until the debounce ends. // Why: clear the old Codex view immediately, else the previous account's limits show under the newly selected identity until the next poll. this.updateState({ ...this.state, @@ -611,6 +635,7 @@ export class RateLimitService { } this.pushToRenderer() + let staggerNextProbe = false try { for (const account of accounts) { if ( @@ -625,6 +650,23 @@ export class RateLimitService { this.pushToRenderer() continue } + if (staggerNextProbe) { + await delayUnlessAborted(INACTIVE_CODEX_PROBE_STAGGER_MS, signal) + // Why: the account set can change while the stagger delay runs. + if ( + signal.aborted || + fetchGeneration !== this.inactiveCodexAccountsGeneration || + !this.isCurrentInactiveCodexAccount(account.id) + ) { + this.inactiveCodexFetching.delete(account.id) + if (!this.isCurrentInactiveCodexAccount(account.id)) { + this.inactiveCodexCache.delete(account.id) + } + this.pushToRenderer() + continue + } + } + staggerNextProbe = true try { // Why: point fetchCodexRateLimits at the managed home directly, avoiding materializing credentials into the shared runtime location. // Why: no PTY fallback — the switcher preview shouldn't spawn hidden PTYs per account (can crash ConPTY on Windows); RPC-only is enough. diff --git a/src/main/text-generation/commit-message-text-generation.ts b/src/main/text-generation/commit-message-text-generation.ts index fde91c1f746..3a391ecd03c 100644 --- a/src/main/text-generation/commit-message-text-generation.ts +++ b/src/main/text-generation/commit-message-text-generation.ts @@ -47,6 +47,10 @@ import type { SourceControlAiOperation } from '../../shared/source-control-ai-ty import { formatLinkedIssueTemplateValue } from '../../shared/source-control-ai-action-variables' import { renderSourceControlActionCommandTemplate } from '../../shared/source-control-ai-actions' import { resolveCliCommand } from '../codex-cli/command' +import { + resolveCodexHomeProcessLockKeyForSpawnEnv, + withCodexHomeProcessLock +} from '../codex-cli/codex-home-process-lock' import { getSpawnArgsForWindows, UnsafeWindowsBatchArgumentsError, @@ -312,121 +316,132 @@ export async function discoverCommitMessageModelsLocal( return toModelDiscoveryCapability(spec) } - return new Promise((resolve) => { - let child: ChildProcess - const spawnEnv = env ?? process.env - try { - const planned = planModelDiscovery(spec, agentCommandOverride) - if (!planned.ok) { - resolve({ success: false, error: planned.error }) + const runDiscovery = (): Promise => + new Promise((resolve) => { + let child: ChildProcess + const spawnEnv = env ?? process.env + try { + const planned = planModelDiscovery(spec, agentCommandOverride) + if (!planned.ok) { + resolve({ success: false, error: planned.error }) + return + } + if (process.platform === 'win32' && options.wslDistro) { + child = wslAwareSpawn(planned.plan.binary, planned.plan.args, { + cwd: options.cwd, + env: buildWslLauncherEnv(env), + stdio: ['ignore', 'pipe', 'pipe'], + windowsHide: true, + wslDistro: options.wslDistro, + useWslLoginShell: true + }) + } else { + const resolvedBinary = + process.platform === 'win32' + ? resolveCliCommand(planned.plan.binary, { + pathEnv: spawnEnv.PATH ?? spawnEnv.Path ?? null + }) + : planned.plan.binary + const { spawnCmd, spawnArgs } = getSpawnArgsForWindows(resolvedBinary, planned.plan.args) + child = spawn(spawnCmd, spawnArgs, { + env: spawnEnv, + stdio: ['ignore', 'pipe', 'pipe'], + windowsHide: true + }) + } + } catch (error) { + console.error('[commit-message] Failed to spawn model discovery:', error) + resolve({ + success: false, + error: `${spec.label} model discovery could not be started. Check the agent CLI configuration and try again.` + }) return } - if (process.platform === 'win32' && options.wslDistro) { - child = wslAwareSpawn(planned.plan.binary, planned.plan.args, { - cwd: options.cwd, - env: buildWslLauncherEnv(env), - stdio: ['ignore', 'pipe', 'pipe'], - windowsHide: true, - wslDistro: options.wslDistro, - useWslLoginShell: true - }) - } else { - const resolvedBinary = - process.platform === 'win32' - ? resolveCliCommand(planned.plan.binary, { - pathEnv: spawnEnv.PATH ?? spawnEnv.Path ?? null - }) - : planned.plan.binary - const { spawnCmd, spawnArgs } = getSpawnArgsForWindows(resolvedBinary, planned.plan.args) - child = spawn(spawnCmd, spawnArgs, { - env: spawnEnv, - stdio: ['ignore', 'pipe', 'pipe'], - windowsHide: true - }) - } - } catch (error) { - console.error('[commit-message] Failed to spawn model discovery:', error) - resolve({ - success: false, - error: `${spec.label} model discovery could not be started. Check the agent CLI configuration and try again.` - }) - return - } - let stdout = '' - let stderr = '' - let outputLimitExceeded = false - let settled = false - let timer: ReturnType | null = null - let detachChildListeners = (): void => {} - const finish = (result: DiscoverCommitMessageModelsResult): void => { - if (settled) { - return + let stdout = '' + let stderr = '' + let outputLimitExceeded = false + let settled = false + let timer: ReturnType | null = null + let detachChildListeners = (): void => {} + const finish = (result: DiscoverCommitMessageModelsResult): void => { + if (settled) { + return + } + settled = true + if (timer) { + clearTimeout(timer) + timer = null + } + detachChildListeners() + resolve(result) } - settled = true - if (timer) { - clearTimeout(timer) - timer = null - } - detachChildListeners() - resolve(result) - } - timer = setTimeout(() => { - killProcessTree(child) - finish({ - success: false, - error: `${spec.label} model discovery timed out after ${GENERATION_TIMEOUT_MS / 1000}s.` - }) - }, GENERATION_TIMEOUT_MS) - - const onData = (chunk: Buffer, append: (text: string) => void): void => { - if (stdout.length + stderr.length + chunk.byteLength > MAX_AGENT_OUTPUT_BYTES) { - outputLimitExceeded = true + timer = setTimeout(() => { killProcessTree(child) - finish({ success: false, error: `${spec.label} returned too much model data.` }) - return - } - append(chunk.toString('utf-8')) - } - - const onStdoutData = (chunk: Buffer): void => onData(chunk, (text) => (stdout += text)) - const onStderrData = (chunk: Buffer): void => onData(chunk, (text) => (stderr += text)) - const onError = (error: Error): void => { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { finish({ success: false, - error: `${spec.modelDiscovery?.binary ?? spec.binary} not found on PATH. Install ${spec.label} to discover models.` + error: `${spec.label} model discovery timed out after ${GENERATION_TIMEOUT_MS / 1000}s.` }) - return - } - finish({ - success: false, - error: `${spec.label} model discovery failed to start. Check the agent CLI configuration and try again.` - }) - } - const onClose = (code: number | null): void => { - if (outputLimitExceeded) { - finish({ success: false, error: `${spec.label} returned too much model data.` }) - return - } - if (code !== 0) { - finish(finalizeModelDiscoveryOutput(spec, stdout, stderr, code)) - return - } - finish(finalizeModelDiscoveryOutput(spec, stdout, stderr, code)) - } + }, GENERATION_TIMEOUT_MS) - child.stdout?.on('data', onStdoutData) - child.stderr?.on('data', onStderrData) - child.on('error', onError) - child.on('close', onClose) - detachChildListeners = () => { - child.stdout?.off?.('data', onStdoutData) - child.stderr?.off?.('data', onStderrData) - child.off?.('error', onError) - child.off?.('close', onClose) - } - }) + const onData = (chunk: Buffer, append: (text: string) => void): void => { + if (stdout.length + stderr.length + chunk.byteLength > MAX_AGENT_OUTPUT_BYTES) { + outputLimitExceeded = true + killProcessTree(child) + finish({ success: false, error: `${spec.label} returned too much model data.` }) + return + } + append(chunk.toString('utf-8')) + } + + const onStdoutData = (chunk: Buffer): void => onData(chunk, (text) => (stdout += text)) + const onStderrData = (chunk: Buffer): void => onData(chunk, (text) => (stderr += text)) + const onError = (error: Error): void => { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + finish({ + success: false, + error: `${spec.modelDiscovery?.binary ?? spec.binary} not found on PATH. Install ${spec.label} to discover models.` + }) + return + } + finish({ + success: false, + error: `${spec.label} model discovery failed to start. Check the agent CLI configuration and try again.` + }) + } + const onClose = (code: number | null): void => { + if (outputLimitExceeded) { + finish({ success: false, error: `${spec.label} returned too much model data.` }) + return + } + if (code !== 0) { + finish(finalizeModelDiscoveryOutput(spec, stdout, stderr, code)) + return + } + finish(finalizeModelDiscoveryOutput(spec, stdout, stderr, code)) + } + + child.stdout?.on('data', onStdoutData) + child.stderr?.on('data', onStderrData) + child.on('error', onError) + child.on('close', onClose) + detachChildListeners = () => { + child.stdout?.off?.('data', onStdoutData) + child.stderr?.off?.('data', onStderrData) + child.off?.('error', onError) + child.off?.('close', onClose) + } + }) + + if (agentId === 'codex') { + // Why: discovery spawns a real codex process in the selected home; keep it + // off an auth.json a quota probe may be refreshing at the same time. + return withCodexHomeProcessLock( + resolveCodexHomeProcessLockKeyForSpawnEnv(env, options.wslDistro), + runDiscovery + ) + } + return runDiscovery() } export async function discoverCommitMessageModelsRemote( @@ -716,6 +731,56 @@ async function runLocalPlan( }) } +type LocalGenerationTarget = Extract + +function runLocalPlanForAgent( + agentId: string, + plan: CommitMessagePlan, + target: LocalGenerationTarget, + emptyResultName: string, + operation: TextGenerationOperation +): Promise { + const run = (): Promise => + runLocalPlan(plan, target.cwd, target.env, emptyResultName, operation, target.wslDistro) + if (agentId !== 'codex') { + // Why: no extra promise hops here — cancellation timing for non-codex + // agents must stay byte-identical to a direct runLocalPlan call. + return run() + } + return runCodexLocalPlanUnderHomeLock(run, target, operation) +} + +// Why: codex rewrites rotating OAuth tokens in its home's auth.json; the +// per-home lock keeps this run from racing Orca's own quota probes there. +async function runCodexLocalPlanUnderHomeLock( + run: () => Promise, + target: LocalGenerationTarget, + operation: TextGenerationOperation +): Promise { + const laneKey = localLaneKey(operation, target.cwd) + let canceledWhileQueued = false + const queuedCancelToken = (): void => { + canceledWhileQueued = true + } + // Why: Stop must work while this run waits behind a probe holding the lock. + cancelTokensByLane.set(laneKey, queuedCancelToken) + try { + return await withCodexHomeProcessLock( + resolveCodexHomeProcessLockKeyForSpawnEnv(target.env, target.wslDistro), + async () => { + if (canceledWhileQueued) { + return { success: false, error: 'Generation canceled.', canceled: true } + } + return run() + } + ) + } finally { + if (cancelTokensByLane.get(laneKey) === queuedCancelToken) { + cancelTokensByLane.delete(laneKey) + } + } +} + function finalizeFromAgentOutput(args: { code: number | null stdout: string @@ -890,13 +955,12 @@ export async function generateCommitMessageFromContext( const internalResult = target.kind === 'remote' ? await runRemotePlan(planned.plan, target) - : await runLocalPlan( + : await runLocalPlanForAgent( + params.agentId, planned.plan, - target.cwd, - target.env, + target, 'message', - 'commit-message', - target.wslDistro + 'commit-message' ) return formatCommitMessageGenerationResult(internalResult) } @@ -967,13 +1031,12 @@ export async function generatePullRequestFieldsFromContext( const internalResult = target.kind === 'remote' ? await runRemotePlan(planned.plan, target, 'details', 'pull-request-fields') - : await runLocalPlan( + : await runLocalPlanForAgent( + params.agentId, planned.plan, - target.cwd, - target.env, + target, 'details', - 'pull-request-fields', - target.wslDistro + 'pull-request-fields' ) return formatPullRequestFieldsGenerationResult(internalResult, context) } @@ -1014,13 +1077,12 @@ export async function generateBranchNameFromContext( const internalResult = target.kind === 'remote' ? await runRemotePlan(planned.plan, target, 'branch name', 'branch-name') - : await runLocalPlan( + : await runLocalPlanForAgent( + params.agentId, planned.plan, - target.cwd, - target.env, + target, 'branch name', - 'branch-name', - target.wslDistro + 'branch-name' ) if (!internalResult.success) { return internalResult