[robobun]

Problem

• FrameHeader::write, StreamPriority::write and UInt31WithReserved::write in src/runtime/api/bun/h2_frame_parser.rs each wrap one writer.write_all(..) call and return .is_ok(), turning the writer's bun_core::Error into a bool.
• 31 of the 32 call sites discard that bool (let _ = frame.write(..)). The one reader, Stream::flush_queue, uses it as a backpressure flag, the same thing its sibling branches compute with writer.write_all(..).is_ok() directly.
• This is the error_collapsed_to_bool:src/runtime/api/bun/h2_frame_parser.rs = 32 entry in mordant-baseline.toml.

Fix

• The three methods return the bun_io::Result<()> they get from write_all, matching the signature of what they wrap (and of DirectWriterStruct::write_all / write_padded further down the file). The let _ = call sites compile unchanged; flush_queue appends .is_ok().
• No behavior change: every site either discarded the value before and still does, or (flush_queue) maps the same write_all outcome to the same bool. Discarding is intentional at those sites: the writers are either a stack FixedBufferStream sized for the frame, which cannot fail, or DirectWriterStruct, whose Err means the bytes were buffered for a later flush, and the rest of the frame still has to be written in order.
• mordant-baseline.toml regenerated with bun run rust:mordant:baseline: the 32-count entry is gone. The regeneration also drops two empty crate tables ([bun_ast], [bun_sql_jsc]) that earlier hand edits had left in the file (the generated file never contains empty tables); bun run rust:mordant then reports nothing over the baseline.
• Verified on the debug build after each rebase: test/js/node/http2/node-http2.test.js (361 pass, 6 skip; the describe.concurrent detached-payload block times out intermittently at its 5 s default on debug builds, the pre-existing flake test(http2): give the detached-payload subprocess cases a debug-scaled timeout #38078 covers, and does so without this change too), plus h2-conformance.test.ts, node-http2-continuation.test.ts, node-http2-invalid-padding.test.ts and node-http2-streams-rehash.test.ts (88 pass). cargo fmt --check clean. Buildkite build 100195 ran this same diff on the previous rebase and passed fully.
• No new test: nothing observable from JS changes, so no runtime test can distinguish before from after. The regression guard is the lowered baseline itself: the mordant CI job fails if a write_all result is collapsed to a bool in this file again.

Background

• These three types serialize the fixed-size parts of outbound HTTP/2 frames (the 9-byte frame header, the 5-byte PRIORITY payload, a 31-bit id with its reserved bit) into a bun_io::Write sink.
• Two sinks are used: bun_io::FixedBufferStream over a stack array sized to the frame, and DirectWriterStruct, which forwards to H2FrameParser::write. That function returns false when the socket did not take the bytes; they are kept in the parser's write buffer and flushed later, so Err from these serializers means backpressure, not lost data.
• mordant-baseline.toml is the per-(lint, file) count of accepted findings for the repo's Rust lint pack; CI fails only on findings above it, and fixing some means regenerating the file so the count stays down.

Rebase note

Rebased onto main after #39448 removed the dead inbound half of this file. The only conflict was the deleted UInt31WithReserved::from_bytes sitting next to the changed write signature; resolved by keeping the deletion. #39448 also removed the unused FullSettingsPayload::write, which an earlier version of this description mentioned leaving alone. The baseline was regenerated again on the rebased tree and came out identical to the merged result.

Rebased a second time when other sweep PRs changed mordant-baseline.toml (the only conflicting file). Resolved by taking main's copy and running bun run rust:mordant:baseline again on the rebased tree; the result is main's current baseline minus this file's entry and the two empty tables. The source change applied cleanly.

[coderabbitai]

Warning

Review limit reached

Your included review limit has been reached.

You’re in a promotional period — use the checkbox below to run this review for free:

• Run review for free

_{On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file.}

How can I continue?

Run this review now using the option above, or comment @coderabbitai review --use-credits.

You can also wait for the limit to reset (next review available in 13 minutes), then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4c6db8bc-2ac3-45d8-8ed6-6bf654c1cbee

📥 Commits

Reviewing files that changed from the base of the PR and between a63ef2d and 1f653a4.

📒 Files selected for processing (2)

• mordant-baseline.toml
• src/runtime/api/bun/h2_frame_parser.rs

---

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

[robobun]

Status: ready to merge once re-approved (the first rebase force-push dismissed the earlier approval). Requested in the mordant baseline sweep (the 32-count error_collapsed_to_bool entry for h2_frame_parser.rs).

• Rebased twice as main moved: first for Remove dead code from the node:http2 legacy inbound path, the watcher loader column, unused native bindings, builtins.d.ts and misc crates #39448 (trivial source conflict), then for baseline churn from other sweep PRs (baseline taken from main and regenerated). The diff is unchanged: three signatures, three bodies, one .is_ok() at the reader, and the regenerated baseline.
• Verified on the rebased debug build: the http2 suites pass (node-http2.test.js 361 pass, plus 88 across the conformance, continuation, padding and rehash files); the only local failures were the detached-payload block's 5 s debug-build timeouts that test(http2): give the detached-payload subprocess cases a debug-scaled timeout #38078 already tracks, which happen without this change too. bun run rust:mordant reports nothing over the regenerated baseline; cargo fmt --check is clean.
• CI: build 100195 ran this same diff on the previous rebase and passed fully. On the current head (1f653a4, build 101955) the one red lane is again test/bake/deinitialization.test.ts segfaulting on exit on Windows x64, a pre-existing intermittent crash on main (for example main build 99388) that is reported separately; the other entries passed on retry or when run alone, including h2-conformance.test.ts on one darwin lane, whose failing case is the GC-based stream-release check that flakes on most main builds and that test(http2): check the release cases through the native handles instead of collecting #39609 is replacing. Build 101955 finished 178 of 179 jobs green. Nothing in the diff is on those code paths, and the mordant and Source lints checks are green on this head.
• No new test: nothing is observable from JS, so the lowered baseline is the regression guard (the mordant job fails if the pattern returns in this file).
• The CodeRabbit comments are rate-limit notices with no review content.

[claude]

LGTM — mechanical return-type change from bool to bun_io::Result<()> on three frame serializers, with no behavior change.

What was reviewed:

• Confirmed all ~31 let _ = ….write(…) call sites in h2_frame_parser.rs still discard the result identically; the one reader in Stream::flush_queue now appends .is_ok() to recover the same bool.
• Checked for any if x.write(…) / boolean-condition uses that would break with a Result return — none found.
• mordant-baseline.toml regeneration drops the resolved 32-count entry plus three empty crate tables; no unrelated entries touched.

Extended reasoning...

Overview

This PR changes three HTTP/2 frame serializer methods (FrameHeader::write, StreamPriority::write, UInt31WithReserved::write) in src/runtime/api/bun/h2_frame_parser.rs to return bun_io::Result<()> directly from writer.write_all(..) instead of collapsing it to a bool via .is_ok(). The one call site that consumed the bool (Stream::flush_queue, line 1743) now applies .is_ok() at the call site. mordant-baseline.toml is regenerated to drop the corresponding error_collapsed_to_bool lint entry and three empty crate tables left by prior hand edits.

Security risks

None. This is a return-type refactor on internal wire-serialization helpers with no change to what bytes are written, when, or how errors propagate. No user input handling, auth, crypto, or permissions are touched.

Level of scrutiny

Low. The change is purely mechanical: dropping .is_ok() from three method bodies and adding it back at the single call site that needs a bool. I grepped the file for every .write( call on these types — all remaining ~30 sites use let _ =, which discards a Result<()> exactly as it discarded a bool, so they compile and behave unchanged. I also searched for any if …write(…) conditionals or other bool-typed uses that would break; there are none. The sibling branches in flush_queue already compute writer.write_all(..).is_ok() directly, so the updated site now matches them.

Other factors

The PR description explains why discarding is intentional (fixed-size stack buffers cannot fail; DirectWriterStruct errors mean buffered-for-later, not lost data), which matches the existing pattern throughout the file. FullSettingsPayload::write was intentionally left unchanged as dead code under the file's transitional #[allow(dead_code)]. The author ran the http2 test suite (447 tests) on the debug build. The baseline file diff is a clean regeneration — only the resolved entry and empty tables are removed, nothing else altered.

[robobun]

^{Updated 6:13 PM PT - Aug 20th, 2026}

❌ @robobun, your commit 1f653a4 has 1 failures in Build #101955 (All Failures):

• test/bake/deinitialization.test.ts - pid 2184 segmentation fault at address 0x0 on 🪟 2019 x64

---

🧪   To try this PR locally:

bunx bun-pr 39262

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

bun-39262 --bun

[claude]

LGTM — mechanical signature change with no behavior difference.

What was reviewed:

• The three write methods now return bun_io::Result<()> instead of collapsing to .is_ok(); all 27 grepped call sites use let _ = (discards Result the same as bool) except the one in Stream::flush_queue, which correctly appends .is_ok() to preserve the backpressure-flag semantics.
• Confirmed no other call site read the old bool return value, so nothing else needs .is_ok().
• mordant-baseline.toml changes are the removed 32-count entry plus three empty-table drops from regeneration; no unrelated counts touched.

Extended reasoning...

Overview

This PR changes the return type of three HTTP/2 frame-serializer helpers (FrameHeader::write, StreamPriority::write, UInt31WithReserved::write) in src/runtime/api/bun/h2_frame_parser.rs from bool to bun_io::Result<()>, dropping the trailing .is_ok() so they return the underlying write_all result directly. One call site (Stream::flush_queue) that consumed the bool now calls .is_ok() itself; all others already discard via let _ =. mordant-baseline.toml is regenerated to drop the 32-count error_collapsed_to_bool entry and three empty crate tables.

Security risks

None. This is a return-type refactor on internal wire serializers with no change to what bytes are written, no new inputs, and no error-path behavior change.

Level of scrutiny

Low. The diff is a lint-driven cleanup requested in the mordant baseline sweep. Each of the three method bodies is a single write_all call whose result was previously mapped through .is_ok(); now it's returned as-is. let _ = expr discards a #[must_use] Result exactly as it discarded a bool, so the ~30 discard sites compile and behave identically. I grepped every .write(&mut ...) call in the file and confirmed the flush_queue site is the only one that consumed the return, and it now maps back to the same bool via .is_ok().

Other factors

• The PR notes FullSettingsPayload::write has the same shape but is dead code under the file-level #[allow(dead_code)]; leaving it out of this change is reasonable and it isn't called anywhere.
• The http2 test suites (359 + 88 tests) were run on the debug build per the PR body/status comment; no test changes are expected since there is no observable behavior difference.
• Already approved by alii per the robobun status comment; no outstanding reviewer feedback and no prior claude[bot] review on this PR.

[claude]

LGTM — mechanical signature change (bool → bun_io::Result<()>) with no behavior difference.

What was reviewed:

• Traced all 32 call sites: 31 are let _ = ... (discard either way); the one reader in Stream::flush_queue now appends .is_ok(), producing the identical bool it got before.
• Confirmed let _ = on a Result compiles without #[must_use] warnings, so the discarding sites are unaffected.
• Baseline regen drops the 32-count error_collapsed_to_bool entry and three empty crate tables — matches what rust:mordant:baseline emits (never writes empty tables).

Extended reasoning...

Overview

The PR changes the return type of three HTTP/2 frame serializer methods in src/runtime/api/bun/h2_frame_parser.rs — FrameHeader::write, StreamPriority::write, and UInt31WithReserved::write — from bool to bun_io::Result<()>. Each method wraps a single writer.write_all(..) call; previously they collapsed the result with .is_ok(), now they propagate it directly. The one call site that consumed the bool (Stream::flush_queue, using it as a backpressure flag) appends .is_ok() to preserve identical semantics. The other 31 call sites use let _ = ..., which discards either a bool or a Result<()> identically and suppresses #[must_use]. mordant-baseline.toml is regenerated, removing the 32-count error_collapsed_to_bool entry for this file plus three empty crate table headers left over from earlier hand edits.

Security risks

None. No untrusted input handling, no allocation, no control-flow changes. The bytes written to the wire are byte-for-byte identical; only the Rust return type of three internal helpers changes.

Level of scrutiny

Low. This is a type-only refactor targeting a repo lint baseline entry. I grepped every .write( call site against these three types in the file (32 hits, matching the baseline count) and confirmed each is either let _ = or the single flush_queue site that now has .is_ok(). The frames_sent.set(frames_sent.get() + 1) side effect in FrameHeader::write still runs unconditionally before the tail write_all, so the counter behavior is unchanged too. The sibling branches of the 'brk block in flush_queue already compute their bool via writer.write_all(..).is_ok(), so this brings the zero-payload branch to the same shape.

Other factors

The PR description reports the full http2 test suites pass on the debug build after rebase, bun run rust:mordant:baseline reproduces the committed file exactly, and cargo fmt --check is clean. The one red CI check (Source lints / jsresult-swallow.test.ts) is stated to fail identically on main and does not touch lines in this diff. No CODEOWNERS entry covers the touched paths. There are no outstanding human review comments; the CodeRabbit comment is a rate-limit notice with no content. The description notes an earlier approval was dismissed by the rebase force-push; the rebase conflict was trivial (a deleted from_bytes adjacent to the changed write signature), and I re-verified the post-rebase state directly.

[claude]

LGTM — mechanical return-type change with no behavior difference.

What was reviewed:

• The three write bodies: only the trailing .is_ok() is dropped; the write_all call and byte layout are unchanged.
• All call sites in h2_frame_parser.rs: every one is let _ = … except flush_queue, which now appends .is_ok() — same bool as before.
• mordant-baseline.toml: only the 32-count entry and two empty crate headers are removed; no other counts changed.

Extended reasoning...

Overview

The PR changes three tiny frame-serializer methods in src/runtime/api/bun/h2_frame_parser.rs (FrameHeader::write, StreamPriority::write, UInt31WithReserved::write) to return bun_io::Result<()> instead of bool, by deleting the trailing .is_ok() on the single write_all each wraps. The one call site that read the return value (Stream::flush_queue) gets .is_ok() appended so it computes the same bool. mordant-baseline.toml is regenerated, dropping the error_collapsed_to_bool:…h2_frame_parser.rs = 32 entry and two empty crate table headers.

Security risks

None. This is a return-type refactor in outbound HTTP/2 frame serialization; no input parsing, allocation sizing, auth, or crypto is touched, and no new error path is introduced.

Level of scrutiny

Low. The bytes written are byte-for-byte identical (only the return expression changed), and I grepped every call site in the file: all are let _ = … discards, which discard a Result exactly as they discarded the bool, so those sites compile and behave identically. The single non-discarding site now maps the same write_all outcome to the same bool via .is_ok(). This is the definition of a behavior-preserving lint cleanup.

Other factors

The full node-http2 test suite plus the h2 conformance/continuation/padding/rehash tests were run on the debug build and pass. The mordant CI job is the regression guard for the pattern returning. The baseline diff is minimal (one entry removed, two empty section headers dropped by the regenerator) and touches no other counts. No outstanding human review comments; the only timeline entries are bot status/rate-limit notices.