Skip to content

Remove dead code from react_compiler and the node:http2 frame parser - #37272

Open
robobun wants to merge 9 commits into
mainfrom
claude/farm/d4de2af7/dead-code-react-compiler-h2
Open

Remove dead code from react_compiler and the node:http2 frame parser#37272
robobun wants to merge 9 commits into
mainfrom
claude/farm/d4de2af7/dead-code-react-compiler-h2

Conversation

@robobun

@robobun robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Net -3,128 lines. Every deletion is a zero-reference symbol confirmed by a rustc --force-warn dead_code fixpoint on the touched crates (not just grep), plus a repo-wide reference search covering generated codegen output.

react_compiler (-1,139)

  • The experimental variant of the derived-computations-in-effects validation (validate_no_derived_computations_in_effects_exp + its 22 exclusive helpers/types, ~1.1k lines in one file). The pipeline only ever calls the non-exp version; the _exp env-config flag is parsed from fixture pragmas but read by nothing, so this was unreachable under every cfg combination. The fixture suite (3,329 tests, including the effect-derived-computations fixtures that set the _exp pragma) passes unchanged.
  • SymbolHost: a back-compat alias for a previous parser-hook API, zero users (DESIGN.md updated to name Host).
  • HirBox: arena-box alias with zero uses.

h2_frame_parser.rs (-2,171)

The pre-engine inbound frame-handling path. Since rewrite_read() routed all inbound bytes through the h2 Connection engine, these were transitively unreachable (private items, so the compiler verdict is exact):

  • 19 handler methods: handle_incomming_payload, handle_window_update_frame, handle_unknown_frame, handle_push_promise_frame, decode_header_block, handle_data_frame, handle_go_away_frame, handle_origin_frame, handle_altsvc_frame, handle_rst_stream_frame, handle_ping_frame, handle_priority_frame, handle_continuation_frame, finish_headers_end_stream, handle_headers_frame, handle_settings_frame, lookup_inbound_stream, read_bytes, dispatch_frame
  • Decode-direction wire helpers only they used: u32_from_bytes, SettingsFlags, UInt31WithReserved::{from, from_bytes}, StreamPriority::from, SettingsPayloadUnit::from, FullSettingsPayload::{update_with, write}, get_http2_common_string + its extern declaration, type HeaderValue, struct Payload
  • Helpers with no remaining callers: dispatch_with_3_extra, send_settings_ack, adjust_window_size, increment_window_size_if_needed
  • Write-only parser/stream state those handlers maintained: current_frame, remaining_length, expecting_continuation, preface_received_len, pending_header_block, pending_header_flags, is_waiting_more_headers, header_block_size, header_block_count, Stream.padding

exe_format (-2)

  • pe::Error::InsufficientSpace: never constructed.

Verification

  • rustc --force-warn dead_code reaches a warning-free fixpoint on bun_react_compiler, bun_runtime, bun_exe_format
  • cargo check on all 10 CI target triples: 10 ok
  • Full debug build
  • test/js/node/http2/node-http2.test.js: 347 pass / 0 fail; h2-conformance + continuation + invalid-padding: 77 pass
  • react-compiler.test.ts + react-compiler-fixtures.test.ts: 3,329 pass / 0 fail
  • test/internal/source-lints/: 78 pass, including the new guard test that keeps these symbols from reappearing

Noted but deliberately not removed

  • compile_result.rs logger-serialization types and the always-empty events/ordered_log fields: zero-referenced, but annotated (unreachable_pub reason) as staged for logger wiring.
  • ReactCompilerOptions placeholder fields (gating, ignore_use_no_forget, ...): write-only, but commented as placeholders for a future flag.
  • The C++ definition of JSC__JSGlobalObject__getHTTP2CommonString in ZigGlobalObject.cpp is now unreferenced from Rust; left for a follow-up since that file has pending changes in other open PRs.
  • ThreadSafeRefCount::release and the h2 outbound-encoder API: no production callers, but covered by unit tests and deliberately staged.

[review] gate passed · iteration 1 · 11 files touched

fails on main (without fix)
ASAN without fix: 3 failed, 320 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/transpiler/react-compiler-fixtures.test.ts "test/internal/source-lints/dead-symbols-react-compiler-h2.test.ts"
bun test v1.4.0 (0dad17989)

test/internal/source-lints/dead-symbols-react-compiler-h2.test.ts:
46 |     // Arena box alias with zero uses (HirVec is the one HIR actually uses).
47 |     ["src/react_compiler/hir/mod.rs", /\bHirBox\b/],
48 |     // Type predicate whose only callers were in the removed _exp validation.
49 |     ["src/react_compiler/hir/mod.rs", /\bis_use_state_type\b/],
50 |   ];
51 |   expect(resurrected(checks)).toEqual([]);
                                   ^
error: expect(received).toEqual(expected)

