Repository navigation
Conversation
us_udp_socket_send only classified EWOULDBLOCK as retryable; ENOBUFS and ENOMEM (transient kernel resource exhaustion, routine on macOS when bursty sends fill the interface queue) fell through to the error return, so Bun.udpSocket.send threw and node:dgram's send callback errored where libuv classifies both as EAGAIN for UDP sends and retries on the next writable event. Route them through the existing would-block path (bsd_send_is_transient_error, already used by the TCP send path), which re-arms writable so on_drain drives the retry. Also hook bsd_sendmmsg into the sendmsg fault-injection chokepoint so the UDP send path can be exercised deterministically.
|
Warning Review limit reached
Next review available in: 12 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 (4)
Comment |
Cleanup placed after the assertions leaked the sockets whenever an expect threw; Bun.udpSocket implements Symbol.dispose and node:dgram implements Symbol.asyncDispose, so using declarations cover every exit path.
|
Updated 11:54 PM PT - Aug 6th, 2026
✅ @robobun, your commit e2a05fa8f9021e94608245ae8e3aafb964176c88 passed in 🧪 To try this PR locally: bunx bun-pr 37092That installs a local version of the PR into your bun-37092 --bun |
There was a problem hiding this comment.
LGTM — the socket-cleanup nit from the earlier pass is addressed (all sockets now held by using/await using), and no new issues turned up on the follow-up commit.
What was reviewed:
udp.c: the one-line|| bsd_send_is_transient_error()reuses the helper the TCP send path already relies on and matches libuv's ENOBUFS/ENOMEM→EAGAIN classification; hard errnos still fall through to the error return.bsd.c: the newUS_FAULT_CHECKinbsd_sendmmsgis byte-for-byte the same shape as the existing hooks inbsd_sendmsg/bsd_recvmsgand compiles out when fault injection is off;bsd_sendmmsg's only caller isus_udp_socket_send.- Tests: confirmed
UDPSocketdefines@@dispose→closeandnode:dgramdefinesSymbol.asyncDispose, so theusingdeclarations clean up on every exit path; the negative case (EINVAL still throws) and the multi-batchsendManypartial-count path are both covered.
Extended reasoning...
Overview
Two native files and one new test file. packages/bun-usockets/src/udp.c gains a single || bsd_send_is_transient_error() clause so us_udp_socket_send treats ENOBUFS/ENOMEM the same as EAGAIN: re-arm writable and return the partial count instead of erroring. packages/bun-usockets/src/bsd.c adds a three-line US_FAULT_CHECK(US_FAULT_SENDMSG, ...) hook at the top of bsd_sendmmsg so the UDP send path can be fault-injected like every other send/recv wrapper in that file. test/js/bun/udp/udp-syscall-fault.test.ts exercises both Bun.udpSocket and node:dgram under injected ENOBUFS/ENOMEM/EINVAL, plus a >1-batch sendMany case.
Security risks
None identified. This narrows the set of errnos treated as hard failures on a send path that is already caller-driven; no untrusted input parsing, no new resource acquisition, and the fault-injection hook is compiled out unless LIBUS_SOCKET_FAULT_INJECTION is defined (the macro expands to a constant 0 otherwise, per fault_inject.h).
Level of scrutiny
Native networking code, but the behavioral change is a one-clause OR that reuses bsd_send_is_transient_error() — the exact helper #33803 introduced for the TCP send path — and mirrors libuv's uv__udp_sendmsg classification cited in the PR body. I confirmed bsd_sendmmsg's sole caller is us_udp_socket_send, so no other consumer's error contract changes. The bsd.c hook is verbatim the pattern used by bsd_sendmsg, bsd_recvmsg, bsd_writev, bsd_write2, bsd_recv, bsd_send, bsd_accept_socket, and bsd_do_connect_raw in the same file. Given the precedent on both axes (libuv semantics; existing in-tree helper and hook pattern), I'm treating this as a low-risk mechanical extension rather than novel networking logic.
Other factors
My earlier review flagged that the tests placed .close() after assertions; commit 76dc5e45 switched every socket to using/await using. I verified the disposers exist: UDPSocket defines "@@dispose": { fn: "close" } in sockets.classes.ts, and node:dgram defines Socket.prototype[Symbol.asyncDispose]. The test file guards with !fault.available() || isWindows, clears fault state in afterEach, and asserts the negative contract (EINVAL still throws) so the reclassification isn't over-broad. The PR body reports the full test/js/bun/udp/ suite and all 76 test-dgram-* parallel tests still pass. Nothing outstanding on the thread; the inline comment is resolved.
libuv retries EAGAIN/EWOULDBLOCK/ENOBUFS for UDP sends and surfaces ENOMEM as a send error, as does Node; inheriting the TCP helper's wider set made ENOMEM silently retryable. Add bsd_udp_send_is_transient_error with exactly libuv's set and keep the TCP helper unchanged. Tests: pin the EWOULDBLOCK baseline through the new hook, assert ENOMEM still throws, and consume the creation-time writable drain before injecting so the post-send drain await genuinely requires the re-arm (verified by deleting the re-arm: four tests then fail).
There was a problem hiding this comment.
No issues found; the earlier socket-cleanup nit is addressed. Deferring because this reclassifies an errno on the native UDP send path — small and well-argued against libuv, but a semantic change to error handling that a maintainer should sign off on.
What was reviewed:
us_udp_socket_send— ENOBUFS now takes the existing would-block re-arm path; hard errnos still return negative.bsd_udp_send_is_transient_error()vs libuv'suv__udp_sendmsgretry set (EAGAIN/EWOULDBLOCK/ENOBUFS only; ENOMEM excluded and covered by a negative test).- New
US_FAULT_CHECKinbsd_sendmmsg— compiles to 0 when fault injection is off;ssize_talready available at that point on all platforms. - Windows path: real
WSAENOBUFSstill reaches the new predicate viaWSAGetLastError(); test file skips Windows so injected paths aren't relied on there.
Extended reasoning...
Overview
The PR reclassifies ENOBUFS from UDP sendmmsg/sendmsg as backpressure rather than a hard error, matching libuv's UDP send retry set. The functional change is one added disjunct in us_udp_socket_send (|| bsd_udp_send_is_transient_error()) plus an 8-line helper in bsd.c and its header declaration. The rest is test infrastructure: a 3-line US_FAULT_CHECK hook at the top of bsd_sendmmsg (following the exact pattern already used in bsd_send/bsd_sendmsg/bsd_writev/quic.c) and a new 138-line fault-injection test file.
Security risks
None identified. The change loosens classification of one transient kernel errno on the send path — no user-controlled input parsing, no auth/crypto, no bounds. The fault-injection hook is debug-only (compiled out unless LIBUS_SOCKET_FAULT_INJECTION is set) and follows the existing pattern.
Level of scrutiny
Medium-high. The diff is small but sits on the hot UDP send path used by both Bun.udpSocket and node:dgram, and it changes what surfaces as an error vs. backpressure. The PR is well-argued (cites libuv unix/udp.c, explains why ENOMEM is deliberately excluded to match Node) and well-tested (both entry points, both retry-set members, both hard-errno negatives, batch-boundary case, drain re-arm proven load-bearing). This is exactly the kind of Node-compat semantic decision — narrowing/widening an error classification — that a maintainer should confirm rather than land on bot approval alone.
Other factors
- My prior nit about bare
.close()after assertions was addressed in 76dc5e4 withusing/await using; the thread is resolved. - I checked that
us_fault_hitsets botherrnoandWSASetLastErrorforUS_FAULT_ERRNO, so the injected -1 correctly satisfiesbsd_udp_send_is_transient_error()on POSIX (Windows is skipped in the test). - On the real (non-injected) Windows path,
bsd_sendmmsgmaps unknown WSA errors toerrno = EIObut leavesWSAGetLastError()intact, so the new predicate (which readsWSAGetLastError() == WSAENOBUFSon Windows) still fires for a genuineWSAENOBUFS. - The PR body notes the tests are platform-gated and were deferred to CI on the author's machine, which is another reason to have a human confirm CI is green before merging.
|
A measurement from #44800 that applies to this PR. #44800 tried the same approach for With this PR, |
Repro
On macOS, flooding
send()with large datagrams routinely fails withENOBUFSonce the interface queue fills (transient; the same burst completes under Node). Deterministically, with the syscall fault hooks in a debug build:node:dgram's send callback gets the samesend ENOBUFSerror. Under Node/libuv neither happens: the datagram is retried on the next writable event and the callback completes without an error.Cause
us_udp_socket_sendclassified onlyEWOULDBLOCKas retryable (bsd_would_block()), soENOBUFSfromsendmmsgfell through to the error return with no writable re-arm. libuv's UDP send retry set is exactlyEAGAIN/EWOULDBLOCK/ENOBUFS(uv__udp_sendmsg/uv__udp_try_send; ENOBUFS was added in libuv v1.15.0 because macOS reports it for transient interface-queue fullness), and it retries onPOLLOUT.Fix
packages/bun-usockets/src/udp.c: routeENOBUFSthrough the same path as would-block: re-arm writable (soon_drainfires) and report the packets already accepted.Bun.udpSocket.sendthen returnsfalse(resend ondrain), andnode:dgram's existing queue/drain machinery retries automatically. Hard errnos still surface as errors.packages/bun-usockets/src/bsd.c: newbsd_udp_send_is_transient_error()with exactly libuv's set. It is deliberately narrower thanbsd_send_is_transient_error()(the TCP drain-path helper from usockets: treat ENOBUFS/ENOMEM from send() as would-block, not fatal #33803, which also treatsENOMEMas transient): libuv and Node surfaceENOMEMfor UDP sends, so Bun does too.packages/bun-usockets/src/bsd.c: hookbsd_sendmmsginto the existingsendmsgfault-injection chokepoint so the UDP send path can be tested deterministically (it was the only send path without a hook).Matching libuv here is the whole argument: Node applications never observe transient
ENOBUFSas a send failure, so code ported to Bun errored (or crashed, with no callback) on a condition it was never written to handle, and there was no writable event to recover with even for callers that did handle it.Verification
test/js/bun/udp/udp-syscall-fault.test.ts(POSIX, skipped when fault injection is compiled out):udpSocket.sendunder injectedENOBUFSreturnsfalse, firesdrain, and the retry delivers the datagram; anEWOULDBLOCKcell pins the would-block baseline the new classification is specified againstEINVAL, andENOMEM(not in libuv's UDP retry set)node:dgramsend callback completes without error after three consecutive transient failures (initial try + two drain retries, so at least two drains require the failed send's re-arm)sendManyhittingENOBUFSat a batch boundary (>1 sendbuf batch) returns the partial count and re-arms writable so the tail drains, instead of stranding itus_poll_changere-arm makes four of the six tests failWith
packages/reverted to the merge base (test file kept), 5 of 6 tests fail; with the fix the file passes. Also green:test/js/bun/udp/(276 tests), all 76test/js/node/test/parallel/test-dgram-*.js,net-syscall-fault.test.ts,socket-fault-injection.test.ts,fetch-http3-syscall-fault.test.ts.Related: #36249 handles the hard-errno case at a batch boundary (stale ICMP
sk_err); this PR only reclassifiesENOBUFS, so the two compose.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/bun/udp/udp-syscall-fault.test.ts