Bun.serve: stop using sendfile(2) for file responses on macOS - #33728
Conversation
On macOS sendfile can park uninterruptibly on an XNU turnstile under kernel mbuf pressure and stay parked after the peer task is torn down, leaving the Bun process unkillable (state U, then ?E during exit, then a watchdog panic on shutdown). Observed repeatedly on the 8 GB CI runner. can_sendfile() now returns false on macOS so large plain-HTTP file responses use the BufferedReader path, which is the same non-blocking read/write loop already used for SSL, HTTP/3, Windows, and sub-1MB files. The macOS-only sendfile call in on_sendfile and the cfg gates that kept it reachable are removed. Also skip the 5000-concurrent-connection file-kill test on low-memory macOS; the mbuf exhaustion it causes makes the test unreliable there regardless of the sendfile change.
|
Warning Review limit reached
Next review available in: 6 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 (2)
WalkthroughSendfile support in FileResponseStream.rs is restricted to Linux/Android only, removing macOS-specific fields, initialization, and send-loop implementation, with macOS now falling back to non-sendfile paths. The mid-send stop test in serve.test.ts is rewritten for determinism using fewer concurrent streams. ChangesmacOS sendfile disablement
Sequence Diagram(s)sequenceDiagram
participant Client
participant FileResponseStream
participant OS
Client->>FileResponseStream: start file response
FileResponseStream->>FileResponseStream: can_sendfile()
alt Linux or Android
FileResponseStream->>OS: on_sendfile send loop
OS-->>FileResponseStream: bytes sent
else macOS or other
FileResponseStream->>FileResponseStream: fall back to non-sendfile path
end
FileResponseStream-->>Client: streamed response
Compact metadata
Poem 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
Drop the request count from 5000 to 512 so the test no longer drives macOS runners into kernel mbuf exhaustion (the 16 GB boxes were hitting tens of thousands of denied allocations with the old count). The test only needs concurrent in-flight file sends at the moment of kill, not a specific volume. Add a dedicated test that starts a few file-response streams, stops reading, sends SIGTERM, and awaits proc.exited. That is the assertion missing from the existing test: before the macOS sendfile removal the server could park uninterruptibly in kernel and never exit.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Same hazard as the Bun.serve file-response path: XNU's sendfile allocates mbufs with M_WAIT/no PCATCH before checking SS_NBIO, so under mbuf pressure the client thread can park uninterruptibly. is_eligible() now returns false on macOS and the upload falls back to the buffered write path, matching Windows.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/runtime/server/FileResponseStream.rs`:
- Around line 554-563: The can_sendfile() gate is too broad and can still return
true on FreeBSD even though the on_sendfile backend is not compiled there, which
can make FileResponseStream::start() hit the unreachable! branch. Tighten
can_sendfile() to use the same target_os check as the sendfile backend in
src/runtime/server/FileResponseStream.rs, limiting eligibility to linux/android
and keeping the existing macOS/windows false path intact.
🪄 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: 38d3e7a4-4920-4231-92fd-558576214eea
📒 Files selected for processing (3)
src/http/SendFile.rssrc/runtime/server/FileResponseStream.rstest/js/bun/http/serve.test.ts
… align can_sendfile cfg - Rewrite the stop-in-the-middle test: the old AbortSignal.timeout(10) raced the synchronous fetch-creation loop so requests often aborted before connecting and the server saw zero of them. Now each of 16 requests is opened and one chunk is read so they are provably mid-send when the server is killed, and proc.exited is awaited. The separate killed-mid-send test is folded into this one. - Revert the src/http/SendFile.rs client-side gate. Its fallback is a synchronous whole-file read on the JS thread; changing that path is a separate tradeoff and out of scope for the server fix. - Gate can_sendfile positively on linux/android so it matches on_sendfile exactly; the previous negative form also admitted FreeBSD.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/http/SendFile.rs:22-25— Nit: 403f4a2 leaves two macOS-only blocks dead — the#[cfg(target_os = "macos")]ISREG check atsrc/runtime/webcore/fetch.rs:1673-1679(inside a'use_sendfileblock thatis_eligible()now never enters on macOS), and thewrite()arm atsrc/http/SendFile.rs:100-131. On the latter, the PR-thread rationale "its cfg also covers other non-linux/android/freebsd unixes" doesn't hold:bun_sys::c::sendfileis only defined for macOS (6-arg) and FreeBSD (7-arg), so on any other unix that arm would fail to compile — macOS is the only target it exists for. Per the repo's "delete dead code in the same PR that makes it dead" rule, both should go (narrowing theuse core::ptr;import at :1-2 tofreebsd-only, since the FreeBSD arm still uses it).Extended reasoning...
What this is. Commit 403f4a2 adds
cfg!(target_os = "macos")toSendFile::is_eligible()'s early return, so on macOSis_eligible()now unconditionally returnsfalse. That change makes two macOS-only blocks provably unreachable, and the PR-thread justification for keeping one of them is factually incorrect.Block 1 —
fetch.rs:1673-1679. The'use_sendfileblock atsrc/runtime/webcore/fetch.rs:1666is entered only whenhttp::SendFile::is_eligible(&url)is true (:1665 — the sole call site ofis_eligible()in the tree). Inside it sits:#[cfg(target_os = "macos")] { // macOS only supports regular files for sendfile() if !bun_sys::S::ISREG(stat.st_mode as u32) { break 'use_sendfile; } }
This block only compiles on macOS (it's
#[cfg]-gated, notcfg!()), and on macOS the enclosing guard is now provably false at compile time (cfg!(target_os = "macos")is a constanttrue, sois_eligible()returnsfalsebefore touching the URL). It is therefore dead on the only platform it exists for.Block 2 —
SendFile.rs:100-131. In the resolved review thread, the author wrote: "The write() arm at :96-126 is left in place since its cfg also covers other non-linux/android/freebsd unixes; it just is not reached on macOS anymore." That justification doesn't hold. The arm is gated#[cfg(all(unix, not(any(target_os = "linux", target_os = "android")), not(target_os = "freebsd")))]and callsbun_sys::c::sendfile(fd, s, signed_offset, &raw mut sbytes, ptr::null_mut(), 0)— six arguments matching the Darwin signature. Butbun_sys::c::sendfileis defined only under#[cfg(target_os = "macos")](6 params,src/sys/lib.rs:5221-5232) and#[cfg(target_os = "freebsd")](7 params,:5234-5245). On any other unix matching that cfg (OpenBSD, NetBSD, illumos, …) there is nobun_sys::c::sendfilesymbol at all, so the block would fail to compile — it does not "cover other non-linux/android/freebsd unixes". macOS is the only target that both matches the cfg and successfully compiles the body.Why it's now dead.
is_eligible()is the sole gate beforefetch.rs:1665constructsHTTPRequestBody::Sendfile(at :1714 — the only construction site;FetchTasklet.rsonly copies an existing one), which is the only path that reachesSendFile::write(). On macOS,is_eligible()now returnsfalse, sowrite()is never invoked, and the arm at :100-131 — which only exists on macOS — is unreachable.Step-by-step proof.
- Build target =
aarch64-apple-darwin.cfg!(target_os = "macos")evaluates totrue. SendFile::is_eligible(url)atSendFile.rs:22-26: theifcondition isfalse || true || …, which istrue; returnsfalse.fetch.rs:1665:proxy.is_none() && compress.is_none() && false=false; the'use_sendfileblock at :1666-1720 is skipped. The#[cfg(target_os = "macos")]block at :1673-1679 (which compiled into that skipped block) never executes.- Since :1714 (the sole
HTTPRequestBody::Sendfileconstruction) is inside the skipped block, noSendFileis ever constructed on macOS, soSendFile::write()is never called on macOS. - On any non-macOS/non-FreeBSD/non-Linux/non-Android unix, the arm at
SendFile.rs:100-131referencesbun_sys::c::sendfile, which has no definition (src/sys/lib.rsdefines it only for macos and freebsd) → compile error. So macOS is the only target the arm exists for, and per (4) it never runs there.
Impact. None at runtime — the compiler DCEs both blocks. But CLAUDE.md is explicit: "Delete dead code in the same PR that makes it dead — required scope; name the deletions in the description." Leaving them (a) misleads future readers into thinking macOS still enters the client sendfile path, and (b) leaves an on-record rationale in the PR thread that is verifiably wrong.
Fix. Delete
fetch.rs:1673-1679andSendFile.rs:100-131. Theuse core::ptr;import atSendFile.rs:1-2is currently gated#[cfg(all(unix, not(any(target_os = "linux", target_os = "android"))))], which includes FreeBSD — and the FreeBSD arm at :73-98 still usesptr::null_mut()— so narrow that import's cfg to#[cfg(target_os = "freebsd")]rather than deleting it. Also drop the now-unusedadjusted_counton macOS if the compiler flags it. - Build target =
|
Re the two latest review-bot findings: both are against superseded commits.
Current diff is two files: |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/server/FileResponseStream.rs (1)
554-577: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment exceeds the repo's 3-line limit.
The explanatory comment above the
#[cfg(not(any(...)))]block spans 4 lines (555-558). Per the repo's development notes, code comments should be kept to 3 lines max, with longer explanations reserved for docs.✏️ Suggested condensed comment
- // Matches the cfg on `on_sendfile`. macOS is intentionally excluded: XNU's - // sendfile allocates mbufs with M_WAIT/no PCATCH before the SS_NBIO check, - // so under mbuf pressure it sleeps uninterruptibly and the process becomes - // unkillable. The BufferedReader path is fully non-blocking. + // Matches the cfg on `on_sendfile`. macOS is excluded: XNU's sendfile can + // sleep uninterruptibly under mbuf pressure, making the process unkillable.As per coding guidelines, "Keep code comments to 3 lines max - Comments must be concise. If the code needs more explanation than that, it belongs in docs — and only when explicitly asked for."
🤖 Prompt for 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. In `@src/runtime/server/FileResponseStream.rs` around lines 554 - 577, The comment in can_sendfile is too long and must be condensed to fit the repo’s 3-line limit. Shorten the explanatory block above the cfg-gated branch in FileResponseStream.rs so it keeps only the essential reason for excluding macOS and the non-blocking BufferedReader fallback, while preserving the intent around sendfile and target_os gating.Source: Coding guidelines
🤖 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 `@test/js/bun/http/serve.test.ts`:
- Around line 2142-2146: The header comment in the HTTP serve test is too
verbose and exceeds the repo’s 3-line limit. Condense the comment near the
fixture setup in the serve test so it keeps only the essential context about the
>1 MB file, mid-send streams, and the macOS sendfile/XNU hang behavior, and move
any extra rationale to docs or the PR description if needed.
- Around line 2157-2158: The stdout readiness parsing in the child-process test
is using a single proc.stdout.getReader().read() call, which can truncate the
URL if the line arrives in multiple chunks. Update the test logic around
proc.stdout.getReader() in serve.test.ts to keep reading and buffering until a
newline-delimited line is complete, then decode and trim that full line before
passing it to new URL or fetch. Keep the fix localized to the readiness-waiting
code so the URL is always framed from complete stdout output rather than a
single chunk.
---
Outside diff comments:
In `@src/runtime/server/FileResponseStream.rs`:
- Around line 554-577: The comment in can_sendfile is too long and must be
condensed to fit the repo’s 3-line limit. Shorten the explanatory block above
the cfg-gated branch in FileResponseStream.rs so it keeps only the essential
reason for excluding macOS and the non-blocking BufferedReader fallback, while
preserving the intent around sendfile and target_os gating.
🪄 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: 46ead74d-a583-4209-9ecf-37d0ee58b890
📒 Files selected for processing (2)
src/runtime/server/FileResponseStream.rstest/js/bun/http/serve.test.ts
There was a problem hiding this comment.
All prior feedback is addressed and I found nothing further, but dropping sendfile(2) entirely on macOS is a performance/behavior tradeoff on a hot Bun.serve path that a maintainer should sign off on.
Extended reasoning...
Overview
Two files: src/runtime/server/FileResponseStream.rs narrows the sendfile backend to Linux/Android only (removing the macOS sendfile(2) loop and gating can_sendfile() positively on target_os = "linux" | "android"), and test/js/bun/http/serve.test.ts rewrites the "stop in the middle of a file response" test to be deterministic (16 readers × 3 iterations, asserting signalCode === "SIGTERM") instead of 5000 abort-timed fetches.
All three of my earlier inline comments are resolved: the client-side fetch() sendfile path is now documented as intentionally deferred in the PR description (with the sync-read fallback rationale); stderr: "inherit" matches the sibling test; and can_sendfile now uses the same positive linux/android cfg as on_sendfile, so FreeBSD and every other target correctly falls through to BufferedReader. CodeRabbit's remaining nits (comment length, stdout framing) were addressed or withdrawn.
Security risks
None identified. No auth, crypto, or untrusted-input parsing is touched. The change removes a syscall path on one platform and routes to an existing, already-exercised fallback.
Level of scrutiny
This warrants human review. It changes the code path taken for every ≥1 MB plain-HTTP file response on macOS — a production-critical Bun.serve hot path — trading sendfile throughput for process killability under mbuf exhaustion. The kernel-level analysis in the PR description is thorough and the BufferedReader fallback is battle-tested (SSL, HTTP/3, Windows, <1 MB files already use it), but the decision to drop sendfile entirely rather than, say, cap sbytes is a design call a maintainer should ratify. The github-actions bot also flagged three open user-facing macOS file-serving issues (#26406, #6961, #16927) this may fix, reinforcing that this is a meaningful behavior change.
Other factors
The PR description is candid that the new test "would likely pass on an unfixed build on a machine with headroom" — it guards killability mid-send rather than reproducing the mbuf deadlock, which the description says is unsafe on shared runners. That's a reasonable position but means the fix is validated primarily by the darwin CI file-response suite plus the kernel-source analysis, not by a failing-before/passing-after regression test. cargo check was verified clean on linux/darwin/windows/freebsd targets.
|
CI status for 10cb358 (build 70370):
The red lanes are unrelated to this diff (node:tls / node:http on Windows, postgres protocol tests on Windows and debian-aarch64, and darwin aarch64 test shards are still queued at the time of writing. The diff is ready; flagging for a maintainer to merge once those complete. |
XNU's sendfile(2) allocates its mbuf chain with an uninterruptible wait before it checks for socket space. Under mbuf pressure the HTTP thread sleeps in the kernel, the process cannot be killed, and it keeps the socket buffers it holds. #33728 removed the server-side call for this reason and left the client upload path in place because its only fallback was a synchronous whole-file read on the JS thread. On macOS the client now copies the file through a userspace buffer on the HTTP thread: pread into a 256 KiB scratch buffer, non-blocking send, resume from the file offset on the next writable event. Content-Length, streaming and the 32 KiB eligibility threshold are unchanged. Linux and FreeBSD keep sendfile(2).
2 participants
Why
darwin-test-arm64-5(the 8 GB arm64 test runner) has been ending every CI day with a string ofbuildkite-agent artifact download timed outfailures (#33116), and every nightly reboot ends in a watchdog panic. The box accumulates dozens ofbun-profileprocesses stuck in stateU/?Ethat hold ~130 MB of kernel socket buffers, which exhausts the mbuf pool and collapses network throughput to ~13 KB/s.Sampling one of the wedged processes shows the server side parked in
sendfile(2)on an XNU turnstile whose owner is a thread in a peer task that is already being torn down:In XNU's
sendfile(bsd/kern/uipc_syscalls.c), the mbuf-chain allocation at:3940usesM_WAITat priorityPZERO-1with noPCATCH, and it happens before theSS_NBIO/sbspace()check at:4039-4040. SoO_NONBLOCKon the socket is never consulted for this wait, the sleep is uninterruptible, andSIGKILLsimply pends. Capping thesbytesargument would shrink the allocation but can still land in the same uninterruptible wait when the pool is empty.Process 52438 above is the client side of the same test, stuck in
?Ebecauseexit()is closing its sockets while the mbuf pool is at21504/215044 KB clusters and20M requests for memory denied. Since neither side can make progress,shutdown -rhangs in vfs teardown andAppleARMWatchdogTimerhard-resets the box. The 16 GB runners recover because their larger pool drains before a second process lands on the same lock.What
can_sendfile()now returnstrueonly on Linux/Android (matching the cfg onon_sendfile), so on macOS large plain-HTTP file responses take theBufferedReaderpath: chunkedread()then non-blocking usocketswrite()with backpressure. That path is already used for SSL, HTTP/3, Windows, and files under 1 MB. The macOSsendfilecall inon_sendfileand the cfg gates that kept it reachable are removed as dead code.This does not prevent mbuf exhaustion; a burst of large localhost file responses can still fill the pool. The difference is recovery:
send()on theBufferedReaderpath returnsENOBUFSwhen the pool is empty, and usockets'bsd_would_block()only treatsEWOULDBLOCKas retryable, soENOBUFSsurfaces as a fatal write error, the socket is closed, its clusters are freed, and the process stays killable.sendfilesleeping uninterruptibly in the allocation path is what turned the same exhaustion into an unkillable process requiring a reboot.Not changed here: client-side file uploads
fetch()with aBun.file()body over plain HTTP also usessendfile(2)on macOS (src/http/SendFile.rs). That call site has the same kernel-level hazard, but its fallback whenis_eligible()returns false is a synchronous whole-file read on the JS thread (node_fs.read_file(..., Flavor::Sync)atfetch.rs:1741, markedTODO: make this async + lazy). Trading a rare unkillable-under-mbuf-exhaustion risk for a guaranteed blocking read on every large upload is a different balance than the server case, so that path is left as-is. Tracked separately.Tests
The existing
should be able to stop in the middle of a file responsetest is rewritten. The old shape fired 5000 fetches each withAbortSignal.timeout(10); the 10 ms timer fires before the synchronous creation loop yields, so on fast machines every request aborts before connecting and the server sees none of them. The new shape opens 16 requests, reads one chunk from each so they are provably mid-send, asserts the server is still running, then kills it andawait proc.exitedwithsignalCode === "SIGTERM", across three iterations. Deterministic and cheap enough to keep macOS coverage without driving runners toward mbuf exhaustion.This test asserts the process stays killable mid-send; it does not induce mbuf exhaustion and would likely pass on an unfixed build on a machine with headroom. Reproducing the full deadlock in CI would require pinning the kernel mbuf pool, which is unsafe on shared runners.
Verification
cargo check -p bun_runtimeclean onx86_64-unknown-linux-gnu,aarch64-apple-darwin,x86_64-pc-windows-msvc,x86_64-unknown-freebsd.bun bd test test/js/bun/http/bun-serve-file.test.ts: 67 pass, 0 fail.bun bd test test/js/bun/http/serve.test.ts -t "stop in the middle": passes in ~2.3s with 54 assertions.Closes #33116.