From aa50d8cee370e5f36954399cbb4d19d792ef95ec Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 10 Aug 2026 21:50:18 -0700 Subject: [PATCH] fix(ssh): release the relay-loss watcher before teardown mux writes (#13737) * 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 * fix(ssh): hoist the watcher release above abort on every teardown path Review follow-up. Two nits from the readiness pass: - The "stop scanning before teardownProviders" comment had drifted onto releaseRelayLossWatcher(); move it back above the stopPortScanning() call it describes. - Release the watcher ahead of abortController.abort() too. That signal reaches no mux request today, so this is not a live hole, but plumbing it into one would have silently reopened the bug on three paths. Making the release first keeps the invariant structural rather than incidental. detachSshPtyConsumerRecovery stays first on the two detach paths, per the existing synchronous-half-first rule. Co-authored-by: Orca --------- Co-authored-by: Orca --- .../ssh/ssh-relay-session-relay-loss.test.ts | 47 +++++++++++++++++++ src/main/ssh/ssh-relay-session.ts | 21 +++++++-- 2 files changed, 65 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..e002c334aad 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -640,6 +640,7 @@ export class SshRelaySession { return } + this.releaseRelayLossWatcher() // Cancel any in-flight reconnect this.abortController?.abort() const abortController = new AbortController() @@ -835,6 +836,7 @@ export class SshRelaySession { private runDisposal(pendingDetach: Promise | null): Promise { // 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.releaseRelayLossWatcher() this.abortController?.abort() this.stopPortScanning() this.broadcastEmptyLists() @@ -908,6 +910,7 @@ export class SshRelaySession { return } detachSshPtyConsumerRecovery(this.targetId, this.ptyConsumerClientInstanceId) + this.releaseRelayLossWatcher() this.abortController?.abort() this.stopPortScanning() this.broadcastEmptyLists() @@ -927,6 +930,7 @@ export class SshRelaySession { // step — a fast reconnect must reclaim this identity instead of minting one, even if a // teardown call below throws unexpectedly. detachSshPtyConsumerRecovery(this.targetId, this.ptyConsumerClientInstanceId) + this.releaseRelayLossWatcher() this.abortController?.abort() this.stopPortScanning() this.broadcastEmptyLists() @@ -947,9 +951,21 @@ 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. Call this ahead of abortController.abort() + // too — that signal reaches no mux request today, but plumbing it into one would otherwise + // reopen the same hole silently. + 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 +1576,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) {