Repository navigation
Conversation
… when the session's dispatch returns destroy() on a stream that has not written parked its RESET_STREAM and STOP_SENDING. The microtask-drain pass skipped the flush, and a write on any stream before the next flush retired the stream with no frame. The peer then never saw the stream and never returned its MAX_STREAMS credit. The native destroy() now decides the frames of a destroyed stream in one place, together with the reset of a live writer, and sends them in the call. It parks them only for a stream whose send side never reached lsquic: when JS that the session's own dispatch runs destroys a stream the peer cannot know, and for a client whose handshake is not reported yet. QuicSession::flush_deferred_aborts sends them when that dispatch has returned. A parked abort is retired with no frame in two cases: the session's graceful close is applied, or a client's opened continuation wrote on an open stream after the destroy (test-quic-callback-error-stream-onerror.mjs depends on that). The head-of-pass walk over the sessions and the session's write marker are removed.
|
Updated 8:22 AM PT - Oct 19th, 2026
❌ @robobun, your commit af6d9d3 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 46054That installs a local version of the PR into your bun-46054 --bun |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Status Reproduced on main with a client that cancels one unwritten stream before each request (12 rounds, server limit 16). Main answers 12 of 13 requests and then has no stream credit left. Node v26.3.0 and this branch answer 13 of 13. The tests show the same failure: 46 of the 63 new tests in PR: #46054 |
![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 the two close paths for lost frames: settle_deferred_aborts(true) in apply_close and the flush_deferred_aborts after apply_graceful_close in flush_pending_graceful retire a stream the peer cannot know without a frame only on connection close, which matches the behaviour the description lists as item 5, and the flush_deferred_aborts call after maybe_finish_deferred_close in endpoint.rs reaches the same path. I also looked at a stopSending()-then-destroy() sequence on a 0-RTT client for a freed-stream use when the handshake is reported — peer_knows keeps that entry from being parked past the handshake, so it is not affected.
Extended reasoning...
The change reworks how QuicStream.destroy() emits RESET_STREAM/STOP_SENDING in Bun's lsquic-backed node:quic: stream.rs gains destroy_codes/destroying/finish_on_wire/send_abort, session.rs replaces the write_marker scheme with a dispatching flag, peer_knows/silent flags on DeferredAbort, and new park/settle/silence helpers, endpoint.rs moves the flush to after each session's dispatch, and the test file adds ~850 lines. It touches no auth, crypto or injection surface, but it holds raw lsquic stream pointers across dispatches and user callbacks. Two confirmed findings concern the 0-RTT client path (a want_write spin and a misreported wrote_to_lsquic), so a human must look; the description also lists seven behaviour changes the author asks a maintainer to rule on.
| // Parked only while nothing of the send side reached lsquic: lsquic | ||
| // cannot finish such a stream, so its pointer stays valid for | ||
| // `QuicSession::flush_deferred_aborts`. | ||
| let parks = (stop.is_some() || reset.is_some()) && !self.wrote_to_lsquic.get(); | ||
| let stop_sent = self.stop_sent.get(); | ||
| let parked_on = self | ||
| .session_ref() | ||
| .filter(|session| parks && session.parks_stream_abort(stop_sent)); | ||
| match parked_on { | ||
| Some(session) => session.defer_stream_abort(s.raw(), stop, reset, stop_sent), |
There was a problem hiding this comment.
🟡 (optional) A resumed (0-RTT) client that writes to a stream and destroys it before opened can spin the event loop at 100% CPU until the handshake is reported. finish_on_wire parks the stream at stream.rs:947 while awaits_handshake holds, after kick_write (stream.rs:570) left want_write set, and teardown (stream.rs:600-604) nulls the ctx without clearing it. lsquic keeps the stream in its write queue, the null-ctx on_write does nothing, and the conn stays tickable so rearm_timer fires a 1ms timer repeatedly. The base flushed the parked abort on the next pass, bounding the window to one pass. Fix: call s.want_write(false) (and want_read(false)) on every stream handed to defer_stream_abort, or settle parked aborts for a stream whose write side was already kicked.
Why this was flagged
Trigger: a 0-RTT client (stream bound, raw non-null, before handshake_reported) calls write() then destroy() in one turn without a writer, so wantReset is true. write() queued data and kick_write at stream.rs:570 set want_write(true) but drain_outbound never ran, so wrote_to_lsquic is still false. finish_on_wire computes reset Some, skips drain_outbound, and parks because awaits_handshake (session.rs:646-653) is true. destroy then calls teardown, which sets the lsquic ctx to null (stream.rs:602) and never clears want_write. flush_deferred_aborts (session.rs:710-716) returns early while awaits_handshake, so the stream sits in lsquic's write queue with a null ctx until the handshake is reported. lsquic calls on_write each tick, the conn remains tickable, and rearm_timer re-arms, spinning the loop for the whole handshake RTT. On the base, flush_deferred_aborts ran at the start of the next process pass regardless of handshake state, so the spin lasted one pass.
Verification: kick_write sets s.want_write(true) (stream.rs:568-571); finish_on_wire parks because awaits_handshake() is true (session.rs:646-653); teardown nulls the lsquic ctx (stream.rs:600-604) without want_write(false). flush_deferred_aborts returns early while awaits_handshake() (session.rs:706-713), so the stream stays on the write queue and rearm_timer arms max(1) ms (endpoint.rs:1602) every pass; on the base this lasted one pass at most.
| // Leave the opening flight: what follows an echo runs inside the | ||
| // dispatch that delivered it. | ||
| await fromTimer(() => echo(client, "first")); | ||
| write(await open(client), "s".repeat(4096)); |
There was a problem hiding this comment.
🟡 nit (optional): a new test builds its 4096-byte payload with "s".repeat(4096), which test/CLAUDE.md asks to avoid because String#repeat is very slow in debug JSC builds. Fix: build the payload with Buffer.alloc(4096, "s").toString() (or a filled Uint8Array as the sibling tests at quic-stream.test.ts:239 and :324 already do). [also at: test/js/node/quic/quic-stream.test.ts:751 - nit: the harness convention asks for Buffer.alloc(n, fill).toString() instead of "x".repeat(n), which is slow under debug JSC. test/js/node/quic/quic-stream.test.ts:1111 builds the 4096-byte body with "s".repeat(4096).]
Why this was flagged
test/js/node/quic/quic-stream.test.ts:1111 writes "s".repeat(4096) into a stream. test/CLAUDE.md:147 says to use Buffer.alloc(count, fill).toString() instead of "A".repeat(count) because repeat is very slow in debug JavaScriptCore builds. The neighbouring tests in the same describe block at lines 239 and 324 use new Uint8Array(4096).fill(97) for the same purpose, so this one site is inconsistent with the rest of the file and with the repository test conventions. Consequence is only a slower debug/ASAN run of this test; no behaviour is affected.
Verification: nit. Triggering condition: the new test runs under a debug JSC build. /home/claude/bun/test/js/node/quic/quic-stream.test.ts:1111 reads write(await open(client), "s".repeat(4096));. /home/claude/bun/test/CLAUDE.md:147 states: "To create a repetitive string, use Buffer.alloc(count, fill).toString() instead of \"A\".repeat(count). "".repeat is very slow in debug JavaScriptCore builds."
| if reset == NO_FRAME { | ||
| self.destroy_codes.set([stop, code & CODE_MASK]); | ||
| } | ||
| self.wrote_to_lsquic.set(true); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: A resumed (0-RTT) client that touches stream.writer and then destroys the stream before opened has its RESET_STREAM/STOP_SENDING sent before the handshake, where the peer cannot read them, so the stream's place is never returned. stream.rs:843 sets wrote_to_lsquic although nothing reached lsquic, so parks at stream.rs:941 is false and finish_on_wire bypasses the awaits_handshake() hold the PR adds. Fix: hold the frames of every stream whose send side never reached lsquic until the handshake is reported, including one whose only write-side action was the writer's fail. The base also loses these frames, so this is a sibling the new hold misses.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
If the app reads stream.writer and calls stream.destroy(err) before opened, src/js/internal/quic/quic.ts:2083 makes wantReset false and quic.ts:2504 runs writer.fail, which calls handle.resetStream while destroying is set. The new branch at src/runtime/node/quic/stream.rs:836-845 folds the code into destroy_codes and sets wrote_to_lsquic to true without touching lsquic. In finish_on_wire (stream.rs:941) parks becomes false, so session.parks_stream_abort is never consulted and send_abort (stream.rs:1030) hands STOP_SENDING, RESET_STREAM and close to lsquic right away. Per the PR's own comment at src/runtime/node/quic/session.rs:643-645, before LSCONN_HANDSHAKE_DONE lsquic puts such frames in a Handshake-header packet no peer can read. The server therefore never learns of the stream and never returns its MAX_STREAMS credit. The hold awaits_handshake() in parks_stream_abort (session.rs:652-654) looks as if it covers this, but it is gated on parks, which the synthetic flag defeats. The base branch also sent the writer's reset immediately pre-handshake, so the loss is pre-existing.
Verification: In reset_stream (src/runtime/node/quic/stream.rs:836-845) self.wrote_to_lsquic.set(true) (line 843) runs although no lsquic call was made. parks (stream.rs:941) is false, so send_abort hands STOP_SENDING + RESET_STREAM to lsquic, bypassing awaits_handshake(). On 91e7e3b reset_stream (base stream.rs:830-832) calls s.reset(code) directly, so the writer-fail reset also reached lsquic pre-handshake.
This branch has not been deployed
2 participants
Problem
stream.destroy()before the first write can keep the stream's place for good. The peer never sees the stream and never returns its MAX_STREAMS credit. A cancel-then-request loop stalls at request 12 of 13.abort_for_destroy(src/runtime/node/quic/stream.rs) parks the frames. After a write on any stream,flush_deferred_abortscallslsquic_stream_shutdown_internal, which sends no frame.Fix
destroy()now decides the frames once and sends them in the call, as Node does. For a stream that never sent, inside its session's dispatch, they go out when that dispatch returns.openedcontinuation that writes after the destroy (test-quic-callback-error-stream-onerror.mjsneeds it).test/js/node/quic/quic-stream.test.ts(63 new tests, 46 fail on main). Vendoredtest-quic-*: 233 of 235, as on main.Background
Downsides
Bun.serveHTTP/3 peer then spins at 90% CPU (Bun.serve (HTTP/3): 100% CPU after a client abandons a request stream before its HEADERS #44714).destroy()always sends RESET_STREAM(0) + STOP_SENDING. Node sends nothing.destroy()is flushed in the call: 32 of them send 23 packets (Node 35). Release.text: +256 bytes.Notes
Scope
The report behind this PR has two halves. This PR is the half in which the stream has its place: lsquic opened it, and
destroy()comes before its first write. The other half is a stream that is destroyed while it waits for credit. That one is #45342.Two sites still retire a stream with no frame, and this PR does not change them: the
on_new_streamorphan (stream.rs, #45342) andcancel_early_rejected(streams of a rejected 0-RTT phase).All runs are linux-x64, loopback, the same runtime on both ends. The Node column is Node v26.3.0 built from source.
Behaviour changes against main
Each one needs a maintainer's word. The place to undo it is in parentheses.
openedcontinuation) the frames are dropped only when a stream that wrote after the destroy is open when that callback returns. Main also drops them for a write on a stream that waits for a place, for a writer that was destroyed since, and for a write in a later turn (silence_opening_flight_aborts).destroy()for a stream that has sent nothing until its handshake is reported. Until TLS is done, lsquic sends a packet that has only these frames with a Handshake header, and the server cannot read it. Main parks the same streams, and when it sends them depends on the next pass (awaits_handshake).destroy()after a write and an end inside a dispatch delivers the data. Main sends an empty stream there, and delivers the data outside a dispatch. When lsquic cannot take all of it in the call, the stream is reset with code 0, as on main (finish_on_wire).destroy()thenclose()in one dispatch: the peer no longer gets the stream's frames with the close. Its stream count goes from 1 to 0 (settle_deferred_aborts(true)inapply_close).destroy()go out together, in the call. Main sends the reset at once and can drop the STOP_SENDING.destroy(), afteronerrorran. Main hands them over beforeonerror.This replaces a rule from #32602
#32602 says under "Behavioral fidelity" that a stream created and destroyed in one turn "only manifests its RESET/STOP_SENDING to the peer if the connection goes quiet", and that "the frames are dropped if other stream data is written first". Node v26.3.0 hides such a stream in one place only: a client's opening flight. Everywhere else
destroy(err)callsstopSending/resetStreamin the call (lib/internal/quic/quic.js).What the server is shown. Release builds of the merge base and of this PR.
destroy(err)in a response continuation, another stream writes in the same turndestroy(err)from a timer, another stream writes in the same turndestroy()from a timer, another stream writes in the same turndestroy(err), then a writer write on an open streamdestroy(err), then a stream that writes and is destroyeddestroy(err), then a newer stream with{ body }destroy(err), thenclose()in the same dispatch, no other streamopened, then 4 requestswriteSync(); endSync(); destroy()in a dispatchOver 70 raw-QUIC shapes that have a Node result, main equals Node in 31 and this PR in 35. 10 shapes moved to Node's result. 6 moved away: all are a clean
destroy()or adestroy(err)from a timer, where Node loses the place. 13 more changed and still differ from Node: 10 for the two frame differences below, 1 because a bun server returns credit in batches, 1 for the server-opened credit bug, 1 because committed data is delivered.A bun server (lsquic) returns MAX_STREAMS credit in batches: only when the closed streams plus half the limit reach the current limit. Node returns one unit for each closed stream. So "places left" against a bun server is not a leak count.
Differences from Node v26.3.0 that need a maintainer's decision
destroy()sends RESET_STREAM(0) + STOP_SENDING (lsquic sends the STOP_SENDING code 0 as 0x100). Node sends nothing and the place is lost (quic.md). Main'sdestroy()already asks for these frames. With this PR they always arrive. A peer with noonstreamprints one warning for each such stream (3 streams: Node 0 warnings, this PR 3). Upstream discussion: quic: add stream.cancel() sending H3_REQUEST_CANCELLED nodejs/node#66007, quic: emit RFC 9114 stream-closure error codes (H3_REQUEST_CANCELLED, H3_REQUEST_INCOMPLETE) nodejs/node#65509.destroy(err)on a bidirectional stream sends both frames. Node v26.3.0 sends only STOP_SENDING and the place is lost. quic: fix stop sending behaviour & callback nodejs/node#64710 changed Node's STOP_SENDING handling after v26.3.0. I did not run a newer Node.destroy()is delivered. Node leaves such a stream open at the peer.destroy()thenclose(): equal to Node when no other stream is open. With another open stream Node's close waits for it and the frames go out. A raw-QUIC close in bun does not wait for an idle stream (main's rule, unchanged), so the stream stays hidden.Measurements
Release builds of the merge base (91e7e3b) and of this change for sizes, instructions and packets. Debug builds for the gdb counts.
process()per pass, nothing parked: main runs 2 instructions in a drain pass, and 112 (1 session) or 395 (8 sessions) with onemallocand onefreein every other pass. This PR runs 6 instructions for each session in every pass, with no heap call (gdbstepiover the blocks).write(): 219 -> 220 instructions.endWrite(): 54 -> 55 (objdump).QuicEndpoint1096 -> 1096 bytes,QuicSession880 -> 880,QuicStream208 -> 224 (the same 224-byte allocator class),DeferredAbort48 -> 48 (gdbsizeof)..text81,665,765 -> 81,666,021 bytes (+256).QuicEndpoint::process21,082 -> 20,547 bytes. The stripped binary has the same size (107,247,328 bytes).destroy(err)calls in one timer turn (stats.pktSent, 500 ms): this PR 2 / 9 / 23 / 46, Node 2 / 9 / 35 / 103, main 0 (it sends none of them in that time). Inside a dispatch: 2 to 4 packets for any count on all three.catch syscall, halved, whole process, strace is not installed): main 4 / 4 to 5 / 5, this PR 5 / 14 / 44 to 78, an idle client 3.destroy()outside a dispatch: 3 -> 1 (gdb dprintf).Tests
63 new tests in one
describeblock. 46 fail on a release build of main. The 17 that pass on main are guards for behaviour that earlier builds of this change broke: the opening-flight cases in which main already sends, the cases in which the stream stays hidden (also two destroys, in both orders),resetStream()or a read ofstream.writerbeforedestroy()on a unidirectional stream, a destroy from a later callback of the handshake pass, a resumed session that destroys streams beforeopened, and four close cases.The server in these tests does not answer a stream whose read failed, so a bidirectional stream ends there only when STOP_SENDING arrived too.
The file passes 30 of 30 runs on a release build and 10 of 10 on a debug build. The four vendored tests that pin these frames pass:
test-quic-callback-error-stream-onerror,test-quic-stream-destroy-emits-reset,test-quic-stream-destroy-emits-stop-sending,test-quic-stream-destroy-options-code. The 2 vendored files that fail as plain scripts (test-quic-h3-stream-idle-timeout,test-quic-stream-idle-timeout) pass on both builds with their// Flags:line, as the CI runner passes it.Self-review
Two review rounds and a last check of the final diff. The first reviewed a build that parked on the endpoint's send scope and dropped before a predicted close. The second reviewed a build with the park in
abort_for_destroy. The last check found a use after free in a build that also parked streams that had sent data, and a stream with a used writer that was hidden in the opening flight. This version has what they asked for:destroy(), which also takes the reset of a live writer;process()is gone;close()of the lsquic stream after the parked frames, so a parked reset no longer stays until the session ends.Not done, with reasons:
openedon a resumed session,resetStream()/stopSending()and thedestroy()of a stream that has sent data still lose packets, as on main. The hold can go when lsquic is fixed.dispatch_event) is not changed.writeSync(); endSync(); destroy(); close()in one turn with a large body: the close does not wait for data of the destroyed stream that lsquic still holds. From a timer, main and this PR both deliver a part (about 25,000 of 30,000 bytes) with a clean end.Related open PRs
close()waits. No shared code after this version.reset_streamand adds tests to the same file. Textual conflicts.streamsregistry and the outbound queue.silence_opening_flight_abortsandfinish_on_wireread both, so whichever lands second needs a small adaptation.Found, not in this PR
Bun.serveHTTP/3: 4 abandoned streams from a node:quic client leave the server at 880 to 940 ms CPU per second (Bun.serve (HTTP/3): 100% CPU after a client abandons a request stream before its HEADERS #44714, closed as not planned). Main's client triggers it only when no other stream writes before the flush. A node:quic HTTP/3 server does not spin.onstreamreads without a catch loses its session on the first reset stream. The cancel loop against such a server: Node 13/13, main 8/13, this PR 1/13.{ headers }and destroyed in the same tick keeps its place on main and with this PR (12/13, Node 13/13).initialMaxStreamDataBidiRemoteends both sessions with "flow control violation".lsquic_stream_acked. The three resumed-session tests here pass through that window, as the eight vendored 0-RTT tests do.test-quic-callback-error-onstream.mjsand-onstream-async.mjsfail about 1 run in 4 on debug builds of main and of this PR with an uncaughtERR_QUIC_TRANSPORT_ERROR. Neither test aborts a stream.Workaround for released versions
After
opened, callstream.resetStream(code); stream.stopSending(code);beforestream.destroy().