http2: resolve BatchSegment to its bytes in one place

[robobun]

Problem

• H2FrameParser::flush_batch_vectored (src/runtime/api/bun/h2_frame_parser.rs, around line 3510) turns each BatchSegment into the bytes it puts on the wire three separate times: when building the iovecs, in the copy fallback taken when the socket is no longer plain TCP, and when copying the unwritten tail of a partial write into write_buffer. The three matches have the same arms, so a change to one copy would miss the others.
• This is the same_match_twice:src/runtime/api/bun/h2_frame_parser.rs entry in mordant-baseline.toml.

Fix

• Add BatchSegment::raw_parts(self, batch: &[u8]) -> (*const u8, usize), #[inline(always)], holding the one mapping: the Batch arm is the original batch[off as usize..].as_ptr() with its length, the Ext arm is the stored pointer and length. All three loops call it.
• No extra work on the vectored path: the iovec build and the partial-write tail are the original loops with only the match hoisted out, so they compile to what they did before (same single bounds check on the Batch arm, same from_raw_parts(ptr.add(skip), len - skip) in the tail). The cold copy fallback (socket detached mid-call) now forms its slice with from_raw_parts for both variants instead of a checked slice for Batch; its SAFETY comment states why both variants are valid (Batch ranges were recorded inside the buffer being flushed, Ext payloads belong to the send_data call that is still running, since flush_batch_buffer is only called from it).
• An earlier revision returned a &[u8] from an unsafe fn; that added an add and a range check to the hot loop and was reverted to the shape above after review.
• mordant-baseline.toml: the entry above is removed. This is what bun run rust:mordant:baseline produced before the rebase onto current main, minus two stale entries it also dropped at the time, which main has since removed itself; bun run rust:mordant reported nothing over the regenerated baseline. After the rebase the baseline change is this one line.
• Test: test/js/node/http2/node-http2-syscall-fault.test.ts gains two cases that POST a 53248 byte body (three full DATA frames plus a partial one) over h2c and compare what the server received byte for byte. Every u32 of the body holds its own offset, so a slice taken from the wrong place or with the wrong length changes the bytes, not only the count. One case lets writev take the batch (the iovec build); the other faults writev to return 0, so the whole batch goes through the partial-write tail into write_buffer and is drained on writable. With the debug logs on, both cases show us_socket_raw_writev(.., 9) (corked HEADERS frame plus four header/payload pairs), and only the faulted one shows _genericFlush 53309 for the re-buffered batch. The copy fallback is only reachable when the socket is detached mid-call and has no test.
• Verified:
  • bun bd test test/js/node/http2/ on the final revision, rebased onto current main: 475 pass, 6 skip, 0 fail (the two new cases included). node-http2-writable-destroy-fixture.ts in the same file already drove a 40000 byte body through the faulted writev path, checking for a UAF rather than the bytes.
  • Node's own test-http2-backpressure, -large-write-close, -large-write-destroy, -large-write-multiple-requests, -large-writes-session-memory-leak, -many-writes-and-destroy, -multiplex, -pipe, -write-callbacks, -write-finishes-after-stream-destroy and -compat-serverresponse-write against the debug build (11 pass).
  • cargo clippy -p bun_runtime --no-deps is clean; bun run rust:mordant:baseline on the final revision reproduces main's baseline minus exactly the h2_frame_parser same_match_twice line (so the tuple-returning helper adds no finding of its own).

Background

• When send_data has to split a payload larger than one DATA frame (16 KiB), it does not flush each frame separately. It appends the frame headers to a thread-local scratch buffer (BATCH_BUFFER) and records the wire order in BATCH_SEGMENTS: a BatchSegment::Batch is a range of that scratch buffer, a BatchSegment::Ext points straight at a slice of the caller's payload so the 16 KiB chunks are not copied. On plain TCP the batch goes out as one writev over those pieces; flush_batch_vectored is that flush, and the three loops in it are the three things it may have to do with the pieces (send them, copy them when it cannot vectorize, or buffer whatever writev did not accept).
• mordant is the advisory Rust lint pack run by bun run rust:mordant. mordant-baseline.toml holds per-(lint, file) counts of the findings that predate the job; CI fails only on findings over those counts, and fixing a finding lets its entry be deleted so the ratchet tightens.

---

no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/http2/node-http2-syscall-fault.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 15 minutes

Limit details: You’ve used all 5 included reviews currently available under your plan.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5b1372dd-bd2c-48e3-ac3e-cc53a2238dd1

📥 Commits

Reviewing files that changed from the base of the PR and between f0f6b2c and e03997d.

📒 Files selected for processing (3)

