Conversation
…d flow control RFC 9113 6.9.2 says a change to SETTINGS_INITIAL_WINDOW_SIZE shifts every stream's send window by (new - old), and the window can go negative. The legacy outbound bridge only applied the change when the new value was at least the current window and overwrote rather than adding the delta, so a mid-stream reduction was dropped and subsequent WINDOW_UPDATEs were spent as fresh credit instead of paying down the deficit. Bun kept uploading into a window the peer had just driven negative, inviting a FLOW_CONTROL_ERROR from any compliant peer. Apply the signed delta to every stream's remote_window_size on each remote SETTINGS. The cumulative (granted, used) u64 pair with saturating_sub already yields 0 when the effective window is negative, so no type change is needed.
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 4:17 AM PT - Jul 7th, 2026
❌ @robobun, your commit c952ad4 has some failures in 🧪 To try this PR locally: bunx bun-pr 33602That installs a local version of the PR into your bun-33602 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
An increase must be added on top of prior WINDOW_UPDATE credit, not overwritten with the new initial value. Fails on main with 200000 sent out of 210000 granted; the fix drains the full body.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… wire reject paths Moved the two client flow-control tests to node-http2-flow-control.test.ts so the gate does not run into the pre-existing 10k-request maxSessionMemory stress test that sits at its 150s debug timeout in node-http2.test.js. Shared the raw-server frame parser between the two tests and gave every awaited promise a reject path wired to the session error event.
|
CI status: the only hard failure across builds #69643 and #69685 is The two new |
1 participant
What
A
node:http2client that is uploading ignores a mid-streamSETTINGS_INITIAL_WINDOW_SIZEreduction from the peer. RFC 9113 6.9.2 requires the sender to shift every stream's flow-control window by(new - old), which can drive the window negative; a compliant sender must then stop sending DATA until WINDOW_UPDATEs bring it back above zero. Bun dropped the decrease entirely and treated the following WINDOW_UPDATEs as fresh credit, so it kept uploading into a window the peer had just made deeply negative. A compliant peer is entitled to answer withFLOW_CONTROL_ERROR, killing the stream or the whole session.Repro
Raw
netserver lets the client fill the default 65535-byte stream window, then sendsSETTINGS{INITIAL_WINDOW_SIZE:10}followed by 40WINDOW_UPDATE(stream,1000). The stream window is now10 + 40000 - 65535 = -25525. Node sends 0 DATA bytes; Bun sent every remaining queued byte.Cause
The inbound engine already tracks a signed send window and applies the 6.9.2 delta correctly. Outbound DATA, however, is still gated by the legacy per-stream
remote_window_size/remote_used_window_sizeu64 pair. The engine-to-legacy bridge inon_remote_settingsonly touchedremote_window_sizewhennew_iws >= stream.remote_window_sizeand then overwrote it withnew_iwsinstead of adding the delta, so a decrease was a no-op and an increase lost any prior WINDOW_UPDATE credit.Fix
Capture the previous remote
initial_window_size, computedelta = new - old, and apply it to every stream'sremote_window_sizeunconditionally. Becauseavailable = remote_window_size.saturating_sub(remote_used_window_size), a negative effective window already reads as 0 with no type change needed. Applied at all three remote-SETTINGS sites for consistency (theon_remote_settingsSink impl is the live path; the two inhandle_settings_frameare in the retired legacy inbound half).Test
test/js/node/http2/node-http2.test.jsgains a raw-server conformance case that drives the client through the sequence above, uses a PING round-trip to deterministically fence "after the client processed the SETTINGS+WUs", and asserts{ dataAtShrink: 65535, bytesSentWhileNegative: 0 }. Fails with 34465 on main, passes with the fix, then reopens the window and asserts the full body drains.Fixes #30342