diff --git a/apps/desktop/electron/ssh-connection.test.ts b/apps/desktop/electron/ssh-connection.test.ts index 8af1cec722..fa954cebbd 100644 --- a/apps/desktop/electron/ssh-connection.test.ts +++ b/apps/desktop/electron/ssh-connection.test.ts @@ -619,12 +619,12 @@ test('no-mux: an unrelated listener cannot mask a delayed bind failure', async ( srv.close() }) -test('no-mux: tunnel death after readiness makes the connection unhealthy', async () => { +test('no-mux: tunnel death after readiness triggers a bounded restart, then unhealthy', async () => { const net = await import('node:net') const srv = net.createServer() await new Promise(resolve => srv.listen(0, '127.0.0.1', resolve)) const localPort = (srv.address() as any).port - let tunnel + const tunnels: any[] = [] const spawnFn: any = (_cmd, args) => { const child: any = new EventEmitter() @@ -632,10 +632,15 @@ test('no-mux: tunnel death after readiness makes the connection unhealthy', asyn child.stderr = new EventEmitter() child.exitCode = null - child.kill = () => {} + child.kill = () => { + child.exitCode = 0 + process.nextTick(() => child.emit('exit', 0)) + + return true + } if (args.includes('-N')) { - tunnel = child + tunnels.push(child) process.nextTick(() => child.stderr.emit('data', Buffer.from(`Local forwarding listening on 127.0.0.1 port ${localPort}.`)) ) @@ -646,11 +651,129 @@ test('no-mux: tunnel death after readiness makes the connection unhealthy', asyn return child } - const conn = new SshConnection({ host: 'box' }, { spawnFn, mux: false }) + const conn = new SshConnection( + { host: 'box' }, + { spawnFn, mux: false, tunnelRestartLimit: 1, tunnelRestartDelayMs: 5 } + ) + await conn.open() await conn.forward(localPort, 9119) - tunnel.emit('exit', 255) - assert.equal(await conn.isAlive(), false) + assert.equal(tunnels.length, 1) + + // First death after readiness: a restart is pending, so the connection is + // NOT reported dead — the exact flap that used to cascade into a SIGTERM of + // a healthy backend (#96266). + tunnels[0].exitCode = 255 + tunnels[0].emit('exit', 255) + assert.equal(await conn.isAlive(), true, 'flap within the restart budget must not poison isAlive') + + // The restart spawns a replacement -N child. + await new Promise(resolve => setTimeout(resolve, 30)) + assert.equal(tunnels.length, 2, 'a replacement tunnel child is spawned') + assert.equal(await conn.isAlive(), true) + + // Second death exhausts the budget (limit 1): now the connection is dead. + tunnels[1].exitCode = 255 + tunnels[1].emit('exit', 255) + await new Promise(resolve => setTimeout(resolve, 30)) + assert.equal(tunnels.length, 2, 'no restart past the budget') + assert.equal(await conn.isAlive(), false, 'exhausted budget reports the connection dead') + srv.close() +}) + +test('no-mux: cancelForward during a pending tunnel restart cancels the restart', async () => { + const net = await import('node:net') + const srv = net.createServer() + await new Promise(resolve => srv.listen(0, '127.0.0.1', resolve)) + const localPort = (srv.address() as any).port + const tunnels: any[] = [] + + const spawnFn: any = (_cmd, args) => { + const child: any = new EventEmitter() + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + child.exitCode = null + + child.kill = () => { + child.exitCode = 0 + process.nextTick(() => child.emit('exit', 0)) + + return true + } + + if (args.includes('-N')) { + tunnels.push(child) + process.nextTick(() => + child.stderr.emit('data', Buffer.from(`Local forwarding listening on 127.0.0.1 port ${localPort}.`)) + ) + } else { + process.nextTick(() => child.emit('close', 0)) + } + + return child + } + + const conn = new SshConnection( + { host: 'box' }, + { spawnFn, mux: false, tunnelRestartLimit: 5, tunnelRestartDelayMs: 20 } + ) + + await conn.forward(localPort, 9119) + tunnels[0].exitCode = 255 + tunnels[0].emit('exit', 255) + + // Deliberate teardown while the restart timer is pending. + await conn.cancelForward(localPort, 9119) + await new Promise(resolve => setTimeout(resolve, 60)) + assert.equal(tunnels.length, 1, 'cancelled forward must not restart its tunnel') + srv.close() +}) + +test('no-mux: close() during a pending tunnel restart cancels the restart', async () => { + const net = await import('node:net') + const srv = net.createServer() + await new Promise(resolve => srv.listen(0, '127.0.0.1', resolve)) + const localPort = (srv.address() as any).port + const tunnels: any[] = [] + + const spawnFn: any = (_cmd, args) => { + const child: any = new EventEmitter() + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + child.exitCode = null + + child.kill = () => { + child.exitCode = 0 + process.nextTick(() => child.emit('exit', 0)) + + return true + } + + if (args.includes('-N')) { + tunnels.push(child) + process.nextTick(() => + child.stderr.emit('data', Buffer.from(`Local forwarding listening on 127.0.0.1 port ${localPort}.`)) + ) + } else { + process.nextTick(() => child.emit('close', 0)) + } + + return child + } + + const conn = new SshConnection( + { host: 'box' }, + { spawnFn, mux: false, tunnelRestartLimit: 5, tunnelRestartDelayMs: 20 } + ) + + await conn.open() + await conn.forward(localPort, 9119) + tunnels[0].exitCode = 255 + tunnels[0].emit('exit', 255) + + await conn.close() + await new Promise(resolve => setTimeout(resolve, 60)) + assert.equal(tunnels.length, 1, 'closed connection must not restart its tunnels') srv.close() }) diff --git a/apps/desktop/electron/ssh-connection.ts b/apps/desktop/electron/ssh-connection.ts index 57264ec639..9a11a19378 100644 --- a/apps/desktop/electron/ssh-connection.ts +++ b/apps/desktop/electron/ssh-connection.ts @@ -39,6 +39,14 @@ import path from 'node:path' const DEFAULT_CONNECT_TIMEOUT_MS = 15_000 const DEFAULT_EXEC_TIMEOUT_MS = 20_000 const DEFAULT_FORWARD_TIMEOUT_MS = 15_000 +// No-mux tunnels are one `ssh -N -L` child each; a transient child death +// (network blip, sshd restart, laptop resume) used to instantly poison +// isAlive() and cascade upstream into a full teardown that SIGTERM'd a +// healthy backend (#96266). Instead, restart the child a bounded number of +// times; consecutive pre-readiness failures exhaust the budget and only then +// is the connection reported dead. +const DEFAULT_TUNNEL_RESTART_LIMIT = 5 +const DEFAULT_TUNNEL_RESTART_DELAY_MS = 1_000 const CONTROL_PERSIST_SECONDS = 300 // eslint-disable-next-line no-control-regex -- deliberately reject control chars in ssh targets @@ -425,7 +433,7 @@ function runSsh(args, { timeoutMs, spawnFn = spawn, stdin = 'ignore', stdinData, let stderr = '' let settled = false - const timer = setTimeout(() => { + const timer: any = setTimeout(() => { if (settled) { return } @@ -547,6 +555,8 @@ class SshConnection { _connectTimeoutMs: number _execTimeoutMs: number _forwardTimeoutMs: number + _tunnelRestartLimit: number + _tunnelRestartDelayMs: number _opened: boolean _mux: boolean _tunnels: Map @@ -588,6 +598,8 @@ class SshConnection { this._connectTimeoutMs = opts.connectTimeoutMs ?? DEFAULT_CONNECT_TIMEOUT_MS this._execTimeoutMs = opts.execTimeoutMs ?? DEFAULT_EXEC_TIMEOUT_MS this._forwardTimeoutMs = opts.forwardTimeoutMs ?? DEFAULT_FORWARD_TIMEOUT_MS + this._tunnelRestartLimit = opts.tunnelRestartLimit ?? DEFAULT_TUNNEL_RESTART_LIMIT + this._tunnelRestartDelayMs = opts.tunnelRestartDelayMs ?? DEFAULT_TUNNEL_RESTART_DELAY_MS this._opened = false } @@ -795,10 +807,149 @@ class SshConnection { return result.stdout } + // Spawn one persistent `ssh -N -L` child for a no-mux tunnel and wait for it + // to confirm local forwarding on stderr. Resolves once ready. Rejects on a + // pre-readiness death or confirmation timeout, with the captured stderr on + // `error.tunnelStderr` so the caller can classify auth/bind failures. After + // readiness, a child death is a tunnel FLAP: it is routed into + // _handleNoMuxTunnelFlap (bounded restart) instead of poisoning isAlive() + // outright — the old instant-poison path is how a ~10s local tunnel blip + // cascaded into SIGTERM of a healthy backend (#96266). + _startNoMuxTunnelChild(tunnel: any, spec: string, args: string[], localPort: number | string) { + return new Promise((resolve, reject) => { + const child = this._spawnFn('ssh', args, { stdio: ['ignore', 'ignore', 'pipe'] }) + tunnel.child = child + let stderr = '' + let readyConfirmed = false + let settled = false + let downHandled = false + + const readyTimeout: any = setTimeout(() => { + finishFail(new Error('tunnel did not confirm local forwarding')) + }, this._forwardTimeoutMs) + + readyTimeout.unref?.() + + const finishOk = () => { + if (settled) { + return + } + + settled = true + clearTimeout(readyTimeout) + resolve() + } + + const finishFail = (error: any) => { + if (settled) { + return + } + + settled = true + clearTimeout(readyTimeout) + error.tunnelStderr = stderr + reject(error) + } + + const onDown = (cause: string, error: any) => { + if (!readyConfirmed) { + tunnel.alive = tunnel.child === child ? false : tunnel.alive + finishFail(error) + + return + } + + if (downHandled || tunnel.child !== child) { + return + } + + downHandled = true + this._handleNoMuxTunnelFlap(tunnel, spec, args, localPort, cause) + } + + const readyPattern = new RegExp(`Local forwarding listening on .* port ${localPort}\\b`) + child.stderr?.on('data', (d: any) => { + if (readyConfirmed) { + return + } + + stderr = `${stderr}${String(d)}`.slice(-16_384) + + if (readyPattern.test(stderr)) { + readyConfirmed = true + finishOk() + } + }) + child.on('error', (error: any) => onDown(`tunnel process failed (${error?.message || error})`, error)) + child.on('exit', (code: any) => + onDown(`tunnel process exited with code ${code}`, new Error(`tunnel process exited with code ${code}`)) + ) + child.on('close', (code: any) => + onDown(`tunnel process closed with code ${code}`, new Error(`tunnel process closed with code ${code}`)) + ) + }) + } + + // A ready no-mux tunnel child died. Deliberate teardown (cancelForward / + // close) and superseded tunnels stay dead; otherwise restart the child up to + // the bounded budget, and only mark the tunnel (and thus the connection) + // unhealthy once the budget is exhausted. The budget is cumulative per + // forward — a tunnel that keeps dying immediately after confirming readiness + // must not restart forever. + _handleNoMuxTunnelFlap(tunnel: any, spec: string, args: string[], localPort: number | string, cause: string) { + if (tunnel.stopping || this._tunnels.get(spec) !== tunnel) { + tunnel.alive = false + + return + } + + if (tunnel.restarts >= this._tunnelRestartLimit) { + tunnel.alive = false + this._logLine( + `tunnel 127.0.0.1:${localPort} down (${cause}); restart budget exhausted (${this._tunnelRestartLimit})` + ) + + return + } + + tunnel.restarts += 1 + this._logLine( + `tunnel 127.0.0.1:${localPort} flapped (${cause}); restarting ` + + `(${tunnel.restarts}/${this._tunnelRestartLimit}) in ${this._tunnelRestartDelayMs}ms` + ) + + const timer: any = setTimeout(() => { + tunnel.restartTimer = null + + if (tunnel.stopping || this._tunnels.get(spec) !== tunnel) { + tunnel.alive = false + + return + } + + this._startNoMuxTunnelChild(tunnel, spec, args, localPort).then( + () => { + tunnel.alive = true + this._logLine(`tunnel 127.0.0.1:${localPort} restarted`) + }, + (error: any) => { + // A restart that never confirmed readiness may leave its child + // running; stop it before deciding whether to retry. + void Promise.resolve(stopTunnelChild(tunnel.child)).catch(() => undefined) + this._handleNoMuxTunnelFlap(tunnel, spec, args, localPort, `restart failed: ${error?.message || error}`) + } + ) + }, this._tunnelRestartDelayMs) + + timer.unref?.() + tunnel.restartTimer = timer + } + // Establish a local→remote forward. Mux: `-O forward` against the master. // No-mux: spawn a persistent `ssh -N -L` child that IS the tunnel; ready when - // the local port accepts. The child dying = tunnel down (isAlive of the - // backend catches it upstream). + // the local port accepts. A child dying AFTER readiness is a tunnel flap and + // is restarted with a bounded budget (#96266); only an exhausted budget (or + // a deliberate cancel/close) marks the connection unhealthy for isAlive(). async forward(localPort, remotePort, remoteHost = '127.0.0.1') { const spec = forwardSpec(localPort, remotePort, remoteHost) this._logLine(`forwarding 127.0.0.1:${localPort} -> ${remoteHost}:${remotePort}`) @@ -815,67 +966,20 @@ class SshConnection { target(this.user, this.host) ] - const child = this._spawnFn('ssh', args, { stdio: ['ignore', 'ignore', 'pipe'] }) - const tunnel = { child, alive: true } + const tunnel: any = { alive: true, child: null, restarts: 0, restartTimer: null, stopping: false } this._tunnels.set(spec, tunnel) - let stderr = '' - let readyConfirmed = false - let readyResolve - let readyReject - - const ready = new Promise((resolve, reject) => { - readyResolve = resolve - readyReject = reject - }) - - const readyPattern = new RegExp(`Local forwarding listening on .* port ${localPort}\\b`) - child.stderr?.on('data', d => { - if (readyConfirmed) { - return - } - - stderr = `${stderr}${String(d)}`.slice(-16_384) - - if (readyPattern.test(stderr)) { - readyConfirmed = true - readyResolve() - } - }) - child.on('error', error => { - tunnel.alive = false - readyReject(error) - }) - child.on('exit', code => { - tunnel.alive = false - readyReject(new Error(`tunnel process exited with code ${code}`)) - }) - child.on('close', code => { - tunnel.alive = false - readyReject(new Error(`tunnel process closed with code ${code}`)) - }) - let readyTimeout try { - await Promise.race([ - ready, - new Promise((_, reject) => { - readyTimeout = setTimeout( - () => reject(new Error('tunnel did not confirm local forwarding')), - this._forwardTimeoutMs - ) - }) - ]) + await this._startNoMuxTunnelChild(tunnel, spec, args, localPort) } catch (error: any) { try { - await stopTunnelChild(child) + await stopTunnelChild(tunnel.child) this._tunnels.delete(spec) } catch (stopError) { throw this._fail(stopError, SSH_ERROR.UNKNOWN) } - throw this._fail(stderr || error, SSH_ERROR.UNKNOWN) - } finally { - clearTimeout(readyTimeout) + throw this._fail(error?.tunnelStderr || error, SSH_ERROR.UNKNOWN) } return @@ -904,6 +1008,14 @@ class SshConnection { const tunnel = this._tunnels.get(spec) if (tunnel) { + tunnel.stopping = true + tunnel.alive = false + + if (tunnel.restartTimer) { + clearTimeout(tunnel.restartTimer) + tunnel.restartTimer = null + } + await stopTunnelChild(tunnel.child) this._tunnels.delete(spec) this._logLine(`cancelled forward 127.0.0.1:${localPort}`) @@ -931,6 +1043,14 @@ class SshConnection { if (!this._mux) { for (const [spec, tunnel] of this._tunnels) { + tunnel.stopping = true + tunnel.alive = false + + if (tunnel.restartTimer) { + clearTimeout(tunnel.restartTimer) + tunnel.restartTimer = null + } + await stopTunnelChild(tunnel.child) this._tunnels.delete(spec) }