- []
+ [
+   "src/react_compiler/validation/validate_no_derived_computations_in_effects.rs: /validate_no_derived_computations_in_effects_exp/",
+   "src/react_compiler/hir/environment_config.rs: /validate_no_derived_computations_in_effects_exp/",
+   "src/react_compiler/program.rs: /validate_no_derived_computations_in_effects_exp/",
+   "src/react_compiler/program.rs: /\bSymbolHost\b/",
+   "src/re
... (truncated)

release without fix: 3 failed, 1146 skipped
bun test v1.4.0-canary.1 (3a845b7a5)

test/internal/source-lints/dead-symbols-react-compiler-h2.test.ts:
46 |     // Arena box alias with zero uses (HirVec is the one HIR actually uses).
47 |     ["src/react_compiler/hir/mod.rs", /\bHirBox\b/],
48 |     // Type predicate whose only callers were in the removed _exp validation.
49 |     ["src/react_compiler/hir/mod.rs", /\bis_use_state_type\b/],
50 |   ];
51 |   expect(resurrected(checks)).toEqual([]);
                                   ^
error: expect(received).toEqual(expected)

- []
+ [
+   "src/react_compiler/validation/validate_no_derived_computations_in_effects.rs: /validate_no_derived_computations_in_effects_exp/",
+   "src/react_compiler/hir/environment_config.rs: /validate_no_derived_computations_in_effects_exp/",
+   "src/react_compiler/program.rs: /validate_no_derived_computations_in_effects_exp/",
+   "src/react_compiler/program.rs: /\bSymbolHost\b/",
+   "src/react_compiler/lib.rs: /\bSymbolHost\b/",
+   "src/react_compiler/hir/mod.rs: /\bHirBox\b/",
+   "src/react_compiler/hir/mod.rs: /\bis_use_state_type\b/",
+ ]

