usockets: make us_create_loop fallible instead of aborting on EMFILE - #33850
usockets: make us_create_loop fallible instead of aborting on EMFILE#33850robobun wants to merge 7 commits into
Conversation
WalkthroughThis PR makes event-loop creation and async allocation resilient to file-descriptor exhaustion. Bun’s HTTP thread, VM initialization, and Worker startup now propagate failures without aborting, with regression coverage for fetch and Worker scenarios. ChangesEMFILE-safe event loop creation and failure propagation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:15 PM PT - Jul 11th, 2026
❌ @robobun, your commit f29437a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33850That installs a local version of the PR into your bun-33850 --bun |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bun-uws/src/Loop.h (1)
117-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor asymmetry between the two
get()branches.The
existingNativeLoopbranch doesn't explicitly checkcreate(existingNativeLoop)for null, unlike the newly-added null check in the other branch — functionally harmless sincegetLazyLoop().loopjust stays null either way, but worth aligning for readability/consistency.🤖 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 `@packages/bun-uws/src/Loop.h` around lines 117 - 137, The get() method in Loop currently handles the existingNativeLoop path differently from the nullptr path, which makes the control flow asymmetric and harder to read. Update the get(void *existingNativeLoop) logic so both branches follow the same null-check pattern around create(...), using the Loop::get and create symbols to keep the handling consistent while preserving the current behavior of leaving getLazyLoop().loop null on failure.
🤖 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/util/emfile-loop-init.test.ts`:
- Line 79: The current assertion in the EMFILE loop init test is too generic and
can pass for unrelated worker failures; update the check around the stderr
expectation in the test to assert the actual worker initialization failure
message emitted by thread_main, using the Worker initialization failed text (or
the full Worker initialization failed: {err.name()} form) instead of only worker
error:. This should be done in the emfile-loop-init test near the existing
stderr assertion so the test specifically verifies the EMFILE-handling path.
- Around line 14-17: Strengthen the emfile-loop-init test by asserting the exact
error codes produced by the two fetch attempts instead of matching generic
stderr substrings. Update the assertions around the fetch handling in
emfile-loop-init.test.ts so the first fetch explicitly expects e.code to be
"FailedToOpenSocket" and the retry asserts the normal connect-failure code
rather than another "FailedToOpenSocket"; use the existing fetch/retry logging
path and the FetchTasklet mapping as the guide.
- Around line 28-46: The Worker fixture cleanup is not guaranteed to run if the
Promise never settles or an earlier step throws, so move the fd closing loop and
fs.unlinkSync(W) into a try/finally around the Worker setup in workerFixture.
Keep the cleanup in the finally block so held descriptors are always closed and
the temp file is always removed, even on hang or error paths.
---
Outside diff comments:
In `@packages/bun-uws/src/Loop.h`:
- Around line 117-137: The get() method in Loop currently handles the
existingNativeLoop path differently from the nullptr path, which makes the
control flow asymmetric and harder to read. Update the get(void
*existingNativeLoop) logic so both branches follow the same null-check pattern
around create(...), using the Loop::get and create symbols to keep the handling
consistent while preserving the current behavior of leaving getLazyLoop().loop
null on failure.
🪄 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: 6ee92ddc-8856-4e0e-8ce0-882b2683d23f
📒 Files selected for processing (11)
packages/bun-usockets/src/eventing/epoll_kqueue.cpackages/bun-usockets/src/eventing/libuv.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/loop.cpackages/bun-uws/src/Loop.hsrc/http/AsyncHTTP.rssrc/http/HTTPThread.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/webcore/Worker.cppsrc/jsc/web_worker.rstest/js/bun/util/emfile-loop-init.test.ts
|
CI on 751ddf9: the only failure is the Windows x64 build-cpp lane failing to download the libuv tarball ( The x64-asan lane (which caught the real leak on the first push) passes. All Linux and macOS lanes that completed are green, and all review threads are resolved. This is ready for a maintainer to re-run the Windows build lane or merge. |
751ddf9 to
61b3f3e
Compare
|
CI on f29437a (rebased onto 588da0e): the only hard failure is All review threads resolved. Ready for a maintainer to re-run the darwin lane or merge. |
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 `@packages/bun-usockets/src/eventing/libuv.c`:
- Line 178: Handle the return value of us_internal_loop_data_init in
us_create_loop instead of discarding it; when it returns -1, mirror the
epoll/kqueue cleanup path by unwinding uv_pre and uv_check, deleting any owned
uv_loop, and returning NULL.
🪄 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: ee1d8ff0-7671-4d47-a250-babcd7c32342
📒 Files selected for processing (11)
packages/bun-usockets/src/eventing/epoll_kqueue.cpackages/bun-usockets/src/eventing/libuv.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/loop.cpackages/bun-uws/src/Loop.hsrc/http/AsyncHTTP.rssrc/http/HTTPThread.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/webcore/Worker.cppsrc/jsc/web_worker.rstest/js/bun/util/emfile-loop-init.test.ts
On Linux, us_create_loop acquires three file descriptors (epoll_create1, timerfd_create for the sweep timer, and eventfd for the wakeup async). When the process is at its RLIMIT_NOFILE the first of these to fail was a deliberate BUN_PANIC, and the other two were never checked. The first fetch() or new Worker() issued at that moment lazily creates its own per-thread event loop and aborted the whole process (SIGABRT, exit 134), where Node fails just that one operation and recovers once fds are freed. us_create_loop now returns NULL when any of the init syscalls fail, cleaning up whatever was acquired. uWS::Loop::get() returns nullptr and leaves the per-thread lazy slot empty so a later get() retries. Callers: - VirtualMachine::init bails out with EMFILE before ensure_waker would dereference a null loop. On the main thread this propagates to the CLI and exits non-zero; nothing can run without a loop there. - WebWorker::thread_main turns start_vm() Err into an error event on the parent Worker (via dispatchErrorWithMessage) plus a clean shutdown, instead of Output::panic. - The HTTP client thread checks uws::Loop::get() before init_global; while it is null it rejects every queued fetch with FailedToOpenSocket and retries, so a later fetch succeeds once fds free up. Also fixes the pre-existing leak of the poll allocation when timerfd_create or eventfd fails. Related: #33848 handles the adjacent case where the loop itself comes up but the GC controller's us_create_timer fails.
…ail_before_start
- Move the uws::Loop::get() null check to the top of VirtualMachine::init
(before alloc_zeroed / console / runtime_state) so a worker-thread EMFILE
bail leaks nothing. LSan on the x64-asan lane was flagging ~170KB leaked
through init_runtime_state on the old placement.
- AsyncHTTP::fail_before_start: set copy.real = NonNull::new(http), matching
start_queued_task. NetworkTask::notify hard-unwraps real, so a bun install
under EMFILE would have aborted on expect("unreachable") instead of
rejecting the task.
- Test fixture: unlink the worker script after freeing fds.
shutdown() with a null vm_ptr previously sent dispatchExit(0), so the parent saw a 'close' with wasClean=true / code 0 right after the error event. Carry the failure via a Cell<i32> on WebWorker (set to 1 in the Err arm, read as the null-vm default in shutdown()) so the close/exit event reflects abnormal exit, matching Node's ERR_WORKER_INIT_FAILED. The terminate-before-start paths are unchanged (still code 0).
bun_core::err! is gone; use crate::Error::FailedToOpenSocket in bun_http and bun_errno::SystemErrno::EMFILE.into() in bun_jsc.
61b3f3e to
2b4acd6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/http/AsyncHTTP.rs`:
- Around line 599-615: Condense the documentation and safety comments on
AsyncHTTP::fail_before_start to no more than three lines each, while preserving
the contract that http is a live JS-thread-owned value removed from
HttpThread::queued_tasks and that the callback receives the failure.
In `@src/http/HTTPThread.rs`:
- Around line 1274-1278: Condense the startup comment immediately before the
uSockets loop initialization to no more than three lines while preserving its
essential points: ensure the loop exists before init_global dereferences it,
handle loop-creation failure by rejecting queued fetches with
FailedToOpenSocket, and allow retry after file descriptors are available.
In `@src/jsc/VirtualMachine.rs`:
- Around line 1993-1996: Shorten the initialization comment near the uSockets
loop setup to no more than three lines, retaining only the essential invariant
that Loop::get() may return null and must be checked before allocations so the
null-VM shutdown path has nothing to reclaim.
🪄 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: 0ee20e3f-b922-499c-8c61-c2f1665ed2a7
📒 Files selected for processing (8)
packages/bun-usockets/src/eventing/epoll_kqueue.cpackages/bun-usockets/src/eventing/libuv.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/loop.cpackages/bun-uws/src/Loop.hsrc/http/AsyncHTTP.rssrc/http/HTTPThread.rssrc/jsc/VirtualMachine.rs
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This is a fake fix. Just abort.
|
Closing per the above. Happy to take a different approach if there's one you'd prefer (e.g. only the Worker path, or leaving the HTTP thread alone and just failing the first fetch without the retry loop); otherwise I'll leave the original abort in place. |
Repro
Before (both
fetchandnew Worker()at the fd limit):SIGABRT, exit 134. Node at the same limit rejects that one fetch (TypeError: fetch failed) / fires
ERR_WORKER_INIT_FAILEDon that one Worker, then recovers.Cause
On Linux,
us_create_loopacquires three fds:epoll_create1,timerfd_createfor the sweep timer, andeventfdfor the wakeup async. The eventfd failure was a deliberateBUN_PANIC; the other two were never checked. The firstfetch()spawns the HTTP-client thread and the firstnew Worker()spawns a worker thread; both lazily create their own per-thread uSockets loop viauWS::Loop::get(), which reached the panic and took the whole process down.Fix
us_create_loopnow returns NULL when any of the init syscalls fail, cleaning up whatever was acquired.us_internal_loop_data_initreturnsintso the caller can unwind.uWS::Loop::get()returns nullptr and leaves the per-thread lazy slot empty so a later call retries.Callers:
VirtualMachine::initreturnsErr(EMFILE)beforeensure_wakerwould dereference a null loop.WebWorker::thread_mainturnsstart_vm()Err into anerrorevent on the parent Worker (newWebWorker__dispatchErrorMessageshim that only needs the parent-sideWorker*) plus the existing null-vmshutdown(), instead ofOutput::panic.on_startchecksuws::Loop::get()beforeinit_global; while null it rejects every queued fetch withFailedToOpenSocket(via newAsyncHTTP::fail_before_start) and retries, so a later fetch succeeds once fds free up.Also fixes the pre-existing leak of the poll allocation when
timerfd_createoreventfdfails.Related: #33848 handles the adjacent case where the loop itself comes up but the GC controller's
us_create_timerfails (3-4 spare fds). This PR is the 0-spare-fd variant that #33848 explicitly scoped out.Verification
test/js/bun/util/emfile-loop-init.test.ts(POSIX-only) runs both scenarios underulimit -n 512: fills the fd table, issues the firstfetch()/new Worker(), asserts the process exits 0 withSURVIVEDand the per-operation error. Fails on main with exit 134 /panic: eventfd() failed during loop init; passes with this change.[review] gate passed · iteration 4 · 11 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 7 passed · 0 rejected · iteration 4
evidence per changed file