From b8da193b7afbc3a3be3edf18bf32cf3a55ea09e7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 02:00:42 -0700 Subject: [PATCH] fix(ssh): route the remaining expired-lease readers through the reattach predicate (#18378) --- src/main/ipc/ssh-connection-handlers.ts | 29 ++++++++-- src/main/ipc/ssh-relay-reset-resume.test.ts | 16 ++++-- src/main/ipc/ssh-terminate-sessions.test.ts | 45 ++++++++++++++- .../persistence-ssh-pending-pty-kill.test.ts | 55 +++++++++++++++++++ .../ssh-pty-kill-intent-operations.ts | 13 ++++- .../ssh/ssh-orphan-relay-pty-sweep.test.ts | 14 +++++ src/main/ssh/ssh-orphan-relay-pty-sweep.ts | 9 ++- 7 files changed, 163 insertions(+), 18 deletions(-) diff --git a/src/main/ipc/ssh-connection-handlers.ts b/src/main/ipc/ssh-connection-handlers.ts index 93fd6ff0e33..9ea533c77c3 100644 --- a/src/main/ipc/ssh-connection-handlers.ts +++ b/src/main/ipc/ssh-connection-handlers.ts @@ -1,5 +1,9 @@ import { ipcMain } from 'electron' -import type { SshTarget, SshTerminateSessionsResult } from '../../shared/ssh-types' +import { + sshRemotePtyLeaseAllowsReattach, + type SshTarget, + type SshTerminateSessionsResult +} from '../../shared/ssh-types' import { SSH_TERMINATE_RECONNECT_REQUIRED } from '../../shared/constants' import { isSshPtyNotFoundError } from '../providers/ssh-pty-errors' import { toAppSshPtyId, toRelaySshPtyId } from '../providers/ssh-pty-id' @@ -67,6 +71,15 @@ async function doResetRelay(targetId: string, target: SshTarget): Promise } finally { const ptyIds = new Set(getPtyIdsForConnection(targetId)) for (const lease of persistedStore!.getSshRemotePtyLeases(targetId)) { + // Deliberately the raw state, not `sshRemotePtyLeaseAllowsReattach`: this asks which routes + // the force-stop just invalidated, not which leases may be reattached. An already-`expired` + // lease has no route left for reset to retire — re-marking it `expired` is a no-op write, and + // any local handle this connection really holds arrives through `getPtyIdsForConnection` + // above, whatever the lease says. Reset also may not upgrade it to `terminated`: killing the + // relay daemon makes its PTYs permanently unreachable, which is not evidence they exited + // (docs/reference/ssh-execution-boundary.md). Nothing here can adopt a stranger either — the + // replacement relay namespaces every id under a fresh mint epoch, so an old orphan lease can + // only fail its next reattach. if (lease.state !== 'terminated' && lease.state !== 'expired') { ptyIds.add(lease.ptyId) // Why: only a host-acknowledged force-stop may retire a lease. When it threw we never @@ -112,8 +125,9 @@ export function registerSshConnectionHandlers(): void { const provider = getSshPtyProvider(args.targetId) const leases = persistedStore!.getSshRemotePtyLeases(args.targetId) const ptyIdsByRelayId = new Map() - // Why: only leases the app still believes it owns may force a reconnect; 'expired' ones are - // swept opportunistically because they can name a host that is gone for good (issue #2626). + // Why: only leases the app still believes it owns may force a reconnect; a lease whose route + // died for good is swept opportunistically instead, so a target that can no longer answer + // never blocks its own removal (issue #2626, and the renderer tolerates the refusal there). const ownedRelayIds = new Set() const trackPtyId = (ptyId: string, owned: boolean): void => { const relayPtyId = toRelaySshPtyId(args.targetId, ptyId) @@ -131,9 +145,12 @@ export function registerSshConnectionHandlers(): void { if (lease.state === 'terminated') { continue } - // Why: 'expired' records that reattach gave up, never that the remote shell died — those are - // precisely the orphans, so the user's terminate action has to be able to reach them. - trackPtyId(lease.ptyId, lease.state !== 'expired') + // Why the predicate and not `state !== 'expired'`: an `expired` lease carrying no + // retirement mark records only that reattach gave up, never that the remote shell died, so + // it is exactly the orphan the user's terminate must reach — and reaching it needs the + // relay, which is what the fence below demands. Only `supersededBy` / `relayIdRecycled` + // prove the route is dead for good, and those stay unowned. + trackPtyId(lease.ptyId, sshRemotePtyLeaseAllowsReattach(lease)) } const ptyIds = Array.from(ptyIdsByRelayId, ([relayPtyId, appPtyId]) => ({ relayPtyId, diff --git a/src/main/ipc/ssh-relay-reset-resume.test.ts b/src/main/ipc/ssh-relay-reset-resume.test.ts index 36ad2090435..ea460d777f3 100644 --- a/src/main/ipc/ssh-relay-reset-resume.test.ts +++ b/src/main/ipc/ssh-relay-reset-resume.test.ts @@ -60,7 +60,9 @@ describe('SSH IPC handlers', () => { mockConnectionManager.getConnection.mockReturnValue(undefined) mockStore.getSshRemotePtyLeases.mockReturnValue([ { targetId: 'ssh-1', ptyId: 'pty-1', state: 'detached' }, - { targetId: 'ssh-1', ptyId: 'pty-expired', state: 'expired' } + { targetId: 'ssh-1', ptyId: 'pty-expired', state: 'expired' }, + { targetId: 'ssh-1', ptyId: 'pty-superseded', state: 'expired', supersededBy: 'pty-9' }, + { targetId: 'ssh-1', ptyId: 'pty-recycled', state: 'expired', relayIdRecycled: true } ]) vi.mocked(getPtyIdsForConnection).mockReturnValue(['pty-2']) @@ -69,11 +71,13 @@ describe('SSH IPC handlers', () => { expect(mockConnectionManager.connect).toHaveBeenCalledWith(target) expect(mockForceStopRelayForTarget).toHaveBeenCalledWith(conn, 'ssh-1') expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith('ssh-1', 'pty-1', 'expired') - expect(mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( - 'ssh-1', - 'pty-expired', - 'expired' - ) + // Every already-`expired` lease is skipped on its raw state, marked or not: reset retires the + // routes this force-stop invalidated, and an expired lease has none left to retire. It is also + // never upgraded to `terminated` — a killed relay makes its PTYs unreachable, not proven dead. + for (const ptyId of ['pty-expired', 'pty-superseded', 'pty-recycled']) { + expect(mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith('ssh-1', ptyId, 'expired') + expect(mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith('ssh-1', ptyId, 'terminated') + } expect(mockConnectionManager.disconnect).toHaveBeenCalledWith('ssh-1') }) diff --git a/src/main/ipc/ssh-terminate-sessions.test.ts b/src/main/ipc/ssh-terminate-sessions.test.ts index ccdc19bbe34..d8a588a9bad 100644 --- a/src/main/ipc/ssh-terminate-sessions.test.ts +++ b/src/main/ipc/ssh-terminate-sessions.test.ts @@ -22,6 +22,7 @@ vi.mock('../providers/ssh-git-dispatch', () => mocks.sshGitDispatch) vi.mock('../ssh/ssh-port-forward', () => mocks.sshPortForward) vi.mock('../ssh/ssh-port-scanner', () => mocks.sshPortScanner) +import { SSH_TERMINATE_RECONNECT_REQUIRED } from '../../shared/constants' import type { SshConnectionState, SshTarget } from '../../shared/ssh-types' import { clearProviderPtyState, @@ -172,9 +173,9 @@ describe('SSH IPC handlers', () => { // Issue #12661: an offline sweep tears down local transport only. Reporting plain success would // read as "the remote shells are gone" when nobody asked the host. - it('ssh:terminateSessions reports expired leases as unverifiable without a relay', async () => { + it('ssh:terminateSessions reports superseded leases as unverifiable without a relay', async () => { mockStore.getSshRemotePtyLeases.mockReturnValue([ - { targetId: 'ssh-1', ptyId: 'pty-expired', state: 'expired' } + { targetId: 'ssh-1', ptyId: 'pty-expired', state: 'expired', supersededBy: 'pty-2' } ]) vi.mocked(getSshPtyProvider).mockReturnValue(undefined) vi.mocked(getPtyIdsForConnection).mockReturnValue([]) @@ -184,7 +185,8 @@ describe('SSH IPC handlers', () => { ).resolves.toEqual({ terminated: 0, unverifiable: 1 }) expect(mockPtyProvider.shutdown).not.toHaveBeenCalled() - // Still no forced reconnect: an expired lease can name a host that is gone for good (#2626). + // Still no forced reconnect: a newer lease won this pane, so this route died for good and must + // never block a target the user is trying to remove (#2626). expect(mockConnectionManager.disconnect).toHaveBeenCalledWith('ssh-1') expect(mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( 'ssh-1', @@ -193,6 +195,43 @@ describe('SSH IPC handlers', () => { ) }) + it('ssh:terminateSessions leaves a recycled relay id out of the reconnect fence', async () => { + // The host listed this id under a different PTY incarnation, so it no longer routes to this + // lease's shell — reconnecting could only aim the stop at a stranger's process. + mockStore.getSshRemotePtyLeases.mockReturnValue([ + { targetId: 'ssh-1', ptyId: 'pty-recycled', state: 'expired', relayIdRecycled: true } + ]) + vi.mocked(getSshPtyProvider).mockReturnValue(undefined) + vi.mocked(getPtyIdsForConnection).mockReturnValue([]) + + await expect( + handlers.get('ssh:terminateSessions')!(null, { targetId: 'ssh-1' }) + ).resolves.toEqual({ terminated: 0, unverifiable: 1 }) + expect(mockConnectionManager.disconnect).toHaveBeenCalledWith('ssh-1') + }) + + // An `expired` lease carrying neither retirement mark is an orphan, not a corpse: it records only + // that this client lost its route. Answering `unverifiable` there strands a remote shell the user + // just ordered stopped, when a reconnect is exactly what would reach it. + it('ssh:terminateSessions demands a reconnect for an unmarked expired lease', async () => { + mockStore.getSshRemotePtyLeases.mockReturnValue([ + { targetId: 'ssh-1', ptyId: 'pty-orphan', state: 'expired' } + ]) + vi.mocked(getSshPtyProvider).mockReturnValue(undefined) + vi.mocked(getPtyIdsForConnection).mockReturnValue([]) + + await expect( + handlers.get('ssh:terminateSessions')!(null, { targetId: 'ssh-1' }) + ).rejects.toThrow(SSH_TERMINATE_RECONNECT_REQUIRED) + + expect(mockPtyProvider.shutdown).not.toHaveBeenCalled() + expect(mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( + 'ssh-1', + 'pty-orphan', + 'terminated' + ) + }) + it('ssh:terminateSessions reports nothing unverifiable when there is nothing to reach', async () => { mockStore.getSshRemotePtyLeases.mockReturnValue([]) vi.mocked(getSshPtyProvider).mockReturnValue(undefined) diff --git a/src/main/persistence-ssh-pending-pty-kill.test.ts b/src/main/persistence-ssh-pending-pty-kill.test.ts index b5821f404ad..627d9f38de0 100644 --- a/src/main/persistence-ssh-pending-pty-kill.test.ts +++ b/src/main/persistence-ssh-pending-pty-kill.test.ts @@ -234,6 +234,61 @@ describe('Store SSH pending PTY kills', () => { expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([]) }) + // An `expired` lease says this CLIENT lost its handle, never that the shell died. The row is the + // last route back to it, so collecting it with the retired order would take the id out of the + // bulk reattach set and out of the orphan sweep's leave-alone list at once. + it('keeps an unmarked expired lease when its intent ages out', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'pty-1', state: 'attached' }) + store.markSshRemotePtyLease('ssh-1', 'pty-1', 'expired') + store.recordSshRemotePtyKillIntent('ssh-1', 'pty-1', { + requestedAt: NOW, + incarnationId: 'inc-a', + attempts: 0 + }) + + store.pruneExpiredSshRemotePtyKillIntents('ssh-1', NOW + SSH_PENDING_PTY_KILL_TTL_MS + 1) + + const kept = store.getSshRemotePtyLeases('ssh-1') + expect(kept).toMatchObject([{ ptyId: 'pty-1', state: 'expired' }]) + expect(kept[0]).not.toHaveProperty('pendingKill') + }) + + it('collects an expired lease whose relay id the host recycled', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'pty-1', state: 'attached' }) + // The mark the replay writes when the host lists this id under another incarnation: the route + // is dead for good, so nothing can reattach it and the row is pure bookkeeping. + store.markSshRemotePtyLease('ssh-1', 'pty-1', 'expired', { relayIdRecycled: true }) + store.recordSshRemotePtyKillIntent('ssh-1', 'pty-1', { + requestedAt: NOW, + incarnationId: 'inc-a', + attempts: 0 + }) + + store.pruneExpiredSshRemotePtyKillIntents('ssh-1', NOW + SSH_PENDING_PTY_KILL_TTL_MS + 1) + + expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([]) + }) + + it('collects a superseded expired lease that carries no pane identity', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'pty-1', state: 'expired' }) + const leases = store.getSshRemotePtyLeases('ssh-1') + // Supersession normally writes this alongside pane identity; forcing the mark alone isolates + // what the predicate contributes from what the pane-identity guard already covered. + leases[0].supersededBy = 'pty-2' + store.recordSshRemotePtyKillIntent('ssh-1', 'pty-1', { + requestedAt: NOW, + incarnationId: 'inc-a', + attempts: 0 + }) + + store.pruneExpiredSshRemotePtyKillIntents('ssh-1', NOW + SSH_PENDING_PTY_KILL_TTL_MS + 1) + + expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([]) + }) + it('scopes intents to their own target', async () => { const store = await createStore() store.recordSshRemotePtyKillIntent('ssh-1', 'pty-1', { diff --git a/src/main/persistence/leasing-ssh-ptys/ssh-pty-kill-intent-operations.ts b/src/main/persistence/leasing-ssh-ptys/ssh-pty-kill-intent-operations.ts index 61012771207..52d1c51e9f2 100644 --- a/src/main/persistence/leasing-ssh-ptys/ssh-pty-kill-intent-operations.ts +++ b/src/main/persistence/leasing-ssh-ptys/ssh-pty-kill-intent-operations.ts @@ -7,13 +7,22 @@ import { type SshPendingPtyKill, type SshPendingPtyKillEntry } from '../../../shared/ssh-pending-pty-kill' -import type { SshRemotePtyLease } from '../../../shared/ssh-types' +import { sshRemotePtyLeaseAllowsReattach, type SshRemotePtyLease } from '../../../shared/ssh-types' import type { SshPtyLeaseOperations } from './ssh-pty-lease-operations' +/** A row whose only content was the kill order: no pane identity, and a state that names a route + * nothing can reattach. + * + * `expired` alone is not that. It records that this CLIENT lost its handle, never that the shell + * died, and the row is the client's last route back to it — dropping it takes the id out of the + * bulk reattach set (`reattachKnownPtys`) and out of the orphan sweep's leave-alone list in the + * same write, turning a process left running on purpose into a sweepable one. Only a lease + * carrying `supersededBy` or `relayIdRecycled` has a route that died for good, which is exactly + * what `sshRemotePtyLeaseAllowsReattach` already distinguishes. */ function isDisposableKillOnlyLease(lease: SshRemotePtyLease): boolean { return ( lease.pendingKill === undefined && - (lease.state === 'terminated' || lease.state === 'expired') && + !sshRemotePtyLeaseAllowsReattach(lease) && lease.worktreeId === undefined && lease.tabId === undefined && lease.leafId === undefined diff --git a/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts b/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts index e2d8ef6c9dc..3201f8709fe 100644 --- a/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts +++ b/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts @@ -176,6 +176,20 @@ describe('sweepOrphanedRelayPtys', () => { expect(harness.shutdown).not.toHaveBeenCalled() }) + it('leaves a PTY whose expired lease names a recycled relay id alone', async () => { + // `relayIdRecycled` is the one expired lease the reattach predicate refuses, so it is the case + // most likely to be mistaken for a licence to kill. The sweep asks a different question: this + // id now names some OTHER incarnation, which makes a stop more dangerous, not less. + const harness = createHarness( + [hostEntry()], + [{ ...lease('pty-1', 'expired'), relayIdRecycled: true }] + ) + + await run(harness) + + expect(harness.shutdown).not.toHaveBeenCalled() + }) + it('leaves alone a lease the real supersede path expired when a pane re-leased', async () => { // Drives the actual persistence operation rather than asserting the state by hand, so this // stays true only while supersede really does leave the predecessor's process running. diff --git a/src/main/ssh/ssh-orphan-relay-pty-sweep.ts b/src/main/ssh/ssh-orphan-relay-pty-sweep.ts index fbc72bf5ec8..def6de6ebb4 100644 --- a/src/main/ssh/ssh-orphan-relay-pty-sweep.ts +++ b/src/main/ssh/ssh-orphan-relay-pty-sweep.ts @@ -41,7 +41,14 @@ export type SshOrphanRelayPtySweepArgs = { * `reattachAttemptsExhausted` and the lease stays `attached`, hence routed. * * Folding it into `routed` would work, but it would also lose the reason in the skip log, and this - * is the distinction the sweep most needs to be able to explain. */ + * is the distinction the sweep most needs to be able to explain. + * + * Deliberately the raw state rather than `sshRemotePtyLeaseAllowsReattach`: that predicate answers + * "may this lease be reattached", and this asks "may this client stop the process". Every non- + * `terminated` state answers no either way, so sorting the marked leases (`supersededBy`, + * `relayIdRecycled`) into `routed` instead would move nothing but the skip reason — and both marks + * are written by paths that leave the remote process running on purpose, so they must keep + * refusing the stop rather than authorizing one. */ function clientClaims(args: SshOrphanRelayPtySweepArgs): { routed: Set expired: Set