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.
This commit is contained in:
@@ -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')
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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, {
|
||||
|
||||
@@ -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 </dev/null + redirect + &).
|
||||
@@ -960,6 +980,7 @@ export {
|
||||
cleanupStale,
|
||||
connect,
|
||||
DEFAULT_READY_TIMEOUT_MS,
|
||||
disconnect,
|
||||
expandRemotePath,
|
||||
fingerprintToken,
|
||||
isForwardBindCollision,
|
||||
|
||||
@@ -111,6 +111,29 @@ test('forceCleanupAll runs registered pending resource cleanup', async () => {
|
||||
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()]
|
||||
|
||||
@@ -23,8 +23,16 @@ function createBootstrapCoordinator() {
|
||||
const pending = new Map<string, any>()
|
||||
const generations = new Map<string, number>()
|
||||
const drains = new Map<string, Promise<void>>()
|
||||
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 }
|
||||
|
||||
Reference in New Issue
Block a user