perf: reuse SSH connections per server, with a retry rule that never repeats work

Every ssh.exec opened its own connection: a TCP handshake, a key exchange and an
authentication round trip per command. A key rotation paid for that eight times,
a deployment six, and refreshing M profile states M times.

Connections are now kept per server. The three risks that made this worth doing
carefully are handled explicitly:

- Staleness. A pooled connection can be dead exactly when it matters. Liveness is
  tracked through error, close and end, and a lease that finds a dead entry opens
  a new one. The remaining race, where the connection dies between the check and
  the command, is caught by the retry rule below.
- Retrying. Only a failure that proves the command never reached the server is
  retried, and only once, and only on a connection that was already established
  before this call. execClient marks exactly that case, when the channel fails to
  open. A command that opened a stream is never repeated, because the server may
  already be acting on it - repeating a deployment is not this layer's decision.
  Two tests hold that line: widening the rule to any failure fails both.
- Lifetime. Idle connections close after a minute, the pool is reference counted
  so a shared connection survives until its last user is done, closeAll runs
  during quit, and every pooled client keeps a standing error listener so an
  error while idle cannot reach the uncaughtException handler.

A trust-on-first-use connection is never pooled: it was established without
verifying the fingerprint, so it must not serve a later verified call. A change
to host, port, user, auth type, key path or trusted fingerprint invalidates the
pooled connection.

ssh-service coverage rises from 61% to 90% of lines and 97% of functions.

Also in this commit, the smaller items from the same review:

- Diagnostics batched records that queue up while a write is in flight into one
  append, and chmod runs once per file instead of once per record. At the debug
  level every IPC call writes a line, which is exactly when troubleshooting.
- The set that suppresses duplicate deployment notifications is trimmed instead
  of growing for the lifetime of the process.
- The updater kept the same once('error') pattern on its spawned helper that
  took the app down through the SSH client.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

src/main/diagnostics-service.cjs

@@ -46,6 +46,9 @@ class DiagnosticsService {
     this.preferencesProvider = preferencesProvider;
     this.sessionId = crypto.randomUUID();
     this.writeChain = Promise.resolve();
+    this.pendingLines = [];
+    this.pendingFlush = null;
+    this.securedFiles = new Set();
     this.initialized = false;
     this.lastWriteError = null;
     this.lastBundlePath = null;
@@ -115,13 +118,25 @@ class DiagnosticsService {
       sessionId: this.sessionId,
       details
     });
