diff --git a/src/main/ipc/ssh.test.ts b/src/main/ipc/ssh.test.ts index 7ae04e324..e526005b3 100644 --- a/src/main/ipc/ssh.test.ts +++ b/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', diff --git a/src/renderer/src/components/settings/SshPane.tsx b/src/renderer/src/components/settings/SshPane.tsx index 453639e82..00526abaf 100644 --- a/src/renderer/src/components/settings/SshPane.tsx +++ b/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 => { 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) { diff --git a/src/renderer/src/components/settings/ssh-target-remove.test.ts b/src/renderer/src/components/settings/ssh-target-remove.test.ts new file mode 100644 index 000000000..f15ce95bd --- /dev/null +++ b/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 { + return { + terminateSessions: vi.fn().mockResolvedValue(undefined), + connect: vi.fn().mockResolvedValue(undefined), + removeTarget: vi.fn().mockResolvedValue(undefined), + ...overrides + } +} + +describe('removeSshTargetWithBestEffortCleanup', () => { + let warnSpy: ReturnType + + 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' + ) + }) +}) diff --git a/src/renderer/src/components/settings/ssh-target-remove.ts b/src/renderer/src/components/settings/ssh-target-remove.ts new file mode 100644 index 000000000..2cae17589 --- /dev/null +++ b/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 + connect: (args: { targetId: string }) => Promise + removeTarget: (args: { id: string }) => Promise +} + +// 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 { + 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 }) +}