Repository navigation
Conversation
WalkthroughHTTP/2 HPACK settings handling now updates decoder capacity through new wrapper plumbing, tracks full SETTINGS snapshots for ACK processing, adjusts outbound HPACK size updates, and adds conformance coverage for decoder growth and encoder shrinking. ChangesHTTP/2 HPACK SETTINGS capacity handling
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:00 PM PT - Jul 9th, 2026
❌ @robobun, your commit 223e492 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32775That installs a local version of the PR into your bun-32775 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Two of the automated comments above needed verification, so for the record: #19152 is real, but was not fixed by the first commit here. The find-issues match pointed at the encoder side of the remote peer's This PR is not a duplicate of #32488. That PR widens the same per-6.5.3 ack queue, but it does so for |
There was a problem hiding this comment.
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 2205-2212: The HPACK size update handling in h2_frame_parser’s
header encoding path is being cleared too early by take_pending_size_update()
before encode() succeeds. Update the header block generation flow so the pending
size update stays armed until the encoded_headers block is actually produced,
and if encode() fails, restore or requeue the pending update instead of
committing it. Prefer fixing this in the Coder/HPACK state management or in the
header assembly logic around encoded_headers so pending_enc_capacity is only
consumed after successful serialization.
In `@test/js/node/http2/h2-conformance.test.ts`:
- Around line 697-715: The test in h2-conformance.test.ts currently only
verifies that a dynamic table size update to NEW_TABLE is accepted after the
peer ACKs the grown SETTINGS, but it misses the pre-ACK boundary case. In the
same test around serverThatThenCalls, RawH2.connect, and dynamicTableSizeUpdate,
add a small negative step before sending the SETTINGS ACK that attempts
dynamicTableSizeUpdate(NEW_TABLE) and asserts it is not accepted until the ACK
arrives, then keep the existing successful post-ACK path unchanged.
🪄 Autofix (Beta)
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: 7539e91d-03d2-4e17-9b04-1d374effad83
📒 Files selected for processing (6)
src/http/lshpack.rssrc/jsc/bindings/c-bindings.cppsrc/runtime/api/bun/h2/connection.rssrc/runtime/api/bun/h2/hpack.rssrc/runtime/api/bun/h2_frame_parser.rstest/js/node/http2/h2-conformance.test.ts
There was a problem hiding this comment.
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 `@test/js/node/http2/h2-conformance.test.ts`:
- Line 799: Bind the test helper server to the same loopback address used by
RawH2.connect() so the client and server always match. Update the h2-conformance
test setup around h2server.listen() to follow the existing RawH2Server.listen()
pattern in neighboring tests, explicitly using the loopback address instead of
leaving it unspecified to avoid IPv4/IPv6 binding differences.
🪄 Autofix (Beta)
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: cb55d1e7-3b57-43ad-9ae5-ddeb6ef4cbac
📒 Files selected for processing (2)
src/runtime/api/bun/h2/hpack.rstest/js/node/http2/h2-conformance.test.ts
|
Status for a maintainer: the diff is complete and gate-verified on the rebased base, every review thread is resolved, and CI is green except for two recurring unrelated flakes. I have used the one Rebase Rebased onto main ( Verification of the diff itself Against a debug build with What CI failed on (build 71207, final: 228/230 test lanes passed)
No annotation on either build 71173 or 71207 names any http2 or HPACK file. The retrigger cleared the other two unrelated failures from build 71173 ( If anything that does touch this change turns up I will pick it up. |
|
Independent corroboration of the encoder half of this change: a fresh report arrived of the same failure mode (a Reproduced deterministically on current main (3f5d816) using a real The client direction fails the same way (Bun client vs a node server that advertises I put a minimal encoder-only variant with nghttp2-oracle tests (server and client, across This PR currently has merge conflicts with main. |
…LE_SIZE is ACKed
The SETTINGS_HEADER_TABLE_SIZE a node:http2 session advertises bounds
the peer encoder's RFC 7541 6.3 Dynamic Table Size Update, but
lshpack_dec_set_max_capacity was only ever called from
lshpack_wrapper_init. A session.settings({ headerTableSize }) issued
after the connection was up therefore advertised the larger table
without applying it to the decoder, so a conforming peer that ACKed the
SETTINGS and opened its next header block with a size update up to that
maximum was torn down with GOAWAY(COMPRESSION_ERROR) and an uncaught
ERR_HTTP2_SESSION_ERROR. Node accepts the same sequence.
Add the decoder-side wrapper next to the existing encoder one, and
resize the decoder from handle_settings when the peer ACKs our SETTINGS
(6.5.3: the ACK binds the oldest outstanding submission), mirroring the
existing encoder handling for the peer's settings 40 lines below. The
per-SETTINGS ack queue is widened from the initial window size to the
full Settings snapshot so header_table_size is attributed to the right
submission when several SETTINGS are in flight.
…encoder The encoder mirror of the previous commit. The live outbound encoder is the H2FrameParser's own HpackHandle (the engine's encoder path is only reached from its unit tests), and set_encoder_max_capacity had no callers there, so a peer that lowered SETTINGS_HEADER_TABLE_SIZE was ignored: responses kept referencing the dynamic table and never carried the RFC 7541 6.3 Dynamic Table Size Update the change requires. nginx advertises 0 to every h2 upstream and rejects the result with "upstream sent invalid http2 table index" (#19152); a strict nghttp2 decoder rejects the whole block because the mandatory size update is missing. Replace the parser's raw HpackHandle with the engine's hpack::Coder, which already carries the queued-capacity state and the 6.3 opcode writer: on_remote_settings queues the peer's value, and encode_header_into_list (the single funnel every outbound block's fields go through) drains it at the start of the block. The encoder is also now initialized to the 4096 RFC default the peer is bound by, instead of OUR advertised headerTableSize (which governs the decoder, owned by the engine). Also pass the ACKed settings snapshot rather than the latest submission to on_local_settings, so `session.localSettings` and the `localSettings` event report the values the peer actually acknowledged when several SETTINGS are in flight, matching nghttp2_session_update_local_settings. Fixes #19152
…final value RFC 7541 4.2: when the peer changes SETTINGS_HEADER_TABLE_SIZE more than once between two of our header blocks, the SMALLEST value in that interval must be signaled (as the first of at most two Dynamic Table Size Updates), because the peer's decoder evicts eagerly at each of its SETTINGS. Coder kept only the latest queued value, so a peer sending HEADER_TABLE_SIZE=0 then 4096 got a single size-update(4096): its decoder's table is empty while ours still has entries, and a strict nghttp2 inflater rejects the block outright because the first size update after a shrink must not exceed the interval minimum. Track the pending minimum alongside the pending value and, when it is below the final value, apply and emit it first. MAX_SIZE_UPDATE_BYTES grows to cover the two opcodes a block may now open with; both callers size their scratch from it. This was latent in the engine's Coder but only became reachable on the wire once the previous commit routed the live outbound encoder through it.
RawH2.connect always dials 127.0.0.1; match the explicit loopback bind the file's RawH2Server.listen helper already uses, so the client and server can never end up on different address families.
790cf80 to
87cb0aa
Compare
Build 71173: 226/230 test lanes passed. The 4 failures are all unrelated to this change (test/js/sql/postgres-binary-array-bounds: Postgres connection refused on one Windows box; test/js/bun/http/proxy-stress-concurrent: 1/1200 requests failed on darwin; test/regression/issue/26030: MySQL docker harness; test/integration/next-pages/dev-server: Next.js/Puppeteer integration). No annotation names any http2/HPACK file.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-09, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#19152) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Fixes #19152
HTTP/2's
SETTINGS_HEADER_TABLE_SIZEhas two independent effects, andnode:http2applied neither after the connection was up:Both HPACK coder capacities were only ever set at parser construction. Neither
fetch()'s HTTP/2 client nor anything else is affected (it never changes its advertised table size after the preface).1. Local SETTINGS (decoder side)
A server (or client) that calls
session.settings({ headerTableSize })after the connection is established advertises the new maximum but never applies it to its decoder. A conforming peer that ACKs the SETTINGS and opens its next header block with a size update up to that maximum then kills the session:Node v26.3.0 accepts the same byte sequence. So a client that honours the SETTINGS the Bun server itself sent (nghttp2-based clients, gRPC, browsers behind a proxy, Bun's own
fetchh2 client) takes down the session, and the resulting session error is uncaught unless the app installed a handler. PassingheaderTableSizein the constructor options already worked, because the engine is created with that value; only post-constructionsettings()was broken.Cause:
lshpack_dec_set_max_capacityis only called fromlshpack_wrapper_init(src/jsc/bindings/c-bindings.cpp). The ACK branch ofhandle_settingsinsrc/runtime/api/bun/h2/connection.rsnever touches the decoder, so lshpack rejects the peer's size update (new_capacity > dec->hpd_max_capacityinvendor/lshpack/lshpack.c) andfinish_header_blockmaps that tosend_go_away(CompressionError, "HPACK decode error").2. Remote SETTINGS (encoder side, #19152)
A client that advertises
SETTINGS_HEADER_TABLE_SIZE=0(nginx sends this to every h2 upstream) must never be sent a dynamic-table reference, and the first header block after the change must open with a 6.3 size update. Bun does neither: its second response already references dynamic indexes, which nginx rejects:Node v26.3.0 emits the size update and zero dynamic indexes against the same client. A strict nghttp2 decoder (a node client doing
client.settings({ headerTableSize: <smaller> })) rejects the whole block because the mandatory size update is missing.Cause: the live outbound encoder is the
H2FrameParser's ownHpackHandle; the engine's encoder-side handling (Connection::begin_header_block/take_pending_size_update) is only reached from its unit tests, andset_encoder_max_capacityhad no callers on the live handle.Sink::on_remote_settingsbridged the peer's settings into aCellbut never resized the encoder. The encoder was also initialized from OUR advertisedheaderTableSizeinstead of the 4096 default the peer is actually bound by until it receives our SETTINGS, a latent desync with any non-default setting.Fix
src/jsc/bindings/c-bindings.cpp/src/http/lshpack.rs: addlshpack_wrapper_dec_set_max_capacity, the decoder counterpart of the existing encoder wrapper.src/runtime/api/bun/h2/hpack.rs:Coder::set_decoder_capacity, a no-op when unchanged so re-submitting the same size (Bun packs all 7 standard parameters into every SETTINGS frame) cannot clobber the table size the peer's encoder last chose via a size update.src/runtime/api/bun/h2/hpack.rs:Coderalso now tracks the MINIMUM capacity the peer requested since the last header block. RFC 7541 4.2 requires the interval minimum to be signaled before the final value (at most two size updates per block), because the peer's decoder evicts eagerly at each of its SETTINGS; signaling only the latest value left its table smaller than ours, and a strict nghttp2 inflater rejects a first size update above the interval minimum outright.src/runtime/api/bun/h2/connection.rs: resize the decoder from the SETTINGS ACK branch ofhandle_settings. Applying on ACK rather than on send matches nghttp2 (nghttp2_session_update_local_settings): the peer applies our SETTINGS before emitting the ACK, so every header block after the ACK on the wire is encoded under the new limit and none before it is. The per-6.5.3 ack queue previously carried onlyinitial_window_size; it now carries the fullSettingssnapshot soheader_table_sizeis attributed to the correct submission when several SETTINGS are in flight, the same way the window already was.src/runtime/api/bun/h2_frame_parser.rs: replace the rawHpackHandlewith the engine'shpack::Coder, which already carries the queued-capacity state and the 6.3 opcode writer.on_remote_settingsqueues the peer's value, andencode_header_into_list(the single funnel every outbound header block's fields go through) drains it at the start of the block. The encoder now initializes to the 4096 RFC default.on_local_settingsnow receives the ACKed snapshot rather than the latest submission, sosession.localSettingsand thelocalSettingsevent report the values the peer actually acknowledged (what Node reports) when several SETTINGS are in flight.Verification
Four tests in
test/js/node/http2/h2-conformance.test.ts, each driving a raw byte-level h2c client against a Bunnode:http2server:headerTableSizeto 8192 mid-session and the client ACKs, a 6.3 size update to 8192 followed by a 6037-byte incremental-indexing insertion is accepted, and a later request that references that entry from the dynamic table (index 62) gets a response. Proves the table actually grew, not that the size update was merely tolerated.settings()in flight (8192 then 512); the client ACKs only the first and size-updates to 8192, which must be accepted because 8192 is the value that ACK bound. Also assertssession.localSettings.headerTableSizereports 8192, not 512.SETTINGS_HEADER_TABLE_SIZE=0#19152. The client advertisesHEADER_TABLE_SIZE=0in its preface. The first response block must open with a 6.3 size update to 0, and neither response block may reference a dynamic-table index, checked by a small HPACK opcode walker over the raw frame payloads.HEADER_TABLE_SIZE=0thenHEADER_TABLE_SIZE=4096before the server emits any header block. The first response block must open with size-update(0) then size-update(4096), in that order.Both standalone repros pass under the fix and pass under Node v26.3.0 unchanged.
test/js/node/http2/node-http2.test.js(287) and the other http2 suites still pass.Standalone repro for #19152 (raw h2c client against a Bun node:http2 server)
Standalone repro for the decoder side
[review] gate passed · iteration 6 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 6
evidence per changed file