• mordant-baseline.toml
• src/runtime/api/bun/h2_frame_parser.rs
• test/js/node/http2/node-http2-syscall-fault.test.ts

---

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

[robobun]

^{Updated 7:58 PM PT - Aug 15th, 2026}

@robobun, your commit 8753a0d is building: #99038

[claude]

LGTM — mechanical extract-method that collapses three identical match BatchSegment sites into one bytes() helper.

What was reviewed:

• Verified each of the three call sites (iovec build, copy fallback, partial-write tail) produces the same pointer/length as before; the tail's &bytes[skip..] is equivalent to the old from_raw_parts(ptr.add(skip), len - skip).
• Checked the lifetime elision on bytes(self, batch: &[u8]) -> &[u8] — ties the return to batch, which is strictly shorter than the Ext payload's actual lifetime under the stated safety contract.
• The Batch arm now bounds-checks off + len instead of just off — a strengthening, and the PR explains why it can't fire.
• The two extra mordant-baseline.toml deletions are accounted for as already-fixed-on-main entries picked up by regeneration.

Extended reasoning...

Overview

This PR deduplicates three copies of the same match BatchSegment { Batch {..} => .., Ext {..} => .. } in H2FrameParser::flush_batch_vectored into a single BatchSegment::bytes() method, and drops the corresponding same_match_twice entry (plus two stale entries) from mordant-baseline.toml. Net diff in the .rs file is roughly -20/+20 lines.

Security risks

None. The change is a pure refactor of how bytes are read from an existing enum; no new inputs are parsed, no bounds are relaxed. The one bounds check that changes gets tighter (the Batch arm now checks off + len against batch.len() via safe slicing, where the original iovec path only checked off). The unsafe surface shrinks: one documented unsafe fn with a SAFETY contract replaces scattered from_raw_parts calls.

Level of scrutiny

Medium — this is HTTP/2 wire-write code with raw pointers, so I traced each of the three sites against its original to confirm byte-for-byte equivalence. All three match: the iovec build's bytes.as_ptr()/bytes.len() equals the old (ptr, len) tuple; the copy fallback was already using the exact slice expression the helper now returns; the tail copy's &bytes[skip..] is the safe-slice spelling of the old from_raw_parts(ptr.add(skip), len - skip). Method resolution on seg.bytes(batch) works via Copy auto-deref (BatchSegment derives Copy). The elided return lifetime ties to batch, which is conservative and correct.

Other factors

The PR description names the existing test that exercises both the iovec and partial-write paths (node-http2-syscall-fault.test.ts faulting writev to return 0 on a 40 KB body), plus the full test/js/node/http2/ suite and eleven upstream Node http2 tests — appropriate coverage for a no-behavior-change refactor. The two extra baseline deletions are explained with the PRs that removed the underlying findings. No prior reviewer comments to address.

[claude]

LGTM — mechanical dedup of three identical BatchSegment matches into one unsafe fn bytes(), no behavior change.

Checked that all three call sites resolve to the same pointer/length as before; the Batch arm now bounds-checks off + len (previously only off), and every construction site records off + len == batch.len() into an append-only buffer, so it never fires. The tail-copy &bytes[skip..] is the same range as the old from_raw_parts(ptr.add(skip), len - skip). Lifetime elision ties the returned slice to batch, which is fine under the documented safety contract (only caller runs inside send_data). New tests exercise both the writev path and the writev→0 re-buffer path with an offset-stamped body that would catch any slicing error byte-for-byte.

Extended reasoning...

Overview

Three files touched: src/runtime/api/bun/h2_frame_parser.rs collapses three duplicate match BatchSegment blocks in flush_batch_vectored into a single BatchSegment::bytes(self, batch) helper; mordant-baseline.toml drops the corresponding same_match_twice entry plus two entries already fixed on main; and test/js/node/http2/node-http2-syscall-fault.test.ts gains two test.each cases that POST a 53 KiB offset-stamped body over h2c and verify byte-for-byte receipt, once with writev succeeding and once with writev faulted to 0.

Security risks

