fix(ssh): contain relay notification handler failures (#2463)

Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Jinwoo Hong 2026-05-20 20:49:08 -04:00 committed by GitHub
parent 3dcc9e69da
commit 75f7510b1f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 155 additions and 5 deletions

View File

@ -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', () => {

View File

@ -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)
}`
)
}
}
}
}

View File

@ -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)
})
})

View File

@ -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 {