node:http2: deliver the destroy() GOAWAY on sessions whose socket has no native handle

src/js/node/http2.ts

@@ -4930,14 +4930,33 @@ class ServerHttp2Session extends Http2Session {
       const socket = this[bunHTTP2Socket];
       if (!this.#connected) return;
       this.#closed = true;
+      if (socket && (!this[kGoawaySent] || code)) {
+        // close() already announced a graceful shutdown - re-sending NO_ERROR would be redundant
+        // and double-fires the peer's 'goaway' event. An error code is new information, though:
+        // a destroy(err) after close() must still put the error GOAWAY on the wire.
+        this.goaway(code || constants.NGHTTP2_NO_ERROR, 0, Buffer.alloc(0));
+      }
+      const parser = this.#parser;
+      if (parser) {
+        // node destroys every open stream synchronously from inside session.destroy(); the native
+        // dispatch below still runs for connection bookkeeping, but by then each stream is already
+        // ended + destroyed (a write() racing the destroy reports write-after-end without 'error').
+        // Like Node's Http2Stream._destroy: a received GOAWAY's code takes
+        // precedence over the destroy code when streams are torn down.
+        const streamRstCode = this[kGoawayCode] || code || constants.NGHTTP2_NO_ERROR;
+        parser.forEachStream(
+          FunctionPrototypeBind.$call(destroyStreamForSessionDestroy, undefined, error, streamRstCode),
+        );
+        parser.emitErrorToAllStreams(streamRstCode);
+        // The GOAWAY is still corked in the parser: detach() is what writes it out, and on a
+        // socket without a native handle that write goes through #Handlers.write, which drops
+        // it once the session is disconnected or the socket has been ended. So the socket is
+        // torn down after this, not before.
+        parser.detach();
+        this.#parser = null;
+      }
       this.#connected = false;
       if (socket) {
-        if (!this[kGoawaySent] || code) {
-          // close() already announced a graceful shutdown - re-sending NO_ERROR would be redundant
-          // and double-fires the peer's 'goaway' event. An error code is new information, though:
-          // a destroy(err) after close() must still put the error GOAWAY on the wire.
-          this.goaway(code || constants.NGHTTP2_NO_ERROR, 0, Buffer.alloc(0));
-        }
         if (error) {
           // node's finishSessionClose destroys the socket when the session dies
           // with an error (a misbehaving peer must observe the connection going
@@ -4957,21 +4976,6 @@ class ServerHttp2Session extends Http2Session {
           socket.end();
         }
       }
-      const parser = this.#parser;
-      if (parser) {
-        // node destroys every open stream synchronously from inside session.destroy(); the native
-        // dispatch below still runs for connection bookkeeping, but by then each stream is already
-        // ended + destroyed (a write() racing the destroy reports write-after-end without 'error').
-        // Like Node's Http2Stream._destroy: a received GOAWAY's code takes
-        // precedence over the destroy code when streams are torn down.
-        const streamRstCode = this[kGoawayCode] || code || constants.NGHTTP2_NO_ERROR;
-        parser.forEachStream(
-          FunctionPrototypeBind.$call(destroyStreamForSessionDestroy, undefined, error, streamRstCode),
-        );
-        parser.emitErrorToAllStreams(streamRstCode);
-        parser.detach();
-        this.#parser = null;
-      }
     } catch (e) {
       // A throwing destroy did not destroy: argument validation (goaway's
       // validateInteger, the native session rejecting a non-numeric error
@@ -6004,7 +6008,6 @@ class ClientHttp2Session extends Http2Session {
         this[kSessionDestroyError] = error;
       }
       this.#closed = true;
-      this.#connected = false;
       {
         // Requests still queued (waiting for connect or for a concurrency slot) never reached the
         // wire; node cancels them with ERR_HTTP2_STREAM_CANCEL (carrying the session error, if any,
@@ -6022,25 +6025,11 @@ class ClientHttp2Session extends Http2Session {
           }
         }
       }
-      if (socket) {
-        if (!this[kGoawaySent] || code) {
-          // close() already announced a graceful shutdown - re-sending NO_ERROR would be redundant
-          // and double-fires the peer's 'goaway' event. An error code is new information, though:
-          // a destroy(err) after close() must still put the error GOAWAY on the wire.
-          this.goaway(code || constants.NGHTTP2_NO_ERROR, 0, Buffer.alloc(0));
-        }
-        if (error) {
-          // See the client session: end first, destroy a tick later (node's
-          // finishSessionClose Windows-ECONNRESET avoidance).
-          endThenDestroySessionSocket(socket, error);
-        } else {
-          // See the client session's destroy: Node's finishSessionClose resumes
-          // the socket on a graceful close so unread inbound bytes cannot turn
-          // the FIN teardown into an RST.
-          // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L1188
-          socket.resume();
-          socket.end();
-        }
+      if (socket && (!this[kGoawaySent] || code)) {
+        // close() already announced a graceful shutdown - re-sending NO_ERROR would be redundant
+        // and double-fires the peer's 'goaway' event. An error code is new information, though:
+        // a destroy(err) after close() must still put the error GOAWAY on the wire.
+        this.goaway(code || constants.NGHTTP2_NO_ERROR, 0, Buffer.alloc(0));
       }
       const parser = this.#parser;
       if (parser) {
@@ -6058,8 +6047,26 @@ class ClientHttp2Session extends Http2Session {
         } finally {
           this[bunHTTP2SessionTeardownFrame] = kNoSessionTeardown;
         }
+        // Same as the server session's destroy: detach() writes out the corked GOAWAY, and
+        // #Handlers.write only passes it on while the session is connected and the socket is
+        // not ended, so the socket is torn down after this.
         parser.detach();
       }
+      this.#connected = false;
+      if (socket) {
+        if (error) {
+          // See the server session: end first, destroy a tick later (node's
+          // finishSessionClose Windows-ECONNRESET avoidance).
+          endThenDestroySessionSocket(socket, error);
+        } else {
+          // See the server session's destroy: Node's finishSessionClose resumes
+          // the socket on a graceful close so unread inbound bytes cannot turn
+          // the FIN teardown into an RST.
+          // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L1188
+          socket.resume();
+          socket.end();
+        }
+      }
     } catch (e) {
       // A throwing destroy did not destroy: argument validation (goaway's
       // validateInteger, the native session rejecting a non-numeric error

test/js/node/http2/node-http2-upgrade.test.mts

@@ -461,6 +461,92 @@ describe("HTTP/2 upgrade — server TLS options", () => {
   });
 });
 
+describe("HTTP/2 upgrade — frames written while the session is destroyed reach the peer", () => {
+  const SETTINGS = 4;
+  const GOAWAY = 7;
+  const { NGHTTP2_NO_ERROR, NGHTTP2_INTERNAL_ERROR } = http2.constants;
+  type Frame = { type: number; errorCode?: number };
+
+  // Splits the bytes a peer read before the server hung up into frames. The peer never sends
+  // its own preface, so everything here was written by the server session on its own: the
+  // connection preface SETTINGS and then whatever destroy() put on the wire.
+  function parseFrames(buf: Buffer): Frame[] {
+    const frames: Frame[] = [];
+    let off = 0;
+    while (off + 9 <= buf.length) {
+      const length = buf.readUIntBE(off, 3);
+      const frame: Frame = { type: buf[off + 3] };
+      // GOAWAY payload: last stream id (4 bytes), error code (4 bytes), debug data.
+      if (frame.type === GOAWAY) frame.errorCode = buf.readUInt32BE(off + 9 + 4);
+      frames.push(frame);
+      off += 9 + length;
+    }
+    assert.strictEqual(off, buf.length, "peer received a partial frame");
+    return frames;
+  }
+
+  // Injects one raw TCP connection into an Http2SecureServer, connects an h2 peer to it, and
+  // returns the frames the peer read until the server closed the connection. `teardown` runs
+  // on the server session: right away from the 'session' event when `when` is "in-session-event",
+  // otherwise once the peer has read the server's preface (so the SETTINGS frame is already out
+  // and the teardown's own frames are the only thing left to deliver).
+  async function framesReceivedAfterTeardown(
+    when: "in-session-event" | "after-preface",
+    teardown: (session: http2.ServerHttp2Session) => void,
+  ): Promise<Frame[]> {
+    const h2Server = http2.createSecureServer(TLS);
+    h2Server.on("sessionError", () => {});
+    const sessionReady = Promise.withResolvers<http2.ServerHttp2Session>();
+    h2Server.on("session", session => {
+      session.on("error", () => {});
+      if (when === "in-session-event") teardown(session);
+      sessionReady.resolve(session);
+    });
+    const netServer = net.createServer(socket => {
+      socket.on("error", () => {});
+      h2Server.emit("connection", socket);
+    });
+    const port = await new Promise<number>(resolve => {
+      netServer.listen(0, "127.0.0.1", () => resolve((netServer.address() as net.AddressInfo).port));
+    });
+    try {
+      const peer = tls.connect({ host: "127.0.0.1", port, rejectUnauthorized: false, ALPNProtocols: ["h2"] });
+      peer.on("error", () => {});
+      const chunks: Buffer[] = [];
+      const firstChunk = Promise.withResolvers<void>();
+      const closed = Promise.withResolvers<void>();
+      peer.on("data", (chunk: Buffer) => {
+        chunks.push(chunk);
+        firstChunk.resolve();
+      });
+      peer.on("close", () => closed.resolve());
+      if (when === "after-preface") {
+        const [session] = await Promise.all([sessionReady.promise, firstChunk.promise]);
+        teardown(session);
+      }
+      await closed.promise;
+      return parseFrames(Buffer.concat(chunks));
+    } finally {
+      netServer.close();
+    }
+  }
+
+  test("session.destroy(err) from the 'session' event: the preface SETTINGS and the GOAWAY both arrive", async () => {
+    const frames = await framesReceivedAfterTeardown("in-session-event", session => session.destroy(new Error("boom")));
+    assert.deepStrictEqual(frames, [{ type: SETTINGS }, { type: GOAWAY, errorCode: NGHTTP2_INTERNAL_ERROR }]);
+  });
+
+  test("session.destroy(err) on an established session: the GOAWAY arrives before the connection closes", async () => {
+    const frames = await framesReceivedAfterTeardown("after-preface", session => session.destroy(new Error("boom")));
+    assert.deepStrictEqual(frames, [{ type: SETTINGS }, { type: GOAWAY, errorCode: NGHTTP2_INTERNAL_ERROR }]);
+  });
+
+  test("session.destroy() without an error: the NO_ERROR GOAWAY arrives before the connection closes", async () => {
+    const frames = await framesReceivedAfterTeardown("after-preface", session => session.destroy());
+    assert.deepStrictEqual(frames, [{ type: SETTINGS }, { type: GOAWAY, errorCode: NGHTTP2_NO_ERROR }]);
+  });
+});
+
 if (typeof Bun !== "undefined") {
   describe("Node.js compatibility", () => {
     test("tests should run on node.js", async () => {

test/js/node/http2/node-http2.test.js

@@ -4996,3 +4996,120 @@ describe("frames issued from inside a user-supplied Duplex transport's _write",
     },
   );
 });
+
+describe("session.destroy() over a transport without a native handle", () => {
+  // A session whose socket has no native handle (a createConnection Duplex, a Duplex injected with
+  // server.emit('connection'), the TLS proxy behind Http2SecureServer#emit('connection')) writes
+  // through the session's JS write handler. destroy() corks a GOAWAY that the parser's detach()
+  // writes out, and the handler used to refuse that write because destroy() had already marked the
+  // session disconnected and ended the socket: the GOAWAY (and, when destroy() ran in the tick that
+  // wrote the preface, the SETTINGS frame as well) never reached the peer.
+  const PREFACE = Buffer.from("PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n");
+  const SETTINGS = 4;
+  const GOAWAY = 7;
+  const { NGHTTP2_NO_ERROR, NGHTTP2_INTERNAL_ERROR } = http2.constants;
+
+  function parseFrames(buf, { clientPreface }) {
+    let off = 0;
+    if (clientPreface) {
+      expect(buf.subarray(0, PREFACE.length).equals(PREFACE)).toBe(true);
+      off = PREFACE.length;
+    }
+    const frames = [];
+    while (off + 9 <= buf.length) {
+      const length = buf.readUIntBE(off, 3);
+      const frame = { type: buf[off + 3] };
+      // GOAWAY payload: last stream id (4 bytes), error code (4 bytes), debug data.
+      if (frame.type === GOAWAY) frame.errorCode = buf.readUInt32BE(off + 9 + 4);
+      frames.push(frame);
+      off += 9 + length;
+    }
+    expect(off).toBe(buf.length);
+    return frames;
+  }
+
+  const cases = [
+    ["destroy(err)", () => new Error("boom"), NGHTTP2_INTERNAL_ERROR],
+    ["destroy()", () => undefined, NGHTTP2_NO_ERROR],
+  ];
+
+  describe("server session on a Duplex injected with server.emit('connection')", () => {
+    async function framesReadByPeer(when, destroyArg) {
+      const [peerSide, serverSide] = duplexPair();
+      const server = http2.createServer();
+      server.on("sessionError", () => {});
+      const sessionReady = Promise.withResolvers();
+      server.on("session", session => {
+        session.on("error", () => {});
+        if (when === "in-session-event") session.destroy(destroyArg());
+        sessionReady.resolve(session);
+      });
+
+      const chunks = [];
+      const firstChunk = Promise.withResolvers();
+      const ended = Promise.withResolvers();
+      peerSide.on("data", chunk => {
+        chunks.push(chunk);
+        firstChunk.resolve();
+      });
+      peerSide.on("end", ended.resolve);
+      peerSide.on("close", ended.resolve);
+
+      server.emit("connection", serverSide);
+      if (when === "after-preface") {
+        const [session] = await Promise.all([sessionReady.promise, firstChunk.promise]);
+        session.destroy(destroyArg());
+      }
+      await ended.promise;
+      return parseFrames(Buffer.concat(chunks), { clientPreface: false });
+    }
+
+    it.each(cases)("%s from the 'session' event delivers the preface SETTINGS and the GOAWAY", async (_, arg, code) => {
+      expect(await framesReadByPeer("in-session-event", arg)).toEqual([
+        { type: SETTINGS },
+        { type: GOAWAY, errorCode: code },
+      ]);
+    });
+
+    it.each(cases)(
+      "%s on an established session delivers the GOAWAY before ending the socket",
+      async (_, arg, code) => {
+        expect(await framesReadByPeer("after-preface", arg)).toEqual([
+          { type: SETTINGS },
+          { type: GOAWAY, errorCode: code },
+        ]);
+      },
+    );
+  });
+
+  describe("client session on a createConnection Duplex", () => {
+    it.each(cases)("%s writes the GOAWAY to the transport before ending it", async (_, arg, code) => {
+      const chunks = [];
+      const ended = Promise.withResolvers();
+      const transport = new Duplex({
+        read() {},
+        write(chunk, _encoding, callback) {
+          chunks.push(Buffer.from(chunk));
+          callback();
+        },
+        final(callback) {
+          ended.resolve();
+          callback();
+        },
+      });
+      const client = http2.connect("http://localhost", { createConnection: () => transport });
+      client.on("error", () => {});
+      await new Promise(resolve => client.once("connect", resolve));
+
+      client.destroy(arg());
+
+      // A Writable hands nothing to _write once it has been ended, so after destroy() ended the
+      // transport, `chunks` is everything the peer would ever have read.
+      await ended.promise;
+      expect(parseFrames(Buffer.concat(chunks), { clientPreface: true })).toEqual([
+        { type: SETTINGS },
+        { type: GOAWAY, errorCode: code },
+      ]);
+    });
+  });
+});