From 75f7510b1fa994b2d4e9ce1eb9db5d58cb026792 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 20 May 2026 20:49:08 -0400 Subject: [PATCH] fix(ssh): contain relay notification handler failures (#2463) Co-authored-by: Orca --- src/main/ssh/ssh-channel-multiplexer.test.ts | 44 +++++++++++ src/main/ssh/ssh-channel-multiplexer.ts | 24 +++++- ...configure-process-pipe-error-guard.test.ts | 77 +++++++++++++++++++ src/main/startup/configure-process.ts | 15 +++- 4 files changed, 155 insertions(+), 5 deletions(-) create mode 100644 src/main/startup/configure-process-pipe-error-guard.test.ts diff --git a/src/main/ssh/ssh-channel-multiplexer.test.ts b/src/main/ssh/ssh-channel-multiplexer.test.ts index 7df482860..6dc1b6c4b 100644 --- a/src/main/ssh/ssh-channel-multiplexer.test.ts +++ b/src/main/ssh/ssh-channel-multiplexer.test.ts @@ -229,6 +229,50 @@ describe('SshChannelMultiplexer', () => { expect(a).not.toHaveBeenCalled() expect(b).toHaveBeenCalledWith({ streamId: 7 }) }) + + it('contains generic notification handler failures', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const badHandler = vi.fn(() => { + throw new Error('subscriber exploded') + }) + const goodHandler = vi.fn() + mux.onNotification(badHandler) + mux.onNotification(goodHandler) + + expect(() => + transport.dataCallbacks[0](makeNotificationFrame('pty.data', { id: 'pty-1' }, 1)) + ).not.toThrow() + + expect(badHandler).toHaveBeenCalled() + expect(goodHandler).toHaveBeenCalledWith('pty.data', { id: 'pty-1' }) + expect(mux.isDisposed()).toBe(false) + expect(warnSpy).toHaveBeenCalledWith( + '[ssh-mux] Notification handler failed for pty.data: subscriber exploded' + ) + }) + + it('contains method notification handler failures', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const badHandler = vi.fn(() => { + throw new Error('stream consumer exploded') + }) + const goodHandler = vi.fn() + mux.onNotificationByMethod('fs.streamChunk', badHandler) + mux.onNotificationByMethod('fs.streamChunk', goodHandler) + + expect(() => + transport.dataCallbacks[0]( + makeNotificationFrame('fs.streamChunk', { streamId: 1, seq: 0, data: 'aGk=' }, 1) + ) + ).not.toThrow() + + expect(badHandler).toHaveBeenCalled() + expect(goodHandler).toHaveBeenCalledWith({ streamId: 1, seq: 0, data: 'aGk=' }) + expect(mux.isDisposed()).toBe(false) + expect(warnSpy).toHaveBeenCalledWith( + '[ssh-mux] Method notification handler failed for fs.streamChunk: stream consumer exploded' + ) + }) }) describe('keepalive', () => { diff --git a/src/main/ssh/ssh-channel-multiplexer.ts b/src/main/ssh/ssh-channel-multiplexer.ts index a780de724..caf7e4f7e 100644 --- a/src/main/ssh/ssh-channel-multiplexer.ts +++ b/src/main/ssh/ssh-channel-multiplexer.ts @@ -361,13 +361,33 @@ export class SshChannelMultiplexer { // collection and skips the next handler. Iterating a snapshot prevents that. const snapshot = Array.from(this.notificationHandlers) for (const handler of snapshot) { - handler(msg.method, params) + try { + handler(msg.method, params) + } catch (err) { + // Why: relay notifications arrive on the SSH stream callback; one + // bad subscriber must not escape as a main-process uncaught exception. + console.warn( + `[ssh-mux] Notification handler failed for ${msg.method}: ${ + err instanceof Error ? err.message : String(err) + }` + ) + } } const methodHandlers = this.methodNotificationHandlers.get(msg.method) if (methodHandlers && methodHandlers.size > 0) { const methodSnapshot = Array.from(methodHandlers) for (const handler of methodSnapshot) { - handler(params) + try { + handler(params) + } catch (err) { + // Why: file-stream and PTY listeners are per-method subscribers; keep + // the mux alive even if one consumer rejects a malformed notification. + console.warn( + `[ssh-mux] Method notification handler failed for ${msg.method}: ${ + err instanceof Error ? err.message : String(err) + }` + ) + } } } } diff --git a/src/main/startup/configure-process-pipe-error-guard.test.ts b/src/main/startup/configure-process-pipe-error-guard.test.ts new file mode 100644 index 000000000..08f42c357 --- /dev/null +++ b/src/main/startup/configure-process-pipe-error-guard.test.ts @@ -0,0 +1,77 @@ +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.ts b/src/main/startup/configure-process.ts index 7a23599b3..3d52ee258 100644 --- a/src/main/startup/configure-process.ts +++ b/src/main/startup/configure-process.ts @@ -25,9 +25,10 @@ function requestDevParentShutdown(): void { } export function installUncaughtPipeErrorGuard(): void { - process.on('uncaughtException', (error) => { + 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') @@ -35,8 +36,16 @@ export function installUncaughtPipeErrorGuard(): void { return } - throw error - }) + process.off('uncaughtException', onUncaughtException) + // Why: throwing inside an uncaughtException handler makes Node exit with + // status 7, hiding the original fault. Re-throw on the next tick so the + // default fatal-exception path reports the real status and stack. + setImmediate(() => { + throw error + }) + } + + process.on('uncaughtException', onUncaughtException) } export function patchPackagedProcessPath(): void {