node:http2: never split a frame around a JS transport's write callback

[robobun]

Repro

http2.connect() over a user-supplied Duplex (createConnection), where the transport issues a frame of its own from inside its _write (a keepalive/metrics hook in a transport wrapper):

import http2 from "node:http2";
import { Duplex } from "node:stream";
const chunks = []; let armed = false, fired = 0, sess;
const d = new Duplex({
  read() {},
  write(chunk, enc, cb) {
    chunks.push(Buffer.from(chunk));
    if (armed && !fired++) sess.ping(Buffer.from("NESTPING"), () => {});
    cb();
  },
});
sess = http2.connect("http://localhost:1", { createConnection: () => d }); sess.on("error", () => {});
await new Promise(r => sess.once("connect", r)); await new Promise(r => setTimeout(r, 20));
chunks.length = 0;
const req = sess.request({ ":method": "POST", ":path": "/", "x-pad": "p".repeat(15000) }, { endStream: false });
req.on("error", () => {});
armed = true;
req.write(Buffer.alloc(16374, 0x41));
await new Promise(r => setTimeout(r, 100));
const all = Buffer.concat(chunks), fr = [];
for (let i = 0; i + 9 <= all.length; ) { const len = all.readUIntBE(i, 3); fr.push({ t: all[i + 3], len, p: all.subarray(i + 9, i + 9 + len) }); i += 9 + len; }
const data = fr.find(f => f.t === 0), k = data.p.findIndex(b => b !== 0x41);
console.log({ framesAsPeerParsesThem: fr.map(f => `type${f.t}:len${f.len}`).join(","), firstForeignByteInData: k });

Before: { framesAsPeerParsesThem: "type1:len11272,type0:len16374,type65:len4276545", firstForeignByteInData: 5094 }. The PING frame (00 00 08 06 00 00 00 00 00 NESTPING) sits inside the request body at offset 5094, the real PING never reaches the peer, and the body's last 17 bytes are parsed by the peer as a frame header type 0x41 len 4,276,545 (FRAME_SIZE_ERROR / connection desync). Same with settings(), goaway() and request() as the nested call (a complete HEADERS frame for stream 3 inside stream 1's body).

After: type1:len11272,type0:len16374,type6:len8, body intact (matches node).

Cause

With no native socket, the session's bytes reach the wire through the onWrite handler, which calls socket.write() and so runs the transport's _write synchronously. H2FrameParser::write filled the 16 KiB cork to the record boundary with the first avail bytes of the DATA payload, flushed the cork through that handler, and corked the payload's remainder afterwards. A frame serialized from inside _write found the cork empty and was appended first, ahead of the remainder.

Two more faces of the same re-entrancy surfaced while testing:

• flush()'s JS-transport arm handed write_buffer to onWrite and only cleared it after the call returned. When that call re-entered flush() (session.settings() calls it directly; an auto-flush can be drained together with the call's microtasks), the nested flush sent the same bytes again, putting a second connection preface + SETTINGS in the middle of the stream.
• Review (thanks to the header-block finding) showed the per-frame boundary was not enough: a frame issued while a header block larger than one frame was in flight landed between HEADERS(!END_HEADERS) and its CONTINUATION, which RFC 9113 4.3 makes a connection-fatal PROTOCOL_ERROR for the peer.

Fix

For sessions without a native socket, write() routes through a TxFrameTracker that follows the outbound byte stream (frame headers, payload lengths, END_HEADERS across HEADERS/PUSH_PROMISE/CONTINUATION) and only hands bytes to onWrite where another frame may legally follow: at a frame boundary, outside a header block. A unit that overflows the cork is assembled in tx_spill (corked prefix + its chunks) and written whole once its last chunk arrives, which always happens synchronously, so a frame issued re-entrantly corks up behind it instead of landing inside it. flush() refuses to run mid-unit; its JS-transport arm takes write_buffer out before calling onWrite (no double-send on re-entry) and skips the prefix a native socket already wrote before detaching (write_buffer_offset).

Since bytes are now copied into the cork or spill before any JS runs, this also closes the window where the payload slice borrowed from a user ArrayBuffer was read again after _write re-entered JS (detach/resize during the flush sent freed or foreign heap bytes as the DATA payload; verified with a transfer/respray oracle).

The native TCP/TLS path (no JS re-entry) keeps its fill-to-record-boundary behavior unchanged.

Verification

