fix(sftp): preserve shared SSH transport when channel opens abort

docs/research/sftp-shared-channel-cancellation.md

@@ -0,0 +1,20 @@
+# Shared SSH connection cancellation audit
+
+Cancelling a transfer while its isolated SFTP channel is opening calls the
+bounded-open abort path. Previously that path ended/destroyed the physical SSH
+transport. Production pooled SFTP sessions normally share a terminal SSH
+connection, so cancelling one transfer can disconnect terminals, browsing and
+other transfers. The same teardown happened on the channel-open deadline.
+
+The real startTransfer/cancelTransfer path reproduces physical end/destroy calls;
+its old test double lacked those methods and could not detect the problem.
+
+The repair rejects the abandoned caller promptly, retains the shared transport,
+closes a channel arriving after abandonment, and blocks further channel-open
+allocation on that transport while any abandoned request remains unresolved.
+This bounds retry accumulation without queuing controls behind a transfer.
+Healthy parallel opens remain allowed; callback settlement or transport closure
+releases the abandoned-open bookkeeping. Existing active channels remain usable.
+
+This separately reproduced mechanism is relevant to the connection-loss themes
+in #2973 and #2832, but does not establish the cause of their VPN/jump-host reports.

electron/bridges/boundedSftpOpen.cjs

@@ -1,6 +1,30 @@
 "use strict";
 
-const { invalidateSshTransport } = require("./sshTransportInvalidation.cjs");
+// An abandoned OPEN cannot be cancelled in SSH, but its physical connection
+// may own unrelated terminals. Block only additional opens until its callback
+// or transport closure settles; retain no unbounded retry queue.
+const abandonedOpens = new WeakMap();
+
+function abandonOpen(client, token) {
+  let state = abandonedOpens.get(client);
+  if (!state) {
+    state = { tokens: new Set(), onClose: null };
+    state.onClose = () => {
+      if (abandonedOpens.get(client) === state) abandonedOpens.delete(client);
+      state.tokens.clear();
+    };
+    abandonedOpens.set(client, state);
+    client.once?.("close", state.onClose);
+  }
+  state.tokens.add(token);
+}
+
+function settleAbandonedOpen(client, token) {
+  const state = abandonedOpens.get(client);
+  if (!state || !state.tokens.delete(token) || state.tokens.size > 0) return;
+  abandonedOpens.delete(client);
+  client.removeListener?.("close", state.onClose);
+}
 
 const DEFAULT_SFTP_CHANNEL_OPEN_TIMEOUT_MS = 10_000;
 
@@ -24,6 +48,12 @@ function openBoundedSftpChannel(sshClient, options = {}) {
     return Promise.resolve(null);
   }
   const signal = options.signal || null;
