From c19f1b386b49837d7fdc8d3c8a92a47eeaad55dd Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 9 Aug 2026 01:07:58 -0700 Subject: [PATCH] fix(ssh): stop fabricating an exit when a reattach fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed attach never proves the shell exited. The relay answers not-found for a pane-identity mismatch and for any id it merely cannot hand back, so treating it as death sent the pane a synthetic `pty:exit { code: -1 }`, cleared provider state, deleted ownership and expired the lease — four claims about a process we know nothing about, on a shell that is usually still running. Collapse every failure into the non-destructive branch that already existed a few lines above (`restoreRequired = 'reattachAttemptsExhausted'` + wakeRecovery). A branch collapse, not a new mechanism: goalpost S3. Two tests pinned the deleted premise and are INVERTED rather than patched, so the new intent stays covered: - ssh-relay-orphan-abandon-paths: "retires the lease without a kill when the relay proves the PTY is gone" -> "leaves the shell running when the relay only reports the PTY as not found". Its comment claimed attach verifies liveness before answering not-found; it does not. - ssh-relay-session: "invalidates and broadcasts remote PTYs that cannot reattach" -> "leaves an unreattachable remote PTY alone while its sibling reattaches". Also repairs two clauses left red by c51be8072ba (step A), which dropped the expected-identity parameter and the expectedIdentityByPtyId map. Mutation proof: restoring the destructive block reddens 6 of the 8 oracle clauses in ssh-relay-reattach-exit-proof.test.ts; the 2 producer pins stay green. Verified the mutation landed before believing the result. Net production: -21 lines. Co-authored-by: Orca --- .../ssh-relay-orphan-abandon-paths.test.ts | 18 ++++----- ...sh-relay-session-rejected-delivery.test.ts | 6 ++- src/main/ssh/ssh-relay-session.test.ts | 15 ++++---- src/main/ssh/ssh-relay-session.ts | 37 ++++--------------- 4 files changed, 29 insertions(+), 47 deletions(-) diff --git a/src/main/ssh/ssh-relay-orphan-abandon-paths.test.ts b/src/main/ssh/ssh-relay-orphan-abandon-paths.test.ts index 9d08ac7da0f..cab697afd0d 100644 --- a/src/main/ssh/ssh-relay-orphan-abandon-paths.test.ts +++ b/src/main/ssh/ssh-relay-orphan-abandon-paths.test.ts @@ -186,24 +186,24 @@ describe('SshRelaySession abandoned remote PTYs', () => { expect(clearProviderPtyState).not.toHaveBeenCalledWith(APP_PTY_ID) }) - it('retires the lease without a kill when the relay proves the PTY is gone', async () => { - // pty.attach verifies process liveness before answering not-found, so this is the one branch - // with positive proof of death — and a dead process needs no shutdown request. + // INVERTED for STA-3077 step E-0. This case previously asserted that a bare not-found retires the + // lease and reports `pty:exit { code: -1 }`, on the premise that attach proves liveness before + // answering not-found. It does not: the relay answers not-found for a pane-identity mismatch and + // for any id it merely cannot hand back, so the premise licensed a fabricated death certificate. + // The clause is kept — inverted — so the new intent stays covered rather than silently dropped. + it('leaves the shell running when the relay only reports the PTY as not found', async () => { const { deps, shutdown } = await establishWithFailingReattach( new Error('PTY "pty-live" not found') ) expect(shutdown).not.toHaveBeenCalled() - expect(deps.mockStore.markSshRemotePtyLease).toHaveBeenCalledWith( + expect(deps.mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( 'target-1', 'pty-live', 'expired' ) - expect(clearProviderPtyState).toHaveBeenCalledWith(APP_PTY_ID) - expect(deps.mockWindow.webContents.send).toHaveBeenCalledWith('pty:exit', { - id: APP_PTY_ID, - code: -1 - }) + expect(clearProviderPtyState).not.toHaveBeenCalledWith(APP_PTY_ID) + expect(deps.mockWindow.webContents.send).not.toHaveBeenCalledWith('pty:exit', expect.anything()) }) it('keeps a recovered session attached when reattach succeeds after an earlier drop', async () => { 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 e4fbb44142f..90ec6857a46 100644 --- a/src/main/ssh/ssh-relay-session-rejected-delivery.test.ts +++ b/src/main/ssh/ssh-relay-session-rejected-delivery.test.ts @@ -246,7 +246,8 @@ describe('SshRelaySession rejected PTY delivery recovery', () => { const recovery = reattachKnownPty.mock.calls[0]?.[0] expect(recovery?.ptyId).toBe('pty-bad') expect(Array.from(recovery?.activeLeaseByPtyId.keys() ?? [])).toEqual(['pty-bad']) - expect(Array.from(recovery?.expectedIdentityByPtyId.keys() ?? [])).toEqual([]) + // The sibling `expectedIdentityByPtyId` clause is gone with the map itself (STA-3077 step A): + // reattach no longer carries pane identity at all, which subsumes "carries none for this pty". expect(recovery?.targetedDeliveryRecovery).toBe('fresh-activation') expect(deps.mockStore.markSshRemotePtyLeasesAttachedAsync).toHaveBeenCalledWith('target-1', [ 'pty-bad' @@ -356,7 +357,8 @@ describe('SshRelaySession rejected PTY delivery recovery', () => { internals.reattachRejectedPty('pty-bad', mux, 23, 'confirm-existing') ).resolves.toBe(true) - expect(attachForReconnect).toHaveBeenCalledWith('pty-bad', undefined, checkpoint) + // Two args since step A dropped the expected-identity parameter from the attach call. + expect(attachForReconnect).toHaveBeenCalledWith('pty-bad', checkpoint) expect(commit).toHaveBeenCalledOnce() expect(mux.dispose).not.toHaveBeenCalled() }) diff --git a/src/main/ssh/ssh-relay-session.test.ts b/src/main/ssh/ssh-relay-session.test.ts index 7478caa8c48..80ef9857f5e 100644 --- a/src/main/ssh/ssh-relay-session.test.ts +++ b/src/main/ssh/ssh-relay-session.test.ts @@ -648,7 +648,11 @@ describe('SshRelaySession', () => { expect(mockStore.markSshRemotePtyLeasesAttachedAsync).not.toHaveBeenCalled() }) - it('invalidates and broadcasts remote PTYs that cannot reattach after relay reconnect', async () => { + // INVERTED for STA-3077 step E-0. This case previously asserted that a PTY the relay reports as + // not-found is invalidated and broadcast as an exit. A not-found is not proof of death, so the + // broadcast was a fabricated one; what must still hold is that the healthy sibling reattaches and + // the unproven one is left alone rather than written off. + it('leaves an unreattachable remote PTY alone while its sibling reattaches', async () => { const { mockConn, mockStore, mockPortForward, getMainWindow, mockWindow } = createMockDeps() const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) await session.establish(mockConn) @@ -670,12 +674,9 @@ describe('SshRelaySession', () => { expect(mockAttach).toHaveBeenCalledWith('pty-stale') expect(mockAttach).toHaveBeenCalledWith('pty-live') - expect(clearProviderPtyState).toHaveBeenCalledWith('ssh:target-1@@pty-stale') - expect(deletePtyOwnership).toHaveBeenCalledWith('ssh:target-1@@pty-stale') - expect(mockWindow.webContents.send).toHaveBeenCalledWith('pty:exit', { - id: 'ssh:target-1@@pty-stale', - code: -1 - }) + expect(clearProviderPtyState).not.toHaveBeenCalledWith('ssh:target-1@@pty-stale') + expect(deletePtyOwnership).not.toHaveBeenCalledWith('ssh:target-1@@pty-stale') + expect(mockWindow.webContents.send).not.toHaveBeenCalledWith('pty:exit', expect.anything()) }) it('retries transient reattach failure without tearing down provider registration', async () => { diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index ed072b9189b..fcebb46fc9f 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -11,7 +11,7 @@ import { SshPtyProvider } from '../providers/ssh-pty-provider' import type { SshPtyAttachResult } from '../providers/ssh-pty-session-reattach' import type { SshPtyDataCallback, SshPtyExitCallback } from '../providers/ssh-pty-provider-contract' import type { SshPtyRecoveryActivationLease } from '../providers/ssh-pty-notification-routing' -import { isSshPtyIdentityMismatchError, isSshPtyNotFoundError } from '../providers/ssh-pty-errors' +import { isSshPtyNotFoundError } from '../providers/ssh-pty-errors' import { toAppSshPtyId, toRelaySshPtyId } from '../providers/ssh-pty-id' import { SshFilesystemProvider } from '../providers/ssh-filesystem-provider' import { isMethodNotFoundError } from './ssh-filesystem-stream-reader' @@ -2418,7 +2418,7 @@ export class SshRelaySession { if (!shouldContinue()) { return } - this.handlePtyReattachFailure(ptyId, appPtyId, pendingReattach, error) + this.handlePtyReattachFailure(ptyId, pendingReattach, error) } finally { recoveryActivationLease?.retire() sourceActivationLease?.rollback() @@ -2561,40 +2561,19 @@ export class SshRelaySession { private handlePtyReattachFailure( ptyId: string, - appPtyId: string, pending: PendingPtyReattach, error: unknown ): void { - if (!isSshPtyNotFoundError(error)) { - pending.restoreRequired = 'reattachAttemptsExhausted' - this.wakeRecovery(pending) - console.warn( - `[ssh-relay-session] Leaving PTY ${ptyId} detached for ${this.targetId} after bounded reattach attempts failed: ${ - error instanceof Error ? error.message : String(error) - }` - ) - return - } - if (isSshPtyIdentityMismatchError(error)) { - console.warn( - `[ssh-relay-session] Ignoring stale PTY ${ptyId} for ${this.targetId} after relay identity mismatch: ${ - error instanceof Error ? error.message : String(error) - }` - ) - return - } + // No attach failure proves the shell exited — not even a not-found, which the relay also + // returns when it merely cannot hand this id back. Only an exit the relay observed on a live + // stream may report one, so every failure leaves the pane detached and recoverable. + pending.restoreRequired = 'reattachAttemptsExhausted' + this.wakeRecovery(pending) console.warn( - `[ssh-relay-session] Dropping stale PTY ${ptyId} for ${this.targetId} after relay reattach failed: ${ + `[ssh-relay-session] Leaving PTY ${ptyId} detached for ${this.targetId} after reattach failed: ${ error instanceof Error ? error.message : String(error) }` ) - clearProviderPtyState(appPtyId) - deletePtyOwnership(appPtyId) - this.store.markSshRemotePtyLease(this.targetId, ptyId, 'expired') - const win = this.getMainWindow() - if (win && !win.isDestroyed()) { - win.webContents.send('pty:exit', { id: appPtyId, code: -1 }) - } } private async sourceRecoveryRequest(