Allow removing an unreachable SSH target (#2640)

* Allow removing an unreachable SSH target

handleRemove tried to reconnect-then-terminate before deleting the
target. When the remote host is dead, the reconnect handshake times
out and the user is stuck with a stale entry. Reroute removal through
a best-effort cleanup helper so the local entry can always be deleted.

Fixes #2626

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* review: harden SSH target removal tests

- cover removeTarget when disconnect fails in main IPC
- cover retry termination failure after reconnect in renderer removal helper
- verified unreachable-target removal in Electron with IPC and Settings UI signals

Co-authored-by: Orca <help@stably.ai>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Jinwoo-H <jinwoo0825@gmail.com>
Co-authored-by: Orca <help@stably.ai>

src/main/ipc/ssh.test.ts

@@ -275,6 +275,20 @@ describe('SSH IPC handlers', () => {
     expect(mockSshStore.removeTarget).toHaveBeenCalledWith('ssh-1')
   })
 
+  it('ssh:removeTarget removes metadata when disconnect fails', async () => {
+    const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined)
+    mockConnectionManager.disconnect.mockRejectedValueOnce(new Error('host unreachable'))
+    try {
+      await handlers.get('ssh:removeTarget')!(null, { id: 'ssh-1' })
+
+      expect(mockConnectionManager.disconnect).toHaveBeenCalledWith('ssh-1')
+      expect(mockStore.removeSshRemotePtyLeases).toHaveBeenCalledWith('ssh-1')
+      expect(mockSshStore.removeTarget).toHaveBeenCalledWith('ssh-1')
+    } finally {
+      warnSpy.mockRestore()
+    }
+  })
+
   it('ssh:removeTarget tears down an active relay before deleting the target', async () => {
     const target: SshTarget = {
       id: 'ssh-1',

src/renderer/src/components/settings/SshPane.tsx

@@ -11,6 +11,7 @@ import { SSH_TERMINATE_RECONNECT_REQUIRED } from '../../../../shared/constants'
 import { useAppStore } from '@/store'
 import { Button } from '../ui/button'
 import type { SettingsSearchEntry } from './settings-search'
+import { removeSshTargetWithBestEffortCleanup } from './ssh-target-remove'
 import { SshTargetCard } from './SshTargetCard'
 import { SshTargetDestructiveActions } from './SshTargetDestructiveActions'
 import { SshTargetForm, EMPTY_FORM, type EditingTarget } from './SshTargetForm'
@@ -150,10 +151,7 @@ export function SshPane(_props: SshPaneProps): React.JSX.Element {
 
   const handleRemove = async (id: string): Promise<void> => {
     try {
-      // Why: removing a target is destructive even after non-destructive
-      // disconnect, when remote PTYs can still be alive in the grace window.
-      await terminateSessionsWithReconnect(id)
-      await window.api.ssh.removeTarget({ id })
+      await removeSshTargetWithBestEffortCleanup(window.api.ssh, id)
       toast.success('Target removed')
       await loadTargets()
     } catch (err) {

src/renderer/src/components/settings/ssh-target-remove.test.ts

@@ -0,0 +1,100 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { SSH_TERMINATE_RECONNECT_REQUIRED } from '../../../../shared/constants'
+import { removeSshTargetWithBestEffortCleanup, type SshTargetRemoveApi } from './ssh-target-remove'
+
+function createApi(overrides: Partial<SshTargetRemoveApi> = {}): SshTargetRemoveApi {
+  return {
+    terminateSessions: vi.fn().mockResolvedValue(undefined),
+    connect: vi.fn().mockResolvedValue(undefined),
+    removeTarget: vi.fn().mockResolvedValue(undefined),
+    ...overrides
+  }
+}
+
+describe('removeSshTargetWithBestEffortCleanup', () => {
+  let warnSpy: ReturnType<typeof vi.spyOn>
+
+  beforeEach(() => {
+    warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined)
+  })
+  afterEach(() => {
+    warnSpy.mockRestore()
+  })
+
+  it('terminates then removes when the relay is already connected', async () => {
+    const api = createApi()
+    await removeSshTargetWithBestEffortCleanup(api, 'ssh-1')
+    expect(api.terminateSessions).toHaveBeenCalledWith({ targetId: 'ssh-1' })
+    expect(api.connect).not.toHaveBeenCalled()
+    expect(api.removeTarget).toHaveBeenCalledWith({ id: 'ssh-1' })
+  })
+
+  it('reconnects and retries termination when the relay is detached', async () => {
+    const terminateSessions = vi
+      .fn()
+      .mockRejectedValueOnce(new Error(`${SSH_TERMINATE_RECONNECT_REQUIRED}: relay detached`))
+      .mockResolvedValueOnce(undefined)
+    const api = createApi({ terminateSessions })
+
+    await removeSshTargetWithBestEffortCleanup(api, 'ssh-1')
+
+    expect(terminateSessions).toHaveBeenCalledTimes(2)
+    expect(api.connect).toHaveBeenCalledWith({ targetId: 'ssh-1' })
+    expect(api.removeTarget).toHaveBeenCalledWith({ id: 'ssh-1' })
+  })
+
+  it('removes the target even when the relay reconnect times out (#2626)', async () => {
+    // Why: a dead/unreachable host throws on handshake during the reconnect
+    // step; we must still let the user delete the local target entry.
+    const terminateSessions = vi
+      .fn()
+      .mockRejectedValueOnce(new Error(`${SSH_TERMINATE_RECONNECT_REQUIRED}: relay detached`))
+    const connect = vi
+      .fn()
+      .mockRejectedValue(
+        new Error(
+          "Error invoking remote method 'ssh:connect': Timed out while waiting for handshake"
+        )
+      )
+    const api = createApi({ terminateSessions, connect })
+
+    await removeSshTargetWithBestEffortCleanup(api, 'ssh-1')
+
+    expect(terminateSessions).toHaveBeenCalledTimes(1)
+    expect(connect).toHaveBeenCalledTimes(1)
+    expect(api.removeTarget).toHaveBeenCalledWith({ id: 'ssh-1' })
+  })
+
+  it('removes the target when retry termination fails after reconnect', async () => {
+    const terminateSessions = vi
+      .fn()
+      .mockRejectedValueOnce(new Error(`${SSH_TERMINATE_RECONNECT_REQUIRED}: relay detached`))
+      .mockRejectedValueOnce(new Error('shutdown failed after reconnect'))
+    const api = createApi({ terminateSessions })
+
+    await removeSshTargetWithBestEffortCleanup(api, 'ssh-1')
+
+    expect(terminateSessions).toHaveBeenCalledTimes(2)
+    expect(api.connect).toHaveBeenCalledWith({ targetId: 'ssh-1' })
+    expect(api.removeTarget).toHaveBeenCalledWith({ id: 'ssh-1' })
+  })
+
+  it('removes the target when termination fails with an unrelated error', async () => {
+    const terminateSessions = vi.fn().mockRejectedValueOnce(new Error('Some other backend error'))
+    const api = createApi({ terminateSessions })
+
+    await removeSshTargetWithBestEffortCleanup(api, 'ssh-1')
+
+    expect(api.connect).not.toHaveBeenCalled()
+    expect(api.removeTarget).toHaveBeenCalledWith({ id: 'ssh-1' })
+  })
+
+  it('propagates removeTarget failures so the caller can surface them', async () => {
+    const removeTarget = vi.fn().mockRejectedValueOnce(new Error('cannot remove'))
+    const api = createApi({ removeTarget })
+
+    await expect(removeSshTargetWithBestEffortCleanup(api, 'ssh-1')).rejects.toThrow(
+      'cannot remove'
+    )
+  })
+})

src/renderer/src/components/settings/ssh-target-remove.ts

@@ -0,0 +1,37 @@
+import { SSH_TERMINATE_RECONNECT_REQUIRED } from '../../../../shared/constants'
+
+export type SshTargetRemoveApi = {
+  terminateSessions: (args: { targetId: string }) => Promise<unknown>
+  connect: (args: { targetId: string }) => Promise<unknown>
+  removeTarget: (args: { id: string }) => Promise<unknown>
+}
+
+// Why: terminating remote PTYs is best-effort cleanup of the grace window.
+// If the server is unreachable (dead host, blocked port, expired credentials),
+// the reconnect-before-terminate path hangs on the handshake and the user is
+// stuck with a target they cannot delete (issue #2626). Local removal must
+// always succeed; the relay layer disposes any live session on its own side.
+export async function removeSshTargetWithBestEffortCleanup(
+  api: SshTargetRemoveApi,
+  id: string
+): Promise<void> {
+  try {
+    await api.terminateSessions({ targetId: id })
+  } catch (err) {
+    const message = err instanceof Error ? err.message : String(err)
+    if (message.includes(SSH_TERMINATE_RECONNECT_REQUIRED)) {
+      try {
+        await api.connect({ targetId: id })
+        await api.terminateSessions({ targetId: id })
+      } catch (reconnectErr) {
+        console.warn(
+          '[ssh] Skipping remote session cleanup during target removal:',
+          reconnectErr instanceof Error ? reconnectErr.message : String(reconnectErr)
+        )
+      }
+    } else {
+      console.warn('[ssh] Skipping remote session cleanup during target removal:', message)
+    }
+  }
+  await api.removeTarget({ id })
+}