None. This is a pure refactor of an internal HTTP/2 send-batch flush path; no new inputs are parsed, no bounds are loosened (the Batch arm's slice indexing is now strictly tighter, checking off + len instead of only off), and the unsafe block for Ext is the same from_raw_parts that was already present at all three call sites, now behind a documented # Safety contract.

Level of scrutiny

Medium — it's hot-path native code with raw pointers, but the change is a textbook extract-method refactor with no control-flow or arithmetic change. I verified: (1) every BatchSegment::Batch construction site records {off, len} such that off + len == batch.len() at recording time and batch is append-only until flush, so the new full-range bounds check never panics; (2) the elided output lifetime binds to batch (since self is by-value), which is sound because the only caller holds both batch and the Ext payload for the whole function; (3) each of the three sites produces the same (ptr, len) pair as before — the iovec build, the copy fallback's extend_from_slice, and the partial-write tail's &bytes[skip..] (equivalent to the old from_raw_parts(ptr.add(skip), len - skip) given skip < len).

Other factors

The comment-cop bot's flag on the doc comment was addressed in a6dcb85 (thread resolved). The new tests follow the file's existing conventions (fault injection, makeServer, once, cleanup in finally), and the offset-stamped body means any mis-sliced segment would show up as a byte mismatch rather than only a length mismatch. The two extra mordant-baseline deletions are documented as already-fixed-on-main entries picked up by the baseline regeneration. The full test/js/node/http2/ suite and eleven upstream Node http2 large-write tests pass per the PR description.

[claude]

LGTM — mechanical dedup of three identical BatchSegment matches into one unsafe fn bytes(), no behavior change.

What was reviewed:

• Traced each of the three call sites against the old inline matches: iovec build (as_ptr()/len() identical), copy fallback (same range, now off as usize + len as usize instead of (off + len) as usize), and tail copy (&bytes[skip..] replaces from_raw_parts(ptr.add(skip), len - skip)).
• Checked the Batch arm's tightened bounds (off..off+len) against every BatchSegment::Batch push site in send_data — all record len = batch.len() - off, so the range holds.
• Verified the elided lifetime on bytes() ties the return to batch, which is conservative for the Ext arm and sound within flush_batch_vectored.
• New test.each cases cover the writev-accepts and writev→0 re-buffer paths byte-for-byte; comment-cop feedback was addressed in a6dcb85.

Extended reasoning...

Overview

This PR extracts three duplicated match BatchSegment blocks in H2FrameParser::flush_batch_vectored into a single BatchSegment::bytes(self, batch: &[u8]) -> &[u8] helper. The three sites (iovec build, socket-detached copy fallback, partial-write tail copy) now all resolve segments through the same code path. The mordant baseline drops the corresponding same_match_twice entry plus two entries already fixed on main. Two new test cases in node-http2-syscall-fault.test.ts POST a 53 KiB body (four DATA frames) over h2c and verify byte-for-byte delivery, once with writev succeeding and once with writev faulted to return 0.

Security risks

None. This is a pure refactor of an internal wire-assembly path; no new inputs are parsed and no trust boundaries change. The unsafe block is the same from_raw_parts on the Ext payload pointer that was already present at all three original sites — it's now written once with a proper # Safety contract and per-call-site // SAFETY: comments naming why the contract holds (segments are consumed inside the send_data call that recorded them).

Level of scrutiny

Medium — this is hot-path HTTP/2 frame emission with raw pointers, so I traced each call site against the removed code line by line. The iovec build produces the same (ptr, len) pair; the copy fallback produces the same slice (with the minor improvement that the addition now happens in usize rather than u32); the tail copy's &bytes[skip..] is exactly from_raw_parts(ptr.add(skip), len - skip). The Batch arm now bounds-checks off + len (previously only off was checked in the iovec loop); I verified every push site records len as batch.len() - off on the same append-only buffer, so this cannot fire. The elided return lifetime ties to batch, which is conservative for the Ext arm (it actually points elsewhere) but sound because both allocations outlive flush_batch_vectored.

Other factors

The comment-cop bot flagged an over-long doc comment; the author trimmed it in a6dcb85 to just the # Safety contract, and that thread is resolved. The new tests follow harness conventions (test.each, using for the server, try/finally for fault cleanup, offset-encoded body so mis-sliced bytes fail on content not just length). The PR description documents that the copy-fallback branch (socket detached mid-call) has no test — reasonable given it's not reachable via fault injection. No CODEOWNERS entry covers this file. The full test/js/node/http2/ suite and eleven upstream Node http2 large-write tests were run against the debug build.

[robobun]

CI state: every lane that ran passed on all three builds of this branch, most recently 177 of 179 jobs on the build for 458d91f. The two remaining jobs are always the darwin 14 aarch64 test lane, which is working through a multi-hour backlog: on each build they never got an agent, and the build was canceled while they waited. The only real reds were on the first build and unrelated to this change (an Azure agent that failed to provision for the Windows x64 baseline check, and a 30s timeout in test/js/web/timers/setInterval.test.js on the x64 ASAN lane, which passed on the two later builds). Nothing here touches the code in this PR; the new http2 tests passed on every lane that ran.

[claude]

LGTM — mechanical extract-method refactor with byte-exact test coverage.

What was reviewed:

• Traced all three call sites: iovec build, copy fallback, and partial-write tail each produce the same (ptr, len) as before; the Batch arm now bounds-checks off+len (previously only off in two sites), and the tail copy uses safe &bytes[skip..] instead of from_raw_parts(ptr.add(skip), ..).
• BatchSegment is Copy, so seg.bytes(batch) on &BatchSegment compiles via auto-deref; the elided output lifetime binds to batch, which outlives every use.
• New test.each cases: fresh chunks/gotBody per iteration, fault armed after connect and cleared in finally (plus the file-level afterEach), and the offset-encoded body makes wrong-slice bugs byte-visible.
• Both review threads (comment-cop and the // SAFETY: suggestion) are addressed in the current diff.

Extended reasoning...

Overview

This PR deduplicates three identical match BatchSegment { Batch | Ext } blocks in H2FrameParser::flush_batch_vectored (src/runtime/api/bun/h2_frame_parser.rs) into a single BatchSegment::bytes(self, batch: &[u8]) -> &[u8] helper. It also removes the corresponding same_match_twice mordant baseline entry plus two unrelated entries already fixed on main, and adds two byte-for-byte test cases exercising the writev-accepts and writev-returns-0 paths.

Security risks

None. This is pure internal refactoring of the h2 client's batched-write flush; no user-facing surface, parsing, or trust boundary changes. The refactor is strictly safer than the original: the Batch arm now bounds-checks the full [off, off+len) range (via safe slice indexing) where two of three original sites only checked off, and the partial-write tail replaces from_raw_parts(ptr.add(skip), len - skip) with a safe &bytes[skip..]. The Ext arm's from_raw_parts is unchanged and correctly gated behind an unsafe fn whose SAFETY contract (the recording send_data call is still on the stack) is documented and satisfied at every call site.

Level of scrutiny

Medium — this is a hot path in HTTP/2 DATA frame batching that handles raw pointers into caller-owned payload buffers, so any slicing error would corrupt request bodies on the wire. However the change is a textbook extract-method: I traced each of the three sites against its original and confirmed identical (ptr, len) output for both enum variants. BatchSegment derives Copy so the by-value self receiver works with the for seg in &[BatchSegment] iteration. The off as usize + len as usize addition (usize domain) is also safer than the old copy-fallback's (off + len) as usize (u32 wrap then widen).

Other factors

• The new tests use an offset-encoded 53248-byte body so a mis-sliced segment shows up as wrong bytes, not just wrong length; one case exercises the iovec path, the other faults writev to 0 to force the re-buffer path. The copy-fallback path (socket detached mid-call) is noted as untestable, which is reasonable.
• Both prior review comments (comment-cop's long-comment warning and alii's // SAFETY: suggestion) are addressed in the head commit — the helper now has a plain // SAFETY: comment rather than a doc comment.
• CI passed on all lanes that got agents per the author's status comment.
• No CODEOWNERS entry covers this file.

[alii]

@robobun rebase

[robobun]

Rebased onto current main (8753a0d). The only conflicts were mordant-baseline.toml, where main had already dropped the two stale entries so this PR now removes just the h2_frame_parser line, and #39148's SendDataOptions landing next to the new impl block. Rebuilt and reran test/js/node/http2/ on the rebased tree: 475 pass, 0 fail.

[claude]

LGTM — mechanical dedup of three identical BatchSegment matches into one helper, behavior-preserving.

What was reviewed

• Traced all three call sites: iovec build, copy fallback, and partial-write tail each produce the same (ptr, len) as before; the only difference is Batch now bounds-checks off+len (strictly safer).
• Checked lifetime elision on bytes(self, batch: &[u8]) -> &[u8]: the returned slice is tied to batch, and every caller consumes it immediately inside the send_data scope where the Ext payload is alive.
• New tests encode byte offsets into the body so a wrong slice changes bytes, not just length; both writev-accepts and writev→0 re-buffer paths are covered.

Extended reasoning...

Overview

This PR collapses three duplicate match *seg { Batch{..} => .., Ext{..} => .. } blocks in H2FrameParser::flush_batch_vectored into a single BatchSegment::bytes(self, batch) -> &[u8] helper, removes the corresponding same_match_twice entry from mordant-baseline.toml, and adds two byte-exact tests to node-http2-syscall-fault.test.ts covering the iovec and re-buffer paths.

Security risks

None. No new unsafe operations are introduced — the from_raw_parts on the Ext arm is the same call that was inlined at each site before, now behind an unsafe fn with a SAFETY contract. The Batch arm gains a full off..off+len bounds check where the iovec build previously only checked off, which is a strict tightening.

Level of scrutiny

This is hot-path HTTP/2 write code with raw pointers, so I traced each of the three call sites against the removed code:

• iovec build: bytes.as_ptr() / bytes.len() produce the same pointer/length pair as the old (batch[off..].as_ptr(), len) and (ptr, len); empty-skip preserved.
• copy fallback: extend_from_slice(seg.bytes(batch)) is identical to the two old arms; the old (off + len) as usize vs new off as usize + len as usize only differ on u32 overflow (>4 GiB scratch buffer, not reachable).
• partial-write tail: &bytes[skip..] with skip < bytes.len() is exactly from_raw_parts(ptr.add(skip), len - skip).

The elided output lifetime binds to batch; for Ext that's technically narrower than the true payload lifetime, but every call consumes the slice immediately within the same borrow, and flush_batch_vectored runs inside send_data, so no hazard.

Other factors

BatchSegment is #[derive(Clone, Copy)], so seg.bytes(batch) on a &BatchSegment iterator element compiles via copy. CI passed on all lanes that got agents. A maintainer already reviewed and their only feedback (comment style) was addressed and resolved. The mordant baseline diff is exactly the one line this refactor eliminates.

[alii]

Must not regress amount of code

[robobun]

Addressed in 6b7e252 (plus e03997d dropping the helper's doc comment): the helper now returns the same (ptr, len) the inline matches produced and is inline(always), so the iovec build and the partial-write tail are the original loops with only the match hoisted out, no added instructions on that path. Rebuilt and reran test/js/node/http2/ (475 pass, 0 fail); the regenerated mordant baseline still differs from main by exactly the one h2_frame_parser line.

[claude]

LGTM — mechanical extraction of three identical BatchSegment matches into one #[inline(always)] helper, with the maintainer's perf feedback already applied.

What was reviewed:

• raw_parts returns the same (ptr, len) the inline matches produced; the iovec build and partial-write tail loops are byte-for-byte the original code with the match hoisted out.
• The one semantic delta — the cold copy-fallback's Batch arm now uses from_raw_parts instead of a checked slice — is sound: Batch { off, len } ranges are recorded from within the same batch buffer being flushed.
• New tests exercise both the writev path and the writev→0 re-buffer path with offset-as-content bytes; { syscall: "writev", action: "zero" } is an existing fault-injection combo used elsewhere in the suite.

Extended reasoning...

Overview

This PR deduplicates three identical match expressions on BatchSegment inside H2FrameParser::flush_batch_vectored (src/runtime/api/bun/h2_frame_parser.rs) into a single #[inline(always)] fn raw_parts(self, batch: &[u8]) -> (*const u8, usize) on the enum. It removes the corresponding same_match_twice entry from mordant-baseline.toml and adds two byte-exact tests to test/js/node/http2/node-http2-syscall-fault.test.ts covering the iovec-build path and the partial-write re-buffer path.

Security risks

None. No new inputs are parsed and no trust boundary is crossed. The only new unsafe surface is that the cold copy-fallback branch (socket detached mid-call) now forms the Batch slice via from_raw_parts(batch[off..].as_ptr(), len) instead of &batch[off..off+len]. This is safe by construction: Batch { off, len } segments are recorded as ranges into the very batch buffer passed here, so off + len <= batch.len(). The Ext arm's from_raw_parts was already present at all three sites before this change. The SAFETY comment states both invariants.

Level of scrutiny

Medium — this is hot-path unsafe Rust in the HTTP/2 send path, which normally warrants a close look. But the change is a pure match-extraction: the helper body is verbatim the arms of the removed matches, takes self by value on a Copy enum (so seg.raw_parts(batch) on &BatchSegment auto-derefs+copies exactly as match *seg did), and is #[inline(always)]. A maintainer (alii) already reviewed two rounds — first requesting the SAFETY-comment form, then flagging that the earlier &[u8]-returning revision added instructions on the hot path; the final (ptr, len) shape is precisely what was asked for, and all their inline threads are marked resolved.

Other factors

The two new test.each cases follow the file's existing conventions (makeServer with using, once for events, fault.clear() in finally) and use offset-as-content bytes so a wrong slice base or length would show as a byte mismatch, not just a length mismatch. The { syscall: "writev", action: "zero" } fault is already used in test/js/bun/net/tls-low-prio-queue-fixture.ts, so it's a known-supported combination. The PR description reports bun bd test test/js/node/http2/ passing 475/0 on the rebased tree and clean clippy/mordant. CI passed on all lanes that ran per the timeline. The bug-hunting system found nothing.