+  if (signal?.aborted) return Promise.reject(createSftpOpenAbortError(signal));
+  if (abandonedOpens.has(sshClient)) {
+    const error = new Error("A previous SFTP channel open is still pending on this connection");
+    error.code = "SFTP_CHANNEL_OPEN_PENDING";
+    return Promise.reject(error);
+  }
   const setTimeoutFn = options.setTimeoutFn || setTimeout;
   const clearTimeoutFn = options.clearTimeoutFn || clearTimeout;
   const timeoutMs = Math.max(
@@ -33,13 +63,15 @@ function openBoundedSftpChannel(sshClient, options = {}) {
 
   return new Promise((resolve, reject) => {
     let settled = false;
+    let requestPending = false;
+    const token = Symbol("sftp-open");
     let timer = null;
     const cleanup = () => {
       if (timer) clearTimeoutFn(timer);
       timer = null;
       signal?.removeEventListener?.("abort", onAbort);
     };
-    const finish = (error, channel = null, { invalidateTransport = false } = {}) => {
+    const finish = (error, channel = null, { abandon = false } = {}) => {
       if (settled) {
         closeSftpChannel(channel);
         return false;
@@ -47,13 +79,13 @@ function openBoundedSftpChannel(sshClient, options = {}) {
       settled = true;
       cleanup();
       if (error && channel) closeSftpChannel(channel);
-      if (invalidateTransport) invalidateSshTransport(sshClient);
+      if (abandon && requestPending) abandonOpen(sshClient, token);
       if (error) reject(error);
       else resolve(channel);
       return true;
     };
     const onAbort = () => finish(createSftpOpenAbortError(signal), null, {
-      invalidateTransport: true,
+      abandon: true,
     });
 
     if (signal?.aborted) {
@@ -66,15 +98,20 @@ function openBoundedSftpChannel(sshClient, options = {}) {
     timer = setTimeoutFn(() => {
       const error = new Error(`SFTP channel open timed out after ${timeoutMs}ms`);
       error.code = "SFTP_CHANNEL_OPEN_TIMEOUT";
-      finish(error, null, { invalidateTransport: true });
+      finish(error, null, { abandon: true });
     }, timeoutMs);
 
     try {
+      requestPending = true;
       sshClient.sftp((error, channel) => {
+        requestPending = false;
+        settleAbandonedOpen(sshClient, token);
         if (error) finish(error, channel || null);
         else finish(null, channel || null);
       });
     } catch (error) {
+      requestPending = false;
+      settleAbandonedOpen(sshClient, token);
       finish(error);
     }
   });

electron/bridges/boundedSftpOpen.test.cjs

@@ -46,7 +46,7 @@ test("bounded SFTP open times out and closes a late channel", async () => {
   assert.equal(timers.active.size, 1);
   assert.equal([...timers.active][0].hasRef(), true);
   await assert.rejects(result, (error) => error.code === "SFTP_CHANNEL_OPEN_TIMEOUT");
-  assert.equal(invalidations, 1);
+  assert.equal(invalidations, 0, "one channel request must not destroy the shared SSH transport");
   assert.equal(timers.active.size, 0);
 
   const channel = createChannel();
@@ -72,7 +72,7 @@ test("bounded SFTP open cancellation settles immediately and closes a late chann
   assert.equal([...timers.active][0].hasRef(), true);
   controller.abort(new Error("cancelled"));
   await assert.rejects(result, /cancelled/);
-  assert.equal(invalidations, 1);
+  assert.equal(invalidations, 0, "one channel request must not destroy the shared SSH transport");
   assert.equal(timers.active.size, 0);
   assert.equal(getEventListeners(controller.signal, "abort").length, 0);
 
@@ -82,28 +82,44 @@ test("bounded SFTP open cancellation settles immediately and closes a late chann
   assert.equal(timers.active.size, 0);
 });
 
-test("repeated unresponsive SFTP opens cannot retain channel callbacks", async () => {
-  const sshClient = {
-    destroyed: false,
-    pendingChannelCallbacks: [],
-    sftp(callback) {
-      if (this.destroyed) throw new Error("Not connected");
-      this.pendingChannelCallbacks.push(callback);
-    },
-    end() {},
-    destroy() {
-      this.destroyed = true;
-      this.pendingChannelCallbacks.length = 0;
-    },
-  };
+for (const reason of ["timeout", "cancel"]) {
+  test(`abandoned ${reason} opens cannot accumulate requests on a shared transport`, async () => {
+    const callbacks = [];
+    let physicalCloses = 0;
+    const sshClient = new EventEmitter();
+    sshClient.sftp = callback => { callbacks.push(callback); };
+    sshClient.end = sshClient.destroy = () => { physicalCloses++; };
+    const controller = new AbortController();
+    const opening = openBoundedSftpChannel(sshClient, { timeoutMs: 5, signal: controller.signal });
+    if (reason === "cancel") controller.abort(new Error("cancelled"));
+    await assert.rejects(opening);
+    for (let attempt = 0; attempt < 3; attempt++) {
+      await assert.rejects(openBoundedSftpChannel(sshClient, { timeoutMs: 5 }));
+    }
+    assert.equal(callbacks.length, 1, "retries must not allocate more abandoned SSH channel requests");
+    assert.equal(physicalCloses, 0, "existing terminal and SFTP channels remain connected");
 
-  for (let attempt = 0; attempt < 3; attempt += 1) {
-    await assert.rejects(
-      openBoundedSftpChannel(sshClient, { timeoutMs: 2 }),
-      /timed out|Not connected/,
-    );
-  }
-  assert.equal(sshClient.pendingChannelCallbacks.length, 0);
+    const late = createChannel();
+    callbacks[0](null, late);
+    assert.ok(late.endCalls > 0 || late.closeCalls > 0);
+    const next = openBoundedSftpChannel(sshClient, { timeoutMs: 50 });
+    assert.equal(callbacks.length, 2, "a settled abandoned request must release admission");
+    const healthy = createChannel();
+    callbacks[1](null, healthy);
+    assert.equal(await next, healthy);
+    assert.equal(physicalCloses, 0);
+  });
+}
+
+test("healthy SFTP channel openings remain parallel", async () => {
+  const callbacks = [];
+  const sshClient = { sftp(callback) { callbacks.push(callback); } };
+  const first = openBoundedSftpChannel(sshClient, { timeoutMs: 50 });
+  const second = openBoundedSftpChannel(sshClient, { timeoutMs: 50 });
+  assert.equal(callbacks.length, 2);
+  const channels = [createChannel(), createChannel()];
+  callbacks.forEach((callback, index) => callback(null, channels[index]));
+  assert.deepEqual(await Promise.all([first, second]), channels);
 });
 
 test("bounded SFTP open removes cancellation listeners after success and failure", async () => {
@@ -131,3 +147,37 @@ test("bounded SFTP open converts synchronous setup errors into rejections", asyn
   const sshClient = { sftp() { throw new Error("sync failure"); } };
   await assert.rejects(openBoundedSftpChannel(sshClient), /sync failure/);
 });
+
+test("transport closure releases abandoned-open bookkeeping without a late callback", async () => {
+  const sshClient = new EventEmitter();
+  const callbacks = [];
+  sshClient.sftp = callback => callbacks.push(callback);
+  await assert.rejects(openBoundedSftpChannel(sshClient, { timeoutMs: 5 }));
+  assert.equal(sshClient.listenerCount("close"), 1);
+  sshClient.emit("close");
+  assert.equal(sshClient.listenerCount("close"), 0);
+  const next = openBoundedSftpChannel(sshClient, { timeoutMs: 50 });
+  const channel = createChannel();
+  callbacks[1](null, channel);
+  assert.equal(await next, channel);
+  const late = createChannel();
+  callbacks[0](null, late);
+  assert.ok(late.endCalls > 0);
+});
+
+test("all abandoned parallel opens must settle before admitting a new one", async () => {
+  const sshClient = new EventEmitter();
+  const callbacks = [];
+  sshClient.sftp = callback => callbacks.push(callback);
+  const first = openBoundedSftpChannel(sshClient, { timeoutMs: 5 });
+  const second = openBoundedSftpChannel(sshClient, { timeoutMs: 5 });
+  await Promise.all([assert.rejects(first), assert.rejects(second)]);
+  assert.equal(sshClient.listenerCount("close"), 1);
+  callbacks[0](new Error("first failed"));
+  await assert.rejects(openBoundedSftpChannel(sshClient), error => error.code === "SFTP_CHANNEL_OPEN_PENDING");
+  callbacks[1](new Error("second failed"));
+  assert.equal(sshClient.listenerCount("close"), 0);
+  const next = openBoundedSftpChannel(sshClient);
+  callbacks[2](null, createChannel());
+  await next;
+});

electron/bridges/transferBridge.test.cjs

@@ -10532,6 +10532,8 @@ test("SFTP downloads cancelled while opening do not block the session", async (t
   let delayedOpen = null;
   let abandonedChannelClosed = false;
   let openCalls = 0;
+  let physicalEndCalls = 0;
+  let physicalDestroyCalls = 0;
   const abandonedSftp = createFastSftp({
     end() {
       abandonedChannelClosed = true;
@@ -10551,6 +10553,8 @@ test("SFTP downloads cancelled while opening do not block the session", async (t
       return Promise.resolve({ size: 10 });
     },
     client: {
+      end() { physicalEndCalls++; },
+      destroy() { physicalDestroyCalls++; },
       sftp(callback) {
         openCalls += 1;
         if (openCalls === 1) {
@@ -10588,6 +10592,8 @@ test("SFTP downloads cancelled while opening do not block the session", async (t
   const cancelled = await cancelledPromise;
   assert.equal(cancelled.error, "Transfer cancelled");
   assert.equal(abandonedChannelClosed, true);
+  assert.equal(physicalEndCalls, 0, "cancelling one download must preserve the shared SSH connection");
+  assert.equal(physicalDestroyCalls, 0, "terminal and browsing channels must not be destroyed");
 
   const next = await Promise.race([
     start("download-after-cancel"),