diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index c1f8ab9ba..ecda79ff9 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -28,6 +28,8 @@ import { vi, type Mock, type MockInstance } from 'vitest'; import type { LoadedSettings } from './config/settings.js'; import { CommandKind, type ExecutionMode } from './ui/commands/types.js'; import { filterCommandsForMode } from './services/commandUtils.js'; +import { _resetCleanupFunctionsForTest } from './utils/cleanup.js'; +import { _resetExitLatchForTest } from './utils/errors.js'; // Mock core modules vi.mock('./ui/hooks/atCommandProcessor.js'); @@ -79,6 +81,12 @@ describe('runNonInteractive', () => { let mockGetDebugResponses: Mock; beforeEach(async () => { + // Reset module-level state from any prior test in this file. Without + // these resets the once-set exit latch parks subsequent JSON-mode + // handleError tests in the never-resolving promise (5s vitest timeout). + _resetCleanupFunctionsForTest(); + _resetExitLatchForTest(); + mockCoreExecuteToolCall = vi.mocked(executeToolCall); mockShutdownTelemetry = vi.mocked(shutdownTelemetry); mockGetCommandsForMode.mockImplementation((mode: ExecutionMode) => diff --git a/packages/cli/src/nonInteractiveCli.ts b/packages/cli/src/nonInteractiveCli.ts index 23965c7d9..1541660b2 100644 --- a/packages/cli/src/nonInteractiveCli.ts +++ b/packages/cli/src/nonInteractiveCli.ts @@ -349,7 +349,7 @@ export async function runNonInteractive( config.getMaxSessionTurns() >= 0 && turnCount > config.getMaxSessionTurns() ) { - handleMaxTurnsExceededError(config); + await handleMaxTurnsExceededError(config); } const toolCallRequests: ToolCallRequestInfo[] = []; @@ -372,7 +372,7 @@ export async function runNonInteractive( for await (const event of responseStream) { if (abortController.signal.aborted) { - handleCancellationError(config); + await handleCancellationError(config); } // Use adapter for all event processing adapter.processEvent(event); @@ -498,7 +498,7 @@ export async function runNonInteractive( config.getMaxSessionTurns() >= 0 && turnCount > config.getMaxSessionTurns() ) { - handleMaxTurnsExceededError(config); + await handleMaxTurnsExceededError(config); } const inputFormat = @@ -711,7 +711,7 @@ export async function runNonInteractive( while (localQueue.length > 0) { emitNotificationToSdk(localQueue.shift()!); } - handleCancellationError(config); + await handleCancellationError(config); } await drainLocalQueue(); const running = registry.getRunning(); @@ -766,7 +766,7 @@ export async function runNonInteractive( usage, stats, }); - handleError(error, config); + await handleError(error, config); } finally { const reg = config.getBackgroundTaskRegistry(); reg.setNotificationCallback(undefined); diff --git a/packages/cli/src/utils/errors.test.ts b/packages/cli/src/utils/errors.test.ts index 1dfa0deda..b899ab1b5 100644 --- a/packages/cli/src/utils/errors.test.ts +++ b/packages/cli/src/utils/errors.test.ts @@ -12,12 +12,14 @@ import { ToolErrorType, } from '@qwen-code/qwen-code-core'; import { + _resetExitLatchForTest, getErrorMessage, handleError, handleToolError, handleCancellationError, handleMaxTurnsExceededError, } from './errors.js'; +import { _resetCleanupFunctionsForTest, registerCleanup } from './cleanup.js'; const mockWriteStderrLine = vi.hoisted(() => vi.fn()); const debugLoggerSpy = vi.hoisted(() => ({ @@ -99,6 +101,8 @@ describe('errors', () => { debugLoggerSpy.info.mockClear(); debugLoggerSpy.warn.mockClear(); debugLoggerSpy.error.mockClear(); + _resetCleanupFunctionsForTest(); + _resetExitLatchForTest(); // Mock process.stderr.write processStderrWriteSpy = vi @@ -176,24 +180,24 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.TEXT); }); - it('should log error message and re-throw', () => { + it('should log error message and re-throw', async () => { const testError = new Error('Test error'); - expect(() => { - handleError(testError, mockConfig); - }).toThrow(testError); + await expect(handleError(testError, mockConfig)).rejects.toThrow( + testError, + ); expect(mockWriteStderrLine).toHaveBeenCalledWith( 'API Error: Test error', ); }); - it('should handle non-Error objects', () => { + it('should handle non-Error objects', async () => { const testError = 'String error'; - expect(() => { - handleError(testError, mockConfig); - }).toThrow(testError); + await expect(handleError(testError, mockConfig)).rejects.toThrow( + testError, + ); expect(mockWriteStderrLine).toHaveBeenCalledWith( 'API Error: String error', @@ -208,12 +212,12 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.JSON); }); - it('should format error as JSON and exit with default code', () => { + it('should format error as JSON and exit with default code', async () => { const testError = new Error('Test error'); - expect(() => { - handleError(testError, mockConfig); - }).toThrow('process.exit called with code: 1'); + await expect(handleError(testError, mockConfig)).rejects.toThrow( + 'process.exit called with code: 1', + ); expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( @@ -230,12 +234,12 @@ describe('errors', () => { ); }); - it('should use custom error code when provided', () => { + it('should use custom error code when provided', async () => { const testError = new Error('Test error'); - expect(() => { - handleError(testError, mockConfig, 42); - }).toThrow('process.exit called with code: 42'); + await expect(handleError(testError, mockConfig, 42)).rejects.toThrow( + 'process.exit called with code: 42', + ); expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( @@ -252,12 +256,12 @@ describe('errors', () => { ); }); - it('should extract exitCode from FatalError instances', () => { + it('should extract exitCode from FatalError instances', async () => { const fatalError = new FatalInputError('Fatal error'); - expect(() => { - handleError(fatalError, mockConfig); - }).toThrow('process.exit called with code: 42'); + await expect(handleError(fatalError, mockConfig)).rejects.toThrow( + 'process.exit called with code: 42', + ); expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( @@ -274,26 +278,26 @@ describe('errors', () => { ); }); - it('should handle error with code property', () => { + it('should handle error with code property', async () => { const errorWithCode = new Error('Error with code') as Error & { code: number; }; errorWithCode.code = 404; - expect(() => { - handleError(errorWithCode, mockConfig); - }).toThrow('process.exit called with code: 404'); + await expect(handleError(errorWithCode, mockConfig)).rejects.toThrow( + 'process.exit called with code: 404', + ); }); - it('should handle error with status property', () => { + it('should handle error with status property', async () => { const errorWithStatus = new Error('Error with status') as Error & { status: string; }; errorWithStatus.status = 'TIMEOUT'; - expect(() => { - handleError(errorWithStatus, mockConfig); - }).toThrow('process.exit called with code: 1'); // string codes become 1 + await expect(handleError(errorWithStatus, mockConfig)).rejects.toThrow( + 'process.exit called with code: 1', // string codes become 1 + ); expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( @@ -590,10 +594,10 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.TEXT); }); - it('should log cancellation message and exit with 130', () => { - expect(() => { - handleCancellationError(mockConfig); - }).toThrow('process.exit called with code: 130'); + it('should log cancellation message and exit with 130', async () => { + await expect(handleCancellationError(mockConfig)).rejects.toThrow( + 'process.exit called with code: 130', + ); expect(mockWriteStderrLine).toHaveBeenCalledWith( 'Operation cancelled.', @@ -608,10 +612,10 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.JSON); }); - it('should format cancellation as JSON and exit with 130', () => { - expect(() => { - handleCancellationError(mockConfig); - }).toThrow('process.exit called with code: 130'); + it('should format cancellation as JSON and exit with 130', async () => { + await expect(handleCancellationError(mockConfig)).rejects.toThrow( + 'process.exit called with code: 130', + ); expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( @@ -638,10 +642,10 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.TEXT); }); - it('should log max turns message and exit with 53', () => { - expect(() => { - handleMaxTurnsExceededError(mockConfig); - }).toThrow('process.exit called with code: 53'); + it('should log max turns message and exit with 53', async () => { + await expect(handleMaxTurnsExceededError(mockConfig)).rejects.toThrow( + 'process.exit called with code: 53', + ); expect(mockWriteStderrLine).toHaveBeenCalledWith( 'Reached max session turns for this session. Increase the number of turns by specifying maxSessionTurns in settings.json.', @@ -656,10 +660,10 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.JSON); }); - it('should format max turns error as JSON and exit with 53', () => { - expect(() => { - handleMaxTurnsExceededError(mockConfig); - }).toThrow('process.exit called with code: 53'); + it('should format max turns error as JSON and exit with 53', async () => { + await expect(handleMaxTurnsExceededError(mockConfig)).rejects.toThrow( + 'process.exit called with code: 53', + ); expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( @@ -678,4 +682,119 @@ describe('errors', () => { }); }); }); + + describe('cleanup-before-exit invariant', () => { + // Regression: previously these handlers called process.exit synchronously, + // bypassing the caller's runExitCleanup → flush() chain on SIGINT, max- + // turn, and fatal-error paths. Same family as the EPIPE/process.exit + // bug fixed for stdout in nonInteractiveCli. + it('handleCancellationError drains registered cleanups before exit', async () => { + const cleanupOrder: string[] = []; + registerCleanup(() => { + cleanupOrder.push('cleanup'); + }); + processExitSpy.mockImplementation((code) => { + cleanupOrder.push(`exit:${code}`); + throw new Error(`process.exit called with code: ${code}`); + }); + + await expect(handleCancellationError(mockConfig)).rejects.toThrow( + 'process.exit called with code: 130', + ); + + expect(cleanupOrder).toEqual(['cleanup', 'exit:130']); + }); + + it('handleMaxTurnsExceededError drains registered cleanups before exit', async () => { + const cleanupOrder: string[] = []; + registerCleanup(() => { + cleanupOrder.push('cleanup'); + }); + processExitSpy.mockImplementation((code) => { + cleanupOrder.push(`exit:${code}`); + throw new Error(`process.exit called with code: ${code}`); + }); + + await expect(handleMaxTurnsExceededError(mockConfig)).rejects.toThrow( + 'process.exit called with code: 53', + ); + + expect(cleanupOrder).toEqual(['cleanup', 'exit:53']); + }); + + it('handleError drains registered cleanups before exit (JSON mode)', async () => { + (mockConfig.getOutputFormat as ReturnType).mockReturnValue( + OutputFormat.JSON, + ); + const cleanupOrder: string[] = []; + registerCleanup(() => { + cleanupOrder.push('cleanup'); + }); + processExitSpy.mockImplementation((code) => { + cleanupOrder.push(`exit:${code}`); + throw new Error(`process.exit called with code: ${code}`); + }); + + await expect(handleError(new Error('boom'), mockConfig)).rejects.toThrow( + 'process.exit called with code: 1', + ); + + expect(cleanupOrder).toEqual(['cleanup', 'exit:1']); + }); + + it('a second terminating handler does not race the first into double-exit', async () => { + // Models the real concurrency: SIGINT → handleCancellationError fires + // while a stream rejection lands in the catch → handleError(JSON). + // Without the exit-once latch we'd get duplicate cleanup runs + + // duplicate process.exit calls + interleaved stderr writes. + // (Text-mode handleError throws instead of exiting, so it isn't part + // of the race — the latch lives on the exit path.) + (mockConfig.getOutputFormat as ReturnType).mockReturnValue( + OutputFormat.JSON, + ); + + let exitCalls = 0; + processExitSpy.mockImplementation((code) => { + exitCalls += 1; + throw new Error(`process.exit called with code: ${code}`); + }); + + const first = handleCancellationError(mockConfig); + const second = handleError(new Error('boom'), mockConfig); + + await expect(first).rejects.toThrow('process.exit called with code: 130'); + + // The second handler is parked in the latch's unresolved promise. + let secondSettled = false; + void second.then( + () => { + secondSettled = true; + }, + () => { + secondSettled = true; + }, + ); + await new Promise((r) => setTimeout(r, 20)); + + expect(exitCalls).toBe(1); + expect(secondSettled).toBe(false); + }); + + it('handleError drains registered cleanups before re-throw (text mode)', async () => { + // Text mode re-throws to the caller; we still want the queue drained + // first so the unhandled-rejection path doesn't lose records. + (mockConfig.getOutputFormat as ReturnType).mockReturnValue( + OutputFormat.TEXT, + ); + const events: string[] = []; + registerCleanup(() => { + events.push('cleanup'); + }); + + const original = new Error('boom'); + await expect(handleError(original, mockConfig)).rejects.toBe(original); + + expect(events).toEqual(['cleanup']); + }); + }); }); diff --git a/packages/cli/src/utils/errors.ts b/packages/cli/src/utils/errors.ts index 598f535b7..76d95fcfa 100644 --- a/packages/cli/src/utils/errors.ts +++ b/packages/cli/src/utils/errors.ts @@ -14,6 +14,7 @@ import { ToolErrorType, createDebugLogger, } from '@qwen-code/qwen-code-core'; +import { runExitCleanup } from './cleanup.js'; import { writeStderrLine } from './stdioHelpers.js'; const debugLogger = createDebugLogger('CLI_ERRORS'); @@ -81,16 +82,45 @@ function getNumericExitCode(errorCode: string | number): number { return typeof errorCode === 'number' ? errorCode : 1; } +/** + * Drains pending cleanup before terminating. Routing every "we're about + * to die" path through here keeps async exit-side I/O (chat-recording + * flush, telemetry shutdown, MCP disconnect) from being skipped — the + * earlier sync writes were inherently bounded so a bare `process.exit` + * was safe; with the async-jsonl change it is not. + */ +// Guards against double-entry when two terminating paths race (e.g. SIGINT +// fires `handleCancellationError` while a stream rejection routes through +// `handleError`): only the first caller drains cleanup + exits; the second +// suspends forever in the unresolved promise and gets killed when the first +// caller's process.exit fires. +let exiting = false; + +async function exitAfterCleanup(code: number): Promise { + if (exiting) return new Promise(() => {}); + exiting = true; + await runExitCleanup(); + // `return` so process.exit's `never` narrows the function's terminating + // statement — without it TS reports "function returning 'never' cannot + // have a reachable end point" because await doesn't propagate `never`. + return process.exit(code); +} + +/** Test-only — reset the exit-once latch between cases. */ +export function _resetExitLatchForTest(): void { + exiting = false; +} + /** * Handles errors consistently for both JSON and text output formats. * In JSON mode, outputs formatted JSON error and exits. * In text mode, outputs error message and re-throws. */ -export function handleError( +export async function handleError( error: unknown, config: Config, customErrorCode?: string | number, -): never { +): Promise { const errorMessage = parseAndFormatApiError( error, config.getContentGeneratorConfig()?.authType, @@ -106,9 +136,12 @@ export function handleError( ); writeStderrLine(formattedError); - process.exit(getNumericExitCode(errorCode)); + return exitAfterCleanup(getNumericExitCode(errorCode)); } else { writeStderrLine(errorMessage); + // Drain queued writes before re-throwing so the unhandled rejection + // path doesn't lose chat-recording records that are still in the queue. + await runExitCleanup(); throw error; } } @@ -155,7 +188,7 @@ export function handleToolError( /** * Handles cancellation/abort signals consistently. */ -export function handleCancellationError(config: Config): never { +export async function handleCancellationError(config: Config): Promise { const cancellationError = new FatalCancellationError('Operation cancelled.'); if (config.getOutputFormat() === OutputFormat.JSON) { @@ -166,17 +199,18 @@ export function handleCancellationError(config: Config): never { ); writeStderrLine(formattedError); - process.exit(cancellationError.exitCode); } else { writeStderrLine(cancellationError.message); - process.exit(cancellationError.exitCode); } + return exitAfterCleanup(cancellationError.exitCode); } /** * Handles max session turns exceeded consistently. */ -export function handleMaxTurnsExceededError(config: Config): never { +export async function handleMaxTurnsExceededError( + config: Config, +): Promise { const maxTurnsError = new FatalTurnLimitedError( 'Reached max session turns for this session. Increase the number of turns by specifying maxSessionTurns in settings.json.', ); @@ -189,9 +223,8 @@ export function handleMaxTurnsExceededError(config: Config): never { ); writeStderrLine(formattedError); - process.exit(maxTurnsError.exitCode); } else { writeStderrLine(maxTurnsError.message); - process.exit(maxTurnsError.exitCode); } + return exitAfterCleanup(maxTurnsError.exitCode); }