[robobun]

Problem

• A node:http2 session over tls.connect({ socket: duplex }) corrupts its byte stream when the Duplex's _write issues a frame (ping(), settings(), request()). The peer gets the nested frame inside the outer frame's payload, or the connection preface twice. Node v26.3.0 sends every frame once and whole.
• Cause: the native write paths in src/runtime/api/bun/h2_frame_parser.rs assume that a socket write runs no JS. This socket runs _write inside the write. write() (:3190) flushes the cork mid-frame. generic_flush and generic_write (:2513, :2551) keep the pending bytes until the write returns, so a nested flush sends them again.

Fix

• write() sends every transport whose write runs JS (transport_write_runs_js()) through write_to_js_transport (from node:http2: never split a frame around a JS transport's write callback #36918). It writes only at a frame boundary outside a header block.
• For these transports, generic_flush and generic_write take the pending bytes out of write_buffer before the socket write. A nested write finds nothing pending and lands behind them, because SSL_write consumes the whole buffer before _write runs. Other sockets keep the old paths.
• Verified: 15 new cases in test/js/node/http2/node-http2.test.js, 12 fail on bun 1.4.3-canary.1. Also test/js/node/http2/ and the upstream test-http2-*.js tests.
• Self-reviewed: 8 concerns raised, 6 addressed, 2 explained in Notes.

Background

• H2FrameParser is the native engine behind an Http2Session. write() collects its frames in a thread-local 16 KiB cork.
• write_buffer holds bytes that the socket did not take yet. A client fills it before the connect and flushes it on connect.
• tls.connect({ socket: duplex }) makes a native TLS socket (uSockets UpgradedDuplex). It passes each TLS record to duplex.write(), which runs _write synchronously.

Notes

Repro (needs a key.pem and a cert.pem next to it, run with bun and with node):

import fs from "node:fs";
import http2 from "node:http2";
import net from "node:net";
import { Duplex } from "node:stream";
import tls from "node:tls";
const key = fs.readFileSync(new URL("./key.pem", import.meta.url));
const cert = fs.readFileSync(new URL("./cert.pem", import.meta.url));
const server = http2.createSecureServer({ key, cert });
const result = Promise.withResolvers();
server.on("sessionError", err => result.resolve({ sessionError: String(err) }));
server.on("stream", stream => {
  const chunks = [];
  stream.on("data", c => chunks.push(c));
  stream.on("end", () => {
    const body = Buffer.concat(chunks);
    result.resolve({ bodyLength: body.length, bodyIntact: body.equals(Buffer.alloc(16374, 0x41)) });
    stream.respond({ ":status": 200 });
    stream.end();
  });
});
await new Promise(resolve => server.listen(0, "127.0.0.1", resolve));
const raw = net.connect(server.address().port, "127.0.0.1");
await new Promise(resolve => raw.once("connect", resolve));
let armed = false, issued = false, session;
const transport = new Duplex({
  read() {},
  write(chunk, enc, cb) {
    if (armed && !issued) {
      issued = true;
      session.ping(Buffer.from("NESTPING"), () => {});
    }
    raw.write(chunk, cb);
  },
});
transport.on("error", () => {});
raw.on("data", d => transport.push(d));
raw.on("error", () => {});
const tlsSocket = tls.connect({ socket: transport, ALPNProtocols: ["h2"], rejectUnauthorized: false });
session = http2.connect("https://localhost", { createConnection: () => tlsSocket });
session.on("error", err => result.resolve({ clientSessionError: String(err) }));
await new Promise(resolve => session.once("connect", resolve));
const pad = Buffer.alloc(15000, 0x70).toString(); // about 11 KiB of HPACK, stays corked
const req = session.request({ ":method": "POST", ":path": "/", "x-pad": pad }, { endStream: false });
req.on("error", () => {});
armed = true;
req.end(Buffer.alloc(16374, 0x41)); // the DATA frame overflows the cork behind the HEADERS
console.log(JSON.stringify({ issued, ...(await result.promise) }));
process.exit(0);

• node v26.3.0: {"issued":true,"bodyLength":16374,"bodyIntact":true}
• bun 1.4.3-canary.1+367d939d9: {"issued":true,"bodyLength":16374,"bodyIntact":false}
• this branch: {"issued":true,"bodyLength":16374,"bodyIntact":true}

How the splice happens. The ~11 KiB HEADERS frame is in the cork. write() receives the DATA frame header, then the 16374-byte payload. The payload does not fit, so write() copies its first bytes up to the 16 KiB boundary and calls flush_cork_buffer(). That goes through generic_write to the TLS socket. SSL_write seals the record and the socket passes it to duplex.write(), which runs the user's _write before it returns. session.ping() there enters write() again. The cork is empty, so the PING frame goes to offset 0. The outer call then appends the rest of the DATA payload behind the PING frame.

The second face: the connect-time flush. A client serializes its connection preface and SETTINGS before the socket connects, and they wait in write_buffer. On secureConnect the session attaches the native socket and calls flush(), which hands write_buffer to the socket in generic_flush. That write runs the Duplex's _write. A session.settings() there flushes at once, and a request with a header block larger than the cork goes straight to generic_write. Both found the same bytes still pending and sent them again. Measured with a plain tls.createServer peer that records what it decrypts, and a _write that is armed from a secureConnect listener registered before http2.connect():

• node v26.3.0, nested settings(): one preface, SETTINGS:0 SETTINGS:6
• bun 1.4.3-canary.1+367d939d9, nested settings(): two prefaces, the stream no longer parses
• bun 1.4.3-canary.1+367d939d9, nested request with a 22 KiB header block: two prefaces
• this branch: one preface in both cases, and the same frame sequence as node

The same change also removes a borrow hazard: generic_flush and generic_write passed a slice of write_buffer (a JsCell<Vec<u8>>) to a socket write that re-enters code which appends to that Vec. The taken-out Vec is local to the call.

More probes. An end-to-end matrix against a real http2.createSecureServer peer: nested ping(), request() and settings(), outer DATA write and outer HEADERS+CONTINUATION write, and a Duplex that calls cb() synchronously or from raw.write(chunk, cb). That is 12 combinations. On bun 1.4.3-canary.1 the body arrives corrupt in the six DATA combinations, and the server reports ERR_HTTP2_ERROR: Protocol error in the six CONTINUATION combinations. The nested ping is cancelled, the nested request fails with New streams cannot be created after receiving a GOAWAY, and the nested settings never get an ACK. On this branch all 12 match node: the body is intact, the ping and the settings get their ACK, and the nested request gets its response.

A second probe runs an https HTTP/2 session through an h2c CONNECT tunnel (tls.connect({ socket: outerStream })), with request bodies from 1 byte to 300000 bytes. This is the common real use of this socket kind. The bodies echo back intact on node, on bun 1.4.3-canary.1 and on this branch.

The new tests. The existing describe frames issued from inside a user-supplied Duplex transport's _write covered a session whose createConnection returns the Duplex. The new cases put the same Duplex under tls.connect({ socket }). A plain tls.createServer peer decrypts and records the bytes, and the same frame parser runs on them. There are 15 cases: nested ping(), settings(), goaway(), request() and a request with a multi-frame body, each issued during the outer DATA write, during the outer HEADERS+CONTINUATION write, and during the connect-time flush. The first two groups share the assertions of the existing matrix: every byte belongs to one complete frame, header blocks are contiguous, the DATA frame carries exactly the body, and the nested frames come after the unit that they were issued from. The connect group asserts one connection preface, complete frames, and one copy of each nested frame. For that group the Duplex is armed for one tick only, from a secureConnect listener registered before the session's own, so the case fails if the nested call does not run inside the flush.

On bun 1.4.3-canary.1, 12 of the 15 fail. The other 3 (ping(), goaway() and the multi-frame body during the connect-time flush) pass on both builds: the small frames stay in the cork, and the DATA frames are queued because the session sees the pending bytes as backpressure.

The session sends a PING with a known payload as its last frame. The test sends it after the body's write callback, when every other frame is serialized, and waits until the server has that payload. It does not wait for time. Nothing is held back before it, because this transport reports no backpressure and the bodies fit the default flow-control windows. The waits use events.once, so a failed connect reports its error. The request code and the assertions moved into helpers that both matrices share. The 20 existing cases are unchanged in behavior.

Node guards the same layer. Http2Session::SendPendingData (src/node_http2.cc:1951 at v26.3.0) collects whole frames from nghttp2 and returns early when a send is in progress ("SendPendingData should not be called recursively"). A frame issued from the transport's _write goes out with the next send.

Why the TLS layer needs no change. SSL_write is not re-entered. The Duplex's write() runs from handle_traffic, after SSL_write returned. A nested socket write calls SSL_write again, which appends its record to the same FIFO BIO, so ciphertext order equals the order of the SSL_write calls. A handle_traffic that is re-entered only drains the BIO and asks the outer pass to run again.

Self-review. Two independent read-only reviews of the diff, one of the Rust change and one of the tests. Eight concerns. Addressed:

1. generic_flush and generic_write could send write_buffer twice on re-entry, and every TLS-over-Duplex client passes through that window at connect. This became the second half of the fix, with its own cases.
2. The comments on flush_cork_buffer and on the second self.cork() in write() named JS-backed sockets as the reason for re-entry. No such transport reaches that loop now. A socket that closes inside a write still runs JS there, and the comments say that.
3. The new test waits had no failure path, so a failed connect was a bare timeout. They use events.once now, and the end-of-stream wait rejects when the session closes.
4. The teardown left the TLS socket and the Duplex open. It destroys them now.
5. The comment on the last PING did not say why nothing can follow it. It states the two conditions now.
6. Nothing proved that the connect-time nested call runs inside the flush. The one-tick arming does.

Explained:

7. The Tcp arms of transport_write_runs_js() cannot match today, because only a TLS socket is ever built over a Duplex. They are existing code and they are harmless.
8. No server-side case. A server session over a TLS socket that wraps a Duplex takes the same write(), generic_flush and generic_write, so the client cases cover the changed code.

Left alone. A different bug exists in cork(): when a session takes the cork slot from another session, and that session's transport _write writes a frame to the first session, the frame is lost. It also happens with plain Duplex transports and without TLS, so it is not part of this PR. It is tracked in #43538.

Suites run with the debug build. With the final code: test/js/node/http2/node-http2.test.js (408 tests: 402 pass, 6 skip), the 35 nested-frame cases 6 more times (35 of 35 each time), the other ten test files in test/js/node/http2/, and the 40 upstream test-http2-*.js tests that use TLS, createConnection or a Duplex (all exit 0). With the first commit only: all 261 upstream test/js/node/test/{parallel,sequential}/test-http2-*.js tests (all exit 0). h2-conformance.test.ts passes as a whole file (3 of 4 runs, one run had one GC-count case over its limit). Its describe stream release after a queued END_STREAM, run alone with -t, fails one case with 7 live streams on the debug build. A debug build of main fails the same case the same way (2 of 3 runs), and these are plain h2c sessions that the change does not reach.

---

[human-review] gate passed · iteration 1 · 2 files touched

fails on main (without fix)

ASAN without fix: 12 failed, 6 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/node/http2/node-http2.test.js"
bun test v1.4.3 (367d939d9)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [936.25ms]
(pass) node none > Client Basics > should be able to send a POST request [672.41ms]
(pass) node none > Client Basics > constants [25.90ms]
(pass) node none > Client Basics > getDefaultSettings [10.67ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [25.50ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [9.78ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [5.18ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [7.65ms]
(pass) node none > Client Basics > should be able to send data using end [737.12ms]
(pass) node none > Client Basics > should be able to mutiplex GET requests [737.63ms]
(pass) node none > Client Basics > http2 should receive remoteSettings when receiving 
... (truncated)

release without fix: 6 skipped
bun test v1.4.3-canary.1 (4d1ff5086)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > constants [2.79ms]
(pass) node none > Client Basics > getDefaultSettings [0.29ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [0.43ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [0.22ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [0.06ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [0.10ms]
(pass) node none > Client Basics > is possible to abort request [2.34ms]
(pass) node none > Client Basics > aborted event should work with abortController [1.11ms]
(pass) node none > Client Basics > aborted event should work with aborted signal [0.89ms]
(pass) node none > Client Basics > signal validation matches node: non-signal objects throw, duck-typed { aborted } is accepted [2.71ms]
(pass) node none > Client Basics > headers cannot be bigger than 65536 bytes [61.29ms]
(skip) node none > Client Basics > should not leak memory
(pass) node none > Client Basics > should fail to con
... (truncated)

passes on PR (with fix)

ASAN with fix: 6 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/node/http2/node-http2.test.js"
bun test v1.4.3 (367d939d9)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [1005.10ms]
(pass) node none > Client Basics > should be able to send a POST request [714.83ms]
(pass) node none > Client Basics > constants [20.94ms]
(pass) node none > Client Basics > getDefaultSettings [7.09ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [20.27ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [6.34ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.55ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [5.52ms]
(pass) node none > Client Basics > should be able to send data using end [727.14ms]
(pass) node none > Client Basics > should be able to mutiplex GET requests [699.62ms]
(pass) node none > Client Basics > http2 should receive remoteSettings when receiving 
... (truncated)

release with fix: 6 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 1012ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/128] gen ErrorCode+*.h
[2/128] gen generated_host_exports.rs
generated_host_exports.rs: 121 exports (host=5, lazy=10, generic=106, rust=0); 245 extern-C blocks audited
[3/128] gen cpp.rs (cppbind)
[4/128] gen JS modules (bundle-modules)
Preprocess modules (13637ms)
Bundle modules (91ms)
Postprocesss modules (157ms)
Bundle Functions (638ms)
Generate Code (37ms)

[14.57s] Bundled "src/js" for production
  2607 kb
  197 internal modules
  13 native modules
  50 internal functions across 16 files
[4/12] cargo bun_runtime → libbun_runtime.a
�[1m�[33mwarning�[0m�[1m: binary `bun_shim_impl` should have a kebab-case name�[0m
   �[1m�[94m|�[0m
�[1m�[94m 1�[0m �[1m�[94m|�[0m /workspace/bun/build/release/rust-target/.../bun_shim_impl
   �[1m�[94m|�[0m                                              �[1m�[33m^^^^^^^^^^^^^�[0m
   �[1m�[94m|�[0m
   �[1m�[94m= �[0m�[1mnote�[0m: `cargo::non_kebab_case_bins` is set to `warn` by default
�[1m�[96mhelp�[0m: to change the binary name to `bun-shim-impl`, convert `bin.name`
 
... (truncated)

diff hotspot

src/runtime/api/bun/h2_frame_parser.rs |  50 +++++++-
 test/js/node/http2/node-http2.test.js  | 204 +++++++++++++++++++++++++--------
 2 files changed, 204 insertions(+), 50 deletions(-)

gate history · 2 passed · 0 rejected · iteration 1

evidence per changed file

file                                    reads  edits  tests
src/runtime/api/bun/h2_frame_parser.rs     12      7     33
test/js/node/http2/node-http2.test.js       8      9     33

[coderabbitai]

Warning

Review paused — included plan limit reached

Keep your review moving with free on-demand reviews.

• Run this review for free

On-demand reviews are free for one more day.

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Promotion and pricing details

On-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file.

Review limit details

Or wait 8 minutes for your next included review.

Check out review usage here.

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: d58aefeb-30f9-44ca-ae98-60ce6b6eab4c

📥 Commits

Reviewing files that changed from the base of the PR and between 9b7c982 and b66be9f.

📒 Files selected for processing (2)

• src/runtime/api/bun/h2_frame_parser.rs
• test/js/node/http2/node-http2.test.js

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status

Reproduced on bun 1.4.3-canary.1+367d939d9 with the two scripts in the PR notes (a node:http2 client over tls.connect({ socket: duplex }) whose _write issues a frame):

• A session.ping() from _write during a 16374-byte body write: the server receives a corrupt body (bodyIntact:false). Node v26.3.0 and this branch deliver it intact.
• A session.settings() from _write during the connect-time flush: the peer receives the connection preface twice. Node v26.3.0 and this branch send it once.

The 15 new cases in test/js/node/http2/node-http2.test.js cover both. 12 of them fail on that bun build, and all 15 pass with the debug build of this branch.

CI

The diff is green. No test that this PR touches failed in either build, and the robobun/evidence check passes (the test file fails without the fix and passes with it, on the ASAN and the release build).

• #118452: 180 of 181 jobs passed. The one red job is darwin any x64 - test-bun, where test/js/node/net/server.spec.ts failed with listen EADDRINUSE: address already in use 127.0.0.1:1234 (a fixed port in a node:net test). I reported it as a failure on main.
• #118576 (current head): 179 of 181 jobs passed. The two darwin any x64 - test-bun jobs expired without an agent and are scheduled again. Every test failure in that build passed on its retry, and none of them is an HTTP/2 test (no-orphans, bun-lock, in-process-cron, bun-install-registry, serve-stream-body-error).

All review threads are resolved. This PR needs a maintainer to review and merge it.

PR: #43529

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I also checked the partial-write path in write_pending_through_js (the unwritten remainder is restored ahead of anything a re-entrant write buffered, and the offset is rebased to offset + written, matching the existing generic_flush accounting) and the new usize::try_from(result).expect("int cast") (guarded by the result < 0 branch, same shape as the surrounding arms) — neither introduces a new problem.

Extended reasoning...

The inline finding is pre-existing and concerns cork slot contention across sessions, which this change does not alter. Of the rest of the Rust diff, the two spots most likely to hide a bug were the partial-write reorder in write_pending_through_js (src/runtime/api/bun/h2_frame_parser.rs) and the expect cast; both were read against the existing generic_flush/generic_write arms and behave consistently with them. The transport_write_runs_js predicate widening to UpgradedDuplex TLS/TCP sockets is the intended scope change and the test matrix in test/js/node/http2/node-http2.test.js exercises it over a real tls.connect({ socket }) peer. The change is still a re-entrancy fix in a hot native path, so a human look at the broader design (per the ruled-out candidates on stall and re-entrant flush) remains worthwhile.

[robobun]

^{Updated 4:42 PM PT - Sep 19th, 2026}

✅ @robobun, your commit b66be9f9fcfb6db726739d1d346d24aca916e1a9 passed in Build #118576! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 43529

That installs a local version of the PR into your bun-43529 executable, so you can run:

bun-43529 --bun

[claude]

I re-reviewed the current head of this PR and found no bugs; since it changes re-entrancy handling in the native h2 write path, a human look at write_pending_through_js and the write() routing change is still worthwhile.

What was reviewed:

• write_pending_through_js in src/runtime/api/bun/h2_frame_parser.rs: the pending Vec is moved out before the socket write, so no JsCell borrow is live across the JS re-entry; both callers (_write, flush) hold keepalive(); the short-write re-queue puts the remainder ahead of re-entrant bytes, and a short write cannot actually occur here because SSL_write targets an unbounded memory BIO (no partial-write mode) and raw_write reports the full length.
• Ordering of nested frames: a nested _write finds write_buffer empty and calls SSL_write directly; the re-entered handle_traffic drains the write BIO FIFO before deferring, so nested records land behind the outer unit's records.
• Test refactor: the non-TLS matrix keeps its previous (un-awaited) req.write behavior; the TLS matrix uses port 0, a local tls.createServer, awaits a sentinel PING rather than sleeping, and rejects on session close.

Extended reasoning...

Overview

The PR touches two files: src/runtime/api/bun/h2_frame_parser.rs (+50 lines) and test/js/node/http2/node-http2.test.js (+200/-50). The native change adds write_pending_through_js, which takes write_buffer/write_buffer_offset out of self before calling socket.write_maybe_corked, and routes generic_flush/generic_write through it when transport_write_runs_js() is true. write() switches its early guard from native_socket == None to transport_write_runs_js(), so a TLSSocket over a user Duplex now goes through the frame-boundary-aware write_to_js_transport path introduced in an earlier PR. Comments on the cork paths were reworded to describe the narrower remaining case (a close inside the write). The test refactor extracts sendOuterRequest and expectNestedAfterOuterUnit from the existing Duplex matrix and adds a 15-case TLS matrix (5 nested kinds x data/continuation/connect).

Security risks

None specific to this change: no new parsing of peer data, no auth/crypto configuration changes. The write path handles bytes the session itself serialized. The usize::try_from(result).expect(...) is guarded by the result < 0 branch, so it cannot panic. The only adversarial surface is user JS re-entering during _write, which is exactly what the change hardens.

Level of scrutiny

High. This is re-entrancy logic in native code where a JS callback runs under a socket write. I traced: (1) JsCell::take/replace leave no live borrow across the JS call; (2) both callers of generic_flush/generic_write hold _keepalive, so self.global() after the write is safe; (3) the re-queue on short write puts the unsent remainder in front of re-entrant bytes, which would reorder against re-entrant bytes already sent directly, but SSLWrapper::write_data calls SSL_write against a memory BIO with no SSL_MODE_ENABLE_PARTIAL_WRITE and UpgradedDuplex::raw_write returns the full length, so short writes reduce to 0 (nothing sent, nothing to reorder); (4) nested SSL_write inside the outer handle_traffic appends to the FIFO write BIO and the re-entered handle_traffic drains it in order before deferring, so nested frames land after the outer unit's records; (5) tx_tracker is now advanced for these sessions, consistent with the flush() boundary check. I could not run the tests in this environment (no debug build present), so test pass/fail claims rest on the PR author's description.

Other factors

My prior run's inline note about cork() dropping re-entrantly corked bytes when force-uncorking another parser is pre-existing, unchanged by this PR, and was replied to and self-resolved by the author; it is not blocking. Since that review, only a comment-wording commit and a CI retrigger landed. The tests are hermetic (local TLS server, port 0), await observable conditions (sentinel PING, write callback, secureConnect) rather than sleeping, and reject on session close. The existing matrix's behavior is preserved by the refactor. Given the subtlety of the write-path interaction with uSockets' TLS wrapper, a human maintainer familiar with the h2 cork/write machinery should still sign off.

[Jarred-Sumner]

Superseded by #44618, which consolidates the open TLS pull requests. This fix and its tests are in there, either as written, rewritten smaller, or merged with the other PRs that patched the same cause (see the "By area" list in that PR). Closing in favor of it.