Repository navigation
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe accept loop now handles ChangesEMFILE listener throttling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change adds descriptor-exhaustion backoff and POSIX regression coverage. No concrete merge-blocking issue was established; merge after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:32 AM PT - Oct 1st, 2026
❌ @robobun, your commit 402e15c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32705That installs a local version of the PR into your bun-32705 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-listen-emfile.test.ts`:
- Around line 11-115: This Bun.serve regression test should not live in a new
standalone file; move the EMFILE coverage into the existing serve test module
and keep it alongside the other Bun.serve cases. Relocate the test body into
test/js/bun/http/serve.test.ts, preserving the current test name and logic, so
it extends the existing serve coverage rather than creating a sibling file.
- Around line 94-100: The test is starting the CPU sample before the HTTP
request is guaranteed to be flushed from the socket. Update the flow around the
connect/write sequence in serve-listen-emfile.test.ts so the request write is
awaited or confirmed flushed before calling proc.stdin.write("go\n"), using the
existing sock/once path to wait for the actual send condition instead of relying
on timing. Keep the fix localized to the request setup in this test and make the
failure path reject rather than race.
🪄 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: f5578271-40f9-4dc6-b126-67bb107d092b
📒 Files selected for processing (4)
packages/bun-usockets/src/context.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/loop.ctest/js/bun/http/serve-listen-emfile.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found and the approach looks sound, but this touches the core uSockets accept dispatch and sweep-timer refcounting, so it's worth a human pass on the event-loop interactions (and the untested Windows WSAEMFILE branch).
Extended reasoning...
Overview
This PR fixes a 100%-CPU spin when accept() returns EMFILE/ENFILE on a level-triggered listener poll. It adds an emfile_paused flag to us_listen_socket_t, disarms the listener poll on EMFILE, takes a sweep-timer ref, and re-arms the poll on the next sweep tick. Touched files: packages/bun-usockets/src/loop.c (accept dispatch + sweep callback + new pause/resume helpers), context.c (init + close-time ref release), internal.h (struct field), and a new POSIX-only regression test.
Security risks
None identified. No new attack surface; the change only alters error-path behavior for accept failures and does not touch auth, parsing, or memory ownership.
Level of scrutiny
High. This is production-critical event-loop C code in the hot accept path of every Bun.serve listener. Specific areas that warrant human eyes:
- Sweep-timer refcount balance: pause does
enable_sweep_timer(+1), resume/close dodisable_sweep_timer(-1). The guards (if (ls->emfile_paused) returnon pause; flag cleared before disable on both resume and close) look correct, but an imbalance here would either leak the timer or prematurely disarm it and break all socket timeouts process-wide. - Resume runs inside the sweep-timer callback and may drop the refcount to 0, which calls
us_timer_set(..., 0, 0)from inside the timer's own callback. This is likely fine but is the kind of edge a maintainer should confirm for both the epoll/kqueue and libuv backends. LIBUS_ERR(errno / WSAGetLastError) read after the accept loop: relies on the failingbsd_accept_socketbeing the last errno-setting call before the check, which holds in the current control flow but is subtle.- Windows path (
WSAEMFILE/WSAENOBUFS) is asserted to go through the same branch but is not exercised by the new test (POSIX-only via/bin/sh+ulimit).
Other factors
The bug-hunting system found nothing. CodeRabbit's two comments and my earlier stderr: "" nit have all been addressed and resolved. The implementation mirrors the well-known libuv/nginx pattern and the PR description is thorough. No CODEOWNERS cover packages/bun-usockets/. Still, given this is core event-loop machinery with cross-platform refcount invariants, I'm deferring rather than auto-approving.
|
Status: draft. This PR is not ready to merge. The review of 1 October on the rebased head (402e15c) found defects. I confirmed these by reading the code. I did not reproduce them yet.
The shape of the fix is under review again. The two candidates are a timed pause of the listener poll with no reserve descriptor, and the reserve-descriptor drain that this PR has now. The next push follows that review and answers the open review threads. Corrections to earlier text on this PR:
CI on 402e15c (build 122339): |
599340d to
4887732
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
Beyond the inline nit, I also checked: the new InternalLoopData fields are mirrored in the same order/cfg on the Rust side; errno is not clobbered between the failing bsd_accept_socket and the us_internal_is_emfile(LIBUS_ERR) check (only a field read sits between them, and us_internal_accept_emfile captures err before calling into Rust); and the reserve fd is released in us_internal_loop_data_free. On kqueue, us_poll_change(ls, 0) adds a one-shot EVFILT_WRITE to the listen fd via kqueue_change's neither-readable-nor-writable branch — harmless since us_poll_events masks it to 0 in dispatch, but worth a glance.
Extended reasoning...
This PR touches the core usockets accept loop, adds per-loop fd state with a C/Rust struct mirror, and has divergent POSIX (reserve-fd drain + folded-timeout deadline) vs Windows/libuv (sweep-timer refcount) paths. The one finding is a nit (POSIX-only ulimit -n hint on Windows). I verified errno-capture ordering, struct-mirror layout for the new fields, reserve-fd lifecycle, and that the kqueue us_poll_change(0) quirk (adds a one-shot EVFILT_WRITE on the listener) is masked out before dispatch. Deferring because event-loop dispatch changes of this scope warrant maintainer eyes; nothing here is a blocker I can point to.
There was a problem hiding this comment.
No bugs found, but deferring to a human — this touches the core usockets accept/event-loop path with three platform-specific branches (epoll, kqueue, libuv/Windows), adds a per-loop reserve fd, and changes the C/Rust us_internal_loop_data_t layout. Both of my earlier inline nits (exact-empty stderr assertion, POSIX-only ulimit hint on Windows) have been addressed.
What was reviewed:
LIBUS_ERRis read immediately after the failingbsd_accept_socket(no user callback runs in between), so errno is fresh.us_poll_change(ls, loop, 0)on kqueue: the EVFILT_WRITE add on a listening socket fails with EINVAL underKEVENT_FLAG_ERROR_EVENTSand is silently ignored; the EVFILT_READ delete still lands, so the listener goes quiet as intended and re-arms cleanly.InternalLoopDatalayout: the two new#[cfg(not(windows))]fields keepsweep_timer_count/wakeup_asyncat matching offsets on both POSIX and Windows (the Windows-onlyfin_deferred_countsits in what Rust sees as padding, unchanged by this PR).- Closing a paused listener on POSIX leaves
accept_rearm_next_tick_nsset; the next re-arm walks zero listeners and clears it — no leak.
Extended reasoning...
Overview
The PR replaces a long-standing /* Todo: start timer here */ in bun-usockets' accept path: when accept() returns EMFILE/ENFILE, the level-triggered listener poll would spin the loop at 100% CPU. The fix has two parts: (a) a libuv-style reserve-fd drain on POSIX — hold one spare /dev/null fd per loop, close it under EMFILE, accept()+close() the backlog so peers see a clean refusal and the readable clears, then reopen; (b) a timed-backoff fallback that unpolls the listener and re-arms it ~200 ms later via a new accept_rearm_next_tick_ns deadline folded into the existing poll-timeout computation (no timerfd). Windows/libuv has no reserve-fd path and goes straight to backoff via the sweep timer. A one-shot stderr warning is emitted from Rust. Eight files touched: loop.c (core logic), epoll_kqueue.c (call the re-arm check each tick), context.c (init/close bookkeeping), internal.h/loop_data.h (new fields), InternalLoopData.rs (layout mirror), server_body.rs (warning), plus a new test.
Security risks
None identified. The change is defensive resource-exhaustion handling; no new user-controlled input reaches native code. The reserve fd is O_RDONLY|O_CLOEXEC on /dev/null and is opened/closed only by the loop thread. The drain loop is bounded by the listen backlog plus one (it stops on EAGAIN or any non-EMFILE error).
Level of scrutiny
High. This is the core accept dispatch in us_internal_dispatch_ready_poll — every Bun.serve/Bun.listen connection flows through it — and it has three platform arms (epoll, kqueue, libuv) with meaningfully different us_poll_change(…, 0) semantics. The us_internal_loop_data_t layout is mirrored by hand in Rust (#[repr(C)]), so field ordering/cfg-gating must match exactly. The test only exercises the POSIX path (skipIf(!isPosix)), so the Windows sweep-timer backoff and the kqueue unpoll/re-arm are reasoned about but not CI-verified.
Other factors
- Both of my prior inline comments were applied (050f006:
stderr: expect.any(String)→ nowexpect.stringContaining(...)for the warning; 7ce30da: cfg-gated theulimit -nhint off Windows). The comment-cop note onserver_body.rsis also resolved — the doc comment onBun__warnAcceptEMFILEis now one line. - I traced several edge cases and found them handled: a duplicate readable already queued in
ready_polls[]re-entersus_internal_accept_emfileand hits theemfile_pausedearly-return; closing a paused listener on POSIX leaves the deadline set but the nextaccept_rearm_if_duefinds no listeners and clears it; on kqueue,us_poll_change(ls, 0)adds a one-shot EVFILT_WRITE that fails EINVAL on a passive socket, whichKEVENT_FLAG_ERROR_EVENTSreports back andus_poll_changeignores — net effect is the intended "read filter deleted, listener quiet". - The Rust
InternalLoopDatamirror checks out: on POSIX the two new fields (i64 + c_int) landsweep_timer_countat offset 20 andwakeup_asyncat 24, matching C; on Windows the new fields are cfg-gated out and the pre-existing layout (where C'sfin_deferred_countoccupies what Rust sees as pointer-alignment padding) is unchanged. - The test demonstrates fail-before/pass-after on both debug+ASAN and release, awaits the socket write flush before signalling the child (per the earlier CodeRabbit thread), and asserts on the observable warning string rather than exact stderr.
Given the scope — hot-path C in the event loop, hand-mirrored struct layouts, and platform branches without direct test coverage — this warrants a maintainer's eyes even though I found nothing wrong.
7ce30da to
496b55b
Compare
There was a problem hiding this comment.
No bugs found, but this touches the core usockets accept path with per-platform branches (reserve-fd drain on epoll/kqueue, sweep-timer backoff on libuv), a new per-loop fd lifecycle, and an FFI struct-layout change — worth a maintainer's eyes.
What was reviewed:
- Reserve-fd pairing: opened in
us_internal_loop_data_init, closed inus_internal_loop_data_free, re-acquired on rearm; no leak on the drain path. InternalLoopDataRust mirror: new POSIX-only fields inserted at the same offset/order asloop_data.h(i64/i64/c_int beforesweep_timer_count).us_poll_change(ls, loop, 0)on epoll registers only HUP/ERR (no EPOLLIN spin); on kqueue it deletes EVFILT_READ and adds a one-shot EVFILT_WRITE via the shared!readable && !writablebranch — didn't find a concrete failure, but a listen-socket EVFILT_WRITE is unusual enough to flag.- libuv sweep-timer refcount: each
emfile_pausedenable is balanced byus_listen_socket_closeorus_internal_accept_rearm_libuv; the 4 sLIBUS_TIMEOUT_GRANULARITYbackoff vs. 200 ms on POSIX is a design choice to note.
Extended reasoning...
Overview
The PR replaces a long-standing /* Todo: start timer here */ in packages/bun-usockets/src/loop.c with a real EMFILE/ENFILE handler for the listener accept path. On POSIX it holds one /dev/null fd per loop (accept_reserve_fd), closes it under EMFILE to drain-and-close the backlog (libuv's uv__emfile_trick), and if that can't clear the level-triggered readable it disarms the listener poll and schedules a 200 ms re-arm folded into the epoll/kqueue timeout via a new accept_rearm_next_tick_ns deadline. On Windows/libuv it goes straight to disarm + sweep-timer re-arm. It also adds a one-shot stderr warning (Bun__warnAcceptEMFILE), an emfile_paused bit on us_listen_socket_t, mirrors the new loop-data fields in src/uws_sys/InternalLoopData.rs, and ships a POSIX-only regression test that spawns Bun.serve under ulimit -n 128, exhausts fds, and asserts CPU stays ~0 over an 800 ms window with the warning on stderr.
Security risks
None identified. The reserve fd is O_RDONLY|O_CLOEXEC on /dev/null, drained connections are immediately closed without dispatching to JS, and no untrusted input reaches new parsing. The change is defensive against a self-inflicted DoS (100% CPU spin), not attack-surface-expanding.
Level of scrutiny
High. This is the accept loop for every Bun.serve/Bun.listen socket, with three platform arms (epoll, kqueue, libuv) and an FFI struct-layout change that must stay byte-for-byte in sync with the C header. The reserve-fd lifecycle and the sweep-timer refcount balancing on the libuv path both need to be exactly right on every exit path. The kqueue interaction — us_poll_change(listener, loop, 0) routes through kqueue_change's !readable && !writable branch, which EV_ADD|EV_ONESHOTs an EVFILT_WRITE on the listening socket — is the same code path us_socket_pause uses on connected sockets, but a listen socket has no send buffer; I couldn't find a concrete failure and the darwin CI failures were reported as pre-existing on main, but it's worth a maintainer confirming that's benign.
Other factors
All prior review threads (CodeRabbit's write-flush race and file placement, my stderr: "" nit and the Windows ulimit hint) are addressed and resolved. The test is verified to fail under USE_SYSTEM_BUN=1 and pass on the PR build (ASAN + release). The Windows path's backoff granularity is LIBUS_TIMEOUT_GRANULARITY (4 s) rather than the POSIX 200 ms — reasonable given no reserve-fd trick exists there, but a design point a maintainer should sign off on. The 200 ms constant itself and whether accept_reserve_fd should be per-loop (one fd per worker) are similarly maintainer-level calls. Given the breadth of platform-specific event-loop mechanics here, deferring rather than approving.
When the process is at its fd limit and a connection is waiting in the
listen backlog, accept() returns EMFILE. The listener poll is
level-triggered, so the loop immediately wakes again, fails accept()
again, and pegs a core (~98k accept4/epoll_pwait2 alternations in 10s,
80% of a core measured).
Fix:
(a) Hold a reserve fd (open("/dev/null")) per loop. On EMFILE, close
it, accept()+close() each queued connection so the peer sees a clean
refusal instead of a connection that never answers, then reopen the
reserve. Draining the backlog clears the level-triggered readable.
Same trick as libuv's uv__emfile_trick.
(b) If the drain cannot make progress (reserve spent, ENFILE, or a
concurrent thread stole the slot), unpoll the listener and schedule
a re-arm 200ms later via a new accept_rearm_next_tick_ns deadline,
folded into the epoll_pwait2/kevent timeout the same way
sweep_next_tick_ns is. Windows (libuv) has no reserve-fd path and
uses the sweep timer for backoff.
Emit a one-shot warning so the condition is no longer silent.
Closes #3271
The warning is called from the Windows libuv arm too (on WSAEMFILE / WSAENOBUFS), where ulimit does not exist. Match crash_handler's ProcessFdQuotaExceeded handling: keep the remedy on POSIX, drop it on Windows and say 'socket handles' instead of 'file descriptors'.
496b55b to
402e15c
Compare
|
Rebased on main (95690fc). The branch was 1598 commits behind. The previous head was 496b55b. Four files had a conflict:
Apart from the two includes, the diff against main has the same added and removed lines as before. The two new fields are at the same position in Checked on a debug build of the rebased tree:
|
There was a problem hiding this comment.
Beyond the inline findings, I also checked two things and ruled them out: LIBUS_ERR is read at loop.c:705 with no intervening syscall after the last failed bsd_accept_socket, so the errno gate is not clobbered; and us_listen_socket_close unlinks the listener from head_listen_sockets before the re-arm walk at loop.c:68 runs, so a listener closed while paused is never re-armed from freed memory.
Extended reasoning...
The change adds EMFILE drain/backoff state to the usockets loop (reserve fd, re-arm deadline, per-listener pause flag) across epoll/kqueue and libuv, a Rust-side struct mirror, a one-shot stderr warning, and a POSIX-only subprocess test. It touches no auth or crypto surface, but it is core event-loop and fd-lifecycle code with several confirmed findings (fd leak on loop-init failure, untested backoff path, stuck-listener risk on re-arm failure), and the hunt was cut off at the bug cap rather than running dry, so approval is not appropriate.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
packages/bun-usockets/src/loop.c— When loop creation fails, the process now leaks one fd per failed attempt, which the base branch does not do. us_internal_loop_data_init opens accept_reserve_fd at loop.c:242 before the fallible us_internal_create_async, and the failure branch at loop.c:253-259 frees recv_buf and send_buf but never closes the reserve fd, contradicting the comment at loop.c:233 that nothing is left allocated. Fix: close accept_reserve_fd (and reset it to -1) on the init failure path, or open it only after the wakeup async has been created, so every acquisition in loop-data init is released on the -1 return. [also at: packages/bun-usockets/src/loop.c:242 - Creating a loop now needs one more free descriptor, so a process that is one fd from its limit gets a failed Bun.spawnSync or Worker where the base branch succeeded. us_internal_loop_data_init opens accept_reserve_fd at loop.c:242 before the fallible us_internal_create_async at loop.c:252, so the reserve consumes the last slot the eventfd needed and init returns -1. SpawnSyncEventLoop.rs:151 turns that into a hard failure of the call.]Why this was flagged
A process near its RLIMIT_NOFILE creates a new usockets loop (for example
new Worker()on POSIX), reaching us_internal_loop_data_init via us_create_loop at epoll_kqueue.c:249. loop.c:242 opens /dev/null as the reserve fd first; it can take the last free slot. us_internal_create_async then fails (eventfd at epoll_kqueue.c:811 returns -1 with EMFILE) and the branch at loop.c:253-259 frees recv_buf and send_buf and returns -1 without closing loop->data.accept_reserve_fd. The caller at epoll_kqueue.c:249-253 closes loop->fd and frees the loop, so the reserve fd has no owner left and stays open for the life of the process. On the base branch there is no reserve fd, so a failed loop creation leaves no fd behind and the eventfd itself had one more slot available. Each retry of loop creation under fd pressure now leaks another fd, making the shortage worse. The comment at loop.c:233 claims nothing is left allocated, but the new field is not covered by that cleanup.Verification: Triggered when a POSIX (epoll) loop is created at RLIMIT_NOFILE so that open("/dev/null") succeeds but the subsequent eventfd() fails. packages/bun-usockets/src/loop.c:242
loop->data.accept_reserve_fd = us_internal_open_accept_reserve_fd();runs before the fallibleus_internal_create_asyncat loop.c:252. The failure branch at loop.c:253-259 has noclose(loop->data.accept_reserve_fd); the base branch had no reserve fd.
|
|
||
| if (ls->emfile_paused) return; | ||
| us_poll_change((struct us_poll_t *) ls, loop, 0); | ||
| ls->emfile_paused = 1; | ||
| long long deadline = (long long) us_internal_monotonic_ns() + LIBUS_ACCEPT_REARM_NS; | ||
| if (loop->data.accept_rearm_next_tick_ns < 0 || deadline < loop->data.accept_rearm_next_tick_ns) { | ||
| loop->data.accept_rearm_next_tick_ns = deadline; |
There was a problem hiding this comment.
🔴 The timed backoff that actually stops the spin when the reserve-fd trick cannot help, and the whole Windows/libuv path, ship with no test, so a break there merges green. The new test only reaches the drain branch at loop.c:190-203 (reserve fd still held, EMFILE); the unpoll + accept_rearm_next_tick_ns re-arm at loop.c:206-212 and us_internal_accept_emfile at loop.c:103-109 never run under test. Fix: add a case that forces the fallback, e.g. socketFaultInjection.set({ syscall: "accept", action: "errno", errno: , repeat: -1 }) from bun:internal-for-testing, assert low CPU while armed and that a client connects after fault.clear(), and run it on Windows too (no ulimit needed). [also at: packages/bun-usockets/src/loop.c:213 - nit: The timed-backoff fallback and its re-arm path are not exercised by any test, so a regression there would land silently. serve-listen-emfile.test.ts only reaches the reserve-fd drain (loop.c:190-204), which returns before loop.c:207; the pause at loop.c:208-213, us_internal_accept_rearm_if_due, and the libuv sweep-timer branch could be deleted and the suite still passes.; test/js/bun/http/serve-listen-emfile.test.ts:97 - Maintainers get no test for the pause/re-arm half of this fix, so a regression there (listener never resumed) merges green.]
Why this was flagged
test/js/bun/http/serve-listen-emfile.test.ts:14 is skipIf(!isPosix) and exhausts fds with openSync while the loop's reserve fd is still open, so in the child the first EMFILE takes the drain branch at loop.c:190-203, bsd_accept_socket succeeds once, and drain_err is EAGAIN, returning before loop.c:206. Nothing exercises us_poll_change(ls, loop, 0) at loop.c:208, the deadline at loop.c:210-213, us_internal_accept_rearm_if_due at loop.c:216-228, or the libuv arm at loop.c:103-114 and sweep_timer_cb at loop.c:550-553. Deleting the rearm call at epoll_kqueue.c:467/560 leaves every paused listener dead forever and no test fails. On base there is no such code; after merging, a regression in the fallback silently turns a spinning server into one that never accepts again (ENFILE, reserve open failure, or any Windows WSAEMFILE).
Verification: In test/js/bun/http/serve-listen-emfile.test.ts:14 (test.skipIf(!isPosix)), the reserve fd is still held when the single backlog connection triggers EMFILE, so us_internal_accept_emfile takes loop.c:190, bsd_accept_socket succeeds once and the function returns at loop.c:202, before loop.c:205-212. Deleting lines 205-212 or 215-227 leaves the suite green. The libuv path (loop.c:102-113, 551) is skipped entirely.
| void us_internal_accept_emfile(struct us_listen_socket_t *ls, struct us_loop_t *loop) { | ||
| Bun__warnAcceptEMFILE(LIBUS_ERR); | ||
| if (ls->emfile_paused) return; | ||
| us_poll_change((struct us_poll_t *) ls, loop, 0); | ||
| ls->emfile_paused = 1; | ||
| us_internal_enable_sweep_timer(loop); | ||
| } |
There was a problem hiding this comment.
🟡 (optional) Windows servers that hit a transient WSAEMFILE/WSAENOBUFS now stop accepting for up to 4 seconds, where the base branch resumed as soon as a handle freed. The libuv arm of us_internal_accept_emfile at loop.c:103-109 unpolls the listener and relies on sweep_timer_cb to re-arm it, but that timer fires on the LIBUS_TIMEOUT_GRANULARITY cadence (4s, libusockets.h:100), so queued clients wait 0-4s even after handles are available. Fix: re-arm paused listeners on the same ~200ms backoff as POSIX on every backend, e.g. a dedicated one-shot us_create_timer with LIBUS_ACCEPT_REARM_NS, instead of piggybacking on the 4s sweep timer. [also at: packages/bun-usockets/src/loop.c:108 - On Windows a listener that hits WSAEMFILE/WSAENOBUFS stops accepting for up to 4 seconds, not the ~200 ms the PR describes.]
Why this was flagged
On Windows, bsd_accept_socket fails with WSAENOBUFS or WSAEMFILE (us_internal_is_emfile at loop.c:55-61) during a burst, reached from us_internal_dispatch_ready_poll at loop.c:701-705. The libuv us_internal_accept_emfile at loop.c:103-109 calls us_poll_change(ls, loop, 0) and us_internal_enable_sweep_timer. us_internal_enable_sweep_timer at loop.c:86-92 arms sweep_timer with period LIBUS_TIMEOUT_GRANULARITY * 1000 = 4000 ms (libusockets.h:100), and if sweep_timer_count was already >0 the existing 4s cadence is untouched. The only re-arm is us_internal_accept_rearm_libuv called from sweep_timer_cb at loop.c:551, so the listener stays unpolled until the next sweep tick, anywhere from 0 to 4 s later. The base branch kept the listener polled and accepted the queued connection on the very next loop iteration after a handle freed, so clients that arrived during a short handle shortage now see several seconds of connect latency instead of milliseconds. POSIX uses LIBUS_ACCEPT_REARM_NS (200 ms) at loop.c:169, so this is platform-parity only on paper.
Verification: Trigger: on Windows (LIBUS_USE_LIBUV), bsd_accept_socket fails with WSAEMFILE or WSAENOBUFS while the backlog is non-empty. The only path that re-arms a libuv-paused listener is sweep_timer_cb (loop.c:551-553), with period 4000 ms (libusockets.h:100). On base the listener stayed polled and accepted the instant a handle freed; after this change Windows clients wait 0-4 s after recovery.
| loop->data.accept_rearm_next_tick_ns = -1; | ||
| if (loop->data.accept_reserve_fd == -1) { | ||
| loop->data.accept_reserve_fd = us_internal_open_accept_reserve_fd(); | ||
| } | ||
| us_internal_resume_emfile_paused_listeners(loop); |
There was a problem hiding this comment.
🟡 (optional) A server whose listener was paused on EMFILE can silently stop accepting forever if the re-arm's epoll/kqueue registration fails, whereas the base branch never unregisters the listener. us_internal_accept_rearm_if_due at loop.c:223 clears accept_rearm_next_tick_ns and loop.c:70-71 clears emfile_paused before us_poll_change, whose return code is discarded. On kqueue the pause deleted the EVFILT_READ knote (kqueue_change EV_DELETE, epoll_kqueue.c:629), so the re-arm is a fresh EV_ADD that can fail with ENOMEM under exactly the resource pressure that caused the pause. Fix: check the us_poll_change rc on every re-arm site (POSIX loop.c:71 and the libuv arm loop.c:112) and, on failure, keep emfile_paused set and re-schedule the deadline so the listener is retried instead of abandoned.
Why this was flagged
Trigger: a POSIX listener enters the pause path at loop.c:211-218 (reserve fd absent or ENFILE), then 200 ms later us_internal_accept_rearm_if_due runs from us_loop_run_bun_tick (epoll_kqueue.c:560). It sets accept_rearm_next_tick_ns = -1 at loop.c:223, then us_internal_resume_emfile_paused_listeners sets ls->emfile_paused = 0 at loop.c:70 and calls us_poll_change at loop.c:71, ignoring its return. On kqueue the pause ran kqueue_change(old=READABLE,new=0,keep=0) which is EV_DELETE (epoll_kqueue.c:629), so the re-arm is a new EV_ADD that kevent can reject with ENOMEM. After the failure no state records the listener as paused and no deadline is pending, so nothing ever re-arms it: the server keeps running, the port stays bound, and every new connection sits in the backlog until it times out, with no error. The base branch never removes the listener's read interest, so it cannot reach this state. Remedy: on a non-zero rc keep emfile_paused and re-schedule the deadline.
Verification: The re-arm clears accept_rearm_next_tick_ns = -1 first (loop.c:223), then sets ls->emfile_paused = 0 (loop.c:70) and calls us_poll_change(READABLE) (loop.c:71) discarding the result. If the EV_ADD fails, nothing records it, so the listener is never re-registered and silently accepts nothing until close. Base branch never deletes the listener knote, so this is a new failure mode.
| while ((fd = bsd_accept_socket(us_poll_fd((struct us_poll_t *) ls), &addr)) != LIBUS_SOCKET_ERROR) { | ||
| bsd_close_socket(fd); | ||
| } |
There was a problem hiding this comment.
🟡 (optional) An fd-exhausted server under a connection flood now stalls every existing connection and timer, where the base branch returned to the event loop on each failed accept(). The drain loop at loop.c:195-197 runs inside one dispatch until accept() errors; as long as new connections arrive faster than accept()+close() completes, us_internal_dispatch_ready_poll never returns, so no other poll, JS timer, or response on the fds that are actually held gets serviced. Fix: bound the drain to a fixed number of accept()+close() iterations per dispatch and fall through to the timed pause at loop.c:207-213 when the bound is hit; apply the same bound to any future libuv drain.
Why this was flagged
Trigger: a Bun.serve at RLIMIT_NOFILE (an attacker can get it there by holding connections open) receiving a sustained connect rate that exceeds the per-iteration cost of accept4()+close() on the listener; arrives through us_internal_dispatch_ready_poll at loop.c:702-706 into us_internal_accept_emfile. On the base branch the loop body was one failed accept() then return to epoll_pwait2, so other ready polls, Bun timers and the held connections' reads/writes were still dispatched every iteration (the spin cost CPU but not progress). After this change loop.c:195-197 is while (accept != ERROR) close(fd) with no iteration cap and no return to the loop; the only exit is accept() failing, which a flood prevents. us_internal_drain_ready_polls (epoll_kqueue.c:412) caps its re-poll at 48 iterations for exactly this starvation reason, but the new drain sits below that cap and is unbounded. Existing clients on the held fds observe their responses freeze; the one-shot warning gives no indication.
Verification: packages/bun-usockets/src/loop.c:195-197 is while ((fd = bsd_accept_socket(us_poll_fd((struct us_poll_t *) ls), &addr)) != LIBUS_SOCKET_ERROR) { bsd_close_socket(fd); } with no iteration cap, invoked from us_internal_dispatch_ready_poll at loop.c:704-708. On the base, the EMFILE branch was a single failed accept followed by return to epoll, so the starvation is new on this path.
| // refusal and the readable clears), falling back to unpolling the listener | ||
| // with a timed re-arm if the drain cannot make progress. | ||
| // https://github.com/oven-sh/bun/issues/3271 | ||
| test.skipIf(!isPosix)("Bun.serve does not spin at 100% CPU when accept() fails with EMFILE", async () => { |
There was a problem hiding this comment.
🟡 nit (optional): this Bun.serve regression test is added as a new file instead of the existing test/js/bun/http/serve.test.ts, which CLAUDE.md:49 names explicitly for Bun.serve bugs ("Do not create a new file"). Fix: move the test into test/js/bun/http/serve.test.ts (keeping the test.skipIf(!isPosix) gate), or state in the PR why a standalone file is required.
Why this was flagged
The diff creates test/js/bun/http/serve-listen-emfile.test.ts:14 as a new file for a Bun.serve accept() regression. CLAUDE.md:49 states the default is to add to the existing test file and gives Bun.serve bugs -> test/js/bun/http/serve.test.ts as the example; test/CLAUDE.md:152 repeats the rule. Nothing in the test needs a separate file: it spawns a subprocess with its own tempDir and uses harness exports that serve.test.ts already imports. Maintainers looking for accept/EMFILE coverage next to the other Bun.serve listener tests will not find it; this is a convention violation, not a behavioral defect.
Verification: nit. The diff adds test/js/bun/http/serve-listen-emfile.test.ts as a brand-new file (124 lines). CLAUDE.md:49 states "Do not create a new file." and that "a Bun.serve bug goes in test/js/bun/http/serve.test.ts". Nothing in the test requires a standalone file, and neither the PR description nor the test states a reason for a separate file.
| static int us_internal_is_emfile(int err) { | ||
| #ifdef _WIN32 | ||
| return err == WSAEMFILE || err == WSAENOBUFS; | ||
| #else | ||
| return err == EMFILE || err == ENFILE; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Linux servers whose accept() fails with ENOBUFS or ENOMEM still peg a core, the same spin this change claims to remove for EMFILE. us_internal_is_emfile at loop.c:59 only matches EMFILE and ENFILE, so the gate at loop.c:705 sends those errors back to the level-triggered poll with the connection still queued, and the Windows arm at loop.c:57 does include WSAENOBUFS, so the two platforms disagree on the same error. Fix: make the backoff cover every accept() errno that leaves the pending connection in the queue (ENOBUFS, ENOMEM alongside EMFILE/ENFILE), routing them to the pause/re-arm path rather than only the two names, so the spin is fixed as a class rather than for one errno.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: accept4() on Linux returns ENOBUFS or ENOMEM (socket buffer / kernel memory exhaustion, documented in accept(2)) while a connection is pending; reached from us_internal_dispatch_ready_poll loop.c:703-706 on every Bun.serve listener. The new predicate us_internal_is_emfile at loop.c:59 returns 0 for those errnos, so us_internal_accept_emfile is never called, the listener keeps EPOLLIN level-triggered readable, and epoll_pwait2 returns immediately on every tick exactly as the PR's strace shows for EMFILE. The base branch spun on every accept error too, but this PR's purpose is to end that spin and it adds a per-errno gate that leaves siblings unhandled; the Windows branch at loop.c:57 treats WSAENOBUFS as a backoff case, so the omission is a platform-parity gap in the same function. Population: memory-constrained containers under connection bursts. Remedy: extend the predicate to ENOBUFS/ENOMEM (or invert it to 'not EAGAIN/EWOULDBLOCK/ECONNABORTED/EINTR') so any persistent accept failure backs off.
Verification: On Linux accept4() fails with ENOMEM while a connection is still queued in the backlog. packages/bun-usockets/src/loop.c:56-62 us_internal_is_emfile returns true only for err == EMFILE || err == ENFILE, and the only backoff gate is loop.c:703-707. For ENOMEM/ENOBUFS no drain happens and epoll_pwait2 returns immediately every tick. The base branch already spins on these errnos by the same route.
|
Correction to my rebase note above. The rebase left one gap, which the review found. After this PR was written, main changed The cleanup branch needs a CI on the rebased head (build 122339): the test of this PR passed on every lane. The one red test is |
This branch has not been deployed
2 participants
Problem
When
Bun.serve(or any usockets listener) is at the process fd limit and a connection is waiting in the listen backlog,accept()returnsEMFILE. The listener poll is level-triggered, so the event loop wakes immediately, failsaccept()again, and repeats: the process pegs a core for as long as the fd limit is hit, serves nothing, and surfaces no error.strace on the spin (Bun.serve under
prlimit --nofile=64, 200 held connections; ~98kEMFILEreturns in ~10s, ~80% of a core):The backoff was a literal todo in
packages/bun-usockets/src/loop.c:Fix
(a) Reserve-fd drain (POSIX): hold one spare fd (
open("/dev/null", O_RDONLY|O_CLOEXEC)) per loop. OnEMFILE, close it,accept()+close()each queued connection so the peer sees a clean refusal instead of a connection that never answers, then reopen the reserve. Draining the backlog clears the level-triggered readable so the loop returns to idle without touching the listener poll. Same trick as libuv'suv__emfile_trick.(b) Timed backoff fallback: if the drain cannot make progress (
ENFILE, reserve spent, or a concurrent thread stole the freed slot),us_poll_change(listener, loop, 0)and schedule a re-arm ~200 ms later via a newaccept_rearm_next_tick_nsdeadline onus_internal_loop_data_t, folded into theepoll_pwait2/keventtimeout the same waysweep_next_tick_nsis (no timerfd). On Windows/libuv there is no reserve-fd path (sockets are a separate table from file handles), soWSAEMFILE/WSAENOBUFSgoes straight to backoff via the sweep timer.A one-shot warning is written to stderr so the condition is no longer silent:
Verification
test/js/bun/http/serve-listen-emfile.test.tsspawns a child underulimit -n 128, startsBun.serve, exhausts the remaining fds by opening/dev/nulluntilEMFILE, then the parent puts a connection in the backlog (sending a request soTCP_DEFER_ACCEPTlets the listener poll fire). The child samplesprocess.cpuUsage()across an 800 ms window.POSIX-only test (needs
/bin/sh,ulimit,/dev/null); the Windows path goes through the sameus_internal_dispatch_ready_pollbranch.[review] gate passed · iteration 5 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 5
evidence per changed file