http3: stop one dead peer from stalling the shared fetch client engine - #35255
Merged
Merged
Conversation
…underflow A killed HTTP/3 peer on the shared client UDP socket was stalling every other live connection on the engine, showing up as intermittent HTTP3StreamReset on the 1MB POST in serve-protocols.test.ts. - bsd_create_udp_socket sets IP_RECVERR for node:dgram. QUIC never wires on_recv_error, so the only effect is that a queued ICMP port-unreachable surfaces as -1 ECONNREFUSED on the next sendmmsg, for a datagram bound to a live peer. lsquic treats any short packets_out return as socket-level backpressure and clears ENPUB_CAN_SEND for the whole engine; only its one-second resume_sending_at failsafe recovers it. us_quic_set_dontfrag now turns IP_RECVERR back off on QUIC's sockets, and us_quic_packets_out retries once on a non-backpressure send failure so a stale sk_err cannot pause the engine. - lsquic's send_batch requeue loop computed end = &batch->packets[off - 1] with unsigned off, so when the first unsent spec coalesced multiple packets only the last was returned to the connection; the earlier ones (typically INIT ACK and HSK CRYPTO) were dropped and never retransmitted. Patched to iterate [off, off+count) instead. - serve-protocols.test.ts now stops each fixture server gracefully on stdin close so the pooled client session sees CONNECTION_CLOSE, matching serve-http3.test.ts.
Contributor
|
Warning Review limit reached
Next review available in: 5 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 (3)
WalkthroughChangesUDP receive-error option
QUIC send fault handling
LSQUIC packet requeueing
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found, but this touches the QUIC send hot path and patches vendored lsquic — worth a human look.
What was reviewed:
- lsquic patch arithmetic: new
[off, off+count)bounds iterate exactlycounttimes in reverse and avoid theoff==0underflow;&packets[off+count]is at worst one-past-end, which is legal. us_quic_packets_outretry: one-shot, only on non-EAGAIN/ENOBUFS, both sendmmsg and per-packet paths; EAGAIN/EWOULDBLOCK/ENOBUFS still break so real backpressure still pauses the engine.IP_RECVERRoff is Linux-gated and applied viaus_quic_set_dontfrag, which both the server listen path andus_quic_client_endpointcall, so the shared client socket is covered.
Extended reasoning...
Overview
Four files: packages/bun-usockets/src/quic.c (retry-once on non-backpressure send errors, disable IP_RECVERR/IPV6_RECVERR on QUIC UDP sockets), a new patches/lsquic/requeue-unsent-coalesced.patch fixing an unsigned underflow in lsquic's send_batch requeue loop, scripts/build/deps/lsquic.ts registering the patch, and test/js/bun/http/serve-protocols.test.ts switching fixture teardown from proc.kill() to graceful server.stop(true) on stdin close.
Security risks
None identified. The changes are on the send-side error-handling path of an unconnected UDP socket; no new inputs are parsed and no trust boundaries move. Disabling IP_RECVERR on the QUIC socket returns it to the kernel default for unconnected UDP and only affects the socket QUIC itself created.
Level of scrutiny
High. This is production networking code in the HTTP/3 client/server send path plus a pointer-arithmetic patch to a vendored protocol library. The root-cause chain (IP_RECVERR → stale ICMP on shared socket → sendmmsg -1 → engine-wide ENPUB_CAN_SEND clear → 1s failsafe loop) is well-argued and the lsquic debug traces in the description support it, but the interaction between the retry-once heuristic, Windows us_quic_send_one (which maps every WSA error except WOULDBLOCK to EIO, so the retry now fires on any Windows send error), and lsquic's own resend machinery is subtle enough that a maintainer familiar with this stack should sign off.
Other factors
- The PR description states the fix reduces the flake to ~1% rather than eliminating it, and attributes the residual to a separate bug being tracked independently. That's an explicit known-incomplete state a human should acknowledge before merge.
- The test's fixture now has
setTimeout(() => process.exit(0), 50)afterserver.stop(true); the 50ms is in the child server (giving CONNECTION_CLOSE time to flush) rather than a test-side sleep-then-assert, andwithServerbounds it with a 500ms race → kill, so it doesn't violate the "await the condition" rule for assertions. - The lsquic patch header says the fix is "present in upstream lsquic master as well" — I did not verify that claim.
- No prior review comments to address; only a CodeRabbit rate-limit notice.
…t-injection harness Addresses review: - IP_RECVERR is now opt-in via LIBUS_UDP_LINUX_RECVERR, set by us_create_udp_socket when recv_error_cb is non-NULL. node:dgram always passes one and keeps the option; QUIC passes NULL and never gets it. Removes the set-then-unset in us_quic_set_dontfrag. - us_quic_packets_out / us_quic_send_one now go through US_FAULT_CHECK(US_FAULT_SENDMSG, ...), so the lsquic short-return and retry paths are reachable from tests. - test/js/web/fetch/fetch-http3-syscall-fault.test.ts exercises three cases: EAGAIN on the coalesced INIT+HSK+SHORT handshake datagram (the pack_off[0]==0, iovlen>1 spec the lsquic patch fixes), a one-shot ECONNREFUSED that the retry-once branch consumes, and a burst of EAGAIN that the on_drain path recovers from.
Collaborator
Author
|
Updated 9:30 AM PT - Jul 23rd, 2026
✅ @autofix-ci[bot], your commit 6ee72b4913aca2b2a434ce93469394b2a9af96b1 passed in 🧪 To try this PR locally: bunx bun-pr 35255That installs a local version of the PR into your bun-35255 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/bun-usockets/src/udp.c`:
- Around line 187-189: Update the flags handling near bsd_create_udp_socket so
LIBUS_UDP_LINUX_RECVERR is cleared or rejected whenever recv_error_cb is NULL,
rather than only conditionally adding it. Preserve enabling the flag when a
callback is supplied, ensuring caller-provided flags cannot bypass this callback
gating.
In `@test/js/bun/http/serve-protocols.test.ts`:
- Line 122: Update the process teardown around proc.exited to use a cancellable
500 ms timer, and cancel it when proc.exited resolves before the fallback kill
runs. Preserve the existing fallback behavior of calling proc.kill() if the
process does not exit in time.
In `@test/js/web/fetch/fetch-http3-syscall-fault.test.ts`:
- Around line 24-27: Update the spawned-process cleanup loop to await the
process’s graceful exit after calling p.stdin?.end(), while retaining a bounded,
cancellable fallback that invokes p.kill() if exit does not occur in time.
Ensure cleanup still awaits p.exited and releases every process without
immediately killing the stdin-triggered shutdown.
🪄 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: eff5c25a-ada7-485a-a36b-43ce6ebd938f
📒 Files selected for processing (8)
packages/bun-usockets/src/bsd.cpackages/bun-usockets/src/libusockets.hpackages/bun-usockets/src/quic.cpackages/bun-usockets/src/udp.cpatches/lsquic/requeue-unsent-coalesced.patchscripts/build/deps/lsquic.tstest/js/bun/http/serve-protocols.test.tstest/js/web/fetch/fetch-http3-syscall-fault.test.ts
…cellable kill timer in test teardown
…timeouts; retitle non-backpressure case
Contributor
There was a problem hiding this comment.
Thanks for addressing the test-file feedback in d020497 — the withServer scoping, dropped per-test timeouts, and retitled non-backpressure case all look right now. No further issues found on this pass.
Deferring to a human for the native pieces: the us_quic_packets_out retry-once change and the vendored lsquic send_batch requeue patch both sit on the shared HTTP/3 client's hot send path, and the IP_RECVERR gating touches every UDP socket Bun creates. Worth maintainer eyes on those.
What was reviewed
- lsquic patch bounds: verified
[off, off+count)with pre-decrement iterates all packets in reverse and&packets[off+count]is at worst one-past-the-end (never dereferenced). LIBUS_UDP_LINUX_RECVERRgating: confirmed bothus_quic_socket_context_listenandus_quic_client_endpointpassrecv_error_cb=NULL, so QUIC sockets no longer set IP_RECVERR;node:dgramalways passes one and keeps it.bsd_apply_udp_recv_options: the newoptionsparam is only read under__linux__;(void) optionson other platforms avoids the unused-param warning.
Extended reasoning...
Overview
This PR fixes a CI flake in serve-protocols.test.ts where concurrent HTTP/3 tests sharing one lsquic client engine stall when a killed server's ICMP port-unreachable poisons the shared unconnected UDP socket. It touches four native usockets files (bsd.c, libusockets.h, quic.c, udp.c), adds a vendored-dependency patch to lsquic's send_batch requeue loop, wires that patch into scripts/build/deps/lsquic.ts, adjusts serve-protocols.test.ts to shut fixture servers down gracefully, and adds a new fault-injection test file.
Security risks
None identified. IP_RECVERR is now less permissive (opt-in rather than always-on), the retry-once in us_quic_packets_out is bounded to a single retry per batch, and the lsquic patch tightens loop bounds rather than loosening them. No user-controlled input reaches new parsing.
Level of scrutiny
High. us_quic_packets_out is the sole egress for every HTTP/3 datagram Bun's fetch client sends, and the lsquic patch modifies the engine's unsent-packet requeue loop for every short return. A mistake here silently drops handshake packets or double-requeues them across every h3 connection in the process. The IP_RECVERR gating also changes behavior for every us_create_udp_socket caller. These are exactly the shared-infrastructure changes REVIEW.md flags for enumerate-every-consumer scrutiny.
Other factors
- I previously left three inline nits on the test file (withServer lifecycle, per-test timeouts, test title over-claiming); all were addressed in d020497 and verified in the current diff.
- The lsquic patch math checks out for the
off=0, iovlen>1underflow case and preserves reverse iteration order forsend_ctl_sched_prepend. - The PR description acknowledges a residual ~1-2% failure rate from a separate unfixed bug and that the fault-injection gate cannot mechanically distinguish stashed/unstashed runs — both are honestly disclosed but mean the 300-iteration loop is the load-bearing evidence, which a maintainer should weigh.
- Per
.claude/docs/landing-prs.mdthe Dependencies & vendoring section applies (new patch underpatches/lsquic/); a maintainer should confirm the upstream-PR intent noted in the description.
robobun
added a commit
that referenced
this pull request
Jul 23, 2026
scripts/build/deps/lsquic.ts: keep both sides' patch-list additions. Main added requeue-unsent-coalesced.patch (#35255, send_batch off-1 underflow at :2739); this branch adds node-quic-accessors.patch, coalesce-batch-drop.patch and connection-close-pns.patch. The two engine.c patches touch disjoint hunks (:2739 vs :2906+); both apply cleanly. [skip size check]
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 14, 2026
…he HTTP/3 engine (#42732) ### Problem - HTTP/3 uses more than 100% CPU when UDP sends keep failing with an errno other than `EAGAIN`. Both `fetch()` and `Bun.serve({ http3: true })`. - Two triggers: an iptables DROP rule (`EPERM`), and an egress MTU below 1500, where each DPLPMTUD probe fails with `EMSGSIZE`. - The cause is the tail of `us_quic_packets_out` (`packages/bun-usockets/src/quic.c:316`). It reports every send errno as `EAGAIN` and arms `LIBUS_SOCKET_WRITABLE`. The socket is already writable, so `on_drain` fires at once and the send fails again. ### Fix - `us_quic_packets_out` passes `EMSGSIZE` through. lsquic retires that datagram with `ci_packet_too_large` and feeds DPLPMTUD. - Any other refused datagram counts as sent. To lsquic that is a packet lost on the wire, so its loss timer paces the retries and a timeout ends the connection. - Only `EAGAIN`, `EWOULDBLOCK` and `ENOBUFS` still return short and arm `WRITABLE`. quinn draws the same line. - Verified: `fetch-http3-syscall-fault.test.ts`, four new tests, three fail on stock bun. Also the other h3 suites and `node:quic`, on Linux and Windows. ### Background - lsquic calls `us_quic_packets_out` to send a batch of datagrams. One engine serves every HTTP/3 connection of the process, so one refused peer stalls the rest. - A short return with `EAGAIN` stops every send of that engine until `lsquic_engine_send_unsent_packets` runs. Bun runs it from `on_drain`, the callback of a writable UDP poll. - A short return with `EMSGSIZE` retires one datagram and leaves the engine running. - Any other errno closes the connection that owns the first unsent datagram. A pending ICMP error on the shared client socket belongs to no particular peer, so that does not fit. <details><summary>Notes</summary> **Reach: what was run, and what was simulated.** The unsendable-IP-literal trigger is real (`EACCES` from the kernel, no privileges needed). `EPERM` from a firewall rule is simulated two ways: a seccomp filter that fails `sendmsg`/`sendmmsg`, and `US_FAULT_SENDMSG`. The report this came from also saw it with a real `iptables -A OUTPUT -p udp --dport <port> -j DROP`. The low egress MTU is simulated with an `LD_PRELOAD` shim, because the container has no `CAP_NET_ADMIN` to set a link MTU. No user has reported this. The HTTP/3 API is experimental, so this is a low-priority fix. **Repro without privileges.** `fetch("https://255.255.255.255/", { protocol: "http3", signal: AbortSignal.timeout(1500) })`. An IP literal skips DNS and the connect-time route probe (`us_quic_socket_context_connect`), and `sendmsg` to the limited broadcast address fails every time on a socket without `SO_BROADCAST`. Unset `HTTPS_PROXY` first, a proxy makes fetch refuse HTTP/3 up front. Stock bun uses 2056 ms of CPU in a 1502 ms wait, and 1354 ms of it after the abort. With the fix: 69 ms, then 25 ms. **Tests.** - `a datagram the kernel refuses to send is dropped without spinning the HTTP thread` uses that address in a child process and compares `process.cpuUsage()` with the wall time of a 1 s wait. It needs no fault injection, so it runs on release builds too. Unfixed: 1365 ms CPU (release), 1033 ms (debug). Fixed: 34 ms. Skipped on Windows, where the dual-stack socket sends to `::ffff:255.255.255.255` without an error. - `Bun.serve: a send error on every datagram does not spin the server's event loop` arms `EPERM` on every send from inside a request handler and reads the server child's CPU time. Spin: 2124 ms CPU in 1050 ms. Fixed: 135 ms. - `Bun.serve: a refused version-negotiation reply does not spin the loop when no connection exists` sends one long-header datagram with version `0x0a0a0a0a`, built from a real Initial with its version bytes replaced. The server queues a Version Negotiation reply for a connection it does not have. The test first sends the datagram to an unfaulted server and checks that the reply is a Version Negotiation packet that offers QUIC v1, so the CPU check cannot pass without a reply to refuse. This is the only spin with no connection and no timeout to end it. Spin: 2129 ms CPU in 1052 ms. Fixed: 325 ms against a 150 ms idle baseline for a debug ASAN server. On the release binary under seccomp the old code uses 2637 ms in 2002 ms. - `a DPLPMTUD probe above the egress MTU does not stall the transfer` streams 2.5 s of body under an `LD_PRELOAD` shim that refuses any datagram above 1400 payload bytes, which is above lsquic's base packet size and below its first probe. Unfixed: the transfer delivers 32 KB, stalls for the remaining 11 s, and the next request on the same engine times out. Fixed: 3.1 MB delivered, no stall, 696 ms CPU in 4049 ms, and each probe refused once instead of twice. - The MTU shim declares `sendmmsg` with the flags type of the libc it builds against: `int` on glibc, `unsigned int` on musl. - An injected 0 from `US_FAULT_SENDMSG` (the `zero` action) now counts as one datagram sent in the `sendmmsg` path, as it already does in `us_quic_send_one`. Before, it left that loop without progress. Only the test hook can reach this. - The two `Bun.serve` tests pass on stock bun for a different reason: the old `sendmmsg` retry does not go through `US_FAULT_SENDMSG`, so an injected error cannot persist and the retry sends the datagram. This PR puts the retry behind the hook. The CPU numbers above come from a build with the hook and the old errno logic. **`IP_PMTUDISC_PROBE`.** The comment in `us_quic_set_dontfrag` claimed that this option lets DPLPMTUD probe without `EMSGSIZE`. It does not. It makes the kernel ignore its cached path MTU. The device MTU still bounds the datagram: on a 1500 MTU link, a 1472-byte payload is sent and 1473 bytes gives `EMSGSIZE`. The comment now says that, and names where the errno goes. **Prior art.** quinn waits for a writable socket only on `WouldBlock`. It logs every other send error and goes on (`UdpSenderHelper::poll_send` in `quinn/src/runtime/mod.rs`, with the test `non_would_block_send_errors_are_logged_and_ignored` using `PermissionDenied`). Its `quinn-udp` doc says UDP send errors are non-fatal "because higher-level protocols must employ retransmits and timeouts anyway". **Not in this change.** `node:quic`'s own `packets_out` (`src/runtime/node/quic/endpoint.rs:792-803`) has the same errno rewrite. It does not spin: `us_udp_socket_send` arms `WRITABLE` only for `EAGAIN`, so each failure costs one second through lsquic's failsafe. It also keeps `EMSGSIZE`, as this PR now does. A classifier change there belongs in its own PR, because #40220 rewrites that file (+645/-1056). **Relation to #42678.** Both change the same two `sendmmsg` calls, and #42678 calls this behaviour "a separate bug". This PR is the smaller of the two, so it should land first. The conflict is mechanical: the call moves into `us_quic_sendmmsg`. **Other cases that stay as they are.** `ENOBUFS` remains backpressure, as #35255 chose. That holds on Windows too: `us_quic_send_one` keeps `WSAENOBUFS` as `ENOBUFS`. Before, it became `EIO`, which the old tail still treated as backpressure and this PR would have dropped. A UDP socket that is already closed (`!ls->udp`) still returns short with no poll to arm, so lsquic's one-second failsafe paces it. **Self-reviewed:** 8 concerns raised, 5 addressed (the `EMSGSIZE` classifier and its false comment, the zero-connection spin, the `node:quic` twin note, the #42678 note, the reach note). 3 rejected: folding the `node:quic` fix in (#40220 rewrites that file), patching lsquic instead (the callback contract has no fourth outcome), and a client-side address-walk change (a separate behaviour, not this bug). </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch-http3-syscall-fault.test.ts <!-- robobun:evidence:end -->
usrbinkat
pushed a commit
to usrbinkat/bun
that referenced
this pull request
Sep 15, 2026
…he HTTP/3 engine (oven-sh#42732) ### Problem - HTTP/3 uses more than 100% CPU when UDP sends keep failing with an errno other than `EAGAIN`. Both `fetch()` and `Bun.serve({ http3: true })`. - Two triggers: an iptables DROP rule (`EPERM`), and an egress MTU below 1500, where each DPLPMTUD probe fails with `EMSGSIZE`. - The cause is the tail of `us_quic_packets_out` (`packages/bun-usockets/src/quic.c:316`). It reports every send errno as `EAGAIN` and arms `LIBUS_SOCKET_WRITABLE`. The socket is already writable, so `on_drain` fires at once and the send fails again. ### Fix - `us_quic_packets_out` passes `EMSGSIZE` through. lsquic retires that datagram with `ci_packet_too_large` and feeds DPLPMTUD. - Any other refused datagram counts as sent. To lsquic that is a packet lost on the wire, so its loss timer paces the retries and a timeout ends the connection. - Only `EAGAIN`, `EWOULDBLOCK` and `ENOBUFS` still return short and arm `WRITABLE`. quinn draws the same line. - Verified: `fetch-http3-syscall-fault.test.ts`, four new tests, three fail on stock bun. Also the other h3 suites and `node:quic`, on Linux and Windows. ### Background - lsquic calls `us_quic_packets_out` to send a batch of datagrams. One engine serves every HTTP/3 connection of the process, so one refused peer stalls the rest. - A short return with `EAGAIN` stops every send of that engine until `lsquic_engine_send_unsent_packets` runs. Bun runs it from `on_drain`, the callback of a writable UDP poll. - A short return with `EMSGSIZE` retires one datagram and leaves the engine running. - Any other errno closes the connection that owns the first unsent datagram. A pending ICMP error on the shared client socket belongs to no particular peer, so that does not fit. <details><summary>Notes</summary> **Reach: what was run, and what was simulated.** The unsendable-IP-literal trigger is real (`EACCES` from the kernel, no privileges needed). `EPERM` from a firewall rule is simulated two ways: a seccomp filter that fails `sendmsg`/`sendmmsg`, and `US_FAULT_SENDMSG`. The report this came from also saw it with a real `iptables -A OUTPUT -p udp --dport <port> -j DROP`. The low egress MTU is simulated with an `LD_PRELOAD` shim, because the container has no `CAP_NET_ADMIN` to set a link MTU. No user has reported this. The HTTP/3 API is experimental, so this is a low-priority fix. **Repro without privileges.** `fetch("https://255.255.255.255/", { protocol: "http3", signal: AbortSignal.timeout(1500) })`. An IP literal skips DNS and the connect-time route probe (`us_quic_socket_context_connect`), and `sendmsg` to the limited broadcast address fails every time on a socket without `SO_BROADCAST`. Unset `HTTPS_PROXY` first, a proxy makes fetch refuse HTTP/3 up front. Stock bun uses 2056 ms of CPU in a 1502 ms wait, and 1354 ms of it after the abort. With the fix: 69 ms, then 25 ms. **Tests.** - `a datagram the kernel refuses to send is dropped without spinning the HTTP thread` uses that address in a child process and compares `process.cpuUsage()` with the wall time of a 1 s wait. It needs no fault injection, so it runs on release builds too. Unfixed: 1365 ms CPU (release), 1033 ms (debug). Fixed: 34 ms. Skipped on Windows, where the dual-stack socket sends to `::ffff:255.255.255.255` without an error. - `Bun.serve: a send error on every datagram does not spin the server's event loop` arms `EPERM` on every send from inside a request handler and reads the server child's CPU time. Spin: 2124 ms CPU in 1050 ms. Fixed: 135 ms. - `Bun.serve: a refused version-negotiation reply does not spin the loop when no connection exists` sends one long-header datagram with version `0x0a0a0a0a`, built from a real Initial with its version bytes replaced. The server queues a Version Negotiation reply for a connection it does not have. The test first sends the datagram to an unfaulted server and checks that the reply is a Version Negotiation packet that offers QUIC v1, so the CPU check cannot pass without a reply to refuse. This is the only spin with no connection and no timeout to end it. Spin: 2129 ms CPU in 1052 ms. Fixed: 325 ms against a 150 ms idle baseline for a debug ASAN server. On the release binary under seccomp the old code uses 2637 ms in 2002 ms. - `a DPLPMTUD probe above the egress MTU does not stall the transfer` streams 2.5 s of body under an `LD_PRELOAD` shim that refuses any datagram above 1400 payload bytes, which is above lsquic's base packet size and below its first probe. Unfixed: the transfer delivers 32 KB, stalls for the remaining 11 s, and the next request on the same engine times out. Fixed: 3.1 MB delivered, no stall, 696 ms CPU in 4049 ms, and each probe refused once instead of twice. - The MTU shim declares `sendmmsg` with the flags type of the libc it builds against: `int` on glibc, `unsigned int` on musl. - An injected 0 from `US_FAULT_SENDMSG` (the `zero` action) now counts as one datagram sent in the `sendmmsg` path, as it already does in `us_quic_send_one`. Before, it left that loop without progress. Only the test hook can reach this. - The two `Bun.serve` tests pass on stock bun for a different reason: the old `sendmmsg` retry does not go through `US_FAULT_SENDMSG`, so an injected error cannot persist and the retry sends the datagram. This PR puts the retry behind the hook. The CPU numbers above come from a build with the hook and the old errno logic. **`IP_PMTUDISC_PROBE`.** The comment in `us_quic_set_dontfrag` claimed that this option lets DPLPMTUD probe without `EMSGSIZE`. It does not. It makes the kernel ignore its cached path MTU. The device MTU still bounds the datagram: on a 1500 MTU link, a 1472-byte payload is sent and 1473 bytes gives `EMSGSIZE`. The comment now says that, and names where the errno goes. **Prior art.** quinn waits for a writable socket only on `WouldBlock`. It logs every other send error and goes on (`UdpSenderHelper::poll_send` in `quinn/src/runtime/mod.rs`, with the test `non_would_block_send_errors_are_logged_and_ignored` using `PermissionDenied`). Its `quinn-udp` doc says UDP send errors are non-fatal "because higher-level protocols must employ retransmits and timeouts anyway". **Not in this change.** `node:quic`'s own `packets_out` (`src/runtime/node/quic/endpoint.rs:792-803`) has the same errno rewrite. It does not spin: `us_udp_socket_send` arms `WRITABLE` only for `EAGAIN`, so each failure costs one second through lsquic's failsafe. It also keeps `EMSGSIZE`, as this PR now does. A classifier change there belongs in its own PR, because oven-sh#40220 rewrites that file (+645/-1056). **Relation to oven-sh#42678.** Both change the same two `sendmmsg` calls, and oven-sh#42678 calls this behaviour "a separate bug". This PR is the smaller of the two, so it should land first. The conflict is mechanical: the call moves into `us_quic_sendmmsg`. **Other cases that stay as they are.** `ENOBUFS` remains backpressure, as oven-sh#35255 chose. That holds on Windows too: `us_quic_send_one` keeps `WSAENOBUFS` as `ENOBUFS`. Before, it became `EIO`, which the old tail still treated as backpressure and this PR would have dropped. A UDP socket that is already closed (`!ls->udp`) still returns short with no poll to arm, so lsquic's one-second failsafe paces it. **Self-reviewed:** 8 concerns raised, 5 addressed (the `EMSGSIZE` classifier and its false comment, the zero-connection spin, the `node:quic` twin note, the oven-sh#42678 note, the reach note). 3 rejected: folding the `node:quic` fix in (oven-sh#40220 rewrites that file), patching lsquic instead (the callback contract has no fourth outcome), and a client-side address-walk change (a separate behaviour, not this bug). </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch-http3-syscall-fault.test.ts <!-- robobun:evidence:end -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test/js/bun/http/serve-protocols.test.tshas been going red on main (build 78445 darwin-x64 hard, 78462 debian-11-aarch64 hard, plus 78300/78419 with retries), always asReproduced on Linux by looping the file: the h3 subset alone fails about 17 of 100 runs.
Cause
The ten concurrent h3 tests share one lsquic client engine and one unconnected UDP socket.
bsd_create_udp_socket()setsIP_RECVERRon every UDP socket (fornode:dgram's error surfacing, #28827), including QUIC's. When a finished testproc.kill()s its server, the client session is still in the engine and keeps scheduling retransmits /NEW_CONNECTION_IDto an unbound port. WithIP_RECVERRon, the resulting ICMP port-unreachable is queued on the shared socket and the nextsendmmsgreturns-1 ECONNREFUSED, even though that call is sending a datagram to a live peer.us_quic_packets_outreports that as a short return, lsquic clearsENPUB_CAN_SENDfor the whole engine and only its one-secondresume_sending_atfailsafe re-enables it. With several dead sessions generating ICMPs, every failsafe retry fails the same way and the live 1MB upload never advances; the 20s in CI is two idle-timeout rounds throughretry_or_fail.While tracing that I also found an unsigned underflow in lsquic's
send_batchrequeue loop: when the first unsent spec in a batch coalesces multiple packets (pack_off[0] == 0,iovlen > 1),end = &batch->packets[off - 1]indexes withUINT_MAXand only the last packet of the coalesced group is returned to the connection. The earlier ones are the INIT ACK and the HSK CRYPTO carrying the client Finished, so the peer can never complete the handshake. This is the same hang reached from a different direction (real EAGAIN backpressure instead of stale ICMP).Fix
IP_RECVERRis now opt-in viaLIBUS_UDP_LINUX_RECVERR, set byus_create_udp_socketwhen arecv_error_cbis provided.node:dgramalways passes one and keeps the option; QUIC passesNULLand no longer gets it. This matches libuv'sUV_UDP_LINUX_RECVERRgating thatbsd.calready cited.us_quic_packets_out()retries once on a non-EAGAIN/ENOBUFSsend failure before reporting a short return, so a stalesk_errthat does surface cannot pause the engine. Both thesendmmsgand per-packet paths now go throughUS_FAULT_CHECK(US_FAULT_SENDMSG, ...)so the short-return path is reachable from tests.patches/lsquic/requeue-unsent-coalesced.patchrewrites the requeue loop's bounds as[off, off+count)so every packet in an unsent coalesced datagram is returned to the connection. The same underflow is present in upstream lsquic master; I will open a PR there separately.serve-protocols.test.tsnow stops each fixture server gracefully on stdin close (server.stop(true)), matchingserve-http3.test.ts, so the pooled client session seesCONNECTION_CLOSEinstead of leaving the engine retransmitting to unbound ports.test/js/web/fetch/fetch-http3-syscall-fault.test.tsinjectsEAGAINon the coalesced handshake datagram (thepack_off[0]==0,iovlen>1spec the lsquic patch fixes), a one-shotECONNREFUSEDthat the retry-once branch consumes, and a burst ofEAGAINthat theon_drainpath recovers from.Verification
Release build, looped:
serve-protocols -t "http/3"serve-protocols(full)Debug+ASAN:
serve-protocols,serve-http3(46),fetch-http3-client(52),fetch-http3-adversarial(29),fetch-http3-syscall-fault(3) anddgram.test.tsall pass, 211 tests total.The residual ~1-2% is a separate pre-existing bug (the client's 36-byte HSK CRYPTO is buffered but never flushed when
drain_send_bodywrites the whole 1MB body synchronously fromon_stream_open); I've handed that off as its own issue. With CI's retry it is well under the flake threshold.Gate note
The fault-injection hook that makes the new test deterministic lives in
packages/bun-usockets/src/quic.c, sogit stash -- src/ packages/removes it along with the fix and the fault never fires. The lsquic piece lives inpatches/andscripts/, which the stash does not touch. That means a single stashed run passes (no fault, no stall) and a single unstashed run passes (fault fires, fix handles it), and the gate cannot distinguish them mechanically. The 300-iteration probe above is the evidence; the fault-injection tests pin the behavior going forward.lsquic debug trace of the stall
and for the underflow, a batch with
pack_off[0]=0,iovlen[0]=3:no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch-http3-syscall-fault.test.ts