Repository navigation
Conversation
…nging When std::thread::Builder::spawn failed (EAGAIN at a pids / RLIMIT_NPROC limit), ThreadPool::warm() and notify_slow() only undid the `spawned` increment. With no worker in the pool the queued tasks were never run and nothing retried, so `bun build`, Bun.build(), `bun install` and every WorkPool user (node:fs promises, zlib, dns) waited forever. - warm() returns the spawn error when the pool could not get its first worker. The bundler checks it in BundleV2::init and fails the build with the reason in the log, so `bun build` exits 1 with an error message, Bun.build() rejects, and the dev server prints the reason and exits instead of tripping its callers' OOM expects. - A failed spawn that leaves the pool without a worker while tasks are already queued exits the process with the errno and a note about the limit: schedule() is fire-and-forget, so there is nobody to hand the error to, and whoever waits on those tasks would otherwise hang. - The pool's first worker is retried once after 10ms, as Watcher::start already does, since EAGAIN from pthread_create is often momentary. - SystemErrno::from_io_error replaces the io::Error -> errno mapping that was inlined in Watcher::start, so both spawn sites share it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughChangesThread creation failures now propagate from worker pools through bundler initialization and runtime commands. The change adds errno conversion, cleanup, diagnostics, retry handling, stranded-task termination, and Linux regression tests. Thread spawn failure handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fixed and green; ready for a maintainer. Reproduced the hang on bun 1.4.0 and on a debug build of main with an CI for the current head 0ae50e1 (build 93238): 192 jobs passed, no test failures (the seven annotations are flakes that passed on retry or alone, none in this area). The build shows as failed only because the two All review threads are addressed and resolved ( |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the lock-free spawn/unspawn accounting in bun_threading::ThreadPool (including a Release→AcqRel ordering change and a new safety contract on unspawn) and introduces a process-exit path reachable from every WorkPool/bun install schedule, a human look at the concurrency reasoning and the exit-vs-propagate design choice would be worthwhile.
Checked: warm()'s only callers are the two updated bundler sites; tp.deinit() on the new early-return path releases the io-pool ref and (when owned) the boxed worker pool before this drops; the unregister→unspawn split preserves the "don't touch *pool past join_event.notify()" invariant; the Watcher refactor's only behavior change is the unreachable unmapped-code fallback (EIO→EAGAIN) as noted in the description.
Extended reasoning...
Overview
This PR fixes a hang when pthread_create fails (e.g. cgroup pids limit) before a bun_threading::ThreadPool has any worker. It touches the core lock-free thread pool (src/threading/ThreadPool.rs: new spawn_worker/unspawn/exit_with_stranded_tasks, warm() now returns Result, unregister split and its thread argument tightened to NonNull), WaitGroup (new pending() accessor), the bundler's pool wrapper and BundleV2::init (propagate the warm-up error into the log and fail cleanly), DevServer (exit 1 with the logged reason instead of the callers' .expect("oom") crash report), a new shared SystemErrno::from_io_error in src/errno/lib.rs (deduplicating what Watcher::start had inline), and a Linux-only LD_PRELOAD test that drives seven scenarios.
Security risks
None identified. No untrusted input parsing; the change is thread-creation error handling and process-exit messaging.
Level of scrutiny
High. src/threading/ThreadPool.rs is derived-from-zap lock-free code with a bespoke packed Sync word and careful memory-ordering comments. This PR changes an ordering (fetch_sub in unspawn from Release to AcqRel, with a stated pairing rationale), splits a function with a documented "pool may be deallocated after this returns" safety contract, and adds a new condition (workers_left == 0 && wait_group.pending() > 0) that races against concurrent spawns and schedules by design. The author's reasoning (using the same fetch_sub that releases the slot so a concurrently-succeeding spawn is respected) reads correctly to me, but this is exactly the kind of change a maintainer familiar with the pool's invariants should confirm.
The PR also makes a deliberate architectural choice — schedule() failures with no worker exit the process rather than propagate — and defends it (fire-and-forget callers, no error channel, matches the HTTP thread's treatment). That is reasonable but is a design decision a human should sign off on.
Other factors
warm()signature change: grepped for all callers; only the two bundler sites, both updated.- Cleanup on the new
BundleV2::initearly return:tp.deinit()frees the ownedworker_poolbox and releases the io-pool refcount;this(aBox<BundleV2>) drops normally onErr, and the watcher is installed only after this point per the updated comment. unregister/unspawnsplit: the "──*poolmay be invalid past this point ──" boundary is preserved;spawn_workerholds&selfso its post-unspawnwait_group.pending()read is covered by the documented caller exception.- Watcher refactor: pure dedup into
from_io_error; the only semantic difference (unmapped-OS-code fallbackEIO→EAGAIN) is called out and unreachable forpthread_create. - Tests cover the failure, retry, and one-worker-survives paths across five entry points, are Linux+cc-gated, sequential (hang regression), and use a spawn-plan shim keyed on
DEFAULT_THREAD_STACK_SIZE— a coupling the test file documents. One assertion matchesbun install v1.in stdout, which will need updating on a major-version bump; flagged by a finder and judged acceptable.
|
For whoever looks at the concurrency side, the argument behind the
On the exit itself: with the pool at zero workers the alternative is not an error somewhere, it is the existing behavior, a wait that never ends. The bundler gets the recoverable version because it is the one caller that asks before it has queued anything. |
|
Updated 12:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 0ae50e1 has some failures in 🧪 To try this PR locally: bunx bun-pr 37738That installs a local version of the PR into your bun-37738 --bun |
…e errno mapping BundleV2::init now fails with its own Error::ThreadSpawnFailed (the errno stays in the log). That failure happens before the CLI watcher is installed, so build_command must exit for it even under --watch instead of parking until a watcher that does not exist restarts the process; the dev server matches the same variant instead of the broad Sys(_). Add a bun_errno unit test for SystemErrno::from_io_error (a non-EAGAIN errno, a non-OS error, and the Win32 codes on Windows) and let the test shim inject a chosen errno so one end-to-end case shows the mapped value rather than the EAGAIN fallback in the message.
…awns The bundle thread's spawn failure and Watcher::start() each mapped a std::io::Error to a SystemErrno by hand, with the Win32 special case duplicated. One helper in bun_errno does it; both callers pick EAGAIN for an error without a usable code. (Same helper as #37738, so either PR can land first.) singleton::enqueue no longer copies the bundle thread's dequeue protocol for a build that never reached the queue: nothing else can have claimed such a build, so try_start() succeeding is asserted, and the free_released_unstarted branch, which would have freed a task the caller is still setting up, is gone. The thread-limit hint is appended for every errno, matching the pool's message.
|
FYI from #37753 (the bundle thread's spawn failure): it adds the same |
…led spawn warm() returned the error whenever the slot it took had been the pool's first, but by the time the spawn failed (including its retry sleep) another thread may have spawned a worker. Use the count unspawn() returned, the same value the stranded-tasks exit already uses, so a pool that did get a worker is reported as working.
Only EAGAIN means a process or thread limit was hit; ENOMEM (what a failed CreateThread maps to on Windows), EPERM and EINVAL would get advice about ulimit and pids limits that does not apply. One helper in bun_threading now supplies the sentence for both the stranded-tasks exit and the bundler's log message, and the EPERM test cases assert that it is left out.
There was a problem hiding this comment.
I reviewed this PR again after 0ae50e1 and found no bugs. Both earlier findings (the stale pool_was_empty check in warm(), and the unconditional ulimit hint) are addressed — warm() now decides on SpawnFailed::workers_left from unspawn(), and thread_limit_hint() gates the remedy on EAGAIN at both call sites with test coverage for the EPERM case. Because this touches the lock-free pool's atomic ordering (Release→AcqRel on unspawn's fetch_sub) and introduces a process-exit path inside the schedule flow, a maintainer look at the design would still be worthwhile.
Also checked: bundler::ThreadPool::deinit() on the new early-return path in bundle_v2.rs — it only frees the owned worker pool and releases the io_pool refcount, so calling it before graph.pool is set is fine; and the Watcher::start errno-mapping refactor is behavior-preserving apart from the documented unreachable fallback.
Extended reasoning...
Overview
The PR fixes a hang when pthread_create fails (typically EAGAIN at RLIMIT_NPROC/cgroup pids limit) before a bun_threading::ThreadPool has any worker: previously the batch stayed queued forever with the notified latch set. The fix consolidates the two spawn sites (warm, notify_slow) into spawn_worker(), which on failure releases the slot via a new unspawn() (split from unregister), and either returns SpawnFailed { errno, workers_left } or, when workers_left == 0 and tasks are already queued (wait_group.pending() > 0), exits the process with a diagnostic. warm() propagates the error only when workers_left == 0, letting the bundler (BundleV2::init) fail cleanly with a new Error::ThreadSpawnFailed before scheduling anything. Downstream: build_command skips the watch-park for this variant, DevServer prints the log and exits instead of .expect("oom")-crashing, and SystemErrno::from_io_error is factored out of Watcher::start for reuse. A Linux-only LD_PRELOAD-shim test exercises nine scenarios across bun build, --watch, Bun.build(), dev server, fs.promises.readFile, bun install, EPERM mapping, transient-failure retry, and single-worker degradation.
Security risks
None identified. The change is error-path handling for thread-creation failure; no untrusted input parsing, no auth/crypto, no new attack surface. The LD_PRELOAD test shim is Linux-only test infrastructure that the child process opts into via its own env.
Level of scrutiny
High. src/threading/ThreadPool.rs is a lock-free work-stealing pool derived from zap; the diff changes an atomic ordering (fetch_sub on sync from Release to AcqRel, justified by needing to observe another failing spawner's wait_group.add()), splits unregister into unspawn+shutdown-chain, tightens unregister's thread argument to NonNull, and adds a Global::exit(1) reachable from any schedule() caller. The concurrency argument for workers_left == 0 && pending() > 0 is subtle enough that the author posted a separate PR comment walking through the interleavings. My earlier review caught a real race in warm() (stale pre-CAS check), which was fixed correctly by threading workers_left through SpawnFailed.
Other factors
- Design decision needing sign-off: exiting the process from inside
spawn_worker(rather than surfacing an error fromschedule()) is deliberate and well-argued in the description — the ~40WorkPoolcall sites have no error channel and the alternative is the current infinite hang — but it is the kind of choice a maintainer should confirm. - Prior review addressed: both my earlier inline findings are resolved in 0ae50e1/5ea19a5 and covered by the updated test (EPERM cases assert the hint is absent; the
sfcase exercises theworkers_left > 0branch). - Test quality: the
LD_PRELOADshim keys onDEFAULT_THREAD_STACK_SIZEto target only pool workers (with a documented exception for the HTTP client thread handled via the plan string), asserts full{stdout, stderr, exitCode, signalCode}objects withsignalCode: nullproving no timeout kill, and the negative case (sfbuild succeeds with one worker) guards against over-eager exits. - Cross-PR overlap: #37753 shares
SystemErrno::from_io_errorand theWatcher.rschange byte-for-byte, noted in the timeline; whichever lands second should merge cleanly.
get() hands the io::Error back instead of collapsing an OS code bun has no errno name for into EAGAIN, which on Windows is most failed thread creations and which re-enabled the ulimit hint that is only meant for Unix's EAGAIN. The log now names the errno when there is one (with the hint on Unix for EAGAIN) and otherwise carries the OS's own description of the code. src/errno/lib.rs is brought back in line with the same change in #37738, which now also has a unit test for from_io_error. Tests: an unnamed code, and a VM torn down (BUN_DESTRUCT_VM_ON_EXIT) while a failed build's completion is still queued, which is the case the try_start() call in enqueue exists for; without it that fixture hangs in teardown.
|
One more data point for this PR, seen in the wild rather than with the shim: during
So the path hit there is the bundler's |
|
#39864 adds |
|
Another occurrence of this hang, from stress runs of
This PR now conflicts with main. #38604 removed |
|
Issue #43354 reports this hang again, for A smaller version of the same fix is on branch |
Problem
EAGAINfrompthread_createunder a cgroup pids limit orRLIMIT_NPROC),bun build,Bun.build(),bun installand the firstfs.promises/ zlib / dns call hang forever with no message. Reproduced on 1.4.0 and on main.schedule()tries to spawn again and nothing ever drains the queue.Fix
bun build(with or without--watch) and the dev server printFailed to create a worker thread for the bundler: EAGAIN. The process or thread limit may have been reached (...)and exit 1, andBun.build()rejects withAggregateError: Bundle failedcarrying that message.schedule()with no error channel, so a failed spawn that leaves no worker while tasks are already queued exits the process withEAGAIN: failed to create a worker thread for the thread pool; the work already queued on it could never run. Nothing else can ever run those tasks. Theulimit -u/ pids hint is added only forEAGAIN.LD_PRELOADshim that failspthread_createfor pool-sized threads; nine of its ten cases hang unfixed, all ten pass fixed. The errno mapping helper has a unit test, its Windows half run under miri only. The evidence block below records that the test did not run in the final check here and is left to CI.Background
bun_threading::ThreadPoolbacks the runtime'sWorkPool, the bundler's pools andbun install's pool. Workers are spawned lazily:schedule()publishes the tasks first, then sets a latch and asks for a thread; once the latch is set, laterschedule()calls assume someone will drain the queue.warm(n)pre-spawns up to n workers before any task exists. Only the bundler uses it, which is why the bundler can get a returned error and every other consumer gets a process exit.std::io::Errorcarries a Win32 code, not an errno, so it needs a different mapper; the file watcher had this mapping inline and now shares the newSystemErrno::from_io_errorwith the pool.bun build --watchnormally reacts to a build error by parking until the file watcher triggers a rebuild. The watcher is installed after the pool is started, so this error has to skip that path or the watch build would hang.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/sys/thread-pool-spawn-failure.test.ts
Original description
What
When the OS refuses to create a thread (
EAGAINfrompthread_createat the cgroup pids limit orRLIMIT_NPROC), abun_threading::ThreadPoolthat has no worker yet hangs whatever is waiting on it, forever and silently:The same happens to
Bun.build(), tobun install(tarball extraction runs on its pool) and to the first use of the runtime'sWorkPool(fs.promises.readFile, zlib, dns, ...). Reproduced on 1.4.0 and on a debug build of main with anLD_PRELOADshim that failspthread_createfor threads asking for the pool's worker stack size; that shim is what the new test uses.Cause
ThreadPool::warm()andnotify_slow()(src/threading/ThreadPool.rs) handled a failedstd::thread::Builder::spawnby undoing thespawnedincrement and returning. The batch that triggered the spawn stays in the run queue, and becausenotify_slowhad already set thenotifiedlatch, every laterschedule()takes the fast path and never tries to spawn again. With at least one worker in the pool this is harmless (workers never exit before shutdown, and the next one to go idle consumes the latch and drains the queue), so the bug is exactly "the pool could not get its first worker". The upstream zap code this is derived from has the same shape.Fix
warm()returns the errno when a failed spawn leaves the pool with no worker at all, judged by the count the slot release itself returns (so a worker another thread managed to spawn meanwhile counts; failing to add more workers to a working pool still just ends the warm-up). The bundler is the one consumer that warms before scheduling, soBundleV2::initchecks it, adds the reason to the log and fails the build with a newbun_bundler::Error::ThreadSpawnFailed(same name as the existing variants inbun_runtimeandbun_jsc):bun build:error: Failed to create a worker thread for the bundler: EAGAIN. The process or thread limit may have been reached (ulimit -u, or the container's pids limit); raise it or reduce concurrency, exit code 1. The second sentence comes fromthread_limit_hint()and is only added forEAGAIN;ENOMEM(what a failedCreateThreadmaps to on Windows),EPERMorEINVALjust name the errno, since the ulimit / pids advice does not apply to them.bun build --watch: the same. This failure happens beforeinitinstalls the watcher, sobuild_commandmust not take its usual "park and let the watcher restart us" path for it; under a real thread limit that would turn the baseline's clean watcher-spawn failure (Failed to start File Watcher: EAGAIN, exit 1) into a hang. The dedicated variant is what letsbuild_commandtell this apart from the build errors that should keep a watch session alive.Bun.build(): rejects with the usualAggregateError: Bundle failedcarrying that messagestart_async_bundleerror as OOM (.expect("oom")), soDevServerprints the log message and exits 1 for this variant instead of producing anoomcrash report with the reason never shown.spawn_worker(). When a failure leaves the pool with no worker (the count comes from the samefetch_subthat releases the slot, so a spawn that succeeded concurrently on another thread is respected) while tasks are already queued (wait_groupis non-zero), the process exits:note:line again only forEAGAIN). This coversbun install,WorkPoolandeach()/wait_for_all()callers.Watcher::startalready uses for the same error; later spawns are not retried since their failure only costs parallelism.SystemErrno::from_io_errorholds theio::Error-> errno mapping (Win32 code on Windows) thatWatcher::starthad inline, so both users share it. The only behavior difference for the watcher is the fallback for an OS code without aSystemErrnomapping (EIObefore,EAGAINnow);pthread_createonly returnsEAGAIN,EINVALandEPERM, so it is not reachable. It gets a unit test next to the existingbun_errnoones (a non-EAGAINerrno, a non-OS error, and the Win32 codes on Windows; the Windows half was run withcargo miri test -p bun_errno --target x86_64-pc-windows-msvc, since no CI lane executes this crate'scfg(windows)tests).unregister()is split intounspawn()(the accounting, now also used by the failed-spawn path and returning the remaining worker count) and the worker-exit wait chain; itsthreadargument becomesNonNullsince nothing passes null anymore.Why exit instead of returning an error from
schedule()schedule()is fire-and-forget: the tasks are already published when the spawn is attempted, the 40-oddWorkPoolcall sites have no error channel, and the waiters (the bundler's and install's event loops,wait_for_all(), a JS promise) are on the other side of a queue nobody will ever drain. Exiting with the errno and the limit named is the same treatment the HTTP client thread andbun install's startup failures already get, and it is what a process in this state does anyway on its nextfetch()or GC (#36985 covers the crash-report presentation of those). The bundler gets the recoverable path because it is the one consumer that can find out before it schedules anything, andBun.build()in a running process is where a clean rejection matters.Tests
test/js/bun/sys/thread-pool-spawn-failure.test.ts(Linux, needs a C compiler like the existingLD_PRELOADtests) compiles a shim that failspthread_create(withEAGAIN, or an errno the test picks) for threads requestingDEFAULT_THREAD_STACK_SIZE, following a per-attempt plan:bun buildandbun build --watch(exit 1 + message),bun buildandbun installwithEPERMinjected (the messages nameEPERM, so the mapping and not the fallback is what is shown, and neither carries the limit hint),Bun.build()(rejection carrying the message, process exits 0 on its own), dev server (HTML route requested from the test process: message + exit 1),fs.promises.readFileandbun installof a local tarball (both: the two stderr lines above, exit 1)readFilecompletesOn the unfixed build the first nine hang and are killed; with the fix all ten pass. The existing watcher spawn-failure test in
test/cli/watch/watch.test.tsstill passes;cargo checkof the touched crates also passes for thex86_64-pc-windows-msvcandaarch64-apple-darwintargets.