From 0c69cac48a889676b85921a05df3b3c0fbc78ce9 Mon Sep 17 00:00:00 2001 From: 686f6c61 Date: Tue, 25 Aug 2026 11:48:58 +0200 Subject: [PATCH] fix(desktop): terminate owned SSH serve backends on quit teardownSshConnection closed the tunnel and SSH transport but never killed the detached serve --isolated process. Spawn uses setsid/nohup, so the backend reparents to pid 1, keeps state.db open, and accumulates across Cmd+Q. Reuse cleanupStale via disconnect while SSH can still exec, sequence remote kill before close, and seal the bootstrap coordinator so reconnect during a prevented first quit cannot respawn. The quit race is 6s to cover cleanupStale's 5s wait-for-exit loop. --- .../desktop/electron/connection-apply.test.ts | 40 ++++++++++++++++++- apps/desktop/electron/connection-apply.ts | 33 ++++++++++++++- apps/desktop/electron/main.ts | 40 ++++++++++++------- .../desktop/electron/remote-lifecycle.test.ts | 24 +++++++++++ apps/desktop/electron/remote-lifecycle.ts | 21 ++++++++++ .../ssh-bootstrap-coordinator.test.ts | 23 +++++++++++ .../electron/ssh-bootstrap-coordinator.ts | 17 +++++++- 7 files changed, 180 insertions(+), 18 deletions(-) diff --git a/apps/desktop/electron/connection-apply.test.ts b/apps/desktop/electron/connection-apply.test.ts index ccf697a928..e19e90b4a2 100644 --- a/apps/desktop/electron/connection-apply.test.ts +++ b/apps/desktop/electron/connection-apply.test.ts @@ -1,6 +1,11 @@ import { describe, expect, it, vi } from 'vitest' -import { applyConnectionChange, commitConnectionFailure, resolveTerminalConnection } from './connection-apply' +import { + applyConnectionChange, + commitConnectionFailure, + resolveTerminalConnection, + teardownSshState +} from './connection-apply' function deferred() { let resolve!: () => void @@ -86,6 +91,39 @@ describe('resolveTerminalConnection', () => { }) }) +describe('teardownSshState', () => { + it('terminates the owned remote backend before closing its tunnel and SSH transport', async () => { + const events: string[] = [] + + const ssh = { + cancelForward: async () => events.push('forward'), + close: async () => events.push('ssh') + } + + await teardownSshState( + { ssh, ownershipId: 'owner', localPort: 1234, remotePort: 5678 }, + { cleanupRemote: async () => events.push('remote') } + ) + + expect(events).toEqual(['remote', 'forward', 'ssh']) + }) + + it('still closes the SSH transport when remote cleanup fails', async () => { + const close = vi.fn(async () => undefined) + + await teardownSshState( + { ssh: { cancelForward: vi.fn(async () => undefined), close }, ownershipId: 'owner' }, + { + cleanupRemote: async () => { + throw new Error('remote unavailable') + } + } + ) + + expect(close).toHaveBeenCalledOnce() + }) +}) + describe('commitConnectionFailure', () => { it('prevents a stale bootstrap from publishing failure state', () => { const stale = Promise.resolve('stale') diff --git a/apps/desktop/electron/connection-apply.ts b/apps/desktop/electron/connection-apply.ts index 75865a8289..03ad29741b 100644 --- a/apps/desktop/electron/connection-apply.ts +++ b/apps/desktop/electron/connection-apply.ts @@ -61,4 +61,35 @@ async function resolveTerminalConnectionForSender(webContentsId, getTarget, ensu ) } -export { applyConnectionChange, commitConnectionFailure, resolveTerminalConnection, resolveTerminalConnectionForSender } +async function teardownSshState(state, { cleanupRemote }) { + // Remote process first, while the SSH channel can still exec kill. + // Then drop the local forward and close the transport. Each step is + // best-effort so a failed remote cleanup cannot trap Cmd+Q (#91668). + try { + await cleanupRemote(state.ssh, state.ownershipId) + } catch { + // Remote teardown is best-effort; always release the local tunnel and SSH transport. + } + + try { + if (state.localPort && state.remotePort) { + await state.ssh.cancelForward(state.localPort, state.remotePort) + } + } catch { + // Best effort; closing the transport below drops any remaining forwards. + } + + try { + await state.ssh.close() + } catch { + // The app must still be able to quit when SSH teardown fails. + } +} + +export { + applyConnectionChange, + commitConnectionFailure, + resolveTerminalConnection, + resolveTerminalConnectionForSender, + teardownSshState +} diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index a38f15ed7d..dea407b5fb 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -85,7 +85,7 @@ import { buildBrowserWindowUrl } from './browser-windows' import { detectBundleSkew } from './bundle-skew' -import { applyConnectionChange } from './connection-apply' +import { applyConnectionChange, teardownSshState } from './connection-apply' import { apiRequestRegistryConnectionId, authModeFromStatus, @@ -9526,19 +9526,20 @@ async function teardownSshConnection(profile) { terminalIpc.disposeTerminalSessionsForSshScope(scope) - try { - if (state.localPort && state.remotePort) { - await state.ssh.cancelForward(state.localPort, state.remotePort) + // Kill the owned remote serve --isolated *before* closing the SSH + // transport. Spawn detaches with setsid/nohup, so closing the tunnel + // alone leaves the backend at pid 1 holding state.db (#91668). + // Windows remotes use a different lifecycle (connectWindowsRemote) and + // are left to a follow-up; POSIX is the leak that OOM'd gateways. + await teardownSshState( + { + ...state, + ownershipId: state.ownershipId || sshOwnershipKey(profile) + }, + { + cleanupRemote: state.remotePlatform === 'Windows' ? async () => {} : remoteLifecycle.disconnect } - } catch { - // best effort - } - - try { - await state.ssh.close() - } catch { - // best effort - } + ) } // CRITICAL: this must mirror resolveRemoteBackend's precedence, not just return @@ -9811,6 +9812,7 @@ async function bootstrapSshConnectionInner(profile, sshConfig, reuseToken, sourc sshConnections.set(scope, { ssh, fingerprint, + ownershipId: result.ownershipId || sshOwnershipKey(profile), localPort: result.localPort, remotePort: result.remotePort, pid: result.pid, @@ -16217,6 +16219,12 @@ app.on('before-quit', event => { return } + // A prevented first quit leaves the renderer alive while teardown runs. + // Seal the SSH coordinator before touching connections so reconnect + // callbacks cannot recreate a backend for a registration whose app is + // already quitting (#91668). + sshBootstrapCoordinator.shutdown() + if (!backendQuitTeardownDone) { event.preventDefault() void backendShutdown.run().finally(() => { @@ -16227,7 +16235,6 @@ app.on('before-quit', event => { if ((sshConnections.size > 0 || sshBootstrapCoordinator.promises().length > 0) && !sshQuitTeardownDone) { event.preventDefault() - sshBootstrapCoordinator.cancelAll() const scopes = [...sshConnections.keys()] const pending = Promise.allSettled([ @@ -16235,7 +16242,10 @@ app.on('before-quit', event => { ...sshBootstrapCoordinator.promises() ]) - void Promise.race([pending, new Promise(resolve => setTimeout(resolve, 4_000))]).then(async () => { + // cleanupStale waits up to 5s for the owned pid to exit (50 * 100ms). + // The previous 4s race could close SSH first and leave serve --isolated + // reparented to pid 1. + void Promise.race([pending, new Promise(resolve => setTimeout(resolve, 6_000))]).then(async () => { await sshBootstrapCoordinator.forceCleanupAll() sshQuitTeardownDone = true app.quit() diff --git a/apps/desktop/electron/remote-lifecycle.test.ts b/apps/desktop/electron/remote-lifecycle.test.ts index 865e12d705..e2b0456770 100644 --- a/apps/desktop/electron/remote-lifecycle.test.ts +++ b/apps/desktop/electron/remote-lifecycle.test.ts @@ -12,6 +12,7 @@ import { buildSpawnCommand, cleanupStale, connect, + disconnect, expandRemotePath, fingerprintToken, isForwardBindCollision, @@ -470,6 +471,29 @@ test.skipIf(process.platform === 'win32')( } ) +test('disconnect reaps the backend recorded for this desktop ownership', async () => { + const lock = ownedLock() + + const ssh = fakeSsh([ + [/cat .*backend\.lock\.json/, JSON.stringify(lock)], + [/kill -0 333/, 'ALIVE\n'], + [/print\("OWNED"/, 'OWNED\n'] + ]) + + await disconnect(ssh, OWNERSHIP_ID) + + assert.ok(ssh.calls.some(command => /kill 333\b/.test(command))) + assert.ok(ssh.calls.some(command => /rm -f .*backend\.lock\.json/.test(command))) +}) + +test('disconnect is a no-op when this desktop has no lockfile', async () => { + const ssh = fakeSsh([[/cat .*backend\.lock\.json/, '']]) + + await disconnect(ssh, OWNERSHIP_ID) + + assert.ok(!ssh.calls.some(command => /\bkill\b/.test(command))) +}) + test('cleanupStale kills ONLY a provably-ours pid, always drops the lockfile', async () => { const notOurs = fakeSsh([[/print\("OWNED"/, 'FOREIGN\n']]) await cleanupStale(notOurs, OWNERSHIP_ID, { diff --git a/apps/desktop/electron/remote-lifecycle.ts b/apps/desktop/electron/remote-lifecycle.ts index 4a672685ec..06fbe4711e 100644 --- a/apps/desktop/electron/remote-lifecycle.ts +++ b/apps/desktop/electron/remote-lifecycle.ts @@ -518,6 +518,26 @@ async function cleanupStale(ssh, ownershipId, lock, pidAlive = true) { await removeLockfile(ssh, ownershipId) } +// Normal disconnect (quit, connection switch): reuse cleanupStale so we +// kill only a provably-owned serve --isolated and drop our lockfile. +// Closing the SSH transport first is not enough — spawn detaches with +// setsid/nohup, so the backend reparents to pid 1 and keeps state.db +// open (#91668). +async function disconnect(ssh, ownershipId) { + if (!ssh || !ownershipId) { + return + } + + const lock = await readLockfile(ssh, ownershipId) + + if (!lock) { + return + } + + const pidAlive = await remotePidAlive(ssh, lock.pid) + await cleanupStale(ssh, ownershipId, lock, pidAlive) +} + // Detach so the backend survives the SSH channel closing: setsid (Linux) // starts a new session; macOS has no setsid, so fall back to nohup (HUP-immune; // fd-detachment is already handled by { await promise }) +test('shutdown cancels active bootstraps and permanently rejects respawn attempts', async () => { + const coordinator = createBootstrapCoordinator() + const gate = deferred() + + const active = coordinator.start('primary', 'old', async lease => { + await gate.promise + lease.assertCurrent() + }) + + coordinator.shutdown() + gate.resolve() + + await assert.rejects(active, (error: any) => error.kind === 'superseded') + let started = 0 + await assert.rejects( + coordinator.start('primary', 'new', async () => { + started += 1 + }), + (error: any) => error.kind === 'superseded' + ) + assert.equal(started, 0) +}) + test('cancelAll invalidates every pending scope and exposes promises for quit', async () => { const coordinator = createBootstrapCoordinator() const gates = [deferred(), deferred()] diff --git a/apps/desktop/electron/ssh-bootstrap-coordinator.ts b/apps/desktop/electron/ssh-bootstrap-coordinator.ts index 7badbcb594..80d9a2464b 100644 --- a/apps/desktop/electron/ssh-bootstrap-coordinator.ts +++ b/apps/desktop/electron/ssh-bootstrap-coordinator.ts @@ -23,8 +23,16 @@ function createBootstrapCoordinator() { const pending = new Map() const generations = new Map() const drains = new Map>() + let shutdownRequested = false function start(scope, fingerprint, run) { + if (shutdownRequested) { + const error: any = new Error('SSH bootstrap was cancelled because Desktop is quitting.') + error.kind = 'superseded' + + return Promise.reject(error) + } + const current = pending.get(scope) if (current?.fingerprint === fingerprint) { @@ -121,6 +129,13 @@ function createBootstrapCoordinator() { } } + function shutdown() { + // Terminal: reconnect callbacks during a prevented first quit must not + // spawn a replacement serve --isolated for an app that is already leaving. + shutdownRequested = true + cancelAll() + } + async function forceCleanupAll() { const cleanups = [...active].flatMap(entry => [...entry.forceCleanups]) await Promise.allSettled(cleanups.map(cleanup => cleanup())) @@ -130,7 +145,7 @@ function createBootstrapCoordinator() { return [...active].map(entry => entry.promise) } - return { active, cancel, cancelAll, cancelAndWait, forceCleanupAll, pending, promises, start } + return { active, cancel, cancelAll, cancelAndWait, forceCleanupAll, pending, promises, shutdown, start } } export { createBootstrapCoordinator, sshConfigFingerprint }