fix(desktop): bounded auto-restart for no-mux SSH tunnel flaps instead of instant connection death (#96266)

A no-mux tunnel is a single persistent `ssh -N -L` child. On main, ANY death of that child after readiness immediately set tunnel.alive=false, which poisons SshConnection.isAlive() forever; upstream lifecycle probes then treat the whole SSH connection as dead, tear down the scope, and SIGTERM a perfectly healthy backend (~10s after HERMES_BACKEND_READY in the #96266 logs: '[ssh] connection closed (no-mux tunnels killed)' -> 'Ignoring stale Hermes backend exit (SIGTERM)' -> 90s port-announcement timeout, with retry/repair looping the same failure). Now a post-readiness child death is a tunnel FLAP: the child is restarted with a bounded budget (5 attempts, 1s delay by default, injectable for tests) and only an exhausted budget marks the tunnel — and thus the connection — unhealthy. Deliberate teardown (cancelForward / close) sets tunnel.stopping, cancels any pending restart timer, and never restarts. Pre-readiness deaths keep failing fast with classified stderr (auth/bind errors unchanged). Fixes the kill chain of #96266.
2026-08-31 12:02:52 -07:00
parent 18ac3c4fb6
commit 6e41ab3460
2 changed files with 304 additions and 61 deletions
@@ -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<void>(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<void>(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<void>(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()
 })

@@ -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<string, any>
@@ -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<void>((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<void>((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)
      }