Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions packages/cli/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -533,24 +533,37 @@ export function stampCliEntryEnv(entryPath?: string): void {
}
}

/**
* The process-level `uncaughtException` handler registered at the entry point,
* before the session ID (and thus the debug-log path) is known. Benign PTY
* teardown races are suppressed; anything else is reported to stderr and fatal.
*
* `setupUncaughtExceptionHandler` in gemini.tsx removes this handler and
* installs a session-aware replacement once interactive startup is far enough
* along to leave the alternate screen and write the debug file. Exactly one
* listener must be active: two would conflict (the first calls `process.exit`
* before the second runs) and this basic one lacks the visibility behavior.
*/
export function handleUncaughtException(error: unknown): void {
if (isExpectedPtyRaceError(error)) {
return;
}

if (error instanceof Error) {
writeStderrLine(error.stack ?? error.message);
} else {
writeStderrLine(String(error));
}
process.exit(1);
}

export async function runCliEntryPoint(
run: () => Promise<void> = runCliEntry,
handleError: (error: unknown) => Promise<void> = handleCriticalError,
): Promise<void> {
stampCliEntryEnv();

process.on('uncaughtException', (error) => {
if (isExpectedPtyRaceError(error)) {
return;
}

if (error instanceof Error) {
writeStderrLine(error.stack ?? error.message);
} else {
writeStderrLine(String(error));
}
process.exit(1);
});
process.on('uncaughtException', handleUncaughtException);

try {
await run();
Expand Down
125 changes: 125 additions & 0 deletions packages/cli/src/gemini.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import type { Config } from '@qwen-code/qwen-code-core';
import { ApprovalMode, OutputFormat } from '@qwen-code/qwen-code-core';

const mockWriteStderrLine = vi.hoisted(() => vi.fn());
const mockConsumeLastRenderError = vi.hoisted(() => vi.fn());
const mockHandleListExtensions = vi.hoisted(() => vi.fn());
const mockStartEarlyStartupPrefetches = vi.hoisted(() => vi.fn());
const mockStartPostRenderPrefetches = vi.hoisted(() => vi.fn());
Expand Down Expand Up @@ -2056,6 +2057,55 @@ describe('gemini.tsx main function kitty protocol', () => {
processExitSpy.mockRestore();
});

it('still exits on SIGHUP with code 129', async () => {
const { loadCliConfig, parseArguments } = await import(
'./config/config.js'
);
const { loadSettings } = await import('./config/settings.js');
const cleanupModule = await import('./utils/cleanup.js');
const signalHandlers = new Map<string, (...args: unknown[]) => void>();
const realProcessOn = process.on.bind(process);
const processOnSpy = vi.spyOn(process, 'on').mockImplementation(((
eventName: string | symbol,
listener: (...args: unknown[]) => void,
) => {
if (
eventName === 'SIGTERM' ||
eventName === 'SIGINT' ||
eventName === 'SIGHUP'
) {
if (!signalHandlers.has(eventName as string)) {
signalHandlers.set(eventName as string, listener);
}
return process;
}
return realProcessOn(
eventName as string,
listener as (...args: unknown[]) => void,
);
}) as typeof process.on);
const processExitSpy = vi
.spyOn(process, 'exit')
.mockImplementation((() => undefined) as unknown as typeof process.exit);
const runExitCleanupMock = vi.mocked(cleanupModule.runExitCleanup);
runExitCleanupMock.mockResolvedValue(undefined);
applyInteractiveSigintConfigMocks(loadCliConfig, loadSettings);
vi.mocked(parseArguments).mockResolvedValue({
extensions: undefined,
} as never);

await main();
signalHandlers.get('SIGHUP')?.();
await Promise.resolve();
await Promise.resolve();

expect(runExitCleanupMock).toHaveBeenCalledTimes(1);
expect(processExitSpy).toHaveBeenCalledWith(129);

processOnSpy.mockRestore();
processExitSpy.mockRestore();
});

it('rejects --json-schema when running in interactive (TUI) mode', async () => {
// The synthetic structured_output tool only terminates the run inside
// runNonInteractive. In TUI mode it's an inert tool that prints
Expand Down Expand Up @@ -2222,6 +2272,17 @@ describe('startInteractiveUI', () => {
render: vi.fn().mockReturnValue({ unmount: vi.fn() }),
}));

vi.mock('./ui/components/shared/ErrorBoundary.js', async (importOriginal) => {
const original =
await importOriginal<
typeof import('./ui/components/shared/ErrorBoundary.js')
>();
return {
...original,
consumeLastRenderError: mockConsumeLastRenderError,
};
});

let initialExitListeners: NodeJS.ExitListener[] = [];
let originalStdoutIsTTY: boolean | undefined;
let restoreCiEnv = () => {};
Expand Down Expand Up @@ -2607,6 +2668,70 @@ describe('startInteractiveUI', () => {
).toBeGreaterThan(unmount.mock.invocationCallOrder[0]);
});

it('echoes a stored render error to stderr on cleanup (VP exit-time echo)', async () => {
const unmount = vi.fn();
const { render } = await import('ink');
vi.mocked(render).mockReturnValue({ unmount } as never);
mockConsumeLastRenderError.mockReturnValue(new Error('render boom'));
mockWriteStderrLine.mockClear();

await startInteractiveUI(
mockConfig,
mockSettings,
mockStartupWarnings,
mockWorkspaceRoot,
{
authError: null,
themeError: null,
shouldOpenAuthDialog: false,
geminiMdFileCount: 0,
},
);

const { registerCleanup } = await import('./utils/cleanup.js');
const cleanupFn = vi.mocked(registerCleanup).mock.calls.at(-1)?.[0] as
| (() => Promise<void> | void)
| undefined;
expect(cleanupFn).toBeTypeOf('function');
await cleanupFn?.();

expect(unmount).toHaveBeenCalledTimes(1);
expect(mockWriteStderrLine).toHaveBeenCalledWith(
'\nRendering error: render boom',
);
});

it('does not echo when no render error was stored', async () => {
const unmount = vi.fn();
const { render } = await import('ink');
vi.mocked(render).mockReturnValue({ unmount } as never);
mockConsumeLastRenderError.mockReturnValue(undefined);
mockWriteStderrLine.mockClear();

await startInteractiveUI(
mockConfig,
mockSettings,
mockStartupWarnings,
mockWorkspaceRoot,
{
authError: null,
themeError: null,
shouldOpenAuthDialog: false,
geminiMdFileCount: 0,
},
);

const { registerCleanup } = await import('./utils/cleanup.js');
const cleanupFn = vi.mocked(registerCleanup).mock.calls.at(-1)?.[0] as
| (() => Promise<void> | void)
| undefined;
await cleanupFn?.();

expect(mockWriteStderrLine).not.toHaveBeenCalledWith(
expect.stringContaining('Rendering error'),
);
});

describe('periodic memory-pressure check', () => {
beforeEach(() => {
vi.useFakeTimers();
Expand Down
71 changes: 69 additions & 2 deletions packages/cli/src/gemini.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
uiTelemetryService,
} from '@qwen-code/qwen-code-core';
import dns from 'node:dns';
import fs from 'node:fs';
import os from 'node:os';
import path from 'node:path';
import v8 from 'node:v8';
Expand Down Expand Up @@ -81,7 +82,8 @@ import { start_sandbox } from './utils/sandbox.js';
import { getStartupWarnings } from './utils/startupWarnings.js';
import { getUserStartupWarnings } from './utils/userStartupWarnings.js';
import { initializeWarningHandler } from './utils/warningHandler.js';
import { writeStderrLine } from './utils/stdioHelpers.js';
import { writeStderrLine, writeStderrLineSafe } from './utils/stdioHelpers.js';
import { sanitizeTerminalText } from './ui/utils/textUtils.js';
import { getHeadlessYoloSafetyWarning } from './utils/headlessSafetyWarnings.js';
import { initializeLlmOutputLanguage } from './utils/languageUtils.js';
import {
Expand Down Expand Up @@ -166,6 +168,59 @@ function getNodeMemoryArgs(isDebugMode: boolean): string[] {
}

import { loadSandboxConfig } from './config/sandboxConfig.js';
import { handleUncaughtException, isExpectedPtyRaceError } from './cli.js';

let uncaughtExceptionHandler: ((error: unknown) => void) | undefined;

export function setupUncaughtExceptionHandler(config: Config) {
// runCliEntryPoint() registered the basic handleUncaughtException at startup,
// before the session ID existed. Replace it now: two listeners conflict — the
// first calls process.exit(1) so the second never runs — and the basic one
// lacks the debug-log write and the alternate-screen handling below. Also drop
// any handler a previous call installed so exactly one listener is ever active.
process.removeListener('uncaughtException', handleUncaughtException);
Comment on lines +178 to +184

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] setupUncaughtExceptionHandler (~60 lines) has no dedicated unit tests despite having multiple testable branches. — Concrete cost: handler replacement logic (two-listener conflict), synchronous debug log write (async abandoned by process.exit), alternate-screen escape (TTY guard), and PTY race suppression could all silently regress. The SIGHUP and render-error echo paths added in the same PR are tested; this function is not.

Suggested tests: (a) PTY race error is suppressed; (b) debug log is written synchronously with correct format; (c) alternate-screen escape sequences are written when stdout.isTTY; (d) escape sequences are skipped when stdout is not a TTY; (e) process.exit(1) is called; (f) previous handler is removed before new one is installed.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — setupUncaughtExceptionHandler does deserve dedicated coverage for the branches you list (PTY-race suppression, the synchronous debug-log write, the isTTY alternate-screen guard, process.exit(1), and removing the previous handler before installing the new one).

Deferring this one for now: the PR has completed five change-producing rounds and is in critical-only mode, so this round lands only the Critical that was dead-bundling the CLI (the entry↔lazy-module cycle, fixed by moving the helpers into utils/uncaught-exception-handler.ts). Per the repo's review policy, non-Critical suggestions past five rounds are deferred to a follow-up rather than widening the diff here. Leaving this thread open so the six test cases are tracked and not dropped — they'd make a good small follow-up PR.

中文说明

说得对——setupUncaughtExceptionHandler 确实值得为你列出的这些分支补专门的覆盖(PTY 竞态抑制、同步写调试日志、isTTY 备用屏守卫、process.exit(1),以及在安装新处理器前先移除旧处理器)。

先延后这一项:本 PR 已经完成五个产生改动的轮次、进入仅处理 Critical 的模式,因此本轮只落地那个让打包 CLI 失效的 Critical(entry↔懒加载模块成环,已通过将 helper 挪入 utils/uncaught-exception-handler.ts 修复)。按仓库的评审政策,超过五轮后的非 Critical 建议延后到后续处理,以免在此处扩大 diff。保持本线程开放,以便跟踪这六个测试用例、不被丢弃——它们很适合作为一个小的后续 PR。

if (uncaughtExceptionHandler) {
process.removeListener('uncaughtException', uncaughtExceptionHandler);
}
uncaughtExceptionHandler = (rawError) => {
if (isExpectedPtyRaceError(rawError)) {
return;
}
const error =
rawError instanceof Error ? rawError : new Error(String(rawError));
const timestamp = new Date().toISOString();
const line = `${timestamp} [ERROR] [STARTUP] [UNCAUGHT_EXCEPTION] ${error.message}\n${error.stack ?? ''}\n`;
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
// debugLogger.error() uses async fs.appendFile — the write would be
// abandoned by the process.exit() below. Write synchronously instead.
let logged = false;
try {
const logPath = Storage.getDebugLogPath(config.getSessionId());
fs.mkdirSync(path.dirname(logPath), { recursive: true });
fs.appendFileSync(logPath, line, 'utf8');
logged = true;
} catch {
// Best-effort: if the debug dir doesn't exist yet or the disk is
// full, the stderr output below is the fallback record.
}
// In VP / alternate-screen mode, stderr is written to the alternate
// buffer which is discarded on teardown. Leave the alternate screen
// *before* writing the error so the user actually sees it. Guard on
// isTTY: with stdout redirected to a file the escapes would corrupt it.
if (process.stdout.isTTY) {
try {
process.stdout.write('\x1b[?1049l'); // leave alternate screen
process.stdout.write('\x1b[?25h'); // show cursor
} catch {
// stdout may be broken; the debug log above is the primary record.
}
}
writeStderrLineSafe(
`\nFatal: uncaught exception${logged ? ' (logged to debug file)' : ''}\n${sanitizeTerminalText(error.stack ?? error.message)}`,
);
process.exit(1);
};
process.on('uncaughtException', uncaughtExceptionHandler);
}

export function setupUnhandledRejectionHandler() {
let unhandledRejectionOccurred = false;
Expand All @@ -191,7 +246,9 @@ ${reason.stack}`
}

function getSignalExitCode(signal: NodeJS.Signals): number {
return signal === 'SIGINT' ? 130 : 143;
if (signal === 'SIGINT') return 130;
if (signal === 'SIGHUP') return 129;
return 143;
}

// A real SIGINT only reaches the process-level handler while raw mode is
Expand Down Expand Up @@ -241,6 +298,9 @@ function installInteractiveSignalHandlers(wasRaw: boolean): () => void {
const handleSigterm = () => {
beginExit('SIGTERM');
};
const handleSighup = () => {
beginExit('SIGHUP');
};
const handleSigint = () => {
if (cleanupStarted) {
return;
Expand All @@ -260,10 +320,12 @@ function installInteractiveSignalHandlers(wasRaw: boolean): () => void {

process.on('SIGTERM', handleSigterm);
process.on('SIGINT', handleSigint);
process.on('SIGHUP', handleSighup);

return () => {
process.removeListener('SIGTERM', handleSigterm);
process.removeListener('SIGINT', handleSigint);
process.removeListener('SIGHUP', handleSighup);
};
}

Expand Down Expand Up @@ -859,6 +921,11 @@ export async function main() {
// This ensures MCP server subprocesses are properly terminated on exit
registerCleanup(() => config.shutdown());

// Install the uncaughtException handler once the session ID is known.
// Before this point VP mode is not active, so Node's default stderr
// output is visible and sufficient.
setupUncaughtExceptionHandler(config);

startEarlyStartupPrefetches(config);

const wasRaw = process.stdin.isRaw;
Expand Down
36 changes: 28 additions & 8 deletions packages/cli/src/ui/components/messages/ConversationMessages.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,17 @@ import {
SCREEN_READER_USER_PREFIX,
} from '../../textConstants.js';
import { t } from '../../../i18n/index.js';
import { createDebugLogger } from '@qwen-code/qwen-code-core';
import { ErrorBoundary } from '../shared/ErrorBoundary.js';
import { ICON } from '../../constants.js';
import { wrapToVisualLines } from '../../utils/textUtils.js';
import {
wrapToVisualLines,
sanitizeTerminalText,
} from '../../utils/textUtils.js';
import { formatDuration } from '../../utils/displayUtils.js';

const debugLogger = createDebugLogger('THINK_RENDER');

export const THINKING_ICON = `${ICON.THEREFORE} `;
export const THINKING_ICON_PENDING = `${ICON.BECAUSE} `;

Expand Down Expand Up @@ -341,13 +348,26 @@ const ThinkBody: React.FC<{

return (
<Box paddingLeft={2} flexDirection="column">
<MarkdownDisplay
text={text}
isPending={isPending}
availableTerminalHeight={availableTerminalHeight}
contentWidth={contentWidth - 2}
textColor={theme.text.secondary}
/>
<ErrorBoundary
fallback={(err) => (
<Text color={theme.text.secondary} dimColor>
{sanitizeTerminalText(err.message)}
</Text>
)}
onError={(error, info) => {
debugLogger.error(
`[THINK_RENDER_ERROR] ${error.message}\n${info.componentStack ?? ''}\n${error.stack ?? ''}`,
);
}}
>
<MarkdownDisplay
text={text}
isPending={isPending}
availableTerminalHeight={availableTerminalHeight}
contentWidth={contentWidth - 2}
textColor={theme.text.secondary}
/>
</ErrorBoundary>
</Box>
);
};
Expand Down
Loading
Loading