-    const line = `${JSON.stringify(record)}\n`;
-    this.writeChain = this.writeChain.then(async () => {
+    this.pendingLines.push(`${JSON.stringify(record)}\n`);
+    // At the debug level every IPC call and every Gitea request writes a line.
+    // Records that queue up while a write is in flight are appended together, so
+    // a burst costs one open/write/close instead of one per record.
+    if (this.pendingFlush) return this.pendingFlush;
+    this.pendingFlush = this.writeChain.then(async () => {
+      this.pendingFlush = null;
+      const lines = this.pendingLines.splice(0).join('');
+      if (!lines) return true;
       try {
         if (!this.initialized) await fs.mkdir(this.logDirectory, { recursive: true, mode: 0o700 });
         const target = await this.rotateIfNeeded(this.filePathForToday());
-        await fs.appendFile(target, line, { encoding: 'utf8', mode: 0o600 });
-        try { await fs.chmod(target, 0o600); } catch {}
+        await fs.appendFile(target, lines, { encoding: 'utf8', mode: 0o600 });
+        // The mode above only applies when appendFile creates the file, so the
+        // explicit chmod is needed once per file rather than once per record.
+        if (!this.securedFiles.has(target)) {
+          try { await fs.chmod(target, 0o600); } catch { /* best effort */ }
+          this.securedFiles.add(target);
+        }
         this.lastWriteError = null;
         return true;
       } catch (error) {
@@ -129,7 +144,8 @@ class DiagnosticsService {
         return false;
       }
     });
-    return this.writeChain;
+    this.writeChain = this.pendingFlush.catch(() => {});
+    return this.pendingFlush;
   }
 
   debug(event, details) { return this.log('debug', event, details); }

src/main/ssh-service.cjs

@@ -53,9 +53,28 @@ function parseCapabilityOutput(output) {
 }
 
 class SshService {
-  constructor({ store, diagnostics }) {
+  constructor({ store, diagnostics, idleConnectionMs = 60_000 }) {
     this.store = store;
     this.diagnostics = diagnostics;
+    // Every command used to pay for a TCP handshake, a key exchange and an
+    // authentication round trip. Sessions are kept per server for a short while
+    // so a sequence of commands shares one connection.
+    this.sessions = new Map();
+    this.idleConnectionMs = idleConnectionMs;
+  }
+
+  // A connection is only reusable for a server whose identity and credentials
+  // are unchanged. Anything in this key changing means a new connection.
+  sessionKey(server) {
+    return JSON.stringify([
+      server.id,
+      server.host,
+      server.port || 22,
+      server.username,
+      server.authType,
+      server.privateKeyPath || '',
+      server.hostFingerprint || '',
+    ]);
   }
 
   async validateServerConfiguration(server, secrets = {}) {
@@ -116,6 +135,114 @@ class SshService {
   async withClient(serverId, action, options = {}) {
     const server = this.store.getServer(serverId);
     if (!server) throw new Error('The configured SSH server no longer exists.');
+    // A trust-on-first-use connection is established without checking the
+    // fingerprint, so it must never serve a later verified call.
+    if (options.trustOnFirstUse) return this.withDedicatedClient(server, action, options);
+    return this.withPooledClient(server, action, options);
+  }
+
+  // Retrying is only safe while the command has not reached the server. Once a
+  // stream is open the remote side may already be deploying, and repeating that
+  // is not something this layer is allowed to decide.
+  isPreCommandFailure(error) {
+    return error?.beforeCommand === true;
+  }
+
+  async withPooledClient(server, action, options) {
+    const key = this.sessionKey(server);
+    for (let attempt = 0; ; attempt += 1) {
+      const session = await this.leaseSession(server, key, options);
+      try {
+        const result = await action(session.client, server, session.fingerprint);
+        this.releaseSession(session);
+        return result;
+      } catch (error) {
+        const staleConnection = session.reused && attempt === 0 && this.isPreCommandFailure(error);
+        this.discardSession(session);
+        if (!staleConnection) throw error;
+        await this.diagnostics?.debug('ssh.session.stale-retry', { serverId: server.id, host: server.host, message: error.message });
+      }
+    }
+  }
+
+  createSession(server, key, options) {
+    const entry = { key, client: null, fingerprint: null, leases: 0, dead: false, established: false, idleTimer: null, opening: null };
+    entry.opening = this
+      .withDedicatedClient(server, async (client, _server, fingerprint) => ({ client, fingerprint }), options, { keepOpen: true })
+      .then((opened) => {
+        entry.client = opened.client;
+        entry.fingerprint = opened.fingerprint;
+        entry.established = true;
+        // Without a standing listener an error on an idle connection is
+        // unhandled, which terminates the main process.
+        opened.client.on('error', () => this.markSessionDead(entry));
+        opened.client.on('close', () => this.markSessionDead(entry));
+        opened.client.on('end', () => this.markSessionDead(entry));
+      });
+    this.sessions.set(key, entry);
+    return entry;
+  }
+
+  async leaseSession(server, key, options) {
+    const pooled = this.sessions.get(key);
+    // Only a connection that was already up before this call may be retried on
+    // failure. Callers that arrive while one is still being opened share both
+    // the connection and its outcome.
+    const reused = Boolean(pooled && !pooled.dead && pooled.established);
+    const entry = pooled && !pooled.dead ? pooled : this.createSession(server, key, options);
+    entry.leases += 1;
+    if (entry.idleTimer) { clearTimeout(entry.idleTimer); entry.idleTimer = null; }
+    try {
+      await entry.opening;
+    } catch (error) {
+      entry.leases -= 1;
+      this.markSessionDead(entry);
+      throw error;
+    }
+    return { client: entry.client, fingerprint: entry.fingerprint, reused, entry };
+  }
+
+  markSessionDead(entry) {
+    entry.dead = true;
+    if (this.sessions.get(entry.key) === entry) this.sessions.delete(entry.key);
+    if (entry.idleTimer) { clearTimeout(entry.idleTimer); entry.idleTimer = null; }
+    if (entry.leases <= 0) this.endSession(entry);
+  }
+
+  endSession(entry) {
+    if (!entry.client) return;
+    try { entry.client.end(); } catch { /* already closed */ }
+  }
+
+  releaseSession(session) {
+    const entry = session.entry;
+    entry.leases -= 1;
+    if (entry.dead) { if (entry.leases <= 0) this.endSession(entry); return; }
+    if (entry.leases > 0) return;
+    entry.idleTimer = setTimeout(() => {
+      entry.idleTimer = null;
+      this.markSessionDead(entry);
+    }, this.idleConnectionMs);
+    entry.idleTimer.unref?.();
+  }
+
+  discardSession(session) {
+    const entry = session.entry;
+    entry.leases -= 1;
+    this.markSessionDead(entry);
+  }
+
+  // Closes every pooled connection. The application calls this while quitting so
+  // no socket outlives the process.
+  closeAll() {
+    for (const entry of [...this.sessions.values()]) {
+      entry.leases = 0;
+      this.markSessionDead(entry);
+    }
+  }
+
+  async withDedicatedClient(server, action, options = {}, { keepOpen = false } = {}) {
+    const serverId = server.id;
     const Client = loadSshClient();
     const connection = await this.connectionOptions(server, options);
     const client = new Client();
@@ -125,7 +252,8 @@ class SshService {
       const finish = (callback, value) => {
         if (settled) return;
         settled = true;
-        try { client.end(); } catch {}
+        // A session that stays in the pool is closed by the pool, not here.
+        if (!(keepOpen && callback === resolve)) { try { client.end(); } catch { /* already closed */ } }
         callback(value);
       };
       client.once('ready', async () => {
@@ -167,6 +295,9 @@ class SshService {
         if (error) {
           clearTimeout(timer);
           completed = true;
+          // The channel never opened, so the command did not reach the server.
+          // This is the only failure the pool is allowed to retry.
+          error.beforeCommand = true;
           reject(error);
           return;
         }

src/main/update-service.cjs

@@ -499,7 +499,9 @@ class UpdateService {
         5000,
       );
       child.once?.("spawn", () => finish(resolve));
-      child.once?.("error", (error) => finish(reject, error));
+      // Kept attached rather than `once`: a process that fails to start can
+      // report a second error, and an unhandled 'error' event ends this process.
+      child.on?.("error", (error) => finish(reject, error));
       if (!child.once) finish(resolve);
     });
 
@@ -630,7 +632,9 @@ class UpdateService {
         clearTimeout(timer);
         resolve();
       });
-      child.once?.("error", (error) => {
+      // Kept attached rather than `once`: a second error would otherwise have no
+      // listener left, and an unhandled 'error' event ends this process.
+      child.on?.("error", (error) => {
         clearTimeout(timer);
         reject(error);
       });