From 2d7135e0ebf9fc72209a8e24418751d48796cc21 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 22 Jul 2026 15:43:10 -0700 Subject: [PATCH] fix(main): harden process error guard diagnostics --- src/main/index.ts | 6 +- ...configure-process-pipe-error-guard.test.ts | 77 ------- src/main/startup/configure-process.test.ts | 71 ------- src/main/startup/configure-process.ts | 58 ------ .../startup/main-process-error-guards.test.ts | 188 ++++++++++++++++++ src/main/startup/main-process-error-guards.ts | 102 ++++++++++ 6 files changed, 294 insertions(+), 208 deletions(-) delete mode 100644 src/main/startup/configure-process-pipe-error-guard.test.ts create mode 100644 src/main/startup/main-process-error-guards.test.ts create mode 100644 src/main/startup/main-process-error-guards.ts diff --git a/src/main/index.ts b/src/main/index.ts index 6ff6f05feb2..e3c809813e3 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -71,12 +71,14 @@ import { installDevParentDisconnectQuit, installDevParentSignalQuit, installDevParentWatchdog, - installUncaughtPipeErrorGuard, - installUnhandledRejectionLogging, isDevParentShutdownRequested, patchPackagedProcessPath, shouldInstallManagedHooks } from './startup/configure-process' +import { + installUncaughtPipeErrorGuard, + installUnhandledRejectionLogging +} from './startup/main-process-error-guards' import { enableRendererHeapHeadroom } from './startup/renderer-heap-headroom' import { ensureVirtualDisplayForHeadlessServe } from './startup/ensure-virtual-display' import { diff --git a/src/main/startup/configure-process-pipe-error-guard.test.ts b/src/main/startup/configure-process-pipe-error-guard.test.ts deleted file mode 100644 index 08f42c357d7..00000000000 --- a/src/main/startup/configure-process-pipe-error-guard.test.ts +++ /dev/null @@ -1,77 +0,0 @@ -import { afterEach, describe, expect, it, vi } from 'vitest' - -vi.mock('electron', () => { - return { - app: { - getPath: vi.fn(() => ''), - setPath: vi.fn(), - quit: vi.fn(), - exit: vi.fn(), - isPackaged: false, - disableHardwareAcceleration: vi.fn(), - commandLine: { - appendSwitch: vi.fn(), - getSwitchValue: vi.fn(() => '') - } - } - } -}) - -describe('installUncaughtPipeErrorGuard', () => { - afterEach(() => { - vi.restoreAllMocks() - }) - - it('suppresses uncaught pipe errors', async () => { - const { installUncaughtPipeErrorGuard } = await import('./configure-process') - const originalOn = process.on.bind(process) - let handler: ((error: unknown) => void) | null = null - const onSpy = vi.spyOn(process, 'on').mockImplementation(((event, listener) => { - if (event === 'uncaughtException') { - handler = listener as (error: unknown) => void - return process - } - return originalOn(event, listener) - }) as typeof process.on) - - installUncaughtPipeErrorGuard() - - const pipeError = new Error('broken pipe') as NodeJS.ErrnoException - pipeError.code = 'EPIPE' - expect(() => handler?.(pipeError)).not.toThrow() - expect(onSpy).toHaveBeenCalledWith('uncaughtException', expect.any(Function)) - }) - - it('rethrows non-pipe errors outside the uncaughtException handler', async () => { - const { installUncaughtPipeErrorGuard } = await import('./configure-process') - const originalOn = process.on.bind(process) - const originalOff = process.off.bind(process) - let handler: ((error: unknown) => void) | null = null - let scheduled: (() => void) | null = null - vi.spyOn(process, 'on').mockImplementation(((event, listener) => { - if (event === 'uncaughtException') { - handler = listener as (error: unknown) => void - return process - } - return originalOn(event, listener) - }) as typeof process.on) - const offSpy = vi.spyOn(process, 'off').mockImplementation(((event, listener) => { - if (event === 'uncaughtException') { - return process - } - return originalOff(event, listener) - }) as typeof process.off) - vi.spyOn(globalThis, 'setImmediate').mockImplementation(((callback) => { - scheduled = callback as () => void - return {} as NodeJS.Immediate - }) as typeof setImmediate) - - installUncaughtPipeErrorGuard() - - const error = new Error('boom') - expect(() => handler?.(error)).not.toThrow() - expect(offSpy).toHaveBeenCalledWith('uncaughtException', handler) - expect(scheduled).not.toBeNull() - expect(() => scheduled?.()).toThrow(error) - }) -}) diff --git a/src/main/startup/configure-process.test.ts b/src/main/startup/configure-process.test.ts index 311b5cda6e3..7d582880f28 100644 --- a/src/main/startup/configure-process.test.ts +++ b/src/main/startup/configure-process.test.ts @@ -596,74 +596,3 @@ describe('enableMainProcessGpuFeatures', () => { expect(app.commandLine.appendSwitch).toHaveBeenCalledWith('enable-features', 'ExistingFeature') }) }) - -describe('main-process fatal error guards (issue #9441)', () => { - it('installUnhandledRejectionLogging records durably and keeps the process alive', async () => { - vi.resetModules() - const record = vi.fn() - vi.doMock('../crash-reporting/durable-crash-breadcrumb', () => ({ - recordDurableCrashBreadcrumb: record - })) - const { installUnhandledRejectionLogging } = await import('./configure-process') - const before = process.listeners('unhandledRejection').length - installUnhandledRejectionLogging() - const listeners = process.listeners('unhandledRejection') - expect(listeners.length).toBe(before + 1) - const listener = listeners.at(-1) as (reason: unknown, p: Promise) => void - const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) - try { - // Why: invoking the listener directly must not throw — a throwing handler would still kill main. - expect(() => - listener(Object.assign(new Error('spawn EAGAIN'), { code: 'EAGAIN' }), Promise.resolve()) - ).not.toThrow() - } finally { - process.removeListener('unhandledRejection', listener as never) - consoleError.mockRestore() - } - expect(record).toHaveBeenCalledWith( - 'main_unhandled_rejection', - expect.objectContaining({ errorMessage: 'spawn EAGAIN', errorCode: 'EAGAIN' }), - 'main_unhandled_rejection' - ) - }) - - it('recordFatalMainProcessError never throws even when the breadcrumb sink fails', async () => { - vi.resetModules() - vi.doMock('../crash-reporting/durable-crash-breadcrumb', () => ({ - recordDurableCrashBreadcrumb: vi.fn(() => { - throw new Error('sink offline') - }) - })) - const { recordFatalMainProcessError } = await import('./configure-process') - const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) - try { - expect(() => - recordFatalMainProcessError('main_uncaught_exception', 'not-an-error') - ).not.toThrow() - } finally { - consoleError.mockRestore() - } - }) - - it('uncaught pipe errors stay swallowed without a durable record', async () => { - vi.resetModules() - const record = vi.fn() - vi.doMock('../crash-reporting/durable-crash-breadcrumb', () => ({ - recordDurableCrashBreadcrumb: record - })) - const { installUncaughtPipeErrorGuard } = await import('./configure-process') - const before = process.listeners('uncaughtException').length - installUncaughtPipeErrorGuard() - const listeners = process.listeners('uncaughtException') - expect(listeners.length).toBe(before + 1) - const listener = listeners.at(-1) as (error: unknown) => void - try { - listener(Object.assign(new Error('write EPIPE'), { code: 'EPIPE' })) - } finally { - process.removeListener('uncaughtException', listener as never) - } - // Why: EPIPE/EIO are expected pipe churn; recording them would flood the breadcrumb store. - expect(record).not.toHaveBeenCalled() - // The guard must still be installed after a swallowed pipe error (only real errors detach it). - }) -}) diff --git a/src/main/startup/configure-process.ts b/src/main/startup/configure-process.ts index fb5614ae797..40ec5e2efbe 100644 --- a/src/main/startup/configure-process.ts +++ b/src/main/startup/configure-process.ts @@ -3,7 +3,6 @@ import { existsSync, mkdirSync, readFileSync } from 'node:fs' import { homedir } from 'node:os' import { join, resolve } from 'node:path' import { getVersionManagerBinPaths } from '../codex-cli/command' -import { recordDurableCrashBreadcrumb } from '../crash-reporting/durable-crash-breadcrumb' import { getMainE2EConfig } from '../e2e-config' const DEV_PARENT_SHUTDOWN_GRACE_MS = 3000 @@ -91,63 +90,6 @@ export function resetDevParentShutdownRequestForTests(): void { devParentShutdownRequested = false } -/** Durably record a main-process fatal/near-fatal error before default handling runs. Exported for tests. */ -export function recordFatalMainProcessError(kind: string, error: unknown): void { - try { - const err = error instanceof Error ? error : null - recordDurableCrashBreadcrumb( - kind, - { - errorName: err?.name ?? typeof error, - errorMessage: (err?.message ?? String(error)).slice(0, 500), - errorStack: (err?.stack ?? '').split('\n').slice(0, 12).join('\n'), - errorCode: String((error as NodeJS.ErrnoException | null)?.code ?? '') - }, - kind - ) - } catch { - // Why: diagnostics must never turn a fatal-error report into a second fault. - } - console.error(`[${kind}]`, error) -} - -export function installUncaughtPipeErrorGuard(): void { - const onUncaughtException = (error: unknown): void => { - if ( - error && - typeof error === 'object' && - 'code' in error && - ((error as NodeJS.ErrnoException).code === 'EIO' || - (error as NodeJS.ErrnoException).code === 'EPIPE') - ) { - return - } - - // Why (issue #9441): the re-throw below exits with a clean code and no macOS crash report; record durably first or the death is undiagnosable in the field. - recordFatalMainProcessError('main_uncaught_exception', error) - process.off('uncaughtException', onUncaughtException) - // Why: throwing inside an uncaughtException handler exits with status 7 and hides the fault; re-throw next tick for the real stack. - setImmediate(() => { - throw error - }) - } - - process.on('uncaughtException', onUncaughtException) -} - -/** Keep one failed background promise from silently killing the whole app. - * - * Node's default kills the process on an unhandled rejection. Large-profile startup restore runs - * hundreds of concurrent async chains (worktree scans, terminal reconnects) in main; a single - * rejection in any of them exited the app with no crash report (issue #9441). Log it durably and - * stay alive — dying cannot be less disruptive than continuing with one failed background task. - */ -export function installUnhandledRejectionLogging(): void { - process.on('unhandledRejection', (reason) => { - recordFatalMainProcessError('main_unhandled_rejection', reason) - }) -} - export function patchPackagedProcessPath(): void { if (!app.isPackaged) { return diff --git a/src/main/startup/main-process-error-guards.test.ts b/src/main/startup/main-process-error-guards.test.ts new file mode 100644 index 00000000000..efa6ebf0602 --- /dev/null +++ b/src/main/startup/main-process-error-guards.test.ts @@ -0,0 +1,188 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' + +afterEach(() => { + vi.restoreAllMocks() +}) + +describe('main-process fatal error guards (issue #9441)', () => { + it('records unhandled rejections durably and keeps the process alive', async () => { + vi.resetModules() + const record = vi.fn() + vi.doMock('../crash-reporting/durable-crash-breadcrumb', () => ({ + recordDurableCrashBreadcrumb: record + })) + const { installUnhandledRejectionLogging } = await import('./main-process-error-guards') + const before = process.listeners('unhandledRejection').length + installUnhandledRejectionLogging() + const listeners = process.listeners('unhandledRejection') + expect(listeners.length).toBe(before + 1) + const listener = listeners.at(-1) as (reason: unknown, promise: Promise) => void + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + // Why: invoking the listener directly must not throw — a throwing handler would still kill main. + expect(() => + listener(Object.assign(new Error('spawn EAGAIN'), { code: 'EAGAIN' }), Promise.resolve()) + ).not.toThrow() + } finally { + process.removeListener('unhandledRejection', listener as never) + consoleError.mockRestore() + } + expect(record).toHaveBeenCalledWith( + 'main_unhandled_rejection', + expect.objectContaining({ errorMessage: 'spawn EAGAIN', errorCode: 'EAGAIN' }), + 'main_unhandled_rejection' + ) + }) + + it('never throws when the breadcrumb sink fails', async () => { + vi.resetModules() + vi.doMock('../crash-reporting/durable-crash-breadcrumb', () => ({ + recordDurableCrashBreadcrumb: vi.fn(() => { + throw new Error('sink offline') + }) + })) + const { recordFatalMainProcessError } = await import('./main-process-error-guards') + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + expect(() => + recordFatalMainProcessError('main_uncaught_exception', 'not-an-error') + ).not.toThrow() + } finally { + consoleError.mockRestore() + } + }) + + it('keeps absent optional error fields empty', async () => { + vi.resetModules() + const record = vi.fn() + vi.doMock('../crash-reporting/durable-crash-breadcrumb', () => ({ + recordDurableCrashBreadcrumb: record + })) + const { recordFatalMainProcessError } = await import('./main-process-error-guards') + vi.spyOn(console, 'error').mockImplementation(() => {}) + + recordFatalMainProcessError('main_unhandled_rejection', new Error('boom')) + + expect(record).toHaveBeenCalledWith( + 'main_unhandled_rejection', + expect.objectContaining({ errorMessage: 'boom', errorCode: '' }), + 'main_unhandled_rejection' + ) + }) + + it('bounds and isolates console formatting for hostile rejection values', async () => { + vi.resetModules() + const record = vi.fn() + vi.doMock('../crash-reporting/durable-crash-breadcrumb', () => ({ + recordDurableCrashBreadcrumb: record + })) + const { recordFatalMainProcessError } = await import('./main-process-error-guards') + const hostileReason = { + toString(): never { + throw new Error('toString failed') + }, + [Symbol.for('nodejs.util.inspect.custom')](): never { + throw new Error('inspect failed') + } + } + const consoleError = vi.spyOn(console, 'error').mockImplementation((...values: unknown[]) => { + if (values.some((value) => typeof value !== 'string')) { + throw new Error('unsafe console formatting') + } + }) + + expect(() => + recordFatalMainProcessError('main_unhandled_rejection', hostileReason) + ).not.toThrow() + expect(record).toHaveBeenCalledWith( + 'main_unhandled_rejection', + expect.objectContaining({ errorName: 'object', errorMessage: '[unprintable value]' }), + 'main_unhandled_rejection' + ) + expect(consoleError).toHaveBeenCalledWith( + expect.stringMatching(/^\[main_unhandled_rejection\]/) + ) + expect(String(consoleError.mock.calls[0]?.[0]).length).toBeLessThan(5_000) + }) + + it('caps oversized rejection diagnostics before recording or logging', async () => { + vi.resetModules() + const record = vi.fn() + vi.doMock('../crash-reporting/durable-crash-breadcrumb', () => ({ + recordDurableCrashBreadcrumb: record + })) + const { recordFatalMainProcessError } = await import('./main-process-error-guards') + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + const error = Object.assign(new Error('m'.repeat(100_000)), { code: 'c'.repeat(100_000) }) + error.name = 'n'.repeat(100_000) + error.stack = Array.from({ length: 100 }, () => 's'.repeat(1_000)).join('\n') + + recordFatalMainProcessError('main_unhandled_rejection', error) + + const details = record.mock.calls[0]?.[1] as Record + expect(details.errorName).toHaveLength(100) + expect(details.errorMessage).toHaveLength(500) + expect(details.errorStack.length).toBeLessThanOrEqual(4_000) + expect(details.errorCode).toHaveLength(100) + expect(String(consoleError.mock.calls[0]?.[0]).length).toBeLessThan(5_000) + }) + + it('keeps uncaught pipe errors swallowed without a durable record', async () => { + vi.resetModules() + const record = vi.fn() + vi.doMock('../crash-reporting/durable-crash-breadcrumb', () => ({ + recordDurableCrashBreadcrumb: record + })) + const { installUncaughtPipeErrorGuard } = await import('./main-process-error-guards') + const before = process.listeners('uncaughtException').length + installUncaughtPipeErrorGuard() + const listeners = process.listeners('uncaughtException') + expect(listeners.length).toBe(before + 1) + const listener = listeners.at(-1) as (error: unknown) => void + try { + listener(Object.assign(new Error('write EPIPE'), { code: 'EPIPE' })) + } finally { + process.removeListener('uncaughtException', listener as never) + } + // Why: EPIPE/EIO are expected pipe churn; recording them would flood the breadcrumb store. + expect(record).not.toHaveBeenCalled() + }) + + it('rethrows non-pipe errors outside the uncaughtException handler', async () => { + vi.resetModules() + vi.doMock('../crash-reporting/durable-crash-breadcrumb', () => ({ + recordDurableCrashBreadcrumb: vi.fn() + })) + const { installUncaughtPipeErrorGuard } = await import('./main-process-error-guards') + const originalOn = process.on.bind(process) + const originalOff = process.off.bind(process) + let handler: ((error: unknown) => void) | null = null + let scheduled: (() => void) | null = null + vi.spyOn(console, 'error').mockImplementation(() => {}) + vi.spyOn(process, 'on').mockImplementation(((event, listener) => { + if (event === 'uncaughtException') { + handler = listener as (error: unknown) => void + return process + } + return originalOn(event, listener) + }) as typeof process.on) + const offSpy = vi.spyOn(process, 'off').mockImplementation(((event, listener) => { + if (event === 'uncaughtException') { + return process + } + return originalOff(event, listener) + }) as typeof process.off) + vi.spyOn(globalThis, 'setImmediate').mockImplementation(((callback) => { + scheduled = callback as () => void + return {} as NodeJS.Immediate + }) as typeof setImmediate) + + installUncaughtPipeErrorGuard() + + const error = new Error('boom') + expect(() => handler?.(error)).not.toThrow() + expect(offSpy).toHaveBeenCalledWith('uncaughtException', handler) + expect(scheduled).not.toBeNull() + expect(() => scheduled?.()).toThrow(error) + }) +}) diff --git a/src/main/startup/main-process-error-guards.ts b/src/main/startup/main-process-error-guards.ts new file mode 100644 index 00000000000..5a6bbcd278c --- /dev/null +++ b/src/main/startup/main-process-error-guards.ts @@ -0,0 +1,102 @@ +import { recordDurableCrashBreadcrumb } from '../crash-reporting/durable-crash-breadcrumb' + +type FatalMainProcessErrorKind = 'main_uncaught_exception' | 'main_unhandled_rejection' + +type FatalMainProcessErrorDetails = { + errorName: string + errorMessage: string + errorStack: string + errorCode: string +} + +function readErrorProperty(error: unknown, property: string): unknown { + try { + return error !== null && (typeof error === 'object' || typeof error === 'function') + ? (error as Record)[property] + : undefined + } catch { + return undefined + } +} + +function boundedString(value: unknown, maxLength: number, fallback = ''): string { + try { + return String(value).slice(0, maxLength) + } catch { + return fallback + } +} + +function fatalMainProcessErrorDetails(error: unknown): FatalMainProcessErrorDetails { + let isError = false + try { + isError = error instanceof Error + } catch { + // Why: a proxy can throw from instanceof; fatal diagnostics still need a safe fallback. + } + + return { + errorName: isError + ? boundedString(readErrorProperty(error, 'name') ?? 'Error', 100, 'Error') + : typeof error, + errorMessage: isError + ? boundedString(readErrorProperty(error, 'message') ?? '', 500) + : boundedString(error, 500, '[unprintable value]'), + errorStack: isError + ? boundedString(readErrorProperty(error, 'stack') ?? '', 4_000) + .split('\n') + .slice(0, 12) + .join('\n') + : '', + errorCode: boundedString(readErrorProperty(error, 'code') ?? '', 100) + } +} + +/** Durably record a main-process fatal/near-fatal error before default handling runs. Exported for tests. */ +export function recordFatalMainProcessError(kind: FatalMainProcessErrorKind, error: unknown): void { + const details = fatalMainProcessErrorDetails(error) + try { + recordDurableCrashBreadcrumb(kind, details, kind) + } catch { + // Why: diagnostics must never turn a fatal-error report into a second fault. + } + try { + console.error( + `[${kind}] ${details.errorStack || `${details.errorName}: ${details.errorMessage}`}` + ) + } catch { + // Why: custom console sinks must not defeat the process-level safety guard. + } +} + +export function installUncaughtPipeErrorGuard(): void { + const onUncaughtException = (error: unknown): void => { + const errorCode = readErrorProperty(error, 'code') + if (errorCode === 'EIO' || errorCode === 'EPIPE') { + return + } + + // Why (issue #9441): the re-throw below exits with a clean code and no macOS crash report; record durably first or the death is undiagnosable in the field. + recordFatalMainProcessError('main_uncaught_exception', error) + process.off('uncaughtException', onUncaughtException) + // Why: throwing inside an uncaughtException handler exits with status 7 and hides the fault; re-throw next tick for the real stack. + setImmediate(() => { + throw error + }) + } + + process.on('uncaughtException', onUncaughtException) +} + +/** Keep one failed background promise from silently killing the whole app. + * + * Node's default kills the process on an unhandled rejection. Large-profile startup restore runs + * hundreds of concurrent async chains (worktree scans, terminal reconnects) in main; a single + * rejection in any of them exited the app with no crash report (issue #9441). Log it durably and + * stay alive — dying cannot be less disruptive than continuing with one failed background task. + */ +export function installUnhandledRejectionLogging(): void { + process.on('unhandledRejection', (reason) => { + recordFatalMainProcessError('main_unhandled_rejection', reason) + }) +}