From 247c6a1e1a3e46815494f163e7f785da3000624a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Fern=C3=A1ndez?= <mfernandez@crunchloop.io>
Date: Thu, 13 Aug 2026 00:12:04 -0300
Subject: [PATCH] Channel: invoke _write callback with an error on non-open
 channels
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Channel._write() and ServerStderr._write() returned without invoking the
write callback when outgoing.state !== 'open'. When a channel is closed
while writes are parked awaiting window credit (a normal state for
port-forwarding proxies whose source finishes while the tunnel drains),
the WINDOW_ADJUST that arrives after close() re-enters _write() through
the flush path, hits the guard, and silently discards both the chunk and
its callback. The Writable then never finishes: 'finish' cannot fire and
all subsequently buffered writes wait forever, while the receiver sees a
clean EOF — silent truncation with no error on either side.

Invoke the callback with an error instead, so pending writes complete
observably and the stream can tear down. Deferring CHANNEL_CLOSE until
the writable has flushed (preserving the data instead of erroring it) is
a possible follow-up but changes close() semantics for callers that use
it as an abort, so it is left out of this change.

Fixes: https://github.com/mscdex/ssh2/issues/1508

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 lib/Channel.js                  |  8 +++-
 test/test-misc-client-server.js | 79 +++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/lib/Channel.js b/lib/Channel.js
index 0120779b..7a9368d5 100644
--- a/lib/Channel.js
+++ b/lib/Channel.js
@@ -47,8 +47,10 @@ class ServerStderr extends WritableStream {
     const len = data.length;
     let p = 0;
 
-    if (outgoing.state !== 'open')
+    if (outgoing.state !== 'open') {
+      cb(new Error('Channel is not open'));
       return;
+    }
 
     while (len - p > 0 && window > 0) {
       let sliceLen = len - p;
@@ -162,8 +164,10 @@ class Channel extends DuplexStream {
     const len = data.length;
     let p = 0;
 
-    if (outgoing.state !== 'open')
+    if (outgoing.state !== 'open') {
+      cb(new Error('Channel is not open'));
       return;
+    }
 
     while (len - p > 0 && window > 0) {
       let sliceLen = len - p;
diff --git a/test/test-misc-client-server.js b/test/test-misc-client-server.js
index 2dd5a29d..79b1ab5a 100644
--- a/test/test-misc-client-server.js
+++ b/test/test-misc-client-server.js
@@ -1458,3 +1458,82 @@ const setup = setupSimple.bind(undefined, debug);
     }));
   }));
 }
+
+{
+  // Regression for https://github.com/mscdex/ssh2/issues/1508: closing a
+  // channel while writes are parked awaiting window credit must not drop
+  // the pending write callbacks. Previously, a WINDOW_ADJUST arriving
+  // after close() re-entered _write() with outgoing.state === 'closing',
+  // which returned without invoking the callback — silently discarding the
+  // buffered data and leaving the Writable waiting forever ('finish' never
+  // emitted, every later write queued indefinitely).
+  const { client, server } = setup(
+    'Parked write callbacks are invoked when close() precedes WINDOW_ADJUST'
+  );
+
+  const assignedPort = 31337;
+  let resumeReceiver;
+
+  server.on('connection', mustCall((conn) => {
+    conn.on('ready', mustCall(() => {
+      conn.on('request', mustCall((accept, reject, name, info) => {
+        assert(name === 'tcpip-forward', 'Wrong request name');
+        accept(assignedPort);
+        conn.forwardOut(info.bindAddr,
+                        assignedPort,
+                        'remote',
+                        12345,
+                        mustCall((err, ch) => {
+          assert(!err, `Unexpected error: ${err}`);
+          // Apply backpressure so the sender exhausts its window and parks;
+          // resume (triggering WINDOW_ADJUST) only after the sender closed.
+          ch.pause();
+          resumeReceiver = () => {
+            ch.on('data', () => {});
+            ch.resume();
+          };
+        }));
+      }));
+    }));
+  }));
+
+  client.on('ready', mustCall(() => {
+    client.forwardIn('good', 0, mustCall((err, port) => {
+      assert(!err, `Unexpected error: ${err}`);
+      assert(port === assignedPort, 'Wrong assigned port');
+    }));
+  })).on('tcp connection', mustCall((details, accept, reject) => {
+    const ch = accept();
+    // The write callbacks may complete with an error (the channel closed
+    // before they could be sent); the requirement is that they complete.
+    ch.on('error', () => {});
+
+    // Queue chunks until both the window and the writable buffer are full,
+    // so a tail of writes is parked awaiting WINDOW_ADJUST. Every callback
+    // must eventually be invoked (success or error) — none silently dropped.
+    const chunk = Buffer.alloc(64 * 1024);
+    let queued = 0;
+    let completed = 0;
+    let ok = true;
+    const onWrite = () => {
+      if (++completed === queued)
+        client.end();
+    };
+    while (ok) {
+      ++queued;
+      ok = ch.write(chunk, mustCall(onWrite));
+    }
+
+    ch.end();
+    ch.close();
+    // The receiver resumes only now, so its WINDOW_ADJUST reaches this side
+    // after outgoing.state left 'open'. (The server's forwardOut callback
+    // may not have run yet; wait for it.)
+    const triggerResume = () => {
+      if (resumeReceiver === undefined)
+        return setImmediate(triggerResume);
+      resumeReceiver();
+    };
+    setImmediate(triggerResume);
+  }));
+}