Repository navigation
Conversation
|
Updated 7:51 PM PT - Jul 2nd, 2026
❌ @robobun, your commit 33a7392 has some failures in 🧪 To try this PR locally: bunx bun-pr 29135That installs a local version of the PR into your bun-29135 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdd a helper to detect async UDP Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29116.test.ts`:
- Around line 1-9: The file's top-of-file prose is overly long; trim the
multi-line historical/context comment at the top of
test/regression/issue/29116.test.ts down to the repository regression-test
convention by keeping only a single-line comment with the GitHub issue URL (e.g.
the issue link present in the header) and removing the remaining explanatory
paragraphs and Linux-only notes so the test header is minimal.
🪄 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: c54b3d69-d5f7-4d61-8736-b437f6555edb
📒 Files selected for processing (1)
test/regression/issue/29116.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29116.test.ts`:
- Around line 52-85: Add a positive-control test that verifies connected UDP
sockets still surface recv-side ICMP errors: create a new test (mirroring the
skipped TOCTOU test) that uses runScript to create a temporary bound UDP socket
to obtain a deadPort, close it, then create a client socket, bind it, call
socket.connect(deadPort, "127.0.0.1") first (use createSocket and
socket.connect), perform one or more socket.send(Buffer.from("x"), deadPort,
"127.0.0.1") on the already-connected socket, wait briefly, close the socket,
and assert that stderr contains "ECONNREFUSED" (and stdout prints "done" and
exitCode is 0) to ensure connected sockets still observe ICMP errors; name the
test clearly (e.g., "#29116 connected socket surfaces ICMP errors") so it's easy
to find alongside the existing test.
🪄 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: 449f759a-26e6-4d9c-9b61-04c1a707de9d
📒 Files selected for processing (1)
test/regression/issue/29116.test.ts
There was a problem hiding this comment.
Both issues addressed — the simplified syscall === "recv" suppression is cleaner than the original allowlist and closes the EMSGSIZE gap and TOCTOU race in one stroke. LGTM.
Extended reasoning...
Overview
The PR fixes a regression introduced in #28827 where unconditional IP_RECVERR enablement caused ICMP-class recv errors (ECONNREFUSED, EHOSTUNREACH, EMSGSIZE, etc.) to surface as unhandled error events on node:dgram sockets, crashing processes without an error listener. Changes are confined to src/js/node/dgram.ts (one helper + one guard line) and a Linux-only regression test.
Security risks
None. The change only suppresses async recv-side errors that Node.js/libuv never exposes in the first place; it does not affect auth, crypto, or permissions.
Level of scrutiny
Low — this is a small, well-scoped compatibility shim in the dgram JS layer. The logic is straightforward: any error arriving through the Bun.udpSocket error: callback with syscall === "recv" is silently dropped, matching the observable behavior of Node.js (which never enables IP_RECVERR at all). The comment block explains the reasoning thoroughly.
Prior review
A previous review from this bot flagged two issues: (1) a TOCTOU race where checking connectState at ICMP-delivery time could leak errors queued before connect(), and (2) a missing EMSGSIZE case for PMTU-discovery errors. The author resolved both by dropping the per-code allowlist entirely in favor of the blanket syscall === "recv" check, which is actually the correct long-term fix — it matches Node.js semantics regardless of what new ICMP errnos the kernel might surface in the future. Both inline comments are marked resolved and the TOCTOU test case was added.
There was a problem hiding this comment.
LGTM — the revised approach (suppress any error unconditionally) cleanly resolves both the TOCTOU race and the incomplete error-code allowlist I flagged.
Extended reasoning...
Overview
The PR adds a one-line guard in 's callback that suppresses any error whose is , plus a regression test covering the original crash repro and the TOCTOU scenario. Two files changed: and .
Security risks
None. This is purely a compat-layer suppression in the Node.js dgram polyfill; it does not affect authentication, crypto, or any security-sensitive path.
Level of scrutiny
Low. The change is minimal: a two-line helper and a single early-return. The fix is backed by a detailed comment explaining exactly why is the right discriminant (Node.js never surfaces this channel at all), and both tests exercise the previously crashing scenarios.
Other factors
My prior reviews flagged a TOCTOU race (connectState checked at ICMP-delivery time rather than send time) and a missing case. Both are moot under the new approach: by dropping the allowlist and the connectState check entirely, the implementation is simpler and more correct. CodeRabbitai's positive-control test suggestion would not hold under the new semantics (recv-side ICMP is now suppressed on connected sockets too, intentionally), so the absence of that test is fine.
There was a problem hiding this comment.
LGTM — the core fix is correct and both issues from my prior review (missing EMSGSIZE case, missing positive-control test) were addressed.
Extended reasoning...
Overview
Two files changed: src/js/node/dgram.ts adds an isSuppressibleRecvError helper that suppresses ICMP-class recv errors on unconnected sockets while passing them through on connected ones; test/regression/issue/29116.test.ts adds Linux-only regression tests including a negative case (unconnected send silently drops ECONNREFUSED) and a positive-control case (connected send still crashes with ECONNREFUSED).
Security risks
No security-sensitive paths touched. The change only affects error propagation in the node:dgram JS compat layer and cannot elevate privileges or expose data.
Level of scrutiny
Low. This is a targeted compat-layer fix: one conditional return in the error: callback passed to Bun.udpSocket. The logic is straightforward and matches documented Node.js behavior. The EMSGSIZE case was added after my prior review, completing the allowlist.
Other factors
The remaining inline nit (positive-control subprocess lacking a socket.close() after Bun.sleep(500)) is a minor test-reliability issue in ICMP-filtered CI environments; it does not affect the correctness of the production fix. The inline comment already flags it.
There was a problem hiding this comment.
LGTM — all previously raised issues addressed (EMSGSIZE, EOPNOTSUPP, EACCES/EPROTO for IPv6, test hang fix); the allowlist now covers the full IPv4 and IPv6 ICMP kernel tables with clear documentation.
Extended reasoning...
Overview
Fixes a regression introduced in #28827 where Bun unconditionally enables IP_RECVERR on UDP sockets, causing unconnected sockets to crash on ICMP errors that Node.js/libuv silently drops. Changes are confined to src/js/node/dgram.ts (new isSuppressibleRecvError helper + one-liner integration) and a Linux-only regression test.
Security risks
None. The fix suppresses recv-side ICMP errors on unconnected sockets, matching Node.js semantics. Connected sockets still surface errors (positive-control test verifies this). No auth, crypto, or permission code is touched.
Level of scrutiny
Moderate — this is compat-layer JS code, not a native code change. The logic is straightforward: filter by connect state + syscall + errno allowlist. The allowlist is now exhaustively documented against both the IPv4 icmp_err_convert[] and IPv6 icmpv6_err_convert / tab_unreach[] kernel tables, verified by the author against upstream source.
Other factors
All issues from prior review rounds were correctly addressed. The TOCTOU concern (connect-state check at delivery time vs. send time) was acknowledged and accepted with sound rationale — the window is extremely tight on loopback and real WebRTC/ICE code attaches error listeners before probing. Test coverage includes both the regression repro and a positive-control subprocess to prevent over-suppression from silently passing.
There was a problem hiding this comment.
LGTM — all previously flagged issues resolved; the allowlist now covers the full IPv4 and IPv6 / kernel tables with inline documentation.
Extended reasoning...
Overview
The PR adds to and wires it into the callback in . It suppresses ICMP-class async recv errors on unconnected UDP sockets, matching Node.js/libuv behavior where is never enabled. A Linux-only regression test in validates the fix.
Security risks
None. This is a compat-layer error-suppression change in . It narrows the set of errors surfaced to application code on unconnected sockets, matching what Node.js already does. No auth, crypto, or permissions code is touched.
Level of scrutiny
Moderate. The change is in a compatibility shim (), not in hot-path native code. The logic is straightforward: one extra guard before . The comprehensive kernel table comment (IPv4 + IPv6 /) makes future maintenance clear.
Issues addressed across review rounds
Multiple gaps in the initial allowlist were found and fixed through iterative review:
- EMSGSIZE (PMTU discovery / FRAG_NEEDED) — added after review
- EOPNOTSUPP (ICMP_SR_FAILED) — added after kernel table cross-check
- EACCES + EPROTO — added after discovering IPv6 path was uncovered (ICMPv6 ADM_PROHIBITED, POLICY_FAIL, REJECT_ROUTE → EACCES; PARAMPROB → EPROTO)
- Test socket-close fix to prevent indefinite hang in filtered-ICMP CI environments
The positive-control connected-socket test was added then dropped due to deterministic flakiness on aarch64 ASAN CI; the comment in the test file explains the rationale, and the named allowlist provides adequate review-time regression signal.
There was a problem hiding this comment.
LGTM — all raised issues addressed: EMSGSIZE, EOPNOTSUPP, EACCES/EPROTO (ICMPv6), and the subprocess socket-close hang are all fixed. The final allowlist is well-documented against both the IPv4 and IPv6 kernel tables.
Extended reasoning...
Overview
This PR fixes a regression in on Linux where enabling / unconditionally in uSockets (#28827) caused ICMP-class recv errors to surface on unconnected UDP sockets, crashing processes that sent best-effort UDP without an error listener. The fix is confined to (a new helper + a one-line call in the callback) and a Linux-only regression test.
Security risks
None. The change only suppresses specific ICMP-class recv errors on the compat layer for unconnected sockets. Connected sockets still surface all errors, matching Node.js semantics.
Level of scrutiny
This is a targeted compat-layer fix with clear Node.js semantics as the reference. The allowlist approach (rather than suppressing all recv errors) is the correct design; the comprehensive kernel mapping comments make future maintenance straightforward. Scrutiny is appropriate for a regression fix.
Other factors
All issues I raised during review were addressed: EMSGSIZE (8a82516), EOPNOTSUPP (c5ab6dc), EACCES + EPROTO for ICMPv6 (d23f5b2), and the subprocess socket-close hang that could cause test framework timeouts (a14dc36). The TOCTOU concern about connect-state delivery timing was acknowledged and accepted with sound rationale. The bug hunting system found no additional issues. The test comment explaining why the positive-control subprocess test was removed is a good call given ASAN/aarch64 flakiness concerns.
Linux drops ICMP errors (port unreachable, host unreachable, TTL exceeded) on unconnected UDP sockets unless IP_RECVERR is set. #28827 enabled it at bind time for every UDP socket, so a socket that sent a datagram to a peer that has since gone away now gets the ICMP reply back as an 'error' event. node never delivers those for unconnected sockets, so dgram code does not install an 'error' handler and the uncaught error takes the process down. Enabling IP_RECVERR also sets sk_err, which the socket's next sendmsg consumes: the error surfaces in the callback of an unrelated send() to a different peer, and that datagram is silently dropped. Arm the error queue in bsd_connect_udp_socket instead and purge it in bsd_disconnect_udp_socket, so only a socket with a single peer to attribute an ICMP error to reports one. Connected sockets keep the error event and stay open, which is what node does.
01a591d to
3c69544
Compare
|
Reworked this to fix the root cause instead of filtering at the The previous diff suppressed ICMP-class errnos in dgram's
The sockopt is the layer that owns the invariant. Tests moved to |
setsockopt(IP_RECVERR, 0) purges the socket's error queue but leaves sk_err set, so disconnecting right after a peer's ICMP error left EPOLLERR armed with nothing to drain, and the UDP poll closed the socket. Read SO_ERROR in that case and surface it the same way, which also covers a connected socket whose IP_RECVERR setup failed and therefore never queues an error at all.
The callback also fires after disconnect() via the SO_ERROR fallback in loop.c, when the socket is no longer connected, so drop the inaccurate "only for connected sockets" clause and describe both delivery paths.
|
The only red CI lane on the last two builds (#68082, #68099) is `:darwin: 26 aarch64 - test-bun`, failing before any test runs with: ``` That is a Buildkite artifact-download infra timeout on the darwin runner, not a test failure, and the latest commit (bef19a5) is a comment-only doc fix so it cannot have changed behavior. All other lanes are green and every review thread is resolved. This needs a maintainer re-run / merge; I am not pushing another retrigger. |
There was a problem hiding this comment.
All prior feedback addressed and nothing new from the bug hunt, but this touches the native UDP poll dispatch in loop.c (EPOLLERR/error-queue/SO_ERROR semantics) and connect/disconnect sockopt gating in bsd.c — worth a human sign-off given how many kernel-level edge cases this went through to get right.
Extended reasoning...
Overview
The PR moves IP_RECVERR/IPV6_RECVERR from unconditional-at-create (bsd_create_udp_socket) to armed-on-connect / disarmed-on-disconnect via a new bsd_udp_socket_set_recverr() helper in packages/bun-usockets/src/bsd.c. It also adds an SO_ERROR fallback in the POLL_TYPE_UDP EPOLLERR path of packages/bun-usockets/src/loop.c to consume sk_err when the MSG_ERRQUEUE drain comes up empty (the disconnect-with-pending-ICMP case). src/runtime/socket/udp_socket.rs only has a doc-comment update. Three test files add coverage for unconnected-no-error, unconnected-no-poison, connected-still-errors, and disconnect-keeps-socket-usable.
Security risks
None identified. No user-controlled input reaches new parsing/allocation paths; the change gates a kernel sockopt on connect state and reads SO_ERROR on an fd already owned by the poll. No auth/crypto/permission surface.
Level of scrutiny
High. This is native C in the core event-loop dispatch (us_internal_dispatch_ready_poll, POLL_TYPE_UDP arm) and the BSD socket layer, with correctness depending on Linux kernel internals (sk_err vs sk_error_queue lifetime, skb_errqueue_purge semantics, level-triggered EPOLLERR). The PR went through ~8 revisions during review — the original dgram.ts errno-allowlist approach was abandoned after repeated gaps (EMSGSIZE, EOPNOTSUPP, EACCES/IPv6, EPROTO), and the C-layer rewrite itself needed a follow-up fix for the sk_err-outlives-queue silent-close. That history argues for a maintainer familiar with the uSockets UDP path to confirm the final shape.
Other factors
- All my prior inline findings are resolved; the last commit (bef19a5) was a comment-only fix I requested.
- Test coverage is thorough: both
node:dgramandBun.udpSocket, connected and unconnected, plus the disconnect-with-pending-error edge case that the SO_ERROR fallback exists for. PR description reports the full UDP suite + node parallel dgram tests + HTTP/3 (sharesus_create_udp_socket) green. - The
Bun.udpSocketAPI contract changes here (unconnected sockets no longer receive ICMPerrorcallbacks) — intentional and now correctly documented in the PR description, but it's a user-visible behavior change on a Bun-native API that a maintainer should ratify. - The "disconnect() with pending ICMP error" test relies on loopback ICMP arriving synchronously before
disconnect()runs; it's Linux-gated and awaits the actual'error'event rather than sleeping, so it should be deterministic, but it's the kind of kernel-timing-adjacent test worth a second pair of eyes.
|
Superseded by #35922, which carries this approach forward onto the current two-arg |
|
Closing in favor of #35922. The #29116 repro (an unconnected The second symptom described here is still present on current main (f426a8e). |
Fixes #29116
Repro
nodeexits 0.bunexits 1:Any UDP service (DNS forwarder, statsd emitter, game server) dies the moment a
peer goes away or a host answers with ICMP unreachable.
There is a second symptom. An unrelated
send()that follows inherits theerror, and its datagram never leaves:
Cause
#28827 enabled
IP_RECVERR/IPV6_RECVERRon every UDP socket inbsd_create_udp_socket. Linux normally drops ICMP errors for an unconnected UDPsocket (
__udp4_lib_err:if (!harderr || sk->sk_state != TCP_ESTABLISHED) goto out;), and node relies on that — libuv exposes the flag as an opt-in bind flag(
UV_UDP_LINUX_RECVERR) andlib/dgram.jsnever passes it.With the flag set, the kernel queues the ICMP error and sets
sk_err:EPOLLERRand becomes an'error'eventsk_erris consumed by whatever syscall touches the socket next, so a latersendmsg()returnsECONNREFUSEDand the error lands in that send's callbackFix
Set
IP_RECVERRinbsd_connect_udp_socketand clear it (which purges thekernel's error queue) in
bsd_disconnect_udp_socket. Only a socket with onepeer to attribute an ICMP error to reports one, matching node:
'error'event, nosk_err,so nothing poisons a later
send()ECONNREFUSED, and the socketstays open rather than silently closing (the behavior udp: surface IP_RECVERR ICMP errors and MSG_TRUNC truncation flag #28827 added for Error occurred when attempting to send data to an UDP client which is not reachable #18029)
Disabling
IP_RECVERRpurges the error queue but leavessk_errset, sodisconnect()right after an ICMP error leftEPOLLERRarmed with nothing todrain and the UDP poll closed the socket. The second commit reads
SO_ERRORwhen the error queue comes up empty and surfaces it the same way, which also
covers a connected socket whose
IP_RECVERRsetup failed and therefore neverqueues anything.
This supersedes the previous approach on this branch, which filtered ICMP-class
errnos out of
node:dgram'serrorcallback. That left the same process kill onBun.udpSocket, did not address the mis-attributed send callback at all, andneeded an ever-growing errno allowlist to keep up with the kernel's ICMP tables.
Verification
test/js/bun/udp/dgram.test.ts'error'(handler-less socket, exit 0)send()(5 dead/live send pairs;sendErrorsempty, all 5 live datagrams delivered)disconnect()with a peer's ICMP error pending keeps the socket usabletest/js/bun/udp/udp_socket_recv_flags.test.tsECONNREFUSEDand stays openBoth files fail on
mainand pass with this change. 42/42dgram.test.ts,224/225 across
test/js/bun/udp/+test/js/node/dgram/(the one failure,IPv6 multicast membership, also fails on
mainin this container), 69/69test/js/node/test/parallel/test-dgram-*, and the HTTP/3 suites (97 tests,us_create_udp_socketshares this path) are green.Before, on
main