diff --git a/src/main/claude-accounts/service.test.ts b/src/main/claude-accounts/service.test.ts index 423b6aa99..379a98ae7 100644 --- a/src/main/claude-accounts/service.test.ts +++ b/src/main/claude-accounts/service.test.ts @@ -1,8 +1,10 @@ /* eslint-disable max-lines -- test suite covers Claude capture and rollback edge cases */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { EventEmitter } from 'node:events' import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { PassThrough } from 'node:stream' import { deleteActiveClaudeKeychainCredentialsStrict, readActiveClaudeKeychainCredentials, @@ -803,4 +805,56 @@ describe('ClaudeAccountService credential capture', () => { wslDistro: 'Ubuntu' }) }) + + it('removes command listeners when Claude sign-in times out', async () => { + vi.resetModules() + vi.useFakeTimers() + const child = new EventEmitter() as EventEmitter & { + stdout: PassThrough + stderr: PassThrough + kill: () => void + } + child.stdout = new PassThrough() + child.stderr = new PassThrough() + child.kill = vi.fn() + const spawnMock = vi.fn(() => child) + vi.doMock('node:child_process', () => ({ spawn: spawnMock })) + + try { + const { ClaudeAccountService } = await import('./service') + const service = new ClaudeAccountService( + createService() as never, + createService() as never, + createService() as never + ) + const commandPromise = ( + service as unknown as { + runClaudeCommand( + args: string[], + configDir: { windowsPath: string; linuxPath: string | null; wslDistro: string | null }, + timeoutMs: number + ): Promise + } + ).runClaudeCommand( + ['login'], + { windowsPath: '/tmp/claude-auth', linuxPath: null, wslDistro: null }, + 1000 + ) + const rejection = expect(commandPromise).rejects.toThrow( + 'Claude sign-in took too long to finish.' + ) + + await vi.advanceTimersByTimeAsync(1000) + + await rejection + expect(child.kill).toHaveBeenCalledTimes(1) + expect(child.stdout.listenerCount('data')).toBe(0) + expect(child.stderr.listenerCount('data')).toBe(0) + expect(child.listenerCount('error')).toBe(0) + expect(child.listenerCount('close')).toBe(0) + } finally { + vi.useRealTimers() + vi.doUnmock('node:child_process') + } + }) }) diff --git a/src/main/claude-accounts/service.ts b/src/main/claude-accounts/service.ts index c09183c94..49c6f0f1a 100644 --- a/src/main/claude-accounts/service.ts +++ b/src/main/claude-accounts/service.ts @@ -914,25 +914,35 @@ export class ClaudeAccountService { output = output.slice(-MAX_COMMAND_OUTPUT_CHARS) } } + let timeout: ReturnType | null = null + const cleanupListeners = (): void => { + if (timeout) { + clearTimeout(timeout) + timeout = null + } + child.stdout.off('data', appendOutput) + child.stderr.off('data', appendOutput) + child.off('error', onError) + child.off('close', onClose) + } const settle = (callback: () => void): void => { if (settled) { return } settled = true - clearTimeout(timeout) + cleanupListeners() callback() } - const timeout = setTimeout(() => { + const timeoutError = new Error('Claude sign-in took too long to finish.') + timeout = setTimeout(() => { child.kill() - settle(() => rejectPromise(new Error('Claude sign-in took too long to finish.'))) + settle(() => rejectPromise(timeoutError)) }, timeoutMs) - child.stdout.on('data', appendOutput) - child.stderr.on('data', appendOutput) - child.on('error', (error) => { + const onError = (error: Error): void => { settle(() => rejectPromise(error)) - }) - child.on('close', (code) => { + } + const onClose = (code: number | null): void => { settle(() => { if (code === 0 || options?.allowFailure) { resolvePromise(output) @@ -947,7 +957,12 @@ export class ClaudeAccountService { ) ) }) - }) + } + + child.stdout.on('data', appendOutput) + child.stderr.on('data', appendOutput) + child.on('error', onError) + child.on('close', onClose) }) } diff --git a/src/main/codex-accounts/service.test.ts b/src/main/codex-accounts/service.test.ts index 34b712c01..159689275 100644 --- a/src/main/codex-accounts/service.test.ts +++ b/src/main/codex-accounts/service.test.ts @@ -993,4 +993,59 @@ describe('CodexAccountService config sync', () => { expect(rateLimits.refreshForCodexAccountChange).toHaveBeenCalledTimes(2) }) + + it('removes command listeners when Codex login times out', async () => { + vi.resetModules() + vi.useFakeTimers() + const child = new EventEmitter() as EventEmitter & { + stdout: PassThrough + stderr: PassThrough + kill: () => void + } + child.stdout = new PassThrough() + child.stderr = new PassThrough() + child.kill = vi.fn() + const spawnMock = vi.fn(() => child) + vi.doMock('node:child_process', () => ({ + execFileSync: vi.fn(), + spawn: spawnMock + })) + vi.doMock('../codex-cli/command', () => ({ + resolveCodexCommand: () => 'codex' + })) + + try { + const settings = createSettings() + const store = createStore(settings) + const rateLimits = createRateLimits() + const runtimeHome = createRuntimeHome() + const { CodexAccountService } = await import('./service') + const service = new CodexAccountService( + store as never, + rateLimits as never, + runtimeHome as never + ) + const loginPromise = ( + service as unknown as { + runCodexLogin(managedHomePath: string): Promise + } + ).runCodexLogin(testState.fakeHomeDir) + const rejection = expect(loginPromise).rejects.toThrow( + 'Codex sign-in took too long to finish.' + ) + + await vi.advanceTimersByTimeAsync(120_000) + + await rejection + expect(child.kill).toHaveBeenCalledTimes(1) + expect(child.stdout.listenerCount('data')).toBe(0) + expect(child.stderr.listenerCount('data')).toBe(0) + expect(child.listenerCount('error')).toBe(0) + expect(child.listenerCount('close')).toBe(0) + } finally { + vi.useRealTimers() + vi.doUnmock('node:child_process') + vi.doUnmock('../codex-cli/command') + } + }) }) diff --git a/src/main/codex-accounts/service.ts b/src/main/codex-accounts/service.ts index ce6414ddf..66ffb7e7a 100644 --- a/src/main/codex-accounts/service.ts +++ b/src/main/codex-accounts/service.ts @@ -734,26 +734,36 @@ export class CodexAccountService { } } + let timeout: ReturnType | null = null + const cleanupListeners = (): void => { + if (timeout) { + clearTimeout(timeout) + timeout = null + } + child.stdout.off('data', appendOutput) + child.stderr.off('data', appendOutput) + child.off('error', onError) + child.off('close', onClose) + } + const settle = (callback: () => void): void => { if (settled) { return } settled = true - clearTimeout(timeout) + cleanupListeners() callback() } - const timeout = setTimeout(() => { + const timeoutError = new Error('Codex sign-in took too long to finish. Please try again.') + timeout = setTimeout(() => { child.kill() settle(() => { - rejectPromise(new Error('Codex sign-in took too long to finish. Please try again.')) + rejectPromise(timeoutError) }) }, LOGIN_TIMEOUT_MS) - child.stdout.on('data', appendOutput) - child.stderr.on('data', appendOutput) - - child.on('error', (error) => { + const onError = (error: Error): void => { settle(() => { const isEnoent = (error as NodeJS.ErrnoException).code === 'ENOENT' // Why: ENOENT can mean either the codex binary doesn't exist OR the @@ -767,9 +777,9 @@ export class CodexAccountService { : error.message rejectPromise(new Error(message)) }) - }) + } - child.on('close', (code) => { + const onClose = (code: number | null): void => { settle(() => { if (code === 0) { resolvePromise() @@ -784,7 +794,12 @@ export class CodexAccountService { ) ) }) - }) + } + + child.stdout.on('data', appendOutput) + child.stderr.on('data', appendOutput) + child.on('error', onError) + child.on('close', onClose) }) }