Repository navigation
Bun.serve(http3): abort the request when the client sends STOP_SENDING with its upload still open - #42901
Bun.serve(http3): abort the request when the client sends STOP_SENDING with its upload still open#42901robobun wants to merge 2 commits into
Conversation
…G with its upload still open On STOP_SENDING, lsquic queues RESET_STREAM for the response and drops the stream from its write queue. The server got on_reset(how=1) and ignored it, so a response that waited for the stream to drain was never told, and the request stayed in pendingRequests until the upload ended or the connection closed. On the server, on_reset(how=1) now stops the client's upload with the code the client sent and closes the stream, so on_stream_close aborts the request (RFC 9114 section 4.1.1). The client keeps the old behaviour: its read half stays open for a response that arrives together with STOP_SENDING. lsquic called on_reset(how=1) before it queued RESET_STREAM and before it cleared the stream's stale sending flags. A close from inside the callback sent a FIN in place of the reset, and the STOP_SENDING it queued was cleared. A new patch moves the callback after both.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe change updates HTTP/3 reset handling for both reset directions, reorders lsquic reset callbacks, wires the dependency patch, and adds coverage for client cancellation while a request upload remains open. ChangesHTTP/3 reset lifecycle
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is identified for the HTTP/3 reset lifecycle change. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
|
Updated 4:08 AM PT - Sep 16th, 2026
❌ @robobun, your commit e8c101a has 2 failures in
Add 🧪 To try this PR locally: bunx bun-pr 42901That installs a local version of the PR into your bun-42901 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reorders an lsquic-internal callback via a new vendor patch and changes server-side QUIC stream teardown shared with the fetch() client and node:quic, a human look at the protocol semantics would still be worthwhile.
What was reviewed:
us_quic_on_resetin quic.c: the client path (is_client) still returns early onhow == 1, so fetch() keeps its read-half-open behavior; the server path sends STOP_SENDING with the peer's code and closes soon_stream_closefires.- The lsquic patch: moving
on_reset(how=1)after RESET_STREAM is queued and sending flags are cleared; the other in-treeon_resetconsumer (node:quic shim) only records the error code, so the reorder does not change it. - Checked whether a redundant STOP_SENDING on an already-finished/reset request half or a STOP_SENDING arriving in the same engine pass as HEADERS could misbehave — lsquic drops the redundant frame and the close path still runs
on_stream_close. - The new test awaits the abort event and polls
pendingRequestswith a bounded sleep loop rather than a fixed delay;maxIdleTimeout: 10is client-side only.
Extended reasoning...
Overview
The PR touches four files: packages/bun-usockets/src/quic.c (rewrites us_quic_on_reset so a server context responds to a lone STOP_SENDING by sending STOP_SENDING for the client's upload and closing the stream), a new vendor patch patches/lsquic/stop-sending-on-reset-after-reset.patch (moves the on_reset(how=1) callback in lsquic_stream_stop_sending_in after RESET_STREAM is queued and the sending flags are cleared), registration of that patch in scripts/build/deps/lsquic.ts with an explanatory comment, and one new test in test/js/bun/http/serve-http3.test.ts. The behavior change is scoped to the server how == 1 branch; the how == 0 branch and the client branch are preserved as before.
Security risks
No auth, crypto, or credential handling is involved. The main risk class is protocol state handling on peer-controlled input: a peer sending STOP_SENDING can now cause the server to issue STOP_SENDING + RESET_STREAM and close the stream, which is the RFC 9114 §4.1.1 prescribed outcome and reduces (not increases) resource retention — previously the request stayed pending until the upload ended or the connection idled out. The error code is echoed from lsquic_stream_get_error_code, which is a uint64 passed straight to lsquic; no parsing of untrusted data is added.
Level of scrutiny
Medium-high. The lsquic patch reorders an upstream callback relative to internal flag manipulation and maybe_finish_stream/maybe_schedule_call_on_close, which affects every on_reset consumer in the binary (Bun.serve, fetch(), node:quic). I could not build or run the test here because vendor/ is not fetched in this checkout, so correctness of the reorder rests on reading the patch and the PR's described trace rather than an executed verification. A human familiar with the lsquic stream state machine should confirm that calling lsquic_stream_close from the relocated callback position interacts correctly with maybe_finish_stream that follows it, and that the "FIN in place of reset" symptom from the how == 0 path (acknowledged in the PR as unchanged) is acceptable.
Other factors
The test is added to the existing serve-http3.test.ts file, uses port: 0, await using for cleanup, awaits the abort event rather than sleeping, and polls pendingRequests in a bounded loop. It transfers >64KB of an 8MB response, which is likely acceptable but may be slow on debug+ASAN. The vendored patch is small and the lsquic.ts comment explains what upstream behavior it corrects, consistent with the repo's vendoring guidance, though it lacks an upstream issue link. The bug hunt exited on dry_streak with no findings, and the four ruled-out candidates all concerned edge cases in the same on_reset path.
|
On the open question about the relocated callback: the close from the new position does the following, checked with |
|
CI state for a reviewer: every test lane that exercises this change is green on both runs (builds 116448 and 116470). The red items are not from this diff:
The rest are flakes that passed on retry. |
This branch has not been deployed
1 participant
Problem
Bun.servedoes not abort the request:req.signalstays silent andserver.pendingRequestsstays at 1 until the upload ends or the connection closes. Fixes Bun.serve(http3): STOP_SENDING does not abort the request while the request side is open #42889.on_reset(how = 1).us_quic_on_reset(packages/bun-usockets/src/quic.c:742) ignoredhow == 1, because thefetch()client shares the callback. A response that waits foron_stream_writableis never told, andon_stream_close(which firesonAborted) never runs.Fix
on_reset(how = 1)now sends STOP_SENDING for the client's upload with the code the client sent, and closes the stream.on_stream_closethen aborts the request (RFC 9114 section 4.1.1). Thefetch()client keeps its behaviour: its read half stays open for a response that arrives together with STOP_SENDING.on_reset(how = 1)before it queued RESET_STREAM and before it cleared the stream's stale sending flags. A close from inside the callback sent a FIN in place of the reset, and the STOP_SENDING it queued was cleared. The new patchpatches/lsquic/stop-sending-on-reset-after-reset.patchmoves the callback after both. node:quic only records the code in itson_reset, so the order does not change it.how == 0path, which already closed the stream. This change is for a client that cancels the response alone.test/js/bun/http/serve-http3.test.ts(new test "req.signal aborts on client STOP_SENDING while the request body is still open", times out on stock bun). Also all ofserve-http3.test.ts,test/js/node/quic/, thefetch-http3-*client tests and the node:quic stop/reset/close parallel tests.Background
stream_if:on_writewhen the stream can take data,on_reset(how)when the peer resets a half,on_closewhen both halves are done.Bun.serveandfetch()share onestream_ifinquic.c. node:quic has its own innode_quic_shim.c.vendor/lsquicis fetched from a pinned tarball and patched at build time byscripts/build/deps/lsquic.ts.Notes
BUN_DEBUG_lsquic=1before the fix: afterRX STOP_SENDINGthe server generates RESET_STREAM, then nothing for stream 0 until the connection closes.drop_buffered_data(lsquic_stream.c) removes the stream from the write queue, soon_writenever runs.lsquic_stream_wantwrite(stream, 1)inon_reset, then stop and close fromon_write. It does not work: with upstream order,drop_buffered_dataruns afteron_resetand clears the want-write. The test times out and the request aborts only on idle timeout.stream reset with code 268and closes the stream.how == 0branch (peer RESET_STREAM) closes with a plainlsquic_stream_close, which FINs the response half when the headers were sent. That is the existing behaviour and is not changed here.quic.cnow says why). One deferred (thehow == 0FIN-in-place-of-reset shape, noted above).[policy-decision:dep] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file