test/js/node/http2/node-http2.test.js gains frames issued from inside a user-supplied Duplex transport's _write: a matrix of nested ping() / settings() / goaway() / request() / multi-frame data issued from the transport's _write, crossed with the outer write being a single-frame DATA or a HEADERS+CONTINUATION header block, and with the transport reporting backpressure. Each case captures the raw bytes and asserts every byte belongs to exactly one complete frame, header blocks are contiguous, the DATA frame carries exactly the body, nothing is duplicated, and the nested frames follow the unit they were issued from. 15/15 fail on current canary (the 5 continuation cases also failed against the earlier per-frame version of this fix), 15/15 pass with this change.

Also run with the debug build: node-http2.test.js (331 tests), h2-conformance.test.ts and the other test/js/node/http2/* files, all 254 test/js/node/test/parallel/test-http2-* and the 5 sequential http2 tests, rust:check-all.

---

[review] gate passed · iteration 2 · 2 files touched

fails on main (without fix)

ASAN without fix: 20 failed, 6 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/node-http2.test.js"
bun test v1.4.0 (7a29ece25)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [838.57ms]
(pass) node none > Client Basics > should be able to send a POST request [561.09ms]
(pass) node none > Client Basics > constants [18.54ms]
(pass) node none > Client Basics > getDefaultSettings [7.44ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [21.86ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [4.84ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.52ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [5.09ms]
(pass) node none > Client Basics > should be able to send data using end [590.83ms]
(pass) node none > Client Basics > should be able to mutiplex GET requests [583.48ms]
(pass) node none > Client Basics > http2 should receive remoteSettings when receiving 
... (truncated)

release without fix: 13 failed, 6 skipped
bun test v1.4.0-canary.1 (78e135377)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > constants [1.67ms]
(pass) node none > Client Basics > getDefaultSettings [0.17ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [0.39ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [0.09ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [0.03ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [0.06ms]
(pass) node none > Client Basics > is possible to abort request [2.29ms]
(pass) node none > Client Basics > aborted event should work with abortController [0.71ms]
(pass) node none > Client Basics > aborted event should work with aborted signal [0.66ms]
(pass) node none > Client Basics > signal validation matches node: non-signal objects throw, duck-typed { aborted } is accepted [1.98ms]
(pass) node none > Client Basics > should fail to connect over HTTP/1.1 [38.10ms]
(skip) node none > Client Basics > should not leak memory
580 |             await doHttp2Request(HTTPS_SERVER, HTTPS
... (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/mechgate.xml" "test/js/node/http2/node-http2.test.js"
bun test v1.4.0 (7a29ece25)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [767.88ms]
(pass) node none > Client Basics > should be able to send a POST request [532.84ms]
(pass) node none > Client Basics > constants [20.07ms]
(pass) node none > Client Basics > getDefaultSettings [6.94ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [21.79ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [4.68ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.37ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [4.93ms]
(pass) node none > Client Basics > should be able to send data using end [563.61ms]
(pass) node none > Client Basics > should be able to mutiplex GET requests [553.50ms]
(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)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     7a29ece257
  features     baseline

22 deps, 108 codegen, 1175 objects in 709ms

ninja: Entering directory `/workspace/bun/build/release'
[1/139] gen generated_host_exports.rs
generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 240 extern-C blocks audited
[2/139] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
  - ResolveMessage (13 fields)
  - BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
  - Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
  - ResourceUsage (8 fields)
  - Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
  - CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
  - FileSystemRouter (5 fields)
  - FrameworkFileSystemRouter (2 fields)
  - MatchedRoute (8 fields)
Found 1 classes from /workspace/bu
... (truncated)

diff hotspot

src/runtime/api/bun/h2_frame_parser.rs | 150 ++++++++++++++++++++++++++--
 test/js/node/http2/node-http2.test.js  | 174 +++++++++++++++++++++++++++++++++
 2 files changed, 315 insertions(+), 9 deletions(-)

gate history · 2 passed · 0 rejected · iteration 2

evidence per changed file

file                                    reads  edits  tests
src/runtime/api/bun/h2_frame_parser.rs     42     25      0
test/js/node/http2/node-http2.test.js       3      8      0

[robobun]

Status: diff is complete and review-clean. After the consolidation Jarred asked for (no per-parser spill: overflow units assemble in the shared batch scratch, 7a29ece), the verification set still passes with the debug build: the 20-case matrix plus #36917's padded re-entrant test (21/21, all 20 fail on current canary), node-http2.test.js (344), h2-conformance (61), all 256 test-http2-* parallel and 5 sequential files, and the #36910 repro modes clean.

CI runs 89001, 89007, 89113, and 89122 are red only on lanes unrelated to this diff: the pre-existing worker-terminate ExceptionScope SIGABRT on x64-asan (reported to triage), a pre-existing AsyncLocalStorage leak report, and retry-passing flakes (the http2 entry in 89122 is the pre-existing maxSessionMemory 15s darwin timeout, which also flaked on the pre-fix sha and runs at identical speed on main's parser vs this branch; it passed on retry, and the new tests passed on every lane in every run). The one retrigger was already used. Ready for maintainer review/merge.

[coderabbitai]

Walkthrough

H2FrameParser now tracks outbound HTTP/2 frame boundaries for JS transports, buffers incomplete frames and header blocks, preserves bytes during re-entrant writes, and validates nested transmission.

Changes

HTTP/2 JS transport transmission

Layer / File(s)	Summary
Transmit state and re-entrant flush handling src/runtime/api/bun/h2_frame_parser.rs	H2FrameParser tracks incomplete outbound frames. Flush defers during incomplete transmission. Initialization, memory accounting, and detachment manage transmit state and spill storage.
JS transport frame-boundary writes src/runtime/api/bun/h2_frame_parser.rs	Non-native sessions route writes through write_to_js_transport. The writer combines buffered data, emits complete frame units, and retains incomplete frames and header blocks.
Re-entrant transport validation test/js/node/http2/node-http2.test.js	Tests cover nested PING, SETTINGS, GOAWAY, request HEADERS, DATA, and CONTINUATION writes with and without backpressure. They verify framing, ordering, payloads, counts, and stream IDs.

Possibly related PRs

• oven-sh/bun#33191: Modifies the same parser for re-entrant HTTP/2 writes.
• oven-sh/bun#34432: Changes the same HTTP/2 write and flush framing logic.
• oven-sh/bun#36910: Addresses related JS-backed HTTP/2 re-entrant writes and tests.

Suggested reviewers: jarred-sumner, cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main fix for preventing HTTP/2 frame splitting around JavaScript transport write callbacks.
Description check	✅ Passed	The description explains the problem, implementation, testing, and verification results in sufficient detail, although it does not use the template headings exactly.

---

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

[coderabbitai]

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 3516-3528: The multi-frame send_data path bypasses tx_spill,
allowing nested DATA frames to overtake incomplete frames. In
src/runtime/api/bun/h2_frame_parser.rs#L3516-3528, update flush_batch_buffer and
flush_batch_vectored to use the frame-boundary writer, or ensure _write appends
to tx_spill while tx_frame_remaining is nonzero for JS transports; in
test/js/node/http2/node-http2.test.js#L3779-3788, add a nested req.write case
with a payload larger than 16384 bytes and assert complete framing and payload
integrity.
- Around line 3526-3527: Update the spill handling in the method containing
self._write(&data) to retain and recycle the tx_spill Vec instead of dropping
the allocation returned by core::mem::take. Reuse the existing hand-back pattern
from write_buffer and flush_batch_buffer, returning the buffer to tx_spill when
it remains empty after writing while preserving the current write behavior.
- Around line 3545-3548: Update the partial-header branch in track_tx_frame so
it reports failure/not-at-frame-boundary instead of breaking with remaining at
zero; include the partial chunk length in the tx_spill accounting. Ensure
write_to_js_transport retains those bytes in tx_spill and prepends them to the
next write before re-parsing the frame header, preventing a partial header from
being treated as a complete frame.
- Around line 3027-3038: Update the flush branch around write_buffer_offset and
the binary_type.to_js call to pass only the unwritten suffix
write_buffer[offset..] to JS. After taking that suffix, reset
write_buffer_offset, and if to_js() fails, restore the suffix to write_buffer
while preserving the existing return-0 behavior.

In `@test/js/node/http2/node-http2.test.js`:
- Around line 3779-3788: Add a nested-write variant to the existing HTTP/2
transport test matrix that invokes req.write() with a payload larger than 16384
bytes from inside the transport _write callback, forcing the multi-frame
send_data path and BATCH_BUFFER handling. Reuse the test’s existing framing and
payload-integrity assertions, ensuring this case verifies complete frames and an
unchanged payload.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 38781031-eace-4a8e-a1c5-2e3e677e26a8

📥 Commits

Reviewing files that changed from the base of the PR and between 1bfcb4e and 71ed375.

📒 Files selected for processing (2)

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

[github-actions]

This PR may be a duplicate of:

1. node:http2: keep a stream.write() payload stable while transport JS runs mid-send #36910 - Patches the exact same BunSocket::None cork-straddle branch of H2FrameParser::write for the same re-entrant JS Duplex _write root cause; its owned-Vec copy is subsumed by this PR's write_to_js_transport/tx_spill rewrite of that loop.
2. http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432 - Independently fixes this PR's second defect in the same flush() JS-transport hunk (nested flush() resending write_buffer → duplicate connection preface), using a js_socket_flushing guard plus a consumed-prefix offset.

🤖 Generated with Claude Code

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[robobun]

On the duplicate candidates: #36910 fixes the use-after-free face of this same re-entrancy (the borrowed payload slice read after _write re-enters JS) by copying the straddling slice in write(). This PR's frame-boundary writer copies every JS-transport byte into the cork or spill before any JS runs, so it covers that repro too (verified both its transfer and resize0 modes: 0 foreign bytes, no SEGV on this branch). The ordering bug fixed here is not fixed by copying alone, so these are siblings, not duplicates; whichever lands second needs a trivial rebase in write(). #34432's js_socket_flushing guard addresses the duplicate-preface symptom; this PR's flush() fix (take write_buffer before onWrite, skip the native-sent prefix) covers the same case at the buffer level.

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 1357-1360: Update get_session_memory_usage_bytes to include
self.tx_spill.get().len() in the session memory total alongside write_buffer and
queued_data_size, so is_over_session_memory_limit and getBufferSize account for
all buffered bytes.
- Around line 1185-1189: Update the frame-type check in the header-block
tracking logic to use the existing FrameType constants for HEADERS,
PUSH_PROMISE, and CONTINUATION instead of numeric literals, and remove the
now-unnecessary explanatory comment. Keep the existing header_block_open flag
behavior unchanged.

In `@test/js/node/http2/node-http2.test.js`:
- Around line 3833-3836: Extend the case generation loop over nested kinds to
include the missing ["continuation", true] combination alongside the existing
data and continuation cases. Update the cases construction so the continuation
outer case exercises both backpressured states, preserving all existing
variants.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 93f56235-7eeb-4f21-a74e-43f456d440be

📥 Commits

Reviewing files that changed from the base of the PR and between 9a95458 and 6f93d1f.

📒 Files selected for processing (2)

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

[claude]

No new issues found — the header-block atomicity gap flagged in the earlier pass is closed by TxFrameTracker.header_block_open, and the follow-up review comments (spill in memory accounting, FrameType constants, [continuation, true] matrix arm) are all in. Deferring to a human because this is nontrivial re-entrancy handling on the HTTP/2 wire path.

What was reviewed:

• TxFrameTracker.advance: preface skip, split-header accumulation, END_HEADERS tracking across HEADERS/PUSH_PROMISE/CONTINUATION — matches RFC 9113 §4.3.
• write_to_js_transport: cork-fits / direct-write / spill paths; spill is mem::taken before _write so nested writes cork behind, and the Vec is recycled with the same cap-shrink pattern as write_buffer.
• flush(): at_boundary() guard, take-write_buffer-before-onWrite (nested flush can't double-send), write_buffer_offset prefix drain for the native-detach case, and buffer restore on the to_js failure path.
• Native TCP/TLS path in write() unchanged; new state cleared in the detach/deinit block.

Extended reasoning...

Overview

This PR fixes frame interleaving when an http2 session runs over a user-supplied JS Duplex (createConnection) and the transport's _write synchronously re-enters the session (e.g. sess.ping() from inside _write). Before, the 16 KiB cork was flushed mid-frame, the nested call's frame corked into the emptied buffer, and the outer frame's remainder was appended after it — the peer parsed the nested frame's bytes as the outer frame's payload. The fix adds a TxFrameTracker that follows the outbound byte stream and only hands bytes to onWrite at a legal boundary (between frames, outside a header block); anything larger than the cork is assembled whole in tx_spill first. flush() now takes write_buffer out before calling into JS and honors write_buffer_offset. ~150 lines of Rust in h2_frame_parser.rs plus a 20-case test matrix in node-http2.test.js.

Security risks

Low. The affected path is outbound framing over a JS transport the caller already controls; the fix copies user bytes into owned buffers before re-entering JS, which incidentally closes a use-after-detach window on borrowed ArrayBuffer payloads noted in the PR description. tx_spill is bounded by whatever the caller is already serializing synchronously (a single frame or header block) and is now counted in get_session_memory_usage_bytes. No new untrusted-input parsing — the tracker parses bytes Bun itself just serialized.

Level of scrutiny

High. This is protocol-level re-entrancy handling in native code: cork/spill/flush interact with thread-local cork state, backpressure, and the header-block invariant from RFC 9113 §4.3. The first revision had a real hole (nested frame between HEADERS and CONTINUATION) that was caught in review and fixed; the mechanism now depends on at_boundary() being false at every point where _write/onWrite can run user JS mid-unit. That reasoning looks sound and the 20-case test matrix (nested {ping, settings, goaway, request, multi-frame data} × outer {DATA, HEADERS+CONTINUATION} × backpressure {off, on}) covers the interactions, but a maintainer who owns this file should confirm the cork/CORKED_H2 interaction and the "unit always completes synchronously" assumption the design leans on.

Other factors

All prior review threads (mine, coderabbit's, comment-cop's) are resolved. The PR description reports the full node-http2.test.js, h2-conformance.test.ts, and 259 upstream test-http2-* files pass on the debug build, plus rust:check-all. The native-socket write() loop is untouched. There is a noted overlap with #36910 and #34432 that whichever lands second will need to reconcile in write() / flush(). Given the subtlety and the sibling PRs, this should get a human sign-off.

[Jarred-Sumner]

@robobun fix conflicts

[robobun]

Rebased onto main and resolved the conflicts with #34432 and #36917. The overlap resolution: flush()'s JS-transport arm now uses main's version as-is (the js_socket_flushing guard plus offset-based consume from #34432 covers the re-entrant double-send and the dropped-bytes case this PR had fixed with a take-based approach), with only this PR's mid-unit at_boundary() guard added on top. The TxFrameTracker / write_to_js_transport / tx_spill parts are unchanged.

Re-verified on the rebased branch with the debug build: the 20-case matrix passes (still 20/20 fail on canary), node-http2.test.js (344 tests), h2-conformance.test.ts (61), all 256 test-http2-* parallel tests including the two #34432 added, and the #36910 repro modes stay clean.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 3064-3069: The HEADERS frame path must handle padding-buffer
allocation failure before any frame bytes advance tx_tracker. Update the
relevant frame write/serialization flow to reserve padding before frame.write();
on failure, reset or close the session so buffered header and priority bytes
cannot remain as an incomplete frame, while preserving normal boundary handling
in tx_tracker.

In `@test/js/node/http2/node-http2.test.js`:
- Around line 4249-4255: In the frame-order test around blockEnd, explicitly
assert that findIndex located an END_HEADERS frame before using it when outer is
"continuation". Ensure a missing match fails immediately, while preserving the
existing issuedAfter, before, and stream-3 assertions for valid matches.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: dd8dca6b-3385-4ffe-be85-6896bf1fe237

📥 Commits

Reviewing files that changed from the base of the PR and between 0f53bd0 and 6cabc9f.

📒 Files selected for processing (2)

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

[Jarred-Sumner]

Is there a simpler approach that doesn't need the spill buffer? Can we avoid the allocaiton cost here? Can we consolidate?

[robobun]

Good call: the spill buffer is gone in 7a29ece. The overflowing unit is now assembled in BATCH_BUFFER, the thread-local scratch that send_data's multi-frame batching already owns, with the same take-before-_write/put-back pattern flush_cork_buffer and flush_batch_buffer use. That works because a unit's chunks are emitted back to back with no JS in between, so the scratch is exclusively this write's while it holds a partial unit.

On the cost questions:

• Hot path (frame fits in the cork, the overwhelming majority): unchanged from main, a memcpy into the 16 KiB cork, zero allocation.
• Overflow path: appends into the shared scratch, whose capacity is reused process-wide and capped at 1 MiB exactly like the batch path; no per-session Vec, no pinned per-parser capacity, and the memory-accounting and detach special-cases for it are gone too.
• What remains of the new state is one 16-byte Copy Cell (TxFrameTracker) per parser and the at_boundary() check in flush().

I also tried consolidating onto write_buffer (park the unit there and let flush()'s offset-consume hand it over), which would have been even less code, but it changes the re-entrancy context: bytes parked in write_buffer make has_backpressure() true, so a stream write issued from inside the transport's _write routes through the outbound queue, and queued frames compute PADDING_STRATEGY_MAX differently than the direct path (the padded re-entrant test from #36917 catches exactly that, 0 vs 255 padding). The batch-scratch version keeps today's dispatch context byte-for-byte.

Re-verified with the debug build: the 20-case matrix plus #36917's padded re-entrant test (21/21), node-http2.test.js (344), h2-conformance (61), all 256 test-http2-* parallel and the 5 sequential files, and the #36910 repro modes stay clean.

[claude]

No issues found in the latest revision — the earlier header-block-atomicity finding is addressed (TxFrameTracker.header_block_open + the continuation outer cases in the test matrix). Deferring to a human for the 7a29ece redesign: reusing the thread-local BATCH_BUFFER as the overflow-unit scratch is sound as far as I traced (the scratch is always taken before _write re-enters JS, and the emitters serialize a unit's chunks back-to-back with no JS in between), but that invariant spans send_data, flush_cork_buffer, flush_batch_buffer, and every frame emitter, so it's worth a maintainer's eyes.

Checked: TxFrameTracker.advance handles the client preface, split frame headers, and END_HEADERS across HEADERS/PUSH_PROMISE/CONTINUATION; flush()'s new at_boundary() early-return keeps the auto-flush registered; the padded-HEADERS try_reserve reorder means no fallible op sits between a frame's header and its last payload byte; the native-socket write loop is byte-for-byte unchanged.

Extended reasoning...

Overview

The PR fixes an HTTP/2 wire-protocol re-entrancy bug: when http2.connect() uses a user-supplied Duplex transport (createConnection), the session's onWrite handler runs the transport's _write synchronously, and a frame issued from inside that _write (ping/settings/goaway/request) could land inside another frame's payload or between a HEADERS frame and its CONTINUATION, desyncing the peer. The fix adds TxFrameTracker (a small state machine over the outbound byte stream that knows frame and header-block boundaries), routes JS-transport writes through write_to_js_transport(), and assembles any unit that overflows the 16 KiB cork in the shared thread-local BATCH_BUFFER before handing it to _write whole. flush() gains an at_boundary() guard, and the padded-HEADERS try_reserve is hoisted before frame.write() so allocation failure can't abandon a half-serialized frame. A 20-case test matrix (5 nested ops × {DATA, HEADERS+CONTINUATION} × {backpressured, not}) validates every byte belongs to exactly one complete frame, header blocks are contiguous, and nested frames follow the unit they were issued from.

Security risks

None identified. The change is defensive — it prevents a peer-observable framing desync (which the peer treats as PROTOCOL_ERROR / FRAME_SIZE_ERROR) and closes a window where a user ArrayBuffer payload could be re-read after _write re-entered JS. There is no new parsing of untrusted input; TxFrameTracker.advance walks bytes the session itself just serialized.

Level of scrutiny

High. This is protocol-critical code in the HTTP/2 outbound path with subtle re-entrancy reasoning. The 7a29ece change replaced a per-session tx_spill Vec with the shared thread-local BATCH_BUFFER, which is correct only under the invariant that (a) frame emitters serialize a unit's chunks back-to-back with no JS between them, and (b) send_data's multi-frame batching (the other BATCH_BUFFER owner) never re-enters write() while its batch is populated. I traced both and they hold — write_to_js_transport only calls _write after mem::take-ing the scratch, and the emitters' writer.write_all() calls are contiguous — but the invariant is non-local and a maintainer familiar with send_data should confirm.

Other factors

My earlier inline finding (header blocks must be atomic, not just individual frames) was incorporated: header_block_open in the tracker plus the continuation outer cases in the test. All CodeRabbit and comment-cop threads are resolved; Jarred's only comment was to fix conflicts, which was done. The test suite is thorough (parses raw wire bytes, asserts frame completeness/ordering/no-duplication, polls by condition not time, cleans up in finally), and the PR reports the full http2 test corpus (344 + 61 + 256 + 5) passing on the debug build. The native TCP/TLS path is untouched. Given the cross-function invariant the BATCH_BUFFER reuse rests on, this should get a human sign-off rather than an auto-approval.