usockets: report an out-of-descriptors event loop init as an error, not a crash - #39641
usockets: report an out-of-descriptors event loop init as an error, not a crash#39641robobun wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Limit details: You’ve used all 5 included reviews currently available. You can run this review on demand instead of waiting. On-demand reviews are free until September 18, 2026. After that, they cost $0.25 per reviewed file.
How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (5)
Comment |
|
Status: ready for review. Reproduced with the released build (Linux, With this branch the first case prints CI on e908881: 178 of 179 jobs passed. The one failed job is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested change that turns an environment-caused startup panic into the existing fd-quota error message.
What was reviewed:
- Verified
handle_root_erroris-> !so the EMFILE/ENFILE match arms diverge and the function's!return type holds. - Confirmed the
\\>\\>escape is handled by the pretty formatter (output.rs:1990-1997) and that unescaped>was indeed being dropped (line 2006). - Checked the new C-ABI export follows the
Bun__panic/Bun__outOfMemorypattern next to it;libc::EMFILE/ENFILEcompile on Windows even though the caller is POSIX-only. - Test is properly gated (
isGlibc && cc), usestempDir, drains pipes concurrently, and covers both the fd-limit path and the still-a-crash path.
Extended reasoning...
Overview
Five files. epoll_kqueue.c adds three if (fd == -1) checks (after epoll_create1, kqueue, eventfd) that call a new noreturn Bun__loopInitFailed(syscall, errno). internal.h declares it. c_abi_exports.rs implements it: EMFILE/ENFILE route to the existing handle_root_error fd-quota branches (friendly message + exit 1), anything else still panics with the syscall named. crash_handler/lib.rs escapes >> in the two Linux fd-quota advice strings so the <tag> formatter doesn't drop it. The test file adds a Linux-glibc-only block using an LD_PRELOAD shim to fail epoll_create1/eventfd with specific errnos.
Security risks
None. This only changes how a fatal startup failure is reported — the failure was already fatal (a panic) and stays fatal (an error exit or a panic). No new inputs are parsed, no auth/crypto/permissions touched.
Level of scrutiny
Medium. It touches the event loop init path, but only to add error checks that immediately call a noreturn helper — no control-flow change on the success path. The design (keep it fatal, only change the report) is explicitly justified against a previously-rejected alternative (#33850). The new export mirrors Bun__panic/Bun__outOfMemory exactly.
Other factors
- Tests are thorough: 3 cases for the fd-limit path (both syscalls × EMFILE, plus ENFILE) asserting exit 1 / no crash-report upload / correct advice text, and 1 case proving other errnos still crash-report. The LD_PRELOAD approach is necessary (real
ulimit -nlimits vary between debug/release) and well-explained in comments. - The
us_calloc'd loop struct is leaked whenBun__loopInitFailedfires, which is fine — the process exits immediately, same as the priorBUN_PANIC. - No CODEOWNERS on the touched paths.
…ot a crash us_create_loop did not check the result of epoll_create1() / kqueue(), and an eventfd() failure was a BUN_PANIC. A process that starts with no free file descriptors therefore died in the crash handler, with a crash report that blames Bun. After a failed epoll_create1() it instead came up with loop->fd == -1 and spun in epoll_wait. The three sites now call Bun__loopInitFailed(syscall, errno). For EMFILE and ENFILE it passes the errno to handle_root_error, which prints the file descriptor limit error and exits 1. Any other errno still panics, with the syscall name and the errno in the message. handle_root_error keyed its two file descriptor branches on the Zig era names ProcessFdQuotaExceeded / SystemFdQuotaExceeded only. Errno errors are named EMFILE / ENFILE, so the branches also accept those names.
d4bda61 to
98d6cbe
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. My earlier note about the unescaped >> in the fd-limit advice was answered — that text is pre-existing, already broken on main independently of this change, and #37339 replaces the whole line; escaping it here would half-fix a wrong command and create a conflict.
What was reviewed:
Bun__loopInitFailedcontrol flow —handle_root_erroris-> !so EMFILE/ENFILE never fall through to the panic arm;#[cfg(unix)]matches the only call sites (epoll/kqueue), and the unconditional extern ininternal.his a harmless declaration on Windows.- The three C sites only add checks on paths that previously stored
-1as a poll fd or calledBUN_PANIC; the success path is unchanged. - Test shim:
isGlibcand a compiler are gated on, LD_PRELOAD is chained, RLIMIT_CORE is zeroed for the pre-fix abort case, and both the error and still-panics-on-ENOMEM paths are covered.
Extended reasoning...
Overview
Adds error handling for epoll_create1(), kqueue(), and eventfd() returning -1 during us_create_loop. A new Bun__loopInitFailed(syscall, errno) C-ABI export routes EMFILE/ENFILE through handle_root_error (the existing ulimit -n advice, exit 1) and any other errno through the panic path with a message that names the syscall. handle_root_error's two fd-quota branches now also match the errno names EMFILE/ENFILE alongside the legacy Zig names. Four LD_PRELOAD-shim tests cover epoll_create1+EMFILE, eventfd+EMFILE, eventfd+ENFILE (all → error+exit 1), and eventfd+ENOMEM (→ still panics with the syscall/errno in the message).
Follow-up on my prior finding
I previously flagged that the Linux >> /etc/sysctl.conf advice renders without the >> because pretty_error! drops bare >. The author's response is sound: that string is pre-existing on main, is already wrong for a second reason (sudo echo >> file runs the redirect in the unprivileged shell), and #37339 replaces the whole line with | sudo tee -a. Escaping here would produce a still-broken command and a merge conflict. This PR does not touch those strings and merges cleanly with #37339 in either order. The ulimit -n line, which is the actual per-process remedy and what the new test asserts, renders correctly.
Security risks
None. No user input is parsed; the export takes a compile-time string literal and an errno from a failed syscall. The change only alters how a fatal condition is reported before process exit.
Level of scrutiny
Medium. It touches event-loop init, but only on the error path — the success path is byte-identical. Before this change epoll_create1() == -1 was completely unchecked (fd -1 stored, every epoll_ctl silently failing) and eventfd() == -1 was a BUN_PANIC, so this is a strict improvement. The new export mirrors Bun__panic/Bun__outOfMemory directly above it.
Other factors
The comment-cop bot flagged verbose doc comments; the author trimmed them across two commits and all threads are resolved. The loop allocation leaks on the new error paths, but the process exits immediately so that is moot (same as the old BUN_PANIC path). Test is correctly gated on isGlibc && cc since LD_PRELOAD cannot interpose the statically-linked musl build. The PR body documents interactions with #37339, #35033, and #39629 and the author verified clean merge with git merge-tree.
1 participant
Problem
bun app.jswith no free file descriptors (ulimit -n 4) dies in the crash handler:panic: eventfd() failed during loop init (out of file descriptors?), then a crash report. The limit is the environment's, not a bug. No user has reported this site. It was found next to the reported watcher site that watch: report a failed watcher init as an error instead of a crash #39629 fixes.us_create_loop(packages/bun-usockets/src/eventing/epoll_kqueue.c:235) does not checkepoll_create1()orkqueue().us_internal_create_async(epoll_kqueue.c:788) turns aneventfd()failure intoBUN_PANIC.Fix
Bun__loopInitFailed(syscall, errno)insrc/bun_bin/c_abi_exports.rs. For EMFILE and ENFILE it passes the errno tohandle_root_error: the existingulimit -nadvice, exit 1, no crash report. Any other errno still panics, and the message names the syscall and the errno.handle_root_errorkeyed its two descriptor branches on the Zig namesProcessFdQuotaExceeded/SystemFdQuotaExceeded. Errno errors are namedEMFILE/ENFILE, so the branches now accept those names too.test/cli/run/run-crash-handler.test.ts, block "out of file descriptors while creating the event loop" (4 tests, all fail on the current build).Background
us_create_loopcreates the uSockets loop of a thread. On Linux it needs an epoll instance and aneventfdthat other threads write to wake the loop. On macOS and FreeBSD it needs a kqueue. The main thread creates its loop inVirtualMachine::init, before the entry point is read.handle_root_error(src/crash_handler/lib.rs) is whatmaincalls when a command returns an error. It maps some error names to advice and exits 1. It accepts aSystemErrnodirectly.Bun__panicandBun__outOfMemoryare the entry points uSockets already uses to end the process. The new export sits next to them.Notes
Second site, not fixed here. With one more free descriptor (
ulimit -n 5) startup gets past the loop and intoJSC::VM::VM, which callsWTF::cryptographicallyRandomNumber.WTF::RandomDevice::RandomDevice(Source/WTF/wtf/RandomDevice.cpp:75in the WebKit fork) opens/dev/urandomand callsCRASH()when that fails. Bun then printspanic(main thread): abort() calledwith a crash report. Found with gdb on the debug build. The fix belongs in the WebKit fork, for examplegetrandom(2)on Linux. That needs no descriptor, and it would also free a descriptor that every bun process holds for its whole lifetime today. This site is also the one behind the--watchrestart loop from the report: the signal path of the crash handler honors auto reload on crash, the Rust panic hook path does not. So theeventfdpanic never restarted, and the WebKit abort still does.Related PRs.
sudo launchctl limit maxfiles 2147483646; following it makes every new Terminalloginspin ~5 minutes of CPU, and SIP blocks undoing it until reboot #37337). This PR does not touch those strings, so the two merge cleanly in either order (checked withgit merge-tree). The only overlap is theharnessimport line of the test file. On current main the Linux advice still printssudo echo ... /etc/sysctl.confwithout the>>, because the<tag>formatter drops a bare>. Suggest bounded fd limits in the fd-exhaustion error hints #37339 replaces that line.us_create_loopreturns NULL) for the privatespawnSyncloop, and also fixes a Windowsuv_loop_new()NULL dereference inlibuv.cthat this PR does not touch. If this PR lands, the three usockets hunks of fix(spawnSync): throw instead of segfaulting when event-loop creation fails #35033 are superseded and its Windows hunk still stands. Left a note there.IoRequestLoop, a different loop. watch: report a failed watcher init as an error instead of a crash #39629 fixes the watcher site from the same report (ulimit -n 6with--watch).Descriptor budget at startup (Linux, release build, in order): epoll, eventfd,
/dev/urandom(WTF, kept open),/proc/self/statm(kept open), then the entry point is read.ulimit -n 6and7give the normalerror: EMFILE reading "app.js".8runs. The debug build opens/dev/urandomonce more before the loop, so every site fires one limit higher there. That is why the test injects the failure with an LD_PRELOAD shim instead of a realulimit -n. The shim exercises the same code path: the kernel fails the same call with the same errno. With the debug build, a realulimit -n 5prints the descriptor error and exits 1.Unchecked
epoll_create1before this change. Theepoll_create1test case runsapp.jsand exits 0 on the current build. Everyepoll_ctlon fd -1 fails silently andepoll_waitreturns EBADF at once, so the loop spins, timers still fire, and no socket can ever be registered.Who creates loops.
uWS::Loop::get()creates the loop of the main thread (fromVirtualMachine::init), of the HTTP client thread, and of each worker.SpawnSyncEventLoopcreates a private one forspawnSync. All of them dereference the result. For the lazily created loops this change also turns the crash report into the descriptor error: the process still exits.Earlier shape of this PR. The first push kept
handle_root_errorkeyed on the Zig names, mapped the errno to those names in the export, and escaped the>>in the two Linux messages. The self-review pointed out the escape hunks conflict with #37339 and that the Zig-name branches were unreachable for every errno error in the Rust port. The branches now accept the errno names and the message text is left to #37339.Runs. New block: 4 pass with
bun bd, 4 fail with the released build (two SIGABRT crash reports, one exit 0 with the entry point run, one with the old panic text). Wholerun-crash-handler.test.ts: 21 pass.test/js/bun/http/serve-epoll-add-fail.test.ts: 6 pass. A fetch + Worker + spawnSync smoke run with the debug build.cargo fmt --check.cargo clippy -p bun_bin -p bun_crash_handler: clean.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/run-crash-handler.test.ts