From d029fc9dfd0e0c6010986f476cd21647105a4962 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 10 Aug 2026 21:30:09 -0700 Subject: [PATCH] fix(ssh): release the relay-loss watcher before teardown mux writes Cancelling the in-flight port scan emits rpc.cancel on the control lane. If that lane is saturated, enqueue admission fails and the writer calls fail() -> mux.dispose('connection_lost'), which fires the relay-loss watcher during our own teardown and schedules a redundant relay redeploy. teardownProviders already released the watcher first, but stopPortScanning runs ahead of it and is what emits the frame. Hoist the release into a named method and call it before stopPortScanning on all four teardown paths. Co-authored-by: Orca --- .../ssh/ssh-relay-session-relay-loss.test.ts | 47 +++++++++++++++++++ src/main/ssh/ssh-relay-session.ts | 19 ++++++-- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/main/ssh/ssh-relay-session-relay-loss.test.ts b/src/main/ssh/ssh-relay-session-relay-loss.test.ts index 9e70466ab31..5053426131d 100644 --- a/src/main/ssh/ssh-relay-session-relay-loss.test.ts +++ b/src/main/ssh/ssh-relay-session-relay-loss.test.ts @@ -232,6 +232,53 @@ describe('SshRelaySession relay loss during setup', () => { expect(unregisterSshFilesystemProvider).toHaveBeenCalledWith('target-1') }) + // #13548 follow-up: cancelling the in-flight port scan emits rpc.cancel on the control + // lane, which a saturated writer turns into mux.dispose('connection_lost'). That fires + // during our own teardown, so it must not be reported as a lost relay. + it.each([ + ['detach', (session: SshRelaySession) => session.beginShutdownDetach()], + ['dispose', (session: SshRelaySession) => session.dispose()] + ])( + 'does not report relay loss when the port-scan cancel kills the mux on %s', + async (_path, teardown) => { + const { session, onRelayLost } = createSession() + // Mirrors SshChannelMultiplexer.request: an in-flight request whose signal aborts + // emits rpc.cancel, and the mock mux dies on that notify like a saturated writer. + muxRequestMock.mockImplementation( + async (method: string, _params?: unknown, options?: { signal?: AbortSignal }) => { + if (method !== 'ports.detect') { + return [] + } + const mux = session.getMux() as unknown as { + failNotifyMethod: string | null + notify: (method: string, params?: unknown) => void + } + mux.failNotifyMethod = 'rpc.cancel' + return new Promise((_resolve, reject) => { + options?.signal?.addEventListener( + 'abort', + () => { + mux.notify('rpc.cancel', { id: 1 }) + reject(new Error('cancelled')) + }, + { once: true } + ) + }) + } + ) + + await session.establish({} as SshConnection) + expect(session.getState()).toBe('ready') + expect(muxRequestMock).toHaveBeenCalledWith('ports.detect', undefined, expect.anything()) + const mux = session.getMux() as unknown as { notify: ReturnType } + + teardown(session) + + expect(mux.notify).toHaveBeenCalledWith('rpc.cancel', { id: 1 }) + expect(onRelayLost).not.toHaveBeenCalled() + } + ) + // #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 () => { diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 8cb6e642812..dbdc5bcb4ee 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -651,6 +651,7 @@ export class SshRelaySession { this.currentConnection = conn // Why: stop scanning before teardownProviders so the poll timer can't fire against a disposed multiplexer. + this.releaseRelayLossWatcher() this.stopPortScanning() await this.portForwardManager.removeAllForwards(this.targetId) this.broadcastEmptyLists() @@ -836,6 +837,7 @@ export class SshRelaySession { // Why: the whole in-memory half runs before any await so a concurrent connect can never observe // a half-torn session; only the durability barriers below are deferred onto the returned promise. this.abortController?.abort() + this.releaseRelayLossWatcher() this.stopPortScanning() this.broadcastEmptyLists() this.teardownProviders('shutdown') @@ -909,6 +911,7 @@ export class SshRelaySession { } detachSshPtyConsumerRecovery(this.targetId, this.ptyConsumerClientInstanceId) this.abortController?.abort() + this.releaseRelayLossWatcher() this.stopPortScanning() this.broadcastEmptyLists() this.teardownProviders('connection_lost') @@ -928,6 +931,7 @@ export class SshRelaySession { // teardown call below throws unexpectedly. detachSshPtyConsumerRecovery(this.targetId, this.ptyConsumerClientInstanceId) this.abortController?.abort() + this.releaseRelayLossWatcher() this.stopPortScanning() this.broadcastEmptyLists() // Why: disconnect keeps PTY ownership so a later manual connect can reattach. @@ -947,9 +951,19 @@ export class SshRelaySession { // ── Private ─────────────────────────────────────────────────────── + // Why: teardown itself can kill the mux — an aborted request emits rpc.cancel, and a saturated + // control lane turns that admission failure into mux.dispose('connection_lost'). Every teardown + // path must release the watcher before its first mux write, or our own shutdown re-enters + // recovery as a spurious relay loss. teardownProviders is not early enough: stopPortScanning + // runs ahead of it and is what emits that frame. + private releaseRelayLossWatcher(): void { + this.muxDisposeCleanup?.() + this.muxDisposeCleanup = null + } + // Why: onStateChange only fires on SSH-level reconnects, so watch for relay-channel loss while SSH stays up and fire onRelayLost. private watchMuxForRelayLoss(mux: SshChannelMultiplexer): void { - this.muxDisposeCleanup?.() + this.releaseRelayLossWatcher() this.muxDisposeCleanup = mux.onDispose((reason) => { if (reason === 'connection_lost' && this.mux === mux && !this.isDisposed()) { console.warn( @@ -1560,8 +1574,7 @@ export class SshRelaySession { reason: 'shutdown' | 'connection_lost', outputGenerationReason: string = reason ): void { - this.muxDisposeCleanup?.() - this.muxDisposeCleanup = null + this.releaseRelayLossWatcher() this.muxNotificationCleanup?.() this.muxNotificationCleanup = null for (const cleanup of this.ptyRecoveryNotificationCleanups) {