diff --git a/src/cliproxy/auth/oauth-process.ts b/src/cliproxy/auth/oauth-process.ts index 9f6fb39d..ba157784 100644 --- a/src/cliproxy/auth/oauth-process.ts +++ b/src/cliproxy/auth/oauth-process.ts @@ -7,6 +7,7 @@ import { spawn, ChildProcess } from 'child_process'; import { ok, fail, info, warn } from '../../utils/ui'; +import { killWithEscalation } from '../../utils/process-utils'; import { tryKiroImport } from './kiro-import'; import { CLIProxyProvider } from '../types'; import { AccountInfo } from '../account-manager'; @@ -333,8 +334,8 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise { if (stdinKeepalive) clearInterval(stdinKeepalive); - if (authProcess && !authProcess.killed) { - authProcess.kill('SIGTERM'); + if (authProcess && authProcess.exitCode === null) { + killWithEscalation(authProcess); } }; process.on('SIGINT', cleanup); @@ -358,9 +359,9 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise { - if (cancelledSessionId === state.sessionId && authProcess && !authProcess.killed) { + if (cancelledSessionId === state.sessionId && authProcess && authProcess.exitCode === null) { log('Session cancelled externally'); - authProcess.kill('SIGTERM'); + killWithEscalation(authProcess); } }; authSessionEvents.on('session:cancelled', handleCancel); @@ -425,7 +426,8 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise { // H7: Clear stdin keepalive interval if (stdinKeepalive) clearInterval(stdinKeepalive); @@ -435,9 +437,9 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise { - if (!proc.killed) { - proc.kill('SIGTERM'); - setTimeout(() => { - if (!proc.killed) proc.kill('SIGKILL'); - }, 2000); + if (proc.exitCode === null) { + killWithEscalation(proc, 2000); } }; process.once('SIGINT', cleanupHandler); @@ -326,16 +324,14 @@ export class HeadlessExecutor { // Handle timeout if (timeout > 0) { const timeoutHandle = setTimeout(() => { - if (!proc.killed) { + if (proc.exitCode === null) { timedOut = true; if (progressInterval) { clearInterval(progressInterval); process.stderr.write('\r\x1b[K'); } - proc.kill('SIGTERM'); - setTimeout(() => { - if (!proc.killed) proc.kill('SIGKILL'); - }, 10000); + // Longer grace period for timeout (vs 2s for SIGINT) since delegated sessions may need time to flush output + killWithEscalation(proc, 10000); } }, timeout); proc.on('close', () => clearTimeout(timeoutHandle)); diff --git a/src/utils/process-utils.ts b/src/utils/process-utils.ts new file mode 100644 index 00000000..571ce743 --- /dev/null +++ b/src/utils/process-utils.ts @@ -0,0 +1,21 @@ +/** + * Process management utilities + */ + +import { ChildProcess } from 'child_process'; + +/** + * Kill process with SIGTERM, escalating to SIGKILL if it doesn't exit. + * Uses exitCode === null (not proc.killed) to check if process is still running, + * since proc.killed only indicates a signal was sent, not that the process exited. + */ +export function killWithEscalation(proc: ChildProcess, gracePeriodMs = 3000): void { + proc.kill('SIGTERM'); + const timer = setTimeout(() => { + if (proc.exitCode === null) { + proc.kill('SIGKILL'); + } + }, gracePeriodMs); + timer.unref(); // Don't keep event loop alive just for escalation + proc.once('exit', () => clearTimeout(timer)); +} diff --git a/tests/unit/utils/process-utils.test.ts b/tests/unit/utils/process-utils.test.ts new file mode 100644 index 00000000..cfae232a --- /dev/null +++ b/tests/unit/utils/process-utils.test.ts @@ -0,0 +1,132 @@ +/** + * Unit tests for process-utils.ts + */ +import { describe, it, expect, beforeEach, afterEach, jest } from 'bun:test'; +import { EventEmitter } from 'events'; +import { killWithEscalation } from '../../../src/utils/process-utils'; +import type { ChildProcess } from 'child_process'; + +// Mock ChildProcess using EventEmitter +function createMockProcess(exitCode: number | null = null): ChildProcess { + const proc = new EventEmitter() as any; + proc.killed = false; + proc.exitCode = exitCode; + proc.kill = jest.fn((signal?: string) => { + if (signal === 'SIGTERM' || signal === 'SIGKILL') { + proc.killed = true; + } + return true; + }); + return proc as ChildProcess; +} + +describe('killWithEscalation', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('should send SIGTERM immediately', () => { + const proc = createMockProcess(); + killWithEscalation(proc); + + expect(proc.kill).toHaveBeenCalledWith('SIGTERM'); + expect(proc.kill).toHaveBeenCalledTimes(1); + }); + + it('should send SIGKILL after grace period if process still running', () => { + const proc = createMockProcess(null); // exitCode null = still running + killWithEscalation(proc, 3000); + + // SIGTERM sent immediately + expect(proc.kill).toHaveBeenCalledWith('SIGTERM'); + expect(proc.kill).toHaveBeenCalledTimes(1); + + // Advance time by grace period + jest.advanceTimersByTime(3000); + + // SIGKILL sent after grace period + expect(proc.kill).toHaveBeenCalledWith('SIGKILL'); + expect(proc.kill).toHaveBeenCalledTimes(2); + }); + + it('should NOT send SIGKILL if process exits before grace period', () => { + const proc = createMockProcess(null); + killWithEscalation(proc, 3000); + + expect(proc.kill).toHaveBeenCalledWith('SIGTERM'); + expect(proc.kill).toHaveBeenCalledTimes(1); + + // Simulate process exit after 1 second + jest.advanceTimersByTime(1000); + proc.exitCode = 0; // Process exited + proc.emit('exit', 0); + + // Advance remaining time + jest.advanceTimersByTime(2000); + + // SIGKILL should NOT have been sent + expect(proc.kill).toHaveBeenCalledTimes(1); + expect(proc.kill).not.toHaveBeenCalledWith('SIGKILL'); + }); + + it('should use default grace period of 3000ms', () => { + const proc = createMockProcess(null); + killWithEscalation(proc); // No grace period argument + + expect(proc.kill).toHaveBeenCalledWith('SIGTERM'); + + // Advance by default 3000ms + jest.advanceTimersByTime(3000); + + expect(proc.kill).toHaveBeenCalledWith('SIGKILL'); + }); + + it('should respect custom grace period', () => { + const proc = createMockProcess(null); + killWithEscalation(proc, 5000); // Custom 5 second grace period + + expect(proc.kill).toHaveBeenCalledWith('SIGTERM'); + + // Advance by less than grace period + jest.advanceTimersByTime(4999); + expect(proc.kill).toHaveBeenCalledTimes(1); // Still only SIGTERM + + // Advance to grace period + jest.advanceTimersByTime(1); + expect(proc.kill).toHaveBeenCalledWith('SIGKILL'); + }); + + it('should clear timer when process exits', () => { + const proc = createMockProcess(null); + killWithEscalation(proc, 3000); + + // Simulate immediate exit + proc.exitCode = 0; + proc.emit('exit', 0); + + // Advance way past grace period + jest.advanceTimersByTime(10000); + + // Should only have SIGTERM, timer was cleared + expect(proc.kill).toHaveBeenCalledTimes(1); + expect(proc.kill).toHaveBeenCalledWith('SIGTERM'); + }); + + it('should handle process that already exited', () => { + const proc = createMockProcess(0); // Already exited + killWithEscalation(proc, 3000); + + expect(proc.kill).toHaveBeenCalledWith('SIGTERM'); + + // Even though exitCode is not null, timer still fires + // (because we check exitCode at timer callback time) + jest.advanceTimersByTime(3000); + + // SIGKILL should NOT be sent because exitCode is not null + expect(proc.kill).toHaveBeenCalledTimes(1); + }); +});