Repository navigation
Conversation
send_data() writes the DATA frames that fit the flow-control window as one batch, then queues the rest of the payload. Over a synchronous JS transport (stream.duplexPair) the peer's WINDOW_UPDATE for that batch is handled before the write returns, when nothing is queued yet. The rest was then queued on the stale window and no later event sent it. Read the window again after that write and continue if it reopened. Stop if a frame handled under that write closed the stream.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for one more day.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 5 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 5:28 PM PT - Sep 19th, 2026
✅ @robobun, your commit c47b28c92e3aff9a233e60059320db37bb71597c passed in 🧪 To try this PR locally: bunx bun-pr 43562That installs a local version of the PR into your bun-43562 --bun |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether the debug_assert! that replaced flush_batch_buffer() in the queue branch can trip on the has_backpressure() / outbound_queue_size > 0 cases: the batch is only appended inside this loop, and the only point where the loop re-enters JS (window hit zero) drains it first, so those conditions cannot flip between batched frames — the assertion holds. The native-socket path is unaffected by the new re-read since no JS runs under its write.
Extended reasoning...
Findings were reported inline, so this is the brief ruled-out note only. I traced the batch buffer's producers in send_data (src/runtime/api/bun/h2_frame_parser.rs:5293) and confirmed the only append site is the multi-frame else-branch of this same loop, which is reachable only when none of the queue conditions hold; the flush at the max_size == 0 branch is the sole re-entrant transport write within the loop, and it runs before the conditions are re-evaluated, so the removed flush_batch_buffer() was indeed a no-op on the queue branch. The remaining concerns (trailers/no_trailers path, the close tail, reliance on black_box) are covered by the inline findings and should be addressed by a human before merge.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/api/bun/h2_frame_parser.rs— A server that useswaitForTrailersover a TLS-upgraded Duplex can dereference a freedStreamand crash.no_trailersat h2_frame_parser.rs:5409 takes a raw&mut *streamwithoutenter_stream_dispatch, then callssend_datawhose empty-payload cork write can overflow intogeneric_writeand the JS Duplex without armingdispatch_depth. A nestedrewrite_readat depth 0 (3614) frees streams queued by an earlierfree_resources, and the close tail at 5355 then reads the freedStream. Fix: reborrow throughenter_stream_dispatchinno_trailerslikewrite_streamdoes at 5906, so the deferred free cannot run while the borrow is live.Extended reasoning...
write_stream(5906) guards its borrow withenter_stream_dispatch, which armsdispatch_depthfor the wholesend_datacall.no_trailers(5405-5421) does not: it dereferences the map pointer directly and passes it tosend_datawith an empty payload. The empty path at 5185 writes the header throughto_writer(); when the cork is full that flushes via_write(2761). OnBunSocket::Nonethe JScallat 2414 arms the guard, but onBunSocket::Tlswrapping anUpgradedDuplex(the casetransport_write_runs_jsat 2826 names)generic_writewrites into the JS Duplex with no guard. The peer processes the frame synchronously and replies; the first inbound record delivers RST_STREAM soon_stream_resetrunsfree_resources, which pushes the id ontopending_engine_stream_closes. A second synchronous record triggers a secondrewrite_read;dispatch_depthis 0, so 3614-3627 removes the map entry and drops the heapStream. Control returns tosend_data, which readsstream.wait_for_trailersandstream.stateat 5356-5367 from freed memory. This is pre-existing on the…Verification: pre-existing (the base already has this route; the diff modifies
send_data— adding theblack_boxat 5150 and thecan_send_data()re-check at 5201 only on the non-empty branch — but leaves the empty-payload sibling andno_trailersunguarded). Trigger: a stream whosefree_resourcesalready queued its id (e.g. JS_finalat src/js/node/http2.ts:2706 callsnative.noTrailerswhen the… -
🟣
src/runtime/api/bun/h2_frame_parser.rs— pre-existing, partial fix: over a JS transport (duplexPair, custom Duplex) a stream the peer resets inside the write of its last DATA frame is re-marked HALF_CLOSED_LOCAL, so Bun later answers the peer's RST_STREAM with its own RST_STREAM and reports a second stream error. The new can_send_data() check guards only the mid-loop flush at h2_frame_parser.rs:5200; the final flush_batch_buffer() at :5344 runs the same re-entrant JS and the close tail at :5361-5365 then overwrites CLOSED. Fix: after every transport write that can re-enter JS, skip the close tail when the stream is no longer sendable, at the 3 sites. Same pattern at 3 sites (h2_frame_parser.rs:5365, :1657, :5866).Extended reasoning...
The PR notes list this as found but not changed; it is the same missing re-check the diff adds at :5201, one write earlier, so the fix is one line at each tail.
Trigger: a peer whose handler calls stream.close(NGHTTP2_CANCEL) on the last DATA frame (or the RST_STREAM-then-WINDOW_UPDATE transport of the new it.each test, arriving on the write at :5344 instead of :5200).
send_data batches the last frames of a payload that fits the window, so no iteration hits max_size == 0 and :5200 never runs.
:5344 flush_batch_buffer -> _write -> onWrite -> the JS Duplex delivers the peer's RST_STREAM synchronously -> rewrite_read -> on_stream_reset at :4181-4187 sets state = CLOSED, :4210 free_resources (detach_context, queue id in pending_engine_stream_closes), dispatches onAborted/onStreamError to JS.
Back in send_data, enqueued is false and close is true: :5361 compares state to HALF_CLOSED_REMOTE, it is CLOSED, so :5365 sets HALF_CLOSED_LOCAL and settled_state = 5; write_stream returns 5 and http2.ts:2905 runs onEndStreamSettled as if the stream ended cleanly.
Later the JS close path calls…Verification: pre-existing, acknowledged in PR description ("found on the way, not changed here"; the note's claim that the tail overwrites CLOSED and a later RST_STREAM follows is accurate). Trigger: a JS transport (duplexPair / hand-driven Duplex,
transport_write_runs_js()) whose peer answers with RST_STREAM synchronously inside the write of a multi-frame payload that fits the window, so no loop…
|
Reply to the two findings outside the diff. Both exist on main, and this PR does not change them.
The |
node-http2.test.js spawns many debug-build subprocesses with 5 s timeouts. Keep the four new cases in a small file that runs in seconds.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Problem
stream.duplexPair()), anode:http2body larger than the flow-control window can stop after the first window: the second 200 KB response never arrives. Node v26.3.0 completes it.send_data(src/runtime/api/bun/h2_frame_parser.rs:5193) writes the frames that fit the window as one batch, then queues the rest. OverduplexPair()the peer's WINDOW_UPDATE arrives inside that write, when nothing is queued to flush.send_datathen queues the rest on the stale window value, and no later event sends it.Fix
send_datareads the window again after that batch write and continues to send if it reopened. A native socket runs no JS under the write, so nothing changes there.send_datacompletes the write callback and stops. On main the rest stays queued and the callback never runs.test/js/node/http2/node-http2-sync-transport.test.ts(4 cases, main fails all 4). Alsotest/js/node/http2/, the vendoredtest-http2-*files, and grpc-js.Background
Duplexthe session writes throughsocket.write(), which runs JS. WithduplexPair()that write can emit 'data' on the peer side before it returns.flush()sends it on WINDOW_UPDATE, on 'drain', and from a deferred auto-flush task.Notes
Repro (hangs on main after
request 0 closed after 200000 bytes, node prints both lines):Why the first request completes on main: the client's WINDOW_UPDATE write finds
clientSidebusy with an earlier write, so theWritablebuffers it and the server sees it on a later turn, after the rest of the body is queued. The server's first batch is also larger than the 64 KiB high-water mark, so a 'drain' follows and flushes the queue. On the second request both conditions are gone: the WINDOW_UPDATE arrives insideserverSide.write(), and that write reports no backpressure. Variants hang on the first request on main: a response written as ten 20 kB chunks, and a server that pipes the request back. With the Windows default high-water mark (16 KiB) every window-sized write reports backpressure and the 'drain' rescues this exact repro, so the first new test passes an explicit 64 KiB.BUN_DEBUG_H2FrameParser=1on the second request, before the fix: fourpadding: ... size: 16374lines for the batch,rewriteRead 62109(the client reads it),engine WU received stream=0 inc=65535andflushStreamQueue 0(the server handles the update with an empty queue), then nineserver queued ...lines and no further flush.In the repro the peer session also takes the thread-wide cork slot inside that write (
cork()force-uncorks the other parser), which unregisters the auto-flush that the response HEADERS had registered. With a hand-drivenDuplexthe same hang needs the body to be written after the HEADERS frame left, which is what the hand-driven tests do.The
black_boxonstream:send_dataholds&mut Streamacross the write, andon_window_update/on_stream_resetwrite the sameStreamthrough the pointer in thestreamsmap. A minimal model of the loop, compiled with the pinned nightly atopt-level=3, keeps the second read only while the&mut Streamescapes somewhere (today through theblack_boxinqueue_frame). Without an escape, LLVM hoists the stream window load out of the loop and the read after the write is folded into the one before it. The launder insend_datamakes the second read independent ofqueue_frame. #40240 turns theStreamfields intoCells, which makes the launder unnecessary.The second
flush_batch_buffer()in the queue branch became a no-op with this change (a batch exists only while no queue condition holds, and the window case is flushed earlier now), so it is adebug_assert!that the batch is empty.New tests, in their own file (
node-http2.test.jsspawns many debug-build subprocesses with 5 s timeouts, and these four cases run in seconds):duplexPair(), the reported case.Duplexthat answers from inside its_writewhen the body has used up a window. One case limits the body by the connection window, one by the stream window (that one covers the read through&mut Stream). All 200000 bytes and END_STREAM must arrive.end()callback must run, andsession.state.outboundQueueSizemust be 0. On bun 1.4.2 the callback never runs andoutboundQueueSizestays at 9. On this branch without thecan_send_data()check, 134465 bytes of DATA followed the reset.Fail-before and pass-after were checked the way CI does it: a debug build of main's
src/fails all 4 new cases (timeouts), a debug build of this branch passes them.Suites run with the debug build:
test/js/node/http2/node-http2.test.js(391 pass), the other ten files intest/js/node/http2/, all 261test/js/node/test/{parallel,sequential}/test-http2-*files, and eight grpc-js files. All oftest/js/third_party/grpc-js/with a release build of this branch: the same 322 pass and 6 fail as stock bun (DNS cases that need the public internet, and the tonic case). A release build passes the repro.Found on the way, not changed here:
send_dataqueues whenever the session-wideoutbound_queue_sizeis not zero. node:http2: do not hold a stream's DATA behind another stream's queued frames #43422 covers it.send_datasetsHALF_CLOSED_LOCALoverCLOSED, and the later teardown answers the RST_STREAM with a RST_STREAM (RFC 9113 5.4.2 forbids that). Node sends none. The same tail exists inStream::flush_queueand in the trailers path. Same result on bun 1.4.2. Tracked in node:http2: a RST_STREAM handled inside the write of a stream's last DATA frame is answered with RST_STREAM #43572.duplexPair(): when a clientread()resumes a paused stream while the server holds the cork slot with a complete response, the response bytes stay parked until the next inbound byte (a PING releases them). Node delivers them at once. Same result on bun 1.4.2. Tracked in node:http2 over duplexPair: a response stays parked until the next inbound byte when read() resumes another stream #43574.no_trailersholds&mut Streamacross a transport write without the dispatch guard, which can free the stream under it over a TLS-upgraded Duplex. It sends an empty payload only, so it does not reach the loop this PR changes. Tracked in node:http2: noTrailers() holds a Stream across a transport write without the dispatch guard (possible use-after-free over a TLS-upgraded Duplex) #43573.[human-review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file