Repository navigation
Conversation
…atch us_udp_socket_send() sends a large batch in passes of about 204 datagrams. When a later pass fails outright with an errno that is not EAGAIN, it returns the count from the earlier passes. That path did not arm the writable poll, so the caller received a short count and then waited for a drain event that never fired. Every other path that returns a short count arms the poll. This path now does the same. The caller resends from the failed datagram on drain, and that call reports the errno because the datagram is now the first one.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 4 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 4 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 2 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review. The fix and the test are pushed. How to reproduce. The script follows the documented protocol: if // bun repro.ts
const rx = await Bun.udpSocket({ socket: { data() {} } });
let drained = Promise.withResolvers<void>();
const tx = await Bun.udpSocket({
connect: { port: rx.port, hostname: "127.0.0.1" },
socket: { drain: () => drained.resolve() },
});
await drained.promise; // socket creation fires one drain, wait it out
const BAD = 204; // first datagram of pass 2 on Linux. Use 227 on macOS. Try 1 as the control.
const packets = Array.from({ length: BAD + 2 }, (_, i) => (i === BAD ? Buffer.alloc(70000) : "x"));
let offset = 0;
while (offset < packets.length) {
drained = Promise.withResolvers<void>();
const sent = tx.sendMany(packets.slice(offset)); // throws EMSGSIZE once the bad one is first
console.log(`sendMany(from #${offset}) -> ${sent}`);
offset += sent;
if (offset < packets.length) {
const ok = await Promise.race([drained.promise.then(() => true), Bun.sleep(1500).then(() => false)]);
if (!ok) { console.log("HANG: short count, and drain never fired"); process.exit(1); }
}
}
A pass holds 204 datagrams on Linux and 227 on macOS, so the index that starts pass 2 differs. The table above is from Linux. The macOS hang at index 227 follows from the same code path but was not reproduced at run time. |
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 finding, I also checked for sibling sites needing the same re-arm: us_udp_socket_send in packages/bun-usockets/src/udp.c is the single send loop for all backends (Windows/macOS go through the bsd_sendmmsg emulation in bsd.c and return a short count via the sent < count branch, which already arms writable), so no parallel path is missed. I also checked whether the extra arm can misfire: loop.c:1053 clears WRITABLE before on_drain, so it fires once and cannot spin.
Extended reasoning...
Findings were reported inline, so this is the brief ruled-out note only. The fix is a small, self-contained change to one exit path of us_udp_socket_send, and the two concerns most likely to be raised in addition to the posted finding (a missed sibling backend, and a spurious or repeating drain) were checked against the code: udp.c is not platform-gated, bsd_sendmmsg is the per-platform shim, and the writable poll is one-shot with the bit cleared before the callback. The test's reliance on a Linux-specific batch size is covered by the inline comment and is the reason a human should still weigh whether macOS coverage matters before merging.
A pass holds (16384 - 8) / (sizeof(mmsghdr) + sizeof(iovec)) datagrams. That is 204 on Linux and 227 on macOS, because struct msghdr is 48 bytes on Darwin and 56 on Linux. Index 204 and 408 are inside a pass on macOS, so the test passed there without the fix. Add index 227 and 454. Each index starts a pass on one platform and is a control on the other.
|
Updated 9:51 PM PT - Sep 16th, 2026
✅ @robobun, your commit 9038c395da4af038da495dc8864198120c78d131 passed in 🧪 To try this PR locally: bunx bun-pr 42999That installs a local version of the PR into your bun-42999 --bun |
There was a problem hiding this comment.
LGTM — the follow-up commit adds the 227/454 cases, so the re-armed failed-batch branch is now exercised at the pass boundary on macOS as well as Linux.
What was reviewed:
- The new
total_sent > 0branch inus_udp_socket_send(packages/bun-usockets/src/udp.c:62-70): it only re-arms the one-shot writable poll and returns the same count the old ternary did; thetotal_sent == 0path still returns the negativesentso errno is thrown, and the EAGAIN branch above is untouched. - The test's batch-boundary indices against
bsd_udp_setup_sendbufsizing (204 Linux / 227 macOS) and its wait for the socket-creation drain before the sendMany loop, so an unfixed build cannot satisfy the awaited drain. - Whether the fix could spin: the poll is cleared in loop.c before
on_drain, so an extra arm produces one extra drain at most.
Extended reasoning...
Overview
The PR changes one branch in us_udp_socket_send (packages/bun-usockets/src/udp.c) and adds a test.each case to test/js/bun/udp/udp_socket.test.ts. Previously, when a later sendmmsg pass failed hard (non-EAGAIN) after earlier passes succeeded, the function returned the short count without arming the writable poll, so a sendMany() caller following the documented wait-for-drain-then-resend protocol never got a drain. The fix arms the poll before returning the short count, matching what the other two short-count exits in the same function already do. When nothing was sent, the negative return is preserved so the errno surfaces as a throw.
Security risks
None. The change does not touch parsing, allocation, bounds, or credentials; it only adds a us_poll_change call on an existing return path. The kernel-reported sent value is used exactly as before.
Level of scrutiny
Low to moderate. The C change is a few lines with clear intent and no new state or ownership. The main things worth checking were (a) that the added arm cannot cause a busy loop — the writable poll is one-shot and cleared before on_drain, so at worst one extra drain fires, and (b) that the test actually fails on an unfixed build for the right reason. The test waits out the creation-time drain before sending, and on an unfixed Linux build the index-204/408 cases hang on the awaited drain (timing out), while the fixed build throws EMSGSIZE at the expected offset. The prior review's nit about macOS pass size (227, not 204) was addressed in the second commit by adding 227 and 454 to the matrix, which are harmless mid-pass controls on Linux.
Other factors
The test does not wire socket error/close to reject the drain promise, so a regression manifests as a timeout rather than an explicit failure; that is acceptable for a hang-detection regression test and matches the existing tests in this file. The bug-hunting run exited with dry_streak and reported no findings. No third-party CHANGES_REQUESTED reviews are recorded in the timeline; the only prior inline comment was from this bot and has been addressed by the follow-up commit.
Problem
Bun.udpSocket().sendMany()can return a short count and never firedrain. A caller that follows the documented protocol (wait fordrain, resend the rest) waits forever, with no error.EMSGSIZE) at index 204 return204.us_udp_socket_send(packages/bun-usockets/src/udp.c:62).return total_sent > 0 ? total_sent : sentreturns the short count but does not arm the writable poll.Fix
drainthe caller resends from the failed datagram. It is now the first one, so the call throws its errno.sendMany()with more than one pass reaches this path.send(),node:dgramandnode:quicsend one datagram per call.test/js/bun/udp/udp_socket.test.ts. On Linux main, index 204 and 408 time out. With the fix all 223 pass. Alsodgram.test.ts,udp_socket_recv_flags.test.ts, 75test-dgram-*.jsfiles.Background
sendmmsg(2)sends many datagrams per call. If one fails after another was sent, it returns the count and drops the errno. A retry from that datagram reports it.us_udp_socket_sendsends a large batch in passes of 204 datagrams on Linux and 227 on macOS. node:quic on lsquic — Node v26 compat, HTTP/3 #32602 made all three platforms return a count, never-1, for a failure after the first datagram.loop.cclears it beforeon_drain, so an extra arm cannot spin.Notes
Why mid-pass already worked. An oversized datagram inside a pass makes
sendmmsgreturn short, which takes thesent < countbranch, and that branch arms the poll. When it is the first datagram of a pass, the whole pass returns-1, which takes the other branch. The two branches that return a short count disagreed about the poll.Trace from a caller that follows the protocol, Linux, main
c6b7fcb5b:With the fix:
Pass size per platform.
bsd.ccomputes it as(LIBUS_SEND_BUFFER_LENGTH - sizeof(struct udp_sendbuf)) / (sizeof(struct mmsghdr) + sizeof(struct iovec)), which is(16384 - 8) / (mmsghdr + 16).struct msghdrstruct mmsghdrbsd.h:65)Darwin declares
msg_iovlenasintandmsg_controllenassocklen_t, 4 bytes each. Linux usessize_tfor both. The Linux row is checked with a clang_Static_assert. Both rows are checked withconstasserts against thelibccrate forx86_64-unknown-linux-gnu,aarch64-apple-darwinandx86_64-apple-darwin. A negative control that expects 204 on Apple fails to compile, so the assert is live.What is verified where. Linux: the hang and the fix are reproduced at run time. Windows: a canary (
ebfad4cda) converges for every index without this change, because its single pass returns short through thesent < countbranch, so the test passes there before and after. macOS: the pass size is machine-checked and the code path is the same as Linux, but the hang at index 227 was not reproduced at run time. No macOS machine was available for that.Test design. The test follows the protocol and does not assert a count per call, so a loaded kernel that accepts fewer datagrams cannot make it fail. It first waits for the one
drainthat socket creation produces. Without that wait, the creation-time event would satisfy the test on an unfixed build. The finaloffsetassertion holds by construction:sendMany()only throws for the first datagram of a call. Index 204 and 408 start a pass on Linux. Index 227 and 454 start a pass on macOS. Each is a control on the other platform, and index 1 is a control everywhere. New test ran 25 times with 0 failures.Review. The first version of the test used only index 204 and 408, and this description said that macOS also holds about 204 datagrams per pass. Both were wrong. On macOS those two indexes are inside a pass, so the test passed there without the fix. Commit
9038c395daadds 227 and 454.Relation to #33391 (closed, stale). #32625 fixed its main finding, the 204-datagram loop, in a different way. Its second half made a mid-batch errno throw at any position. That half is not revived here, because #32602 chose the opposite contract on purpose and it matches
sendmmsg(2)and libuv'suv_udp_try_send2. This PR only repairs the one site where that contract left the caller without adrain.Neighbor PR. #37092 edits the
if (bsd_would_block())line directly above. This change leaves that line alone so the two do not conflict.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_socket.test.ts