- Expected  - 1
+ Received  + 9

      at <anonymous> (/workspace/bun/test/internal/source
... (truncated)
passes on PR (with fix)
ASAN with fix: 320 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/transpiler/react-compiler-fixtures.test.ts "test/internal/source-lints/dead-symbols-react-compiler-h2.test.ts"
bun test v1.4.0 (0dad17989)

test/internal/source-lints/dead-symbols-react-compiler-h2.test.ts:
(pass) dead react_compiler symbols do not reappear [20.51ms]
(pass) dead h2_frame_parser inbound-path symbols do not reappear [149.68ms]
(pass) dead exe_format symbols do not reappear [3.03ms]

test/bundler/transpiler/react-compiler-fixtures.test.ts:
(pass) react-compiler upstream fixtures > fixture corpus is present [2.25ms]
(pass) react-compiler upstream fixtures > alias-capture-in-method-receiver > minify.syntax [8.78ms]
(pass) react-compiler upstream fixtures > alias-capture-in-method-receiver-and-mutate > minify.syntax [0.73ms]
(pass) react-compiler upstream fixtures > alias-computed-load > minify.syntax [0.47ms]
(pass) react-compiler upstream fixtures > alias-nested-member-path > minify.syntax [0.49ms]
(pass) react-compiler upstream fixtures > alias-capture-in-method-receiver > compile [39.10ms]
(pass) react-compi
... (truncated)

release with fix: 1146 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 683ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/6] gen generated_host_exports.rs
generated_host_exports.rs: 93 exports (host=3, lazy=10, generic=80, rust=0); 239 extern-C blocks audited
[1/6] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
�[1m�[92m   Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
�[1m�[92m   Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys)
�[1m�[92m   Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys)
�[1m�[92m   Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd)
�[1m�[92m   Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp)
�[1m�[92m   Compiling�[0m bun_brotli v
... (truncated)
diff hotspot
src/exe_format/pe.rs                               |    2 -
 src/react_compiler/DESIGN.md                       |    4 +-
 src/react_compiler/hir/environment_config.rs       |    2 -
 src/react_compiler/hir/mod.rs                      |   11 +-
 src/react_compiler/lib.rs                          |    2 +-
 src/react_compiler/program.rs                      |    7 -
 .../validate_no_derived_computations_in_effects.rs | 1161 +---------
 src/runtime/api/bun/h2_frame_parser.rs             | 2350 +-------------------
 src/runtime/error.rs                               |    3 -
 .../transpiler/react-compiler-fixtures.test.ts     |    6 +-
 .../dead-symbols-react-compiler-h2.test.ts         |  120 +
 11 files changed, 230 insertions(+), 3438 deletions(-)

gate history · 7 passed · 0 rejected · iteration 1

evidence per changed file
file                                                      reads  edits  tests
src/exe_format/pe.rs                                          0      0      0
src/react_compiler/DESIGN.md                                  0      0      0
src/react_compiler/hir/environment_config.rs                  1      1      0
src/react_compiler/hir/mod.rs                                 2      5      0
src/react_compiler/lib.rs                                     1      2      0
src/react_compiler/program.rs                                 1      1      0
…lidation/validate_no_derived_computations_in_effects.rs      3      3      0
src/runtime/api/bun/h2_frame_parser.rs                        4     17      0
src/runtime/error.rs                                          1      1      0
test/bundler/transpiler/react-compiler-fixtures.test.ts       2      2      0
…nal/source-lints/dead-symbols-react-compiler-h2.test.ts      2      5      0

react_compiler:
- Delete the experimental variant of the derived-computations-in-effects
  validation (validate_no_derived_computations_in_effects_exp and its 22
  exclusive helpers, ~1.1k lines). The pipeline only ever invoked the
  non-exp version; the _exp pragma flag is parsed but never read, so
  behavior is unchanged (fixture suite passes identically).
- Delete the SymbolHost back-compat alias and the unused HirBox arena
  alias.

h2_frame_parser.rs:
- Delete the pre-engine inbound frame-handling path: the 19 handler
  methods (handle_*_frame, decode_header_block, read_bytes,
  dispatch_frame, ...), their wire-decoding helpers (u32_from_bytes,
  SettingsFlags, UInt31WithReserved::from/from_bytes,
  StreamPriority::from, SettingsPayloadUnit::from,
  FullSettingsPayload::update_with/write, get_http2_common_string and its
  extern declaration, HeaderValue, Payload), the event-dispatch and
  window-bookkeeping helpers only they called (dispatch_with_3_extra,
  send_settings_ack, adjust_window_size,
  increment_window_size_if_needed), and the write-only parser/stream
  state they maintained (current_frame, remaining_length,
  expecting_continuation, preface_received_len, pending_header_block,
  pending_header_flags, is_waiting_more_headers, header_block_size,
  header_block_count, padding). All inbound bytes go through the h2
  engine via rewrite_read.

exe_format:
- Delete pe::Error::InsufficientSpace, never constructed.

Verified with a rustc --force-warn dead_code fixpoint on the touched
crates, cargo check on all 10 target triples, a full debug build, and
the http2 and react-compiler test suites.
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The PR removes obsolete React Compiler APIs and experimental validation configuration. It deletes legacy HTTP/2 inbound parsing while retaining rewrite-engine stream handling. It removes obsolete runtime and PE error symbols and adds source-lint checks for deleted symbols.

Changes

React Compiler cleanup

Layer / File(s) Summary
Public API cleanup
src/react_compiler/DESIGN.md, src/react_compiler/hir/mod.rs, src/react_compiler/lib.rs, src/react_compiler/program.rs
Documentation now uses Host. The public SymbolHost, HirBox, and is_use_state_type APIs are removed.
Validator and fixture cleanup
src/react_compiler/hir/environment_config.rs, src/react_compiler/program.rs, src/react_compiler/validation/..., test/bundler/transpiler/react-compiler-fixtures.test.ts
Experimental validator configuration and fixture handling are removed. The retained validator uses renamed helpers and direct environment diagnostics. The experimental pragmas are recognized as ignored.

HTTP/2 inbound parser migration

Layer / File(s) Summary
Legacy inbound parsing removal
src/runtime/api/bun/h2_frame_parser.rs
Legacy frame decoding, buffering, HPACK decoding, settings mutation, continuation tracking, preface tracking, and related state are removed. Outbound serialization remains.
Rewrite-engine stream and socket integration
src/runtime/api/bun/h2_frame_parser.rs
Shared-reference helpers handle stream creation, JavaScript context registration, callback dispatch, and native socket writing.

Dead symbol guardrails

Layer / File(s) Summary
Removed-symbol source scans
test/internal/source-lints/dead-symbols-react-compiler-h2.test.ts, src/runtime/error.rs
Source-lint tests reject deleted React Compiler, HTTP/2 parser, PE error, and runtime error symbols. Error::UnableToDecode is removed from the runtime error enum.

Possibly related PRs

  • oven-sh/bun#36179: Uses the shared source-lint infrastructure for removed-symbol checks.
  • oven-sh/bun#36803: Removes obsolete runtime error symbols and adds related dead-symbol coverage.
  • oven-sh/bun#36872: Removes unused public APIs and adds dead-symbol coverage.

Suggested reviewers: jarred-sumner

🚥 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 summarizes the primary removal of dead code from the React compiler and HTTP/2 frame parser.
Description check ✅ Passed The description explains the changes and provides detailed verification results, despite not using the template headings exactly.

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

@github-actions github-actions Bot added the claude label Aug 9, 2026
Comment thread src/runtime/api/bun/h2_frame_parser.rs Outdated
Comment thread src/runtime/api/bun/h2_frame_parser.rs
Comment thread src/runtime/api/bun/h2_frame_parser.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 3512-3548: In the stream-start dispatch function containing
callback.call, arm the dispatch guard before invoking onStreamStart so
re-entrant read/rewrite_read cannot free the held stream pointer. After
callback.call returns, re-resolve stream_identifier through self.streams and
only update its context and return the live pointer when the entry still exists;
otherwise return the appropriate empty result, and remove the unsafe dereference
assumption that the map entry remains valid.
🪄 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: 4c086eef-aee5-46a2-8547-cd76b6e72b86

📥 Commits

Reviewing files that changed from the base of the PR and between d2f48c6 and c240452.

📒 Files selected for processing (8)
  • src/exe_format/pe.rs
  • src/react_compiler/DESIGN.md
  • src/react_compiler/hir/mod.rs
  • src/react_compiler/lib.rs
  • src/react_compiler/program.rs
  • src/react_compiler/validation/validate_no_derived_computations_in_effects.rs
  • src/runtime/api/bun/h2_frame_parser.rs
  • test/internal/source-lints/dead-symbols-react-compiler-h2.test.ts
💤 Files with no reviewable changes (2)
  • src/exe_format/pe.rs
  • src/react_compiler/program.rs

Comment thread src/runtime/api/bun/h2_frame_parser.rs
Comment thread src/runtime/api/bun/h2_frame_parser.rs Outdated
- read_buffer: write-only after the inbound path removal (init and
  drop-and-replace in detach, never read or appended)
- SettingsPayloadUnit: the parser's own copy lost its last consumer;
  the h2 client uses the separate bun_http_types::h2 type
- bytemuck Zeroable/Pod impls for FullSettingsPayload: only consumer
  was the removed write()
- Update comments that named deleted functions
Comment thread src/react_compiler/validation/validate_no_derived_computations_in_effects.rs Outdated
With the experimental variant gone there is only one implementation, so
the "Non-experimental version" doc line and the _non_exp helper
suffixes no longer distinguish anything.
Comment thread src/runtime/api/bun/h2_frame_parser.rs
…nting

- EnvironmentConfig.validate_no_derived_computations_in_effects_exp and
  its pragma arm: parsed but read by nothing (unknown pragmas fall into
  the catch-all, so fixtures are unaffected)
- H2FrameParser.used_window_size and Stream.used_window_size: their only
  writers were the removed inbound handlers, so the three read sites
  always saw 0; drop the fields and the vacuous guards
- Drop dead_code from the file-level allow now that the legacy inbound
  half it suppressed is gone; verified warning-free on all 10 targets
- Update two comments that contrasted with the removed handlers

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline nit, I verified 705bdeb addressed the prior round: both used_window_size fields are gone and only the actively-written remote_used_window_size remains (the guard test's \b boundary correctly excludes it); the three read sites were vacuous at 0 so effectiveRecvDataLengthwindow_size and the dropped throw guard are behavior-preserving; dead_code is dropped from the file-level #[allow]; and the _exp config field + pragma arm are removed with the guard test extended to environment_config.rs/program.rs.

Extended reasoning...

Confirmed the fixes from 705bdeb against the concerns I raised at 17:03: grep of h2_frame_parser.rs shows no used_window_size occurrences outside remote_used_window_size (which is still written at L1649/1698/5439 and read at L1591/1598/5382), the file-level allow at L2 no longer lists dead_code, and effectiveRecvDataLength at L4683 now reports window_size alone. The remaining inline finding is the fixture-harness HANDLED_PRAGMAS mirror table — nit-level, no coverage loss. Not approving given the scale (-3.1k lines across node:http2 runtime and react_compiler); a human should sign off on the deletion set.

Comment thread src/react_compiler/program.rs
parse_fixture_pragmas no longer applies them, so the handled table's
contract (read and applied) was stale; ignored (parsed, applies
nothing) is now the accurate bucket. Deleting the entries instead
would make shouldSkip drop every fixture carrying the pragma.
@robobun

robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:24 PM PT - Aug 9th, 2026

@robobun, your commit 0dad179 has 3 failures in Build #91043 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37272

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

bun-37272 --bun

Comment thread src/react_compiler/hir/mod.rs
Comment thread src/runtime/api/bun/h2_frame_parser.rs Outdated
- FullSettingsPayload: the removed write() and bytemuck impls were the
  last byte-layout consumers, so drop repr(C, packed), the seven _type
  tag fields, BYTE_SIZE, the size assert, and the SettingsType newtype
  that only the tag fields referenced; every remaining use reads named
  fields by value
- Error::UnableToDecode: sole constructor was the removed decode()
- is_use_state_type: only callers were in the removed _exp validation
- Drop two comments describing the removed legacy frame state

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR across six passes and the bug-hunting system found nothing on the current head (3a845b7). All prior findings — stale comments, write-only fields, orphaned pub items that escaped the dead_code lint — were addressed. Given the scale (-3.1k lines) and that ~2.2k of it is the node:http2 frame parser, a human sign-off on the h2 deletions is still worthwhile.

What was reviewed:

  • Confirmed the removed h2 inbound handlers, wire helpers, and parser state are unreachable now that rewrite_read() routes everything through the h2 engine; the three simplified used_window_size read sites were vacuous (field was pinned at 0).
  • Checked FullSettingsPayload after dropping repr(packed) / _*_type fields / BYTE_SIZE — remaining users (to_js, to_engine_settings, write_settings_payload) read named fields only; the two struct literals are exhaustive without ..Default::default().
  • Verified Error::UnableToDecode, is_use_state_type, SymbolHost, HirBox, and the _exp config field/pragma arm have no remaining references; the fixture harness's IGNORED_PRAGMAS move keeps the effect-derived-computations fixtures running.
Extended reasoning...

Overview

Pure dead-code removal across three crates: bun_react_compiler (the ~1.1k-line experimental _exp derived-computations validation and its 22 exclusive helpers, plus the SymbolHost alias, HirBox, is_use_state_type, and the write-only _exp config flag), bun_runtime's h2_frame_parser.rs (the ~2.2k-line pre-engine inbound frame path — 19 handlers, decode-direction wire helpers, write-only parser/stream state, and the FullSettingsPayload wire-layout artifacts), and bun_exe_format (pe::Error::InsufficientSpace). Also removes Error::UnableToDecode from runtime/error.rs, updates DESIGN.md and the fixture harness's pragma tables, and adds a source-lint guard test.

Security risks

None. Deletions only; the removed h2 inbound handlers were already unreachable (all inbound bytes go through crate::api::h2::Connection since rewrite_read() landed). No auth, crypto, or validation logic is weakened.

Level of scrutiny

High — production node:http2 networking code, and some deletions simplified live read sites (setLocalWindowSize's throw guard, the per-stream continue, the effectiveRecvDataLength stat) based on the removed field being pinned at 0. I traced each: adjust_window_size (only incrementer) and increment_window_size_if_needed (only resetter) were themselves only reachable from the dead inbound handlers, so the field was already always 0 in practice before this PR. The engine handles its own inbound flow-control accounting.

Other factors

This PR has been through five prior review iterations; every finding was a completeness issue (stale comments, pub-item escapees from the dead_code fixpoint, mirror-table drift), not a correctness bug, and each was addressed in a follow-up commit with re-verification. Test coverage is strong: node-http2 (347 pass), h2-conformance/continuation/invalid-padding (77 pass), react-compiler fixtures (3,329 pass), all 10 CI target triples check clean, and --force-warn dead_code reaches a fixpoint. The remaining reason for a human look is scale: -3.1k lines from a single production networking file is the kind of change a maintainer should acknowledge even when mechanically verified.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed this across several rounds and have no remaining findings — every earlier comment (stale references, write-only _exp config/used_window_size/read_buffer state, is_use_state_type, the FullSettingsPayload wire-layout half, UnableToDecode, the HANDLED_PRAGMAS mirror) has been addressed. Given the scale (-3.1k lines) and that it removes the entire legacy inbound path from the production node:http2 frame parser, a human sign-off would still be worthwhile.

What was reviewed:

  • Confirmed the removed h2 handlers/state are unreachable via rewrite_read() → engine routing; FLOW_CONTROL_ERROR and the removed ErrorCode constant have no remaining users in this file.
  • Verified the three simplified used_window_size read sites (throw guard, per-stream continue, effectiveRecvDataLength) are behaviorally identical with the field pinned at 0.
  • Checked the FullSettingsPayload literal sites after dropping ..Default::default() — both now enumerate all seven fields.
  • Confirmed the IGNORED_PRAGMAS move keeps the effect-derived-computations fixtures running (fallthrough would skip).
Extended reasoning...

Overview

Net -3,128 lines of dead code across three crates, plus a new source-lint guard test. The bulk is (1) the ~1.1k-line experimental _exp variant of the derived-computations-in-effects validation in react_compiler (never invoked by the pipeline), and (2) the ~2.2k-line legacy inbound frame-handling path in h2_frame_parser.rs (unreachable since rewrite_read() routes all inbound bytes through the crate::api::h2::Connection engine). Small orphans: SymbolHost/HirBox/is_use_state_type (react_compiler), pe::Error::InsufficientSpace, bun_runtime::Error::UnableToDecode, and the FullSettingsPayload wire-layout artifacts (#[repr(C, packed)], _*_type tag fields, SettingsType, bytemuck impls). The fixture harness moves two now-ignored pragma spellings from HANDLED_PRAGMAS to IGNORED_PRAGMAS.

Security risks

None identified. This is deletion of unreachable code; no new input parsing, no new trust boundaries. The removed h2 inbound handlers were security-relevant when live, but their replacement (the h2 engine) is already what serves all traffic — this PR does not change which code parses untrusted network bytes.

Level of scrutiny

High. h2_frame_parser.rs is production-critical for node:http2 compatibility, and the deletion is large enough that a subtle live path could be missed. The author's methodology (rustc --force-warn dead_code fixpoint on private items + repo-wide grep for pub escapes) is sound, and five prior review rounds surfaced the lint's blind spots (write-only pub fields, _-prefixed fields, stale comments, mirror tables) which were each addressed. I re-checked the three non-pure-deletion sites — the used_window_size read simplifications, the two FullSettingsPayload struct literals, and the effectiveRecvDataLength stat — and all preserve behavior exactly.

Other factors

Verification is thorough: 10-target cargo check, http2 suites (347 + 77 pass), react-compiler fixture suite (3,329 pass), and a new guard test that fails on main. All prior review threads are resolved. The C++ getHTTP2CommonString definition is deliberately left for a follow-up (noted in the PR description). Deferring solely because a -3.1k-line removal from a production protocol handler warrants a maintainer's sign-off, not because of any open concern.

@robobun

robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

CI status for the red builds (91038, 91043): every failure is either marked also-failing-on-main (node-http2 maxSessionMemory timeout, cli/test/parallel 64MB-line timeout, napi reference-unref, all on the darwin 14 x64 lane) or a retry-passing flake on other lanes. None touch this diff: the http2 test passed these same lanes on builds 90994 and 91005 with the identical deletions, passes locally under the ASAN debug build, and is now red on main without this change. The main-side failures have been reported for triage separately. The diff itself is green: all 10 target triples check clean, the http2, react-compiler, and source-lint suites pass, and both automated reviews closed with no remaining findings.

Jarred-Sumner pushed a commit that referenced this pull request Aug 9, 2026
…37273)

### Problem

`H2FrameParser::handle_received_stream_id` creates a `Stream` box,
inserts it into the stream map, and then invokes the JS `streamStart`
callback directly via `callback.call` without arming the
`DispatchGuard`, while still holding the raw `*mut Stream`. Every other
JS dispatch site in the parser arms the guard, because `rewrite_read`
frees streams queued in `pending_engine_stream_closes` only at dispatch
depth 0.

JS reached from inside that callback (the `Http2Stream` constructor
calls `this.on("pause", ...)`, so a patched `EventEmitter.prototype.on`
runs there; the handler also calls back into native `rstStream` for
refused streams) can close the just-created stream, queueing its
deferred free, and then re-enter `parser.read()` at depth 0. The drain
frees the box, and the callback return path writes the stream context
through the dangling pointer:

```
==ERROR: AddressSanitizer: heap-use-after-free ...
    #1 <bun_runtime::api::h2_frame_parser_body::Stream>::set_context src/runtime/api/bun/h2_frame_parser.rs:2110
    #2 <...H2FrameParser>::handle_received_stream_id src/runtime/api/bun/h2_frame_parser.rs:5372
    #3 <...H2FrameParser>::get_next_stream src/runtime/api/bun/h2_frame_parser.rs:8335
freed by:
   #12 <...H2FrameParser>::rewrite_read::{closure#3} src/runtime/api/bun/h2_frame_parser.rs:5804
```

The callers that keep dereferencing the returned pointer (`request()`,
`get_next_stream`, the engine HEADERS path) were exposed to the same
freed box.

### Fix

Arm `enter_dispatch` across the callback, matching the invariant
documented on `enter_dispatch` (every section that holds a `Stream`
pointer while user JS can run must arm the guard). With the guard armed,
the deferred-close drain cannot run while the callback executes, so the
pointer stays valid for `set_context` and for the callers.

Also skip the context install when the callback closed the stream:
`free_resources` already dropped its `sctx` root, and re-inserting one
afterwards would pin the dead JS stream object until the session dies.

This is the guard-arming fix for the pre-existing issue flagged during
review of #37272 (that PR only removes dead code around it).

### Verification

New test in `test/js/node/http2/node-http2-streams-rehash.test.ts` (the
file covering this class of reentrancy bugs) reproduces the exact
sequence: close the new stream and re-enter `read()` from inside the
`streamStart` callback. Without the fix it fails on every build tier:
heap-use-after-free under the ASAN debug build, and on release builds
`getStreamContext(2)` throws "Invalid stream id" because the drain
already freed the entry inside the callback. With the fix the entry
survives the callback with no context installed (covering the
skip-install branch), and a follow-up depth-0 `read()` asserts the
deferred close then actually drains. Existing http2 suites
(`node-http2.test.js`, `h2-conformance.test.ts`, the staged h2 tests,
node's server-push parallel tests) pass with the change.

<!-- robobun:evidence:begin -->

---

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

<details><summary>fails on main (without fix)</summary>

```console
ASAN without fix: 1 FAILED
$ 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-streams-rehash.test.ts"
bun test v1.4.0 (8f79562)

test/js/node/http2/node-http2-streams-rehash.test.ts:
(pass) session.request() from a stream 'timeout' listener during forEachStream does not UAF on hashmap rehash [3284.09ms]
(pass) http2 client request() does not hold *Stream across user-controlled options getters [6184.76ms]
198 |       env: bunEnv,
199 |       stdout: "pipe",
200 |       stderr: "pipe",
201 |     });
202 |     const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
203 |     expect({ stdout: stdout.trim(), exitCode, stderr }).toMatchObject({ stdout: "OK", exitCode: 0 });
                                                              ^
error: expect(received).toMatchObject(expected)

  {
-   "exitCode": 0,
-   "stdout": "OK",
+   "exitCode": 1,
+   "stderr": 
+ "=================================================================
+ ==101685==ERROR: AddressSanitizer: heap-use-after-free on address 0x79be9bb005c0 at pc 0x00000e7ec22e bp 
... (truncated)

release without fix: all passed
bun test v1.4.0-canary.1 (7725ac8)

test/js/node/http2/node-http2-streams-rehash.test.ts:
(pass) session.request() from a stream 'timeout' listener during forEachStream does not UAF on hashmap rehash [163.99ms]
(pass) http2 client request() does not hold *Stream across user-controlled options getters [78.42ms]
(pass) closing the new stream and re-entering read() inside the streamStart callback does not UAF [31.29ms]
(pass) http2 client write callback that opens new streams during flushQueue does not UAF [49.40ms]
(pass) DeferredTaskQueue::run tolerates an on_auto_flush callback that unregisters itself and returns true [46.51ms]

 5 pass
 0 fail
 5 expect() calls
Ran 5 tests across 1 file. [513.00ms]
__F:0:S:0
```

</details>

<details><summary>passes on PR (with fix)</summary>

```console
ASAN with fix: all passed
$ 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-streams-rehash.test.ts"
bun test v1.4.0 (8f79562)

test/js/node/http2/node-http2-streams-rehash.test.ts:
(pass) session.request() from a stream 'timeout' listener during forEachStream does not UAF on hashmap rehash [3278.34ms]
(pass) http2 client request() does not hold *Stream across user-controlled options getters [6171.66ms]
(pass) closing the new stream and re-entering read() inside the streamStart callback does not UAF [1906.06ms]
(pass) http2 client write callback that opens new streams during flushQueue does not UAF [2819.28ms]
(pass) DeferredTaskQueue::run tolerates an on_auto_flush callback that unregisters itself and returns true [2618.43ms]

 5 pass
 0 fail
 5 expect() calls
Ran 5 tests across 1 file. [19.19s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 689ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/6] gen generated_host_exports.rs
generated_host_exports.rs: 93 exports (host=3, lazy=10, generic=80, rust=0); 239 extern-C blocks audited
[1/6] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
�[1m�[92m   Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
�[1m�[92m   Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys)
�[1m�[92m   Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys)
�[1m�[92m   Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd)
�[1m�[92m   Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp)
�[1m�[92m   Compiling�[0m bun_brotli v
... (truncated)
```

</details>

<details><summary>diff hotspot</summary>

```
src/runtime/api/bun/h2_frame_parser.rs             |  19 +++-
 .../node/http2/node-http2-streams-rehash.test.ts   | 100 +++++++++++++++++++++
 2 files changed, 115 insertions(+), 4 deletions(-)
```

</details>

**gate history** · 2 passed · 0 rejected · iteration 1

<details><summary>evidence per changed file</summary>

```
file                                                  reads  edits  tests
src/runtime/api/bun/h2_frame_parser.rs                   10      4      0
test/js/node/http2/node-http2-streams-rehash.test.ts      2      3      0
```

</details>

<!-- robobun:evidence:end -->

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
springmin pushed a commit to springmin/bun that referenced this pull request Aug 10, 2026
…ven-sh#37273)

### Problem

`H2FrameParser::handle_received_stream_id` creates a `Stream` box,
inserts it into the stream map, and then invokes the JS `streamStart`
callback directly via `callback.call` without arming the
`DispatchGuard`, while still holding the raw `*mut Stream`. Every other
JS dispatch site in the parser arms the guard, because `rewrite_read`
frees streams queued in `pending_engine_stream_closes` only at dispatch
depth 0.

JS reached from inside that callback (the `Http2Stream` constructor
calls `this.on("pause", ...)`, so a patched `EventEmitter.prototype.on`
runs there; the handler also calls back into native `rstStream` for
refused streams) can close the just-created stream, queueing its
deferred free, and then re-enter `parser.read()` at depth 0. The drain
frees the box, and the callback return path writes the stream context
through the dangling pointer:

```
==ERROR: AddressSanitizer: heap-use-after-free ...
    #1 <bun_runtime::api::h2_frame_parser_body::Stream>::set_context src/runtime/api/bun/h2_frame_parser.rs:2110
    #2 <...H2FrameParser>::handle_received_stream_id src/runtime/api/bun/h2_frame_parser.rs:5372
    #3 <...H2FrameParser>::get_next_stream src/runtime/api/bun/h2_frame_parser.rs:8335
freed by:
   #12 <...H2FrameParser>::rewrite_read::{closure#3} src/runtime/api/bun/h2_frame_parser.rs:5804
```

The callers that keep dereferencing the returned pointer (`request()`,
`get_next_stream`, the engine HEADERS path) were exposed to the same
freed box.

### Fix

Arm `enter_dispatch` across the callback, matching the invariant
documented on `enter_dispatch` (every section that holds a `Stream`
pointer while user JS can run must arm the guard). With the guard armed,
the deferred-close drain cannot run while the callback executes, so the
pointer stays valid for `set_context` and for the callers.

Also skip the context install when the callback closed the stream:
`free_resources` already dropped its `sctx` root, and re-inserting one
afterwards would pin the dead JS stream object until the session dies.

This is the guard-arming fix for the pre-existing issue flagged during
review of oven-sh#37272 (that PR only removes dead code around it).

### Verification

New test in `test/js/node/http2/node-http2-streams-rehash.test.ts` (the
file covering this class of reentrancy bugs) reproduces the exact
sequence: close the new stream and re-enter `read()` from inside the
`streamStart` callback. Without the fix it fails on every build tier:
heap-use-after-free under the ASAN debug build, and on release builds
`getStreamContext(2)` throws "Invalid stream id" because the drain
already freed the entry inside the callback. With the fix the entry
survives the callback with no context installed (covering the
skip-install branch), and a follow-up depth-0 `read()` asserts the
deferred close then actually drains. Existing http2 suites
(`node-http2.test.js`, `h2-conformance.test.ts`, the staged h2 tests,
node's server-push parallel tests) pass with the change.

<!-- robobun:evidence:begin -->

---

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

<details><summary>fails on main (without fix)</summary>

```console
ASAN without fix: 1 FAILED
$ 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-streams-rehash.test.ts"
bun test v1.4.0 (8f79562)

test/js/node/http2/node-http2-streams-rehash.test.ts:
(pass) session.request() from a stream 'timeout' listener during forEachStream does not UAF on hashmap rehash [3284.09ms]
(pass) http2 client request() does not hold *Stream across user-controlled options getters [6184.76ms]
198 |       env: bunEnv,
199 |       stdout: "pipe",
200 |       stderr: "pipe",
201 |     });
202 |     const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
203 |     expect({ stdout: stdout.trim(), exitCode, stderr }).toMatchObject({ stdout: "OK", exitCode: 0 });
                                                              ^
error: expect(received).toMatchObject(expected)

  {
-   "exitCode": 0,
-   "stdout": "OK",
+   "exitCode": 1,
+   "stderr": 
+ "=================================================================
+ ==101685==ERROR: AddressSanitizer: heap-use-after-free on address 0x79be9bb005c0 at pc 0x00000e7ec22e bp 
... (truncated)

release without fix: all passed
bun test v1.4.0-canary.1 (7725ac8)

test/js/node/http2/node-http2-streams-rehash.test.ts:
(pass) session.request() from a stream 'timeout' listener during forEachStream does not UAF on hashmap rehash [163.99ms]
(pass) http2 client request() does not hold *Stream across user-controlled options getters [78.42ms]
(pass) closing the new stream and re-entering read() inside the streamStart callback does not UAF [31.29ms]
(pass) http2 client write callback that opens new streams during flushQueue does not UAF [49.40ms]
(pass) DeferredTaskQueue::run tolerates an on_auto_flush callback that unregisters itself and returns true [46.51ms]

 5 pass
 0 fail
 5 expect() calls
Ran 5 tests across 1 file. [513.00ms]
__F:0:S:0
```

</details>

<details><summary>passes on PR (with fix)</summary>

```console
ASAN with fix: all passed
$ 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-streams-rehash.test.ts"
bun test v1.4.0 (8f79562)

test/js/node/http2/node-http2-streams-rehash.test.ts:
(pass) session.request() from a stream 'timeout' listener during forEachStream does not UAF on hashmap rehash [3278.34ms]
(pass) http2 client request() does not hold *Stream across user-controlled options getters [6171.66ms]
(pass) closing the new stream and re-entering read() inside the streamStart callback does not UAF [1906.06ms]
(pass) http2 client write callback that opens new streams during flushQueue does not UAF [2819.28ms]
(pass) DeferredTaskQueue::run tolerates an on_auto_flush callback that unregisters itself and returns true [2618.43ms]

 5 pass
 0 fail
 5 expect() calls
Ran 5 tests across 1 file. [19.19s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 689ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/6] gen generated_host_exports.rs
generated_host_exports.rs: 93 exports (host=3, lazy=10, generic=80, rust=0); 239 extern-C blocks audited
[1/6] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
�[1m�[92m   Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
�[1m�[92m   Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys)
�[1m�[92m   Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys)
�[1m�[92m   Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd)
�[1m�[92m   Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp)
�[1m�[92m   Compiling�[0m bun_brotli v
... (truncated)
```

</details>

<details><summary>diff hotspot</summary>

```
src/runtime/api/bun/h2_frame_parser.rs             |  19 +++-
 .../node/http2/node-http2-streams-rehash.test.ts   | 100 +++++++++++++++++++++
 2 files changed, 115 insertions(+), 4 deletions(-)
```

</details>

**gate history** · 2 passed · 0 rejected · iteration 1

<details><summary>evidence per changed file</summary>

```
file                                                  reads  edits  tests
src/runtime/api/bun/h2_frame_parser.rs                   10      4      0
test/js/node/http2/node-http2-streams-rehash.test.ts      2      3      0
```

</details>

<!-- robobun:evidence:end -->

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants