fix(ssh): let terminate reach remote PTYs the app gave up reattaching (#12642)

Every SSH reattach failure abandons the remote terminal without shutting it down, and abandoned terminals then became structurally unreachable — excluded from reattach enumeration and, critically, filtered out of the user-facing "Terminate sessions" action, so a user could not kill them even manually. The core problem is a naming trap: `expired` never meant the remote shell died. It means the app gave up reattaching. It is written on reattach failure, on spawn-time expiry, and in bulk by a relay reset inside a `finally` that runs even when the force-stop threw. So the leases most likely to name a still-live orphan were exactly the ones the terminate path excluded. This change is reachability only. Expired leases are now reachable by an explicit user-initiated terminate, with the relay's response used as evidence: a shutdown that reports the PTY gone tombstones the lease, and leases already proven terminated are left alone. **No automatic kills were added.** Every abandon path was enumerated and none of them proves abandonment: attempts-exhausted knows nothing (the relay never answered), identity mismatch means a *live* PTY belongs to a different pane so killing it would destroy someone else's terminal, and not-found is the one branch with real proof of death — where the process is already gone and needs no shutdown. Per the rule that unprovable liveness never authorizes destroying a session, the abandon paths deliberately leave the process running. Relay-side automatic collection of unattached PTYs is deliberately NOT implemented: the relay cannot distinguish an abandoned terminal from a deliberately detached one, and the unlimited default grace exists precisely so long-running work survives disconnects and host sleep. Any bounded reaper would be killing on absence of evidence. Verified: 3 tests red on main. Negative tests assert each abandon path leaves the shell running and the lease terminable, proven real by mutation — adding a shutdown to the exhausted branch or expiring on identity mismatch each turns them red. Fixes STA-3376.
2026-10-20 16:03:20 +00:00 · 2026-08-04 23:35:39 -07:00
parent 817164d3d1
commit 6f607fae7f
3 changed files with 400 additions and 14 deletions
@@ -2467,7 +2467,7 @@ describe('SSH IPC handlers', () => {
    await expect(reconnect).resolves.toMatchObject({ targetId: 'ssh-1', status: 'connected' })
  })

-  it('ssh:terminateSessions ignores expired leases when disconnected', async () => {
+  it('ssh:terminateSessions cannot reach expired leases without a relay', async () => {
    mockStore.getSshRemotePtyLeases.mockReturnValue([
      { targetId: 'ssh-1', ptyId: 'pty-expired', state: 'expired' }
    ])
@@ -2482,6 +2482,140 @@ describe('SSH IPC handlers', () => {
    expect(mockConnectionManager.disconnect).toHaveBeenCalledWith('ssh-1')
  })

+  it('ssh:terminateSessions kills expired leases whose remote PTY may still be alive', async () => {
+    const target: SshTarget = {
+      id: 'ssh-1',
+      label: 'Server',
+      host: 'example.com',
+      port: 22,
+      username: 'deploy'
+    }
+    mockSshStore.getTarget.mockReturnValue(target)
+    mockConnectionManager.connect.mockResolvedValue({})
+    mockConnectionManager.getState.mockReturnValue({
+      targetId: 'ssh-1',
+      status: 'connected',
+      error: null,
+      reconnectAttempt: 0
+    })
+    mockStore.getSshRemotePtyLeases.mockReturnValue([
+      { targetId: 'ssh-1', ptyId: 'pty-abandoned', state: 'expired' }
+    ])
+    vi.mocked(getSshPtyProvider).mockReturnValue(mockPtyProvider as never)
+    vi.mocked(getPtyIdsForConnection).mockReturnValue([])
+    mockPtyProvider.shutdown.mockResolvedValue(undefined)
+
+    await handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' })
+    await handlers.get('ssh:terminateSessions')!(null, { targetId: 'ssh-1' })
+
+    expect(mockPtyProvider.shutdown).toHaveBeenCalledWith('ssh:ssh-1@@pty-abandoned', {
+      immediate: true,
+      keepHistory: false
+    })
+    expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith(
+      'ssh-1',
+      'pty-abandoned',
+      'terminated'
+    )
+  })
+
+  it('ssh:terminateSessions tombstones an expired lease the relay reports gone', async () => {
+    const target: SshTarget = {
+      id: 'ssh-1',
+      label: 'Server',
+      host: 'example.com',
+      port: 22,
+      username: 'deploy'
+    }
+    mockSshStore.getTarget.mockReturnValue(target)
+    mockConnectionManager.connect.mockResolvedValue({})
+    mockConnectionManager.getState.mockReturnValue({
+      targetId: 'ssh-1',
+      status: 'connected',
+      error: null,
+      reconnectAttempt: 0
+    })
+    mockStore.getSshRemotePtyLeases.mockReturnValue([
+      { targetId: 'ssh-1', ptyId: 'pty-abandoned', state: 'expired' }
+    ])
+    vi.mocked(getSshPtyProvider).mockReturnValue(mockPtyProvider as never)
+    vi.mocked(getPtyIdsForConnection).mockReturnValue([])
+    mockPtyProvider.shutdown.mockRejectedValue(new Error('PTY "pty-abandoned" not found'))
+
+    await handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' })
+    await handlers.get('ssh:terminateSessions')!(null, { targetId: 'ssh-1' })
+
+    expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith(
+      'ssh-1',
+      'pty-abandoned',
+      'terminated'
+    )
+  })
+
+  it('ssh:terminateSessions leaves leases it already proved terminated alone', async () => {
+    const target: SshTarget = {
+      id: 'ssh-1',
+      label: 'Server',
+      host: 'example.com',
+      port: 22,
+      username: 'deploy'
+    }
+    mockSshStore.getTarget.mockReturnValue(target)
+    mockConnectionManager.connect.mockResolvedValue({})
+    mockConnectionManager.getState.mockReturnValue({
+      targetId: 'ssh-1',
+      status: 'connected',
+      error: null,
+      reconnectAttempt: 0
+    })
+    mockStore.getSshRemotePtyLeases.mockReturnValue([
+      { targetId: 'ssh-1', ptyId: 'pty-tombstoned', state: 'terminated' }
+    ])
+    vi.mocked(getSshPtyProvider).mockReturnValue(mockPtyProvider as never)
+    vi.mocked(getPtyIdsForConnection).mockReturnValue([])
+    mockPtyProvider.shutdown.mockResolvedValue(undefined)
+
+    await handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' })
+    await handlers.get('ssh:terminateSessions')!(null, { targetId: 'ssh-1' })
+
+    expect(mockPtyProvider.shutdown).not.toHaveBeenCalled()
+  })
+
+  it('ssh:terminateSessions keeps an expired lease when its shutdown fails', async () => {
+    const target: SshTarget = {
+      id: 'ssh-1',
+      label: 'Server',
+      host: 'example.com',
+      port: 22,
+      username: 'deploy'
+    }
+    mockSshStore.getTarget.mockReturnValue(target)
+    mockConnectionManager.connect.mockResolvedValue({})
+    mockConnectionManager.getState.mockReturnValue({
+      targetId: 'ssh-1',
+      status: 'connected',
+      error: null,
+      reconnectAttempt: 0
+    })
+    mockStore.getSshRemotePtyLeases.mockReturnValue([
+      { targetId: 'ssh-1', ptyId: 'pty-abandoned', state: 'expired' }
+    ])
+    vi.mocked(getSshPtyProvider).mockReturnValue(mockPtyProvider as never)
+    vi.mocked(getPtyIdsForConnection).mockReturnValue([])
+    mockPtyProvider.shutdown.mockRejectedValue(new Error('mux down'))
+
+    await handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' })
+
+    await expect(
+      handlers.get('ssh:terminateSessions')!(null, { targetId: 'ssh-1' })
+    ).rejects.toThrow('Failed to terminate SSH host sessions')
+    expect(mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith(
+      'ssh-1',
+      'pty-abandoned',
+      'terminated'
+    )
+  })
+
  it('ssh:resetRelay force-stops the remote relay and expires tracked leases', async () => {
    const target: SshTarget = {
      id: 'ssh-1',
@@ -1279,28 +1279,37 @@ export function registerSshHandlers(
    invalidateConnectAttempt(args.targetId)
    await runTargetLifecycle(args.targetId, async () => {
      const provider = getSshPtyProvider(args.targetId)
-      const leasedIds = persistedStore!
-        .getSshRemotePtyLeases(args.targetId)
-        .filter((lease) => lease.state !== 'terminated' && lease.state !== 'expired')
-        .map((lease) => lease.ptyId)
+      const leases = persistedStore!.getSshRemotePtyLeases(args.targetId)
      const ptyIdsByRelayId = new Map<string, string>()
-      for (const ptyId of getPtyIdsForConnection(args.targetId)) {
+      // 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).
+      const ownedRelayIds = new Set<string>()
+      const trackPtyId = (ptyId: string, owned: boolean): void => {
        const relayPtyId = toRelaySshPtyId(args.targetId, ptyId)
-        ptyIdsByRelayId.set(relayPtyId, toAppSshPtyId(args.targetId, ptyId))
+        if (!ptyIdsByRelayId.has(relayPtyId)) {
+          ptyIdsByRelayId.set(relayPtyId, toAppSshPtyId(args.targetId, ptyId))
+        }
+        if (owned) {
+          ownedRelayIds.add(relayPtyId)
+        }
      }
-      for (const ptyId of leasedIds) {
-        const relayPtyId = toRelaySshPtyId(args.targetId, ptyId)
-        ptyIdsByRelayId.set(
-          relayPtyId,
-          ptyIdsByRelayId.get(relayPtyId) ?? toAppSshPtyId(args.targetId, ptyId)
-        )
+      for (const ptyId of getPtyIdsForConnection(args.targetId)) {
+        trackPtyId(ptyId, true)
+      }
+      for (const lease of leases) {
+        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')
      }
      const ptyIds = Array.from(ptyIdsByRelayId, ([relayPtyId, appPtyId]) => ({
        relayPtyId,
        appPtyId
      }))

-      if (ptyIds.length > 0 && !provider) {
+      if (ownedRelayIds.size > 0 && !provider) {
        throw new Error(
          `${SSH_TERMINATE_RECONNECT_REQUIRED}: SSH relay is not connected; reconnect before terminating remote sessions.`
        )
@@ -0,0 +1,243 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { SshRelaySession } from './ssh-relay-session'
+import { createMockDeps, mockDeploySuccess } from './ssh-relay-session-test-fixtures'
+
+const { muxRequestMock, openConsumerSessionMock } = vi.hoisted(() => ({
+  muxRequestMock: vi.fn(),
+  openConsumerSessionMock: vi.fn(async (_mux: unknown, options: { clientInstanceId: string }) => ({
+    clientInstanceId: options.clientInstanceId,
+    clientGeneration: 1,
+    ownerGeneration: 1,
+    ownerLease: 'test-owner-lease'
+  }))
+}))
+
+vi.mock('./ssh-relay-deploy', () => ({ deployAndLaunchRelay: vi.fn() }))
+vi.mock('./ssh-pty-consumer-session', () => ({
+  openSshPtyConsumerSession: openConsumerSessionMock
+}))
+vi.mock('../ipc/ssh-pty-output-intake-registry', () => ({
+  acceptSshPtyOutputData: vi.fn().mockResolvedValue(undefined),
+  acceptSshPtyOutputExit: vi.fn().mockResolvedValue(undefined),
+  allocateSshPtyProviderGeneration: vi.fn(() => 17),
+  beginSshPtyOutputGenerationMigration: vi.fn(() => ({
+    byPty: new Map(),
+    completion: Promise.resolve()
+  })),
+  closeSshPtyOutputGeneration: vi.fn(),
+  getSshPtyAcceptedSourceCheckpoints: vi.fn(() => []),
+  applySshPtySourceRecoveryCancellationProof: vi.fn(() => true),
+  installSshPtySourceAckPublisher: vi.fn(() => () => {}),
+  installSshPtySourceCancellationPublisher: vi.fn(() => () => {})
+}))
+vi.mock('./ssh-relay-deploy-helpers', () => ({ execCommand: vi.fn().mockResolvedValue('') }))
+vi.mock('./ssh-remote-orca-cli', () => ({
+  runRemoteOrcaCli: vi.fn().mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' })
+}))
+vi.mock('./ssh-channel-multiplexer', () => ({
+  SshChannelMultiplexer: class MockSshChannelMultiplexer {
+    notify = vi.fn()
+    notifyWithSettlement = vi.fn()
+    request = muxRequestMock
+    onNotification = vi.fn().mockReturnValue(() => {})
+    onNotificationByMethod = vi.fn().mockReturnValue(() => {})
+    onRequest = vi.fn().mockReturnValue(() => {})
+    onDispose = vi.fn().mockReturnValue(() => {})
+    dispose = vi.fn()
+    isDisposed = vi.fn().mockReturnValue(false)
+  }
+}))
+vi.mock('../agent-hooks/remote-managed-hook-installers', () => ({
+  installRemoteManagedAgentHooks: vi.fn()
+}))
+vi.mock('../providers/ssh-pty-provider', () => ({
+  SshPtyProvider: class MockSshPtyProvider {
+    onData = vi.fn().mockReturnValue(() => {})
+    onReplay = vi.fn().mockReturnValue(() => {})
+    onExit = vi.fn().mockReturnValue(() => {})
+    attach = vi.fn().mockResolvedValue(undefined)
+    attachForReconnect = vi.fn().mockResolvedValue({})
+    dispose = vi.fn()
+  }
+}))
+vi.mock('../providers/ssh-filesystem-provider', () => ({
+  SshFilesystemProvider: class MockSshFilesystemProvider {
+    dispose = vi.fn()
+  }
+}))
+vi.mock('../providers/ssh-git-provider', () => ({
+  SshGitProvider: class MockSshGitProvider {}
+}))
+vi.mock('../ipc/pty', () => ({
+  registerSshPtyProvider: vi.fn(),
+  unregisterSshPtyProvider: vi.fn(),
+  getSshPtyProvider: vi.fn(),
+  getPtyIdsForConnection: vi.fn().mockReturnValue([]),
+  clearPtyOwnershipForConnection: vi.fn(),
+  clearProviderPtyState: vi.fn(),
+  deletePtyOwnership: vi.fn(),
+  setPtyOwnership: vi.fn(),
+  restorePtyIncarnation: vi.fn(),
+  isCurrentPtyExit: vi.fn(() => true),
+  answerStartupTerminalColorQueriesForPty: vi.fn((_id: string, data: string) => data)
+}))
+vi.mock('../providers/ssh-filesystem-dispatch', () => ({
+  registerSshFilesystemProvider: vi.fn(),
+  unregisterSshFilesystemProvider: vi.fn(),
+  getSshFilesystemProvider: vi.fn().mockReturnValue({ dispose: vi.fn() })
+}))
+vi.mock('../providers/ssh-git-dispatch', () => ({
+  registerSshGitProvider: vi.fn(),
+  unregisterSshGitProvider: vi.fn()
+}))
+
+const { getSshPtyProvider, getPtyIdsForConnection, clearProviderPtyState, deletePtyOwnership } =
+  await import('../ipc/pty')
+
+const APP_PTY_ID = 'ssh:target-1@@pty-live'
+const LEAF_ID = '11111111-1111-4111-8111-111111111111'
+
+function detachedLease() {
+  return {
+    targetId: 'target-1',
+    ptyId: 'pty-live',
+    state: 'detached' as const,
+    worktreeId: 'worktree-1',
+    tabId: 'tab-1',
+    leafId: LEAF_ID
+  }
+}
+
+/**
+ * STA-3376: every reattach failure walks away from a remote PTY that may still be running a user's
+ * work. Orca's rule is that unprovable liveness never authorizes destroying a session, so these
+ * assert the opposite of a leak test: the shell must survive, and the lease must stay in a state
+ * the user-facing terminate action can still reach.
+ */
+describe('SshRelaySession abandoned remote PTYs', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    muxRequestMock.mockReset()
+    muxRequestMock.mockResolvedValue([])
+    mockDeploySuccess()
+    vi.mocked(getPtyIdsForConnection).mockReturnValue([])
+  })
+
+  async function establishWithFailingReattach(error: Error): Promise<{
+    deps: ReturnType<typeof createMockDeps>
+    shutdown: ReturnType<typeof vi.fn>
+    attachForReconnect: ReturnType<typeof vi.fn>
+  }> {
+    const deps = createMockDeps()
+    const shutdown = vi.fn().mockResolvedValue(undefined)
+    const attachForReconnect = vi.fn().mockRejectedValue(error)
+    vi.mocked(getSshPtyProvider).mockReturnValue({
+      attachForReconnect,
+      shutdown,
+      dispose: vi.fn()
+    } as unknown as ReturnType<typeof getSshPtyProvider>)
+    vi.mocked(deps.mockStore.getSshRemotePtyLeases).mockReturnValue([detachedLease()] as ReturnType<
+      typeof deps.mockStore.getSshRemotePtyLeases
+    >)
+    const session = new SshRelaySession(
+      'target-1',
+      deps.getMainWindow,
+      deps.mockStore,
+      deps.mockPortForward
+    )
+
+    await session.establish(deps.mockConn)
+
+    return { deps, shutdown, attachForReconnect }
+  }
+
+  it('leaves a live shell running and reachable when reattach attempts are exhausted', async () => {
+    // A transport stall proves nothing about the remote shell; the user's long-running process
+    // may be untouched, so the lease must stay terminable rather than be tombstoned as expired.
+    const { deps, shutdown, attachForReconnect } = await establishWithFailingReattach(
+      new Error('PTY reattach attempt timed out after 15000ms')
+    )
+
+    expect(attachForReconnect).toHaveBeenCalled()
+    expect(shutdown).not.toHaveBeenCalled()
+    expect(deps.mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith(
+      'target-1',
+      'pty-live',
+      'expired'
+    )
+    expect(deletePtyOwnership).not.toHaveBeenCalledWith(APP_PTY_ID)
+    expect(deps.mockWindow.webContents.send).not.toHaveBeenCalledWith('pty:exit', expect.anything())
+  })
+
+  it('leaves another pane live shell running when the relay reports an identity mismatch', async () => {
+    // The relay answered that a *live* PTY holds this id under a different pane identity. Killing
+    // it would destroy an unrelated terminal, so this path may only stop claiming the id.
+    const { deps, shutdown, attachForReconnect } = await establishWithFailingReattach(
+      new Error('PTY "pty-live" not found (identity mismatch)')
+    )
+
+    expect(attachForReconnect).toHaveBeenCalled()
+    expect(shutdown).not.toHaveBeenCalled()
+    expect(deps.mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith(
+      'target-1',
+      'pty-live',
+      'expired'
+    )
+    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.
+    const { deps, shutdown } = await establishWithFailingReattach(
+      new Error('PTY "pty-live" not found')
+    )
+
+    expect(shutdown).not.toHaveBeenCalled()
+    expect(deps.mockStore.markSshRemotePtyLease).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
+    })
+  })
+
+  it('keeps a recovered session attached when reattach succeeds after an earlier drop', async () => {
+    const deps = createMockDeps()
+    const shutdown = vi.fn().mockResolvedValue(undefined)
+    vi.mocked(getSshPtyProvider).mockReturnValue({
+      attachForReconnect: vi.fn().mockResolvedValue({ incarnationId: 'incarnation-1' }),
+      shutdown,
+      dispose: vi.fn()
+    } as unknown as ReturnType<typeof getSshPtyProvider>)
+    vi.mocked(deps.mockStore.getSshRemotePtyLeases).mockReturnValue([detachedLease()] as ReturnType<
+      typeof deps.mockStore.getSshRemotePtyLeases
+    >)
+    const runtime = { onPtySpawned: vi.fn(), registerPty: vi.fn() }
+    const session = new SshRelaySession(
+      'target-1',
+      deps.getMainWindow,
+      deps.mockStore,
+      deps.mockPortForward,
+      runtime as never
+    )
+
+    await session.establish(deps.mockConn)
+
+    expect(shutdown).not.toHaveBeenCalled()
+    expect(runtime.registerPty).toHaveBeenCalledWith(APP_PTY_ID, 'worktree-1', 'target-1', {
+      tabId: 'tab-1',
+      leafId: LEAF_ID,
+      incarnationId: 'incarnation-1'
+    })
+    expect(deps.mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith(
+      'target-1',
+      'pty-live',
+      'expired'
+    )
+  })
+})