From e233d6a6417bf8bbe50004a4b95824cd7a07a3e6 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 5 Aug 2026 00:13:41 -0700 Subject: [PATCH] Recover automatically instead of getting stuck on "Multiplexer disposed" when an SSH relay drops (#12216) * fix(ssh): recover instead of wedging when the relay channel dies mid-connect Three coupled defects made a dropped SSH relay look like a permanent bug: 1. SshRelaySession.establish()/reconnect() ran their last liveness gate before configureRelayGraceTime(), whose mux.notify() can dispose the mux synchronously (writer control-lane admission cap, or a throwing transport). The session then latched _state='ready' + _onReady (status bar "connected") while watchMuxForRelayLoss() silently no-op'd on the dead mux, so the bounded relay backoff in ipc/ssh.ts never ran and the fs/pty/git providers stayed registered against a dead multiplexer. Both sites now re-check mux.isDisposed() after the notify and take the existing failure path. 2. SshChannelMultiplexer.request()/notifyWithSettlement() always reported the permanent-shutdown string 'Multiplexer disposed' with no code, even when the recorded dispose reason was connection_lost. The reason is now recorded and a shared disposedError() factory serves dispose(), request(), notifyWithSettlement(), so a transient drop reports 'SSH connection lost, reconnecting...' / CONNECTION_LOST. onDispose() on an already-disposed mux now fires the handler synchronously with that reason instead of returning a silent no-op (without retaining it). 3. TerminalErrorToast no longer renders a transient relay drop in the red "please file an issue" style. The marker is matched with includes() because the message reaches the toast IPC-wrapped. ssh-git-response-stream-reader registers its onDispose subscriber after the abort wiring, since an already-dead mux now fails synchronously there and the cleanup must be able to drop the caller's abort listener. Closes #11953 * fix(ssh): treat a mux killed during PTY reattach as relay loss reconnect()'s post-reattach gate bare-returned when ownsAttempt() went false, and reattachKnownPtys swallows every per-PTY error, so a control-lane failure during a large reattach burst disposed the mux without ever reaching the catch: providers stayed bound to the dead mux, no relay-loss watcher was installed, and the session wedged in 'reconnecting' until restart. Take the failure path when our own mux is the one that died so ssh.ts's bounded backoff retries. Co-authored-by: Orca * fix(ssh): recover when relay dies during setup instead of wedging Introduce verifyRelayAttempt() to detect mux disposal at each setup phase (consumer session, home resolution, provider registration, PTY reattach). Routes mid-setup connection loss into relay-loss recovery instead of hanging in reconnecting state. * Extract SSH disposal error factory Multiple sites were duplicating the disposal error creation logic with specific message and code values. The renderer uses these to distinguish temporary disconnects (show reconnection overlay) from permanent shutdown (show error toast), so all producers must use the same factory to avoid silent UI degradation. --------- Co-authored-by: Orca Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com> --- src/main/ssh/ssh-channel-multiplexer.test.ts | 44 +++ src/main/ssh/ssh-channel-multiplexer.ts | 45 ++- .../ssh-git-response-stream-reader.test.ts | 30 ++ .../ssh/ssh-git-response-stream-reader.ts | 17 +- .../ssh/ssh-relay-session-relay-loss.test.ts | 259 ++++++++++++++++++ src/main/ssh/ssh-relay-session.ts | 92 ++++--- .../terminal-pane/TerminalErrorToast.test.ts | 13 + .../terminal-pane/TerminalErrorToast.tsx | 10 +- 8 files changed, 447 insertions(+), 63 deletions(-) create mode 100644 src/main/ssh/ssh-git-response-stream-reader.test.ts create mode 100644 src/main/ssh/ssh-relay-session-relay-loss.test.ts diff --git a/src/main/ssh/ssh-channel-multiplexer.test.ts b/src/main/ssh/ssh-channel-multiplexer.test.ts index f58e41727..eeca0ce51 100644 --- a/src/main/ssh/ssh-channel-multiplexer.test.ts +++ b/src/main/ssh/ssh-channel-multiplexer.test.ts @@ -502,6 +502,50 @@ describe('SshChannelMultiplexer', () => { await expect(mux.request('pty.spawn')).rejects.toThrow('Multiplexer disposed') }) + it('tags a request after a shutdown dispose with DISPOSED', async () => { + mux.dispose() + + const error = (await mux.request('pty.spawn').catch((e: unknown) => e)) as Error & { + code?: string + } + expect(error.code).toBe('DISPOSED') + }) + + it('reports a request after a lost connection as transient', async () => { + transport.closeCallbacks[0]() + + const error = (await mux + .request('fs.readDir', { path: '/home/me' }) + .catch((e: unknown) => e)) as Error & { code?: string } + expect(error.message).toBe('SSH connection lost, reconnecting...') + expect(error.code).toBe('CONNECTION_LOST') + }) + + it('reports a settled notify after a lost connection as transient', () => { + transport.closeCallbacks[0]() + + const settled = vi.fn() + mux.notifyWithSettlement('pty.data', { id: 'pty-1', data: 'x' }, settled) + + expect(settled).toHaveBeenCalledWith({ + ok: false, + error: expect.objectContaining({ + message: 'SSH connection lost, reconnecting...', + code: 'CONNECTION_LOST' + }) + }) + }) + + it('fires a dispose handler registered after dispose with the recorded reason', () => { + mux.dispose('connection_lost') + + const disposeHandler = vi.fn() + mux.onDispose(disposeHandler) + + expect(disposeHandler).toHaveBeenCalledWith('connection_lost') + expect(disposeHandler).toHaveBeenCalledTimes(1) + }) + it('ignores notify after dispose', () => { mux.dispose() mux.notify('pty.data', { id: 'pty-1', data: 'x' }) diff --git a/src/main/ssh/ssh-channel-multiplexer.ts b/src/main/ssh/ssh-channel-multiplexer.ts index 206d008c1..0000f2f35 100644 --- a/src/main/ssh/ssh-channel-multiplexer.ts +++ b/src/main/ssh/ssh-channel-multiplexer.ts @@ -41,6 +41,21 @@ export type NotificationHandler = (method: string, params: Record) => void export type RequestHandler = (params: Record) => Promise | unknown +export type MultiplexerDisposeReason = 'shutdown' | 'connection_lost' + +// Why: the renderer uses the message/code to distinguish temporary disconnects +// (show reconnection overlay) from permanent shutdown (show error toast), so +// every producer of a disposal rejection must mint it here — a divergent copy +// silently downgrades the relay-lost UI to a bug-report toast. +export function createSshDisposalError(reason: MultiplexerDisposeReason): Error & { code: string } { + const lost = reason === 'connection_lost' + const err = new Error( + lost ? 'SSH connection lost, reconnecting...' : 'Multiplexer disposed' + ) as Error & { code: string } + err.code = lost ? 'CONNECTION_LOST' : 'DISPOSED' + return err +} + const REQUEST_TIMEOUT_MS = 30_000 const MAX_ORDINARY_UNACKED_TIMESTAMPS = 4095 const MAX_UNACKED_TIMESTAMPS = MAX_ORDINARY_UNACKED_TIMESTAMPS + 1 @@ -85,6 +100,7 @@ export class SshChannelMultiplexer { private disposeHandlers: ((reason: 'shutdown' | 'connection_lost') => void)[] = [] private connectionHealthTimer: ReturnType | null = null private disposed = false + private disposeReason: 'shutdown' | 'connection_lost' | null = null private decoderReadPaused = false private writerSaturated = false @@ -181,6 +197,12 @@ export class SshChannelMultiplexer { // never fires the reconnect logic. onDispose(handler: (reason: 'shutdown' | 'connection_lost') => void): () => void { if (this.disposed) { + // Why: a late subscriber must still learn the channel died; retaining it would leak the closure (#11953). + try { + handler(this.disposeReason ?? 'shutdown') + } catch { + // Don't let a handler error escape into the subscriber's registration path + } return () => {} } this.disposeHandlers.push(handler) @@ -201,7 +223,7 @@ export class SshChannelMultiplexer { options?: SshMultiplexerRequestOptions ): Promise { if (this.disposed) { - throw new Error('Multiplexer disposed') + throw this.disposedError() } if (options?.signal?.aborted) { const error = new Error(`Request "${method}" was cancelled`) as Error & { name: string } @@ -289,7 +311,7 @@ export class SshChannelMultiplexer { onSettled: (result: { ok: true } | { ok: false; error: Error }) => void ): void { if (this.disposed) { - onSettled({ ok: false, error: new Error('Multiplexer disposed') }) + onSettled({ ok: false, error: this.disposedError() }) return } this.sendMessage( @@ -338,33 +360,24 @@ export class SshChannelMultiplexer { ) } this.disposed = true + this.disposeReason = reason if (this.connectionHealthTimer) { clearInterval(this.connectionHealthTimer) this.connectionHealthTimer = null } - // Why: the renderer uses the error code to distinguish temporary disconnects - // (show reconnection overlay) from permanent shutdown (show error toast). - const errorMessage = - reason === 'connection_lost' ? 'SSH connection lost, reconnecting...' : 'Multiplexer disposed' - const errorCode = reason === 'connection_lost' ? 'CONNECTION_LOST' : 'DISPOSED' - for (const waiter of this.livenessProbeWaiters.splice(0)) { waiter.fail() } for (const [id, pending] of this.pendingRequests) { pending.cleanup() - const err = new Error(errorMessage) as Error & { code: string } - err.code = errorCode - pending.reject(err) + pending.reject(this.disposedError()) this.pendingRequests.delete(id) } - const writerError = new Error(errorMessage) as Error & { code: string } - writerError.code = errorCode - this.writer.dispose(writerError) + this.writer.dispose(this.disposedError()) this.unackedTimestamps.clear() // Why: relay teardown can race with late provider registration; disposed // muxes must not retain provider/session closures through subscribers. @@ -389,6 +402,10 @@ export class SshChannelMultiplexer { // ── Private ─────────────────────────────────────────────────────── + private disposedError(): Error & { code: string } { + return createSshDisposalError(this.disposeReason ?? 'shutdown') + } + private sendMessage( msg: JsonRpcMessage, onSettled?: (result: MultiplexerWriteSettlement) => void diff --git a/src/main/ssh/ssh-git-response-stream-reader.test.ts b/src/main/ssh/ssh-git-response-stream-reader.test.ts new file mode 100644 index 000000000..8b08701a1 --- /dev/null +++ b/src/main/ssh/ssh-git-response-stream-reader.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it, vi } from 'vitest' +import { requestGitStreamable } from './ssh-git-response-stream-reader' +import { SshChannelMultiplexer, type MultiplexerTransport } from './ssh-channel-multiplexer' + +function createMockTransport(): MultiplexerTransport { + return { + write: () => {}, + onData: () => {}, + onClose: () => {} + } +} + +describe('requestGitStreamable on an already-dead multiplexer', () => { + it('rejects as a transient relay loss and leaves no listener on the caller signal', async () => { + const mux = new SshChannelMultiplexer(createMockTransport()) + mux.dispose('connection_lost') + const controller = new AbortController() + const addListener = vi.spyOn(controller.signal, 'addEventListener') + const removeListener = vi.spyOn(controller.signal, 'removeEventListener') + + await expect( + requestGitStreamable(mux, 'git.status', { cwd: '/repo' }, { signal: controller.signal }) + ).rejects.toThrow('SSH connection lost, reconnecting...') + + // #11953: a disposed mux fails synchronously inside onDispose, so the abort + // listener must already be registered when that cleanup runs — otherwise it + // outlives the request for the lifetime of the caller's signal. + expect(removeListener).toHaveBeenCalledTimes(addListener.mock.calls.length) + }) +}) diff --git a/src/main/ssh/ssh-git-response-stream-reader.ts b/src/main/ssh/ssh-git-response-stream-reader.ts index ef63c7a6d..6a50dc28a 100644 --- a/src/main/ssh/ssh-git-response-stream-reader.ts +++ b/src/main/ssh/ssh-git-response-stream-reader.ts @@ -1,4 +1,5 @@ import type { SshChannelMultiplexer } from './ssh-channel-multiplexer' +import { createSshDisposalError } from './ssh-channel-multiplexer' import { RelayErrorCode, isGitResponseStreamMarker } from './relay-protocol' const SENTINEL_STREAM_ID = -1 @@ -245,18 +246,6 @@ export function requestGitStreamable( handleStreamError(p) }) ) - unsubscribers.push( - mux.onDispose((reason) => { - const err = new Error( - reason === 'connection_lost' - ? 'SSH connection lost, reconnecting...' - : 'Multiplexer disposed' - ) as Error & { code: string } - err.code = reason === 'connection_lost' ? 'CONNECTION_LOST' : 'DISPOSED' - fail(err) - }) - ) - if (options?.signal) { const signal = options.signal if (signal.aborted) { @@ -274,6 +263,10 @@ export function requestGitStreamable( unsubscribers.push(() => signal.removeEventListener('abort', onAbort)) } + // Why: registered last because an already-disposed mux fails synchronously here, + // and that cleanup must be able to drop the abort listener above (#11953). + unsubscribers.push(mux.onDispose((reason) => fail(createSshDisposalError(reason)))) + // Why: forward only the mux-request options (signal/timeoutMs) and omit them // entirely when absent, so callers that previously issued a 2-arg // mux.request keep the same call shape (and their tests). inactivityTimeoutMs diff --git a/src/main/ssh/ssh-relay-session-relay-loss.test.ts b/src/main/ssh/ssh-relay-session-relay-loss.test.ts new file mode 100644 index 000000000..9e70466ab --- /dev/null +++ b/src/main/ssh/ssh-relay-session-relay-loss.test.ts @@ -0,0 +1,259 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { SshRelaySession } from './ssh-relay-session' +import type { SshConnection } from './ssh-connection' +import { SSH_RELAY_CONFIGURE_GRACE_TIME_METHOD } from '../../shared/ssh-types' +import { createMockDeps, mockDeploySuccess } from './ssh-relay-session-test-fixtures' + +// #11953: the grace-time notify is the last thing establish()/reconnect() do +// before latching 'ready', and it can dispose the mux synchronously (writer +// admission cap / throwing transport). Latching 'ready' there left the status +// bar on "connected" with fs/pty/git providers bound to a dead mux and no +// relay-loss watcher, so nothing ever scheduled a reconnect. + +const { muxRequestMock, openConsumerSessionMock, registeredPtyProvider } = vi.hoisted(() => ({ + muxRequestMock: vi.fn(), + openConsumerSessionMock: vi.fn(), + registeredPtyProvider: { dispose: vi.fn(), attachForReconnect: vi.fn() } +})) + +vi.mock('./ssh-relay-deploy', () => ({ deployAndLaunchRelay: vi.fn() })) +vi.mock('./ssh-pty-consumer-session', () => ({ + openSshPtyConsumerSession: openConsumerSessionMock +})) +vi.mock('../ipc/ssh-pty-output-intake-registry', () => ({ + acceptSshPtyOutputData: vi.fn().mockResolvedValue(undefined), + acceptSshPtyOutputExit: vi.fn().mockResolvedValue(undefined), + allocateSshPtyProviderGeneration: vi.fn(() => 41), + beginSshPtyOutputGenerationMigration: vi.fn(() => ({ + byPty: new Map(), + completion: Promise.resolve() + })), + closeSshPtyOutputGeneration: vi.fn(), + getSshPtyAcceptedSourceCheckpoints: vi.fn(() => []), + applySshPtySourceCancellationProof: vi.fn(() => true), + applySshPtySourceRecoveryCancellationProof: vi.fn(() => true), + installSshPtySourceAckPublisher: vi.fn(() => () => {}), + installSshPtySourceCancellationPublisher: vi.fn(() => () => {}) +})) +vi.mock('./ssh-relay-deploy-helpers', () => ({ + execCommand: vi.fn().mockResolvedValue('') +})) + +// Mirrors the real multiplexer contract: dispose() latches, and a failed write +// during notify disposes the mux synchronously via handleProtocolError. +vi.mock('./ssh-channel-multiplexer', () => ({ + SshChannelMultiplexer: class MockSshChannelMultiplexer { + private disposed = false + private disposeReason: string | null = null + private disposeHandlers: ((reason: string) => void)[] = [] + /** Set by the test to kill the channel from inside notify(). */ + failNotifyMethod: string | null = null + notify = vi.fn((method: string) => { + if (method === this.failNotifyMethod) { + this.dispose('connection_lost') + } + }) + notifyWithSettlement = vi.fn() + request = muxRequestMock + onNotification = vi.fn().mockReturnValue(() => {}) + onNotificationByMethod = vi.fn().mockReturnValue(() => {}) + onRequest = vi.fn().mockReturnValue(() => {}) + onDispose = vi.fn((handler: (reason: string) => void) => { + if (this.disposed) { + handler(this.disposeReason ?? 'shutdown') + return () => {} + } + this.disposeHandlers.push(handler) + return () => { + const idx = this.disposeHandlers.indexOf(handler) + if (idx !== -1) { + this.disposeHandlers.splice(idx, 1) + } + } + }) + dispose = vi.fn((reason = 'shutdown') => { + if (this.disposed) { + return + } + this.disposed = true + this.disposeReason = reason + for (const handler of this.disposeHandlers.splice(0)) { + handler(reason) + } + }) + isDisposed = vi.fn(() => this.disposed) + } +})) + +vi.mock('../providers/ssh-pty-provider', () => ({ + isSshPtyNotFoundError: () => false, + isSshPtyIdentityMismatchError: () => false, + SshPtyProvider: class MockSshPtyProvider { + onData = vi.fn().mockReturnValue(() => {}) + onReplay = vi.fn().mockReturnValue(() => {}) + onExit = vi.fn().mockReturnValue(() => {}) + attach = vi.fn().mockResolvedValue(undefined) + attachForReconnect = vi.fn().mockResolvedValue({}) + setPtyDeliveryPauseAdapter = vi.fn() + dispose = vi.fn() + } +})) +vi.mock('../providers/ssh-filesystem-provider', () => ({ + SshFilesystemProvider: class MockSshFilesystemProvider { + dispose = vi.fn() + } +})) +vi.mock('../providers/ssh-git-provider', () => ({ + SshGitProvider: class MockSshGitProvider {} +})) +vi.mock('../ipc/pty', () => ({ + registerSshPtyProvider: vi.fn(), + unregisterSshPtyProvider: vi.fn(), + getSshPtyProvider: vi.fn().mockReturnValue(registeredPtyProvider), + getPtyIdsForConnection: vi.fn().mockReturnValue([]), + clearPtyOwnershipForConnection: vi.fn(), + clearProviderPtyState: vi.fn(), + deletePtyOwnership: vi.fn(), + setPtyOwnership: vi.fn(), + restorePtyIncarnation: vi.fn(), + isCurrentPtyExit: vi.fn(() => true) +})) +vi.mock('../providers/ssh-filesystem-dispatch', () => ({ + registerSshFilesystemProvider: vi.fn(), + unregisterSshFilesystemProvider: vi.fn(), + getSshFilesystemProvider: vi.fn().mockReturnValue({ dispose: vi.fn() }) +})) +vi.mock('../providers/ssh-git-dispatch', () => ({ + registerSshGitProvider: vi.fn(), + unregisterSshGitProvider: vi.fn() +})) + +const { registerSshFilesystemProvider, unregisterSshFilesystemProvider } = + await import('../providers/ssh-filesystem-dispatch') +const { getPtyIdsForConnection } = await import('../ipc/pty') + +describe('SshRelaySession relay loss during setup', () => { + /** Armed by a test so the *next* mux dies inside its grace-time notify. */ + let armGraceTimeFailure = false + let armProviderRegistrationFailure = false + + beforeEach(() => { + vi.clearAllMocks() + armGraceTimeFailure = false + armProviderRegistrationFailure = false + muxRequestMock.mockReset() + vi.mocked(getPtyIdsForConnection).mockReturnValue([]) + registeredPtyProvider.attachForReconnect.mockReset().mockResolvedValue({}) + openConsumerSessionMock.mockImplementation(async (_mux, options) => ({ + mode: 'legacy-fallback', + clientInstanceId: options.clientInstanceId, + serverBuildId: 'test-relay-build' + })) + mockDeploySuccess() + }) + + function createSession(): { + session: SshRelaySession + onRelayLost: ReturnType + onReady: ReturnType + mockStore: ReturnType['mockStore'] + } { + const { mockStore, mockPortForward, getMainWindow } = createMockDeps() + const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) + const onRelayLost = vi.fn() + const onReady = vi.fn() + session.setOnRelayLost(onRelayLost) + session.setOnReady(onReady) + // Arm once the mux exists; every attempt issues requests before the notify. + muxRequestMock.mockImplementation(async (method: string) => { + const mux = session.getMux() as unknown as { + failNotifyMethod: string | null + dispose: (reason: string) => void + } | null + if (mux && armGraceTimeFailure) { + mux.failNotifyMethod = SSH_RELAY_CONFIGURE_GRACE_TIME_METHOD + } + if (mux && armProviderRegistrationFailure && method === 'git.listWorktrees') { + mux.dispose('connection_lost') + throw new Error('SSH connection lost, reconnecting...') + } + return [] + }) + return { session, onRelayLost, onReady, mockStore } + } + + it('fails establish instead of reporting ready on a dead channel', async () => { + const { session, onReady } = createSession() + armGraceTimeFailure = true + + await expect(session.establish({} as SshConnection)).rejects.toThrow( + 'Relay connection lost during establish' + ) + + expect(registerSshFilesystemProvider).toHaveBeenCalledWith('target-1', expect.anything()) + expect(session.getState()).not.toBe('ready') + expect(onReady).not.toHaveBeenCalled() + expect(unregisterSshFilesystemProvider).toHaveBeenCalledWith('target-1') + }) + + it('routes a dead channel during reconnect into relay-loss recovery', async () => { + const { session, onRelayLost, onReady } = createSession() + + await session.establish({} as SshConnection) + expect(session.getState()).toBe('ready') + onReady.mockClear() + + armGraceTimeFailure = true + await session.reconnect({} as SshConnection) + + expect(session.getState()).not.toBe('ready') + expect(onReady).not.toHaveBeenCalled() + expect(onRelayLost).toHaveBeenCalledTimes(1) + expect(unregisterSshFilesystemProvider).toHaveBeenCalledWith('target-1') + }) + + it('routes a mux that dies during provider registration into relay-loss recovery', async () => { + const { session, onRelayLost, onReady, mockStore } = createSession() + + await session.establish({} as SshConnection) + expect(session.getState()).toBe('ready') + onReady.mockClear() + + vi.mocked(mockStore.getRepos).mockReturnValue([ + { connectionId: 'target-1', path: '/repo' } as ReturnType[number] + ]) + armProviderRegistrationFailure = true + + await session.reconnect({} as SshConnection) + + expect(session.getState()).not.toBe('ready') + expect(onReady).not.toHaveBeenCalled() + expect(onRelayLost).toHaveBeenCalledTimes(1) + expect(unregisterSshFilesystemProvider).toHaveBeenCalledWith('target-1') + }) + + // #11953: reattachKnownPtys swallows every per-PTY failure, so a mux killed by the + // reattach burst itself never reaches the catch — the post-reattach gate has to notice. + it('routes a mux that dies during PTY reattach into relay-loss recovery', async () => { + const { session, onRelayLost, onReady } = createSession() + + await session.establish({} as SshConnection) + expect(session.getState()).toBe('ready') + onReady.mockClear() + + vi.mocked(getPtyIdsForConnection).mockReturnValue(['pty-1']) + registeredPtyProvider.attachForReconnect.mockImplementation(async () => { + const mux = session.getMux() as unknown as { dispose: (reason: string) => void } | null + mux?.dispose('connection_lost') + throw new Error('SSH connection lost, reconnecting...') + }) + + await session.reconnect({} as SshConnection) + + expect(registeredPtyProvider.attachForReconnect).toHaveBeenCalled() + expect(session.getState()).not.toBe('ready') + expect(onReady).not.toHaveBeenCalled() + expect(onRelayLost).toHaveBeenCalledTimes(1) + expect(unregisterSshFilesystemProvider).toHaveBeenCalledWith('target-1') + }) +}) diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 34bdf4988..5f97828b6 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -126,6 +126,22 @@ const SSH_PTY_REATTACH_ATTEMPT_TIMEOUT_MS = 10_000 const SSH_PTY_REATTACH_RETRY_MIN_DELAY_MS = 50 const SSH_PTY_REATTACH_RETRY_JITTER_MS = 200 const SSH_SOURCE_RECOVERY_CANCELLATION_FAILED = 'ssh_source_recovery_cancellation_failed' + +// Why: superseded attempts stop quietly; a dead mux still owned by this attempt must enter recovery. +function verifyRelayAttempt( + mux: SshChannelMultiplexer, + isAttemptCurrent: () => boolean, + phase: string +): boolean { + if (!isAttemptCurrent()) { + return false + } + if (mux.isDisposed()) { + throw new Error(`Relay connection lost during ${phase}`) + } + return true +} + type PendingPtyReattach = { mux: SshChannelMultiplexer providerGeneration: number @@ -468,14 +484,15 @@ export class SshRelaySession { const mux = new SshChannelMultiplexer(transport) this.mux = mux - const ownsAttempt = (): boolean => this.mux === mux && !mux.isDisposed() && !this.isDisposed() + const isAttemptCurrent = (): boolean => this.mux === mux && !this.isDisposed() + const shouldContinue = (): boolean => isAttemptCurrent() && !mux.isDisposed() const ptyConsumerSessionState = await this.openPtyConsumerSession( mux, serverBuildId, - ownsAttempt + shouldContinue ) - if (!ownsAttempt()) { + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'consumer session setup')) { if (!mux.isDisposed()) { mux.dispose() } @@ -483,7 +500,7 @@ export class SshRelaySession { } this.ptyConsumerSessionState = ptyConsumerSessionState await this.rememberPtyConsumerRecovery(serverBuildId) - if (!ownsAttempt()) { + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'consumer recovery persistence')) { if (!mux.isDisposed()) { mux.dispose() } @@ -491,7 +508,7 @@ export class SshRelaySession { } await mux.request('session.resolveHome', { path: '~' }) - if (!ownsAttempt()) { + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'home resolution')) { if (!mux.isDisposed()) { mux.dispose() } @@ -499,33 +516,35 @@ export class SshRelaySession { } const connectionIncarnation = randomUUID() - const registered = await this.registerProviders(mux, ownsAttempt, connectionIncarnation) + const registered = await this.registerProviders(mux, shouldContinue, connectionIncarnation) if (!registered) { + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'provider registration')) { + if (!mux.isDisposed()) { + mux.dispose() + } + throw new Error('Session disposed during establish') + } + throw new Error('Relay provider registration stopped unexpectedly') + } + + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'provider registration')) { if (!mux.isDisposed()) { mux.dispose() } throw new Error('Session disposed during establish') } - // Why: registerProviders swallows mux errors, so an isDisposed check catches a transport that closed mid-registration before we reach 'ready'. - if (mux.isDisposed()) { - throw new Error('Relay connection lost during provider registration') - } - - if (this.isDisposed()) { - this.teardownProviders('connection_lost') - throw new Error('Session disposed during establish') - } - // Why: explicit disconnect keeps PTY ownership, so a later manual connect must reattach those remote PTYs. - await this.reattachKnownPtys(mux, ownsAttempt) + await this.reattachKnownPtys(mux, shouldContinue) - if (!ownsAttempt()) { + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'PTY reattach')) { throw new Error('Session disposed during establish') } this.configureRelayGraceTime(mux, graceTimeSeconds) + verifyRelayAttempt(mux, isAttemptCurrent, 'establish') this.watchMuxForRelayLoss(mux) + verifyRelayAttempt(mux, isAttemptCurrent, 'establish') this._state = 'ready' this.startPortScanning() this._onReady?.(this.targetId) @@ -609,19 +628,19 @@ export class SshRelaySession { const mux = new SshChannelMultiplexer(transport) this.mux = mux - const ownsAttempt = (): boolean => + const isAttemptCurrent = (): boolean => this.mux === mux && - !mux.isDisposed() && this.abortController === abortController && !abortController.signal.aborted && !this.isDisposed() + const shouldContinue = (): boolean => isAttemptCurrent() && !mux.isDisposed() const ptyConsumerSessionState = await this.openPtyConsumerSession( mux, serverBuildId, - ownsAttempt + shouldContinue ) - if (!ownsAttempt()) { + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'consumer session setup')) { if (!mux.isDisposed()) { mux.dispose() } @@ -629,7 +648,7 @@ export class SshRelaySession { } this.ptyConsumerSessionState = ptyConsumerSessionState await this.rememberPtyConsumerRecovery(serverBuildId) - if (!ownsAttempt()) { + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'consumer recovery persistence')) { if (!mux.isDisposed()) { mux.dispose() } @@ -637,7 +656,7 @@ export class SshRelaySession { } await mux.request('session.resolveHome', { path: '~' }) - if (!ownsAttempt()) { + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'home resolution')) { if (!mux.isDisposed()) { mux.dispose() } @@ -645,20 +664,21 @@ export class SshRelaySession { } const connectionIncarnation = randomUUID() - const registered = await this.registerProviders(mux, ownsAttempt, connectionIncarnation) + const registered = await this.registerProviders(mux, shouldContinue, connectionIncarnation) if (!registered) { - if (!mux.isDisposed()) { - mux.dispose() + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'provider registration')) { + if (this.mux === mux) { + this.teardownProviders('shutdown') + } else if (!mux.isDisposed()) { + mux.dispose() + } + return } - return - } - - if (mux.isDisposed()) { - throw new Error('Relay connection lost during provider registration') + throw new Error('Relay provider registration stopped unexpectedly') } // Why: dispose() during registration/attach already cleaned up, but this.mux was reassigned above — clean up the new mux so it doesn't leak. - if (!ownsAttempt()) { + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'provider registration')) { if (this.mux === mux) { this.teardownProviders('shutdown') } else if (!mux.isDisposed()) { @@ -667,14 +687,16 @@ export class SshRelaySession { return } - await this.reattachKnownPtys(mux, ownsAttempt) + await this.reattachKnownPtys(mux, shouldContinue) - if (!ownsAttempt()) { + if (!verifyRelayAttempt(mux, isAttemptCurrent, 'PTY reattach')) { return } this.configureRelayGraceTime(mux, graceTimeSeconds) + verifyRelayAttempt(mux, isAttemptCurrent, 'reconnect') this.watchMuxForRelayLoss(mux) + verifyRelayAttempt(mux, isAttemptCurrent, 'reconnect') this._state = 'ready' this.startPortScanning() this._onReady?.(this.targetId) diff --git a/src/renderer/src/components/terminal-pane/TerminalErrorToast.test.ts b/src/renderer/src/components/terminal-pane/TerminalErrorToast.test.ts index 03bb896eb..56e7ffd0a 100644 --- a/src/renderer/src/components/terminal-pane/TerminalErrorToast.test.ts +++ b/src/renderer/src/components/terminal-pane/TerminalErrorToast.test.ts @@ -7,6 +7,9 @@ import { const SSH_FAILURE = "SSH connection failed: Error invoking remote method 'ssh:connect': Error: Relay package for linux-x64 not found locally." +// Relay loss reaches reportError already IPC-wrapped, so the marker is mid-string. +const RELAY_LOST = + "Error invoking remote method 'pty:attach': Error: SSH connection lost, reconnecting..." describe('isSshReconnectOwnedTerminalError', () => { it('matches raw ssh:connect failures and inactive-host messages', () => { @@ -22,6 +25,11 @@ describe('isSshReconnectOwnedTerminalError', () => { ).toBe(true) }) + it('matches an IPC-wrapped relay-loss message', () => { + expect(isSshReconnectOwnedTerminalError(RELAY_LOST)).toBe(true) + expect(isSshReconnectOwnedTerminalError('SSH connection lost, reconnecting...')).toBe(true) + }) + it('leaves unrelated terminal errors for the toast', () => { expect(isSshReconnectOwnedTerminalError('Paste failed.')).toBe(false) expect(isSshReconnectOwnedTerminalError('node-pty: open_slave failed: EMFILE')).toBe(false) @@ -49,6 +57,11 @@ describe('stripSshReconnectOwnedErrorLines', () => { ).toBe('Paste failed.') }) + it('drops an IPC-wrapped relay-loss line and keeps the rest', () => { + expect(stripSshReconnectOwnedErrorLines(RELAY_LOST)).toBeNull() + expect(stripSshReconnectOwnedErrorLines(`Paste failed.\n${RELAY_LOST}`)).toBe('Paste failed.') + }) + it('leaves an error with no SSH text untouched', () => { expect(stripSshReconnectOwnedErrorLines('Paste failed.')).toBe('Paste failed.') }) diff --git a/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx b/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx index 7289dba79..7f38733fa 100644 --- a/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx @@ -2,6 +2,8 @@ import { translate } from '@/i18n/i18n' const SSH_PREFIX = 'SSH connection is not active' // Produced by pty-connection.ts reportError() when a PTY reattach can't reach its SSH host. const SSH_CONNECT_FAILURE_PREFIX = 'SSH connection failed' +// Matched with includes(): this arrives IPC-wrapped ("Error invoking remote method 'pty:…': Error: …"). +const SSH_RELAY_LOST_MARKER = 'SSH connection lost, reconnecting' const STALE_NODE_PTY_DAEMON_MARKERS = [ "Daemon's node-pty install is gone", 'node-pty: posix_spawn failed: ENOENT' @@ -12,12 +14,16 @@ const STALE_DAEMON_CWD_MARKERS = [ ] function isSshError(error: string): boolean { - return error.startsWith(SSH_PREFIX) + return error.startsWith(SSH_PREFIX) || error.includes(SSH_RELAY_LOST_MARKER) } /** A single error line the SSH reconnect banner already covers — hide instead of stacking under/over it. */ export function isSshReconnectOwnedTerminalError(error: string): boolean { - return error.startsWith(SSH_CONNECT_FAILURE_PREFIX) || error.startsWith(SSH_PREFIX) + return ( + error.startsWith(SSH_CONNECT_FAILURE_PREFIX) || + error.startsWith(SSH_PREFIX) || + error.includes(SSH_RELAY_LOST_MARKER) + ) } // Why: onPtyError aggregates errors into one newline-joined string, so classify per line —