From cd63a8aed0ed40a4d4dd120017841d141dfc4f6c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 20 Aug 2026 03:34:45 -0700 Subject: [PATCH] fix(ssh): keep recovering a remote pane whose reattach could not reach the host A rejected delivery frame drives a targeted reattach. When that reattach failed for any reason other than a host-confirmed absence, the relay recorded the PTY as unverifiable and then gated the retry on hasPty, which reads false for `unverifiable` and `exited` alike. The pane's output delivery was abandoned outright: no retry, no channel-drop escalation, no user-facing signal, while the remote process kept running. Gate the retry on the tri-state verdict instead, so only a positive `exited` verdict stops recovery and an unverifiable reattach still escalates to the recoverable relay reconnect that restores the pane. The suite could not see this: the provider doubles answered hasPty only, so they read every verdict as two-state. They now use a real SshPtyProvider. --- ...sh-relay-session-rejected-delivery.test.ts | 68 +++++++++++++++++-- src/main/ssh/ssh-relay-session.ts | 31 +++++++-- 2 files changed, 88 insertions(+), 11 deletions(-) diff --git a/src/main/ssh/ssh-relay-session-rejected-delivery.test.ts b/src/main/ssh/ssh-relay-session-rejected-delivery.test.ts index e0f1572d4d3..a84ad1f7c9c 100644 --- a/src/main/ssh/ssh-relay-session-rejected-delivery.test.ts +++ b/src/main/ssh/ssh-relay-session-rejected-delivery.test.ts @@ -1,6 +1,6 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type { SshPtyDataCallback } from '../providers/ssh-pty-provider-contract' -import type { SshPtyProvider } from '../providers/ssh-pty-provider' +import { SshPtyProvider } from '../providers/ssh-pty-provider' import type { SshChannelMultiplexer } from './ssh-channel-multiplexer' import type { SshPtyConsumerSessionState } from './ssh-pty-consumer-session' import { SshRelaySession } from './ssh-relay-session' @@ -113,6 +113,20 @@ function prepareSession() { return { deps, internals, mux, session } } +// Why a real provider instead of `{ hasPty }`: recovery turns on the tri-state liveness verdict, and +// a double that only answers hasPty reads `unverifiable` and `exited` alike — the one distinction +// this path must not collapse. +function livenessProvider(): SshPtyProvider { + const mux = { + request: vi.fn().mockResolvedValue(undefined), + notify: vi.fn(), + onNotification: vi.fn().mockReturnValue(vi.fn()), + dispose: vi.fn(), + isDisposed: vi.fn().mockReturnValue(false) + } + return new SshPtyProvider('target-1', mux as never, undefined, 23) +} + describe('SshRelaySession rejected PTY delivery recovery', () => { beforeEach(() => { vi.clearAllMocks() @@ -364,7 +378,9 @@ describe('SshRelaySession rejected PTY delivery recovery', () => { const { internals, mux, session } = prepareSession() const reattach = vi.fn().mockResolvedValue(false) internals.reattachRejectedPty = reattach - getSshPtyProviderMock.mockReturnValue({ hasPty: () => true } as unknown as SshPtyProvider) + const provider = livenessProvider() + provider.acceptLivePty('ssh:target-1@@pty-bad') + getSshPtyProviderMock.mockReturnValue(provider) const onTerminalError = vi.fn() session.setOnTerminalRelayError(onTerminalError) const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) @@ -385,13 +401,47 @@ describe('SshRelaySession rejected PTY delivery recovery', () => { expect(acceptOutputDataMock).not.toHaveBeenCalled() }) + // Why unverifiable must keep recovering: a reattach that cannot reach the host proves nothing + // about the remote process, only that this client cannot resume its output. Abandoning recovery + // there drops the pane's output silently while the agent keeps running on the host. + it('keeps recovering when a failed reattach leaves the rejected PTY unverifiable', async () => { + const { internals, mux, session } = prepareSession() + const provider = livenessProvider() + provider.acceptLivePty('ssh:target-1@@pty-bad') + getSshPtyProviderMock.mockReturnValue(provider) + // Why: this is what handlePtyReattachFailure records for every non-notFound attach failure. + const reattach = vi.fn(async () => { + provider.acceptUnverifiablePty('ssh:target-1@@pty-bad') + return false + }) + internals.reattachRejectedPty = reattach + const onTerminalError = vi.fn() + session.setOnTerminalRelayError(onTerminalError) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + await internals.acceptPtyData(rejectedPayload()) + await vi.waitFor(() => expect(mux.dispose).toHaveBeenCalledOnce(), { timeout: 2000 }) + + expect(reattach).toHaveBeenCalledTimes(2) + expect(mux.dispose).toHaveBeenCalledWith('connection_lost') + expect(warn).toHaveBeenCalledWith( + expect.stringContaining('PTY pty-bad delivery recovery exhausted') + ) + expect(onTerminalError).not.toHaveBeenCalled() + // Why: nothing here is evidence the remote process died, so it must stay unverifiable. + await expect(provider.probePtyLiveness('ssh:target-1@@pty-bad')).resolves.toBeNull() + }) + it('stops without an error when the rejected PTY exited during recovery', async () => { const { internals, mux, session } = prepareSession() const reattach = vi.fn().mockResolvedValue(false) internals.reattachRejectedPty = reattach // Why: reattachKnownPty resolves without claiming the lease when the PTY exits mid-attach, so a - // plain "not recovered" is indistinguishable from a failure until liveness is checked. - getSshPtyProviderMock.mockReturnValue({ hasPty: () => false } as unknown as SshPtyProvider) + // plain "not recovered" is indistinguishable from a failure until liveness is checked. Only a + // host-confirmed exit stops recovery here, so the provider must carry that exact verdict. + const provider = livenessProvider() + provider.acceptExitedPty('ssh:target-1@@pty-bad') + getSshPtyProviderMock.mockReturnValue(provider) const onTerminalError = vi.fn() session.setOnTerminalRelayError(onTerminalError) @@ -408,7 +458,11 @@ describe('SshRelaySession rejected PTY delivery recovery', () => { // starts rejecting across every PTY at once would otherwise open one attach round trip per PTY. it('caps concurrent targeted reattaches and coalesces repeats for one PTY', async () => { const { internals } = prepareSession() - getSshPtyProviderMock.mockReturnValue({ hasPty: () => true } as unknown as SshPtyProvider) + const provider = livenessProvider() + for (let index = 0; index < 20; index++) { + provider.acceptLivePty(`ssh:target-1@@pty-${index}`) + } + getSshPtyProviderMock.mockReturnValue(provider) vi.spyOn(console, 'warn').mockImplementation(() => {}) const release: (() => void)[] = [] let active = 0 @@ -455,7 +509,9 @@ describe('SshRelaySession rejected PTY delivery recovery', () => { const { internals, mux, session } = prepareSession() const reattach = vi.fn().mockResolvedValue(true) internals.reattachRejectedPty = reattach - getSshPtyProviderMock.mockReturnValue({ hasPty: () => true } as unknown as SshPtyProvider) + const provider = livenessProvider() + provider.acceptLivePty('ssh:target-1@@pty-bad') + getSshPtyProviderMock.mockReturnValue(provider) const onTerminalError = vi.fn() session.setOnTerminalRelayError(onTerminalError) diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index a872521c25e..94bda491462 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -1958,14 +1958,14 @@ export class SshRelaySession { attempt.attempts = 0 return } - this.retryRejectedPtyDelivery(payload, source, appPtyId) + void this.retryRejectedPtyDelivery(payload, source, appPtyId) }, (error: unknown) => { console.warn(`[ssh-relay-session] PTY ${relayPtyId} targeted delivery recovery failed`, { providerGeneration, error: error instanceof Error ? error.message : String(error) }) - this.retryRejectedPtyDelivery(payload, source, appPtyId) + void this.retryRejectedPtyDelivery(payload, source, appPtyId) } ) } @@ -1973,16 +1973,37 @@ export class SshRelaySession { // Why liveness is checked before retrying: reattachKnownPty resolves without claiming the lease // when the PTY exited mid-attach, which is indistinguishable from a failed reattach at the call // site. Retrying that race twice would drop the relay channel over an ordinary PTY exit. - private retryRejectedPtyDelivery( + // Why the tri-state verdict and not hasPty: hasPty reads false for `unverifiable` too, and an + // unverifiable PTY is precisely the one this retry exists for — a reattach that could not reach + // the host is not evidence the pane died. Abandoning it there would drop the pane's output + // silently while the remote process keeps running. + private async retryRejectedPtyDelivery( payload: SshPtyDataPayload, source: SshPtyDataPayload['source'], appPtyId: string - ): void { + ): Promise { + const mux = this.mux + const providerGeneration = this.activePtyProviderGeneration const ptyProvider = getSshPtyProvider(this.targetId) as SshPtyProvider | undefined - if (!ptyProvider || typeof ptyProvider.hasPty !== 'function' || !ptyProvider.hasPty(appPtyId)) { + if (!ptyProvider || typeof ptyProvider.probePtyLiveness !== 'function') { this.rejectedPtyRecoveryAttempts.delete(appPtyId) return } + const verdict = await ptyProvider.probePtyLiveness(appPtyId).catch(() => null) + if (verdict === false) { + this.rejectedPtyRecoveryAttempts.delete(appPtyId) + return + } + // Why re-checked after the probe: teardown clears this retry set, and a timer armed afterwards + // would outlive the channel it was recovering. + if ( + !mux || + this.mux !== mux || + mux.isDisposed() || + this.activePtyProviderGeneration !== providerGeneration + ) { + return + } const timer = setTimeout(() => { this.rejectedPtyRecoveryRetries.delete(timer) this.recoverRejectedPtyDelivery(payload, source)