fix(ssh): route the remaining expired-lease readers through the reattach predicate (#18378)

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<void>
   } 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<string, string>()
-      // 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<string>()
       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,

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')
   })
 

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)

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', {

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

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.

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<string>
   expired: Set<string>