From 601604d17b12ae0560ebca3b3d54d28fbc1fb13d Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 12 Aug 2026 04:12:22 -0700 Subject: [PATCH] fix(ssh): let a due park past the retired-delivery filter, and prove it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cooldown added in 124e00e8a83 was reactive: it needed a later rejected frame to reach recovery. But retirement is ours, not the host's — a stalled source keeps publishing the very token we retired, and acceptPtyData drops those frames before classification. So the wake-up could never fire in the case that actually happens, and the pane stayed dark exactly as before. A parked PTY past its cooldown is now let through that filter. It costs nothing: the frame is re-classified as rejected and re-retired, so no output reaches the terminal — it only regains the ability to ask for recovery. The test shipped with that commit could not have caught this: it woke recovery with a fabricated NEW delivery token. Retirement holds one key per relay PTY, so that frame overwrote the key and un-retired the original — the assertion passed with the wake-up path fully broken. It now reuses the original token throughout, and reverting the guard above reddens it. --- ...sh-relay-session-rejected-delivery.test.ts | 25 +++++++++++-------- src/main/ssh/ssh-relay-session.ts | 16 +++++++++++- 2 files changed, 29 insertions(+), 12 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 c1d51f8d783..6b5e0632298 100644 --- a/src/main/ssh/ssh-relay-session-rejected-delivery.test.ts +++ b/src/main/ssh/ssh-relay-session-rejected-delivery.test.ts @@ -448,17 +448,20 @@ describe('SshRelaySession rejected PTY delivery recovery', () => { ) expect(reattach).toHaveBeenCalledTimes(12) - // Each later frame must carry its own delivery token — a retired delivery is dropped at the - // top of acceptPtyData, so reusing one would make both clauses below pass without proving - // anything. A re-announcing source is exactly what a new token looks like. - const nextFrame = (n: number) => - rejectedPayload({ - source: source({ spanId: `token-bad-${n}:0:4`, deliveryToken: `token-bad-${n}` }) - }) - - // Parked: a further rejected frame must not retry yet, or the cooldown means nothing. + // Let the exhaustion loop go quiet first: its last retries are timer-driven and would + // otherwise land after the clear below and satisfy the final clause on their own. + await new Promise((resolve) => setTimeout(resolve, 1500)) reattach.mockClear() - await internals.acceptPtyData(nextFrame(1)) + + // Every frame below reuses the ORIGINAL delivery token, because that is what actually happens: + // retirement is ours, not the host's, and a stalled source keeps publishing the token we + // retired. It also matters that they are identical — retirement holds ONE key per relay PTY, + // so slipping a different token in here would overwrite that key, un-retire the original and + // let the final clause pass with the wake-up path still broken. + + // Before the cooldown: parked, so nothing retries. + await internals.acceptPtyData(rejectedPayload()) + await new Promise((resolve) => setTimeout(resolve, 600)) expect(reattach).not.toHaveBeenCalled() // Backdate the park instead of burning a minute of real time — the property under test is @@ -468,7 +471,7 @@ describe('SshRelaySession rejected PTY delivery recovery', () => { } parked.parkedAt -= 60_000 - await internals.acceptPtyData(nextFrame(2)) + await internals.acceptPtyData(rejectedPayload()) await vi.waitFor(() => expect(reattach).toHaveBeenCalled(), { timeout: 10_000 }) }) diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 971f8180399..e388dc2e670 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -1774,7 +1774,8 @@ export class SshRelaySession { const offeredSource = payload.source if ( offeredSource && - this.retiredSourceDeliveries.has(payload.providerGeneration, offeredSource) + this.retiredSourceDeliveries.has(payload.providerGeneration, offeredSource) && + !this.isParkedRecoveryDue(payload.id) ) { return Promise.resolve() } @@ -1831,6 +1832,19 @@ export class SshRelaySession { }) } + /** + * A parked PTY whose cooldown has elapsed must be let back past the retired-delivery filter. + * Retirement is ours, not the host's: a stalled source keeps publishing the very token we + * retired, so that filter would swallow every frame that could re-arm the park and the pane + * would stay dark for good — the failure the cooldown exists to rule out. Letting one through + * costs nothing: it is re-classified as rejected and re-retired, so no output reaches the + * terminal; it only regains the ability to ask for recovery. + */ + private isParkedRecoveryDue(appPtyId: string): boolean { + const attempt = this.rejectedPtyRecoveryAttempts.get(appPtyId) + return !!attempt?.parkedAt && Date.now() - attempt.parkedAt >= SSH_REJECTED_PTY_RECOVERY_PARK_MS + } + private recoverRejectedPtyDelivery( payload: SshPtyDataPayload, source: SshPtyDataPayload['source']