Conversation
… plugins All Bun.build() calls share a single bundle thread, which processed builds strictly one at a time and, while running one, parked inside that build's MiniEventLoop waiting on outstanding plugin promises. A plugin's onLoad/onResolve that awaited a nested Bun.build() self-deadlocked: the inner build sat in the bundle thread's queue (the thread was blocked inside the outer build, not on its queue waker), the outer never got its plugin result, and every later Bun.build() in the process hung behind it. Concurrent independent builds were also head-of-line blocked behind any build with a slow plugin. The bundle thread now drains its pending queue each time wait_for_parse wakes, running queued builds to completion before re-checking its own parse counter. enqueue() additionally wakes the bundle thread's per-thread uws loop so a parked wait_for_parse actually returns to look. ASTMemoryAllocator::push() now saves the previously-installed allocator so the per-build push/pop pair nests correctly when generate_in_new_thread recurses.
|
Warning Review limit reached
Next review available in: 17 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 ignored due to path filters (1)
📒 Files selected for processing (10)
Comment |
|
Updated 3:03 AM PT - Jul 22nd, 2026
❌ @robobun, your commit 2ca077c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35060That installs a local version of the PR into your bun-35060 --bun |
|
Found 0 issues this PR may fix. Reviewed all candidate issues. The closest match is #33261 (event loop re-entry tracking issue), which lists "Bun.build plugin waits" as a work item to audit, but that is a broader architectural tracking issue -- this PR improves the situation for one of its work items without closing the issue itself. The remaining candidates were about plugin correctness bugs, HMR/hot reload, thread-safety of PackageManager, crashes, or feature requests -- none report the specific deadlock/hang scenario (nested or concurrent Bun.build() calls blocked on plugin promises) that this PR fixes. 🤖 Generated with Claude Code |
…ested The previous commit drained the bundle thread's queue from inside the in-flight build's wait_for_parse, which ran a later-enqueued build nested on the call stack of an earlier one. That inverted the pre-existing FIFO ordering, so a build whose plugin awaited an earlier build (which used to be settled by the time it ran) deadlocked. It also left the ASTMemoryAllocator thread-locals dangling when create_and_configure_transpiler's early return skipped pop() for a nested inner build. Instead: singleton::enqueue tracks how many Bun.build()s are scheduled but not yet completed (scheduled: AtomicU32). The enqueue that takes the counter from 0 to 1 uses the long-lived singleton thread; any other runs its build on a short-lived overflow thread and frees that thread's uws loop on exit. Sequential builds keep sharing the singleton thread. Concurrent/nested builds run independently so neither completion is gated on the other returning from a nested call. WaitGroup::finish() now broadcasts instead of signalling one waiter so ThreadPool::wait_for_all() releases every concurrent bundler thread when the shared WorkPool's counter reaches zero. Adds a fourth test covering the ordering case (a later build's plugin awaiting an earlier build's result) that passes on main and must keep passing.
Falling through to the queue would re-create the nested-Bun.build deadlock this PR fixes: the singleton thread is parked in wait_for_parse (not on waker), so a queued build awaited from inside its plugin callback would never be popped. Rejecting settles the plugin promise with a diagnosable error instead of hanging the process.
…-lived
- WaitGroup finish() already broadcasts; this commit handles the rest:
- io_thread_pool::acquire(): bump REF_COUNT for a racing acquirer that
loses the init race, so two concurrent bundle threads on macOS/Windows
cannot drive the count under the encoded floor and re-init over a
running pool.
- resolver MODULE_BUFS: wrap the thread-local slot in a Drop type so
the leaked Box is reclaimed when a short-lived bundler thread exits
(matches the existing BufsSlot pattern).
- uws loop on Windows: add uWS::Loop::freeLoopWrapperAtThreadExit and a
Rust binding so an overflow thread can release the per-thread
us_loop_t wrapper (recv/send/cork buffers) and then close its
uv_loop_t, since uWS::Loop::get with a non-null hint leaves cleanMe
false and on_thread_exit would skip the free.
- tests: assert {stdout, stderr, exitCode} as one object so an
unrelated sanitizer line on stderr still shows the build under test
completed.
thread_main unconditionally fetch_sub(scheduled) per popped completion, so pushing to the queue without the paired fetch_add in enqueue_or_spawn would underflow the counter. The function has no callers after singleton::enqueue was rewired to enqueue_or_spawn; delete it and point the remaining doc links at enqueue_or_spawn.
There was a problem hiding this comment.
No new issues found — all prior review findings (io_thread_pool undercount, Windows uws-loop leak, spawn-failure hang, dead enqueue, stale SAFETY comments) are addressed in the current diff. Deferring to a maintainer for sign-off since this changes the bundler's threading model (per-concurrent-build OS threads), flips WaitGroup::finish to broadcast, and adds a new teardown path in vendored uWS + Windows-specific uv_loop_t shutdown.
What was reviewed:
scheduledcounter balance across all three decrement sites (thread_main, overflow thread, spawn-failure) and AcqRel/Release pairing.freeLoopWrapperAtThreadExitvs~LoopCleanerfor double-free —free()nullsgetLazyLoop().loopso the destructor is a no-op.WaitGroupbroadcast + the shared WorkPool WaitGroup being process-lifetime, satisfying the new "outlive every wait()" caller constraint.ModuleBufsSlot::dropmatches the existing lazy-TLS-box pattern;io_thread_poolelse-branch fetch_add is under the mutex.
Extended reasoning...
Overview
This PR changes Bun.build() scheduling so that concurrent/nested builds run on short-lived overflow OS threads instead of serializing behind the singleton bundle thread (which parks in wait_for_parse and cannot service its queue). Touches: src/bundler/BundleThread.rs (new enqueue_or_spawn/spawn_overflow_thread, atomic scheduled counter, generation → AtomicU16), src/threading/WaitGroup.rs (signal → broadcast), src/bundler/ThreadPool.rs (io_thread_pool::acquire racing-acquirer refcount fix), src/resolver/package_json.rs (TLS ModuleBufs gains a Drop slot), packages/bun-uws/src/Loop.h + src/uws_sys/{Loop.rs,libuwsockets.cpp} + src/uws/lib.rs (new freeLoopWrapperAtThreadExit and Rust binding), and four new subprocess tests in test/bundler/bun-build-api.test.ts.
Security risks
None identified. No untrusted-input parsing, auth, or crypto surface. The change is internal scheduling/threading; the only new user-reachable path is the spawn-failure error message, which surfaces the OS error via bun_ast::Log and rejects the build promise.
Level of scrutiny
High. This is core-runtime concurrency: new OS-thread lifetimes, cross-thread atomic protocol, a semantic change to a shared sync primitive (WaitGroup), platform-divergent teardown (POSIX on_thread_exit vs Windows free_loop_wrapper_at_thread_exit + uv::Loop::shutdown), and an addition to vendored third-party C++ (bun-uws/Loop.h). It went through five review iterations here, each producing substantive fixes (the original nested-drain approach was replaced entirely; two 🔴 findings — the io_thread_pool refcount undercount and the Windows uws-loop leak — were fixed in follow-up commits; the spawn-failure fallback was changed from silent-queue to explicit rejection; dead BundleThread::enqueue and its stale doc references were cleaned up in two passes).
Other factors
The current diff looks correct to me on the concerns I raised and re-checked: the scheduled counter is balanced on every terminal path; freeLoopWrapperAtThreadExit cannot double-free with ~LoopCleaner because Loop::free() nulls the thread-local slot; the WaitGroup broadcast constraint ("outlive every wait()") is satisfied because the affected WaitGroup is the process-lifetime WorkPool's; and the io_thread_pool else-branch fetch_add is under MUTEX so Relaxed is sufficient. Four new subprocess tests cover onLoad/onResolve nesting, independent-concurrent, and the reversed-order case; the author reports debug+ASAN passes on Linux and a Windows debug stress loop. That said, the design choice (fresh OS thread per concurrent build vs. e.g. making the singleton service its queue while parked) and the vendored-uWS API addition are the kind of architectural calls a maintainer should approve, so I'm not shadow-approving.
|
Build #77570 on 2ca077c: the red lanes are unrelated flakes ( |
#39947) ### Problem - A worker whose entry point goes through a package.json `imports` or `exports` map leaks 12 KiB (3 `PathBuffer`s, more on Windows) when its thread exits. On an ASAN build LeakSanitizer reports `Direct leak of 12288 byte(s)` allocated in `module_bufs` (`src/resolver/package_json.rs`), reached from `resolve_entry_point_specifier` on the worker thread. - Cause: `MODULE_BUFS` is a thread local `Cell<*mut ModuleBufs>` with nothing that frees the box. The resolver's other per thread buffers (`BufsSlot` in `resolver.rs`, `LazyPathBuf` in `bun_paths`) got a destructor in #30875. This one did not. ### Fix - Wrap the pointer in `ModuleBufsSlot`, whose `Drop` destroys the box when the thread exits. Same shape as `BufsSlot`. Access is unchanged, so the recursion notes on the thread local still hold, and the static TLS template is still one pointer. - Correct because the destructor runs when the thread's TLS is torn down, after every resolver frame on that thread has returned. The main thread's box lives for the process, as before. - Verified: `test/js/web/workers/worker-entry-point.test.ts` (new file) runs a worker through an `imports` alias in a child with `detect_leaks=1`. It fails on main with the report above and passes with this change (checked both ways with a debug build). `test/js/bun/binary/tls-segment-size.test.ts` still passes. ### Background - The resolver keeps a few large scratch buffers per thread instead of on the stack. They are boxed on first use and only a pointer sits in TLS, so the TLS segment stays small on every platform. - A worker thread resolves its own entry point and preloads, so it is the common short lived thread that touches these buffers. The bundler's pool threads live as long as the pool. - The ASAN CI lanes run test children with `detect_leaks=1`. The test sets that itself (plus the repo's `test/leaksan.supp`) so that a local ASAN build checks it too. A build without ASAN ignores the options and checks the behaviour only. <details><summary>Notes</summary> Found through #39811, whose worker test resolves an `imports` alias and failed on the ASAN lanes because of this leak. #39811 carries this change until this lands and is otherwise independent of it. #35060 (overflow bundle threads, open) includes the same change as one of its hunks, because its threads are short lived too. The case is in its own file, for the worker entry point resolution cases, rather than in `worker.test.ts`: three of that file's stress cases go over their budget on a debug build on a slow machine, which would hide whether this case itself flips. #39811 adds its worker case to the same file. Without `print_suppressions=0` LeakSanitizer prints a "Suppressions used" table to stderr on exit when an unrelated, suppressed allocation exists in the process, so the test passes that along with the suppressions file when the environment does not already set `LSAN_OPTIONS`. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 1 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/workers/worker-entry-point.test.ts bun test v1.4.0 (4199361) test/js/web/workers/worker-entry-point.test.ts: 41 | LSAN_OPTIONS: 42 | bunEnv.LSAN_OPTIONS ?? 43 | `print_suppressions=0:suppressions=${path.join(import.meta.dir, "..", "..", "..", "leaksan.supp")}`, 44 | }, 45 | ); 46 | expect(stderr).toBe(""); ^ error: expect(received).toBe(expected) - "" + " + ================================================================= + ==385090==ERROR: LeakSanitizer: detected memory leaks + + Direct leak of 12288 byte(s) in 1 object(s) allocated from: + #0 0x000007dd95c8 in malloc crtstuff.c + #1 0x00000be19934 in std::sys::alloc::unix::alloc /root/.rustup/toolchains/nightly-2026-07-20-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/alloc/unix.rs:31:18 + #2 0x00000be184b9 in <std::alloc::System>::alloc_impl /root/.rustup/toolchains/nightly-2026-07-20-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/alloc.rs:149:78 + #3 ... (truncated) release without fix: all passed bun test v1.4.0-canary.1 (2e16ac4) test/js/web/workers/worker-entry-point.test.ts: (pass) package.json imports alias as the entry point > the worker runs and its thread exits without leaking [11.86ms] 1 pass 0 fail 3 expect() calls Ran 1 test across 1 file. [218.00ms] __F:0:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/workers/worker-entry-point.test.ts bun test v1.4.0 (4199361) test/js/web/workers/worker-entry-point.test.ts: (pass) package.json imports alias as the entry point > the worker runs and its thread exits without leaking [3743.31ms] 1 pass 0 fail 3 expect() calls Ran 1 test across 1 file. [6.05s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 667ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [0/5] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compiling�[0m bun_brotli v0.0.0 (/workspace/bun/src/brotli) �[1m�[92m Compiling�[0m bun_outpu ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/resolver/package_json.rs | 23 +++++++++--- test/js/web/workers/worker-entry-point.test.ts | 50 ++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) ``` </details> **gate history** · 1 passed · 1 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/resolver/package_json.rs 2 2 0 test/js/web/workers/worker-entry-point.test.ts 1 2 0 ``` </details> **root cause** · written by the author bot With --target bun or node, the resolver short-circuits node:, bun: and hardcoded builtin specifiers into an external result whose primary path is the bare specifier rather than an absolute file path, and entry-point resolution passed that through, so enqueue_entry_item either tripped the absolute-path assert, reported a misleading "File not found", or for bun:wrap collided with the runtime's pre-registered source and left the build with no entry points. The fix marks entry-point resolutions with their own ImportKind so the resolver no longer applies externalization rules to them, and resolv… <!-- robobun:evidence:end -->
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-22, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
|
The hang still reproduces on 1.4.3 (b993710): the repro in this PR prints I did not reopen this PR. Main changed too much under it (the VM teardown protocol in
A reworked cut on current main is on
It is not a PR yet because it overlaps four open ones, and the order is a maintainer call:
The alternative to helper threads is to run all in-flight builds on the one bundle thread (split |
Problem
All
Bun.build()calls are serialized behind a single bundle thread, which parks inside the in-flight build'sMiniEventLoopwaiting on outstanding plugin promises. A plugin'sonLoad/onResolvethat awaited a nestedBun.build()permanently deadlocked: the inner build sat in the bundle thread's queue while the thread was blocked inside the outer build (not on its queue waker), so the plugin promise never resolved. Once wedged, every subsequentBun.build()in the process hung too. Independent concurrent builds were also head-of-line blocked behind any build with a slow async plugin.esbuild handles the equivalent in ~150ms.
Cause
BundleThread::thread_mainpops itsUnboundedQueueand callsgenerate_in_new_threadstrictly one completion at a time. That call runs the build throughwait_for_parse, which spins a per-buildMiniEventLoop(parked inUwsLoop::tick()) untilpending_items == 0. A nestedBun.buildcallssingleton::enqueue, which pushes to the queue and wakes the bundle thread'sAsync::Waker, but the thread is not waiting on that waker; it is inside the outer build's uws loop. Nothing ever pops the queue, the plugin promise never resolves, andpending_itemsnever reaches zero.Fix
singleton::enqueuenow tracks how manyBun.builds are scheduled but not yet completed. The first one uses the long-lived singleton thread; anyBun.buildscheduled while another is still outstanding runs on a short-lived overflow thread instead. Sequential builds keep sharing the singleton thread; concurrent or nested builds run independently, so neither's completion is gated on another returning from a nested call. This also removes the head-of-line blocking: an independentBun.buildstarted while another is waiting on a slowonLoadruns immediately. If the OS refuses the overflow thread, the build is rejected with a diagnosable error so the plugin promise settles instead of hanging.Running a build on a short-lived thread exposed a handful of single-thread / process-lifetime assumptions, each fixed here:
WaitGroup::finish()nowbroadcasts instead ofsignalling one waiter, soThreadPool::wait_for_all()releases every concurrent bundler thread when the shared WorkPool's counter reaches zero.io_thread_pool::acquire()bumpsREF_COUNTfor a racing acquirer that loses the init race (previously left under-counted, benign only under the old single-bundle-thread invariant).MODULE_BUFSslot gains aDropimpl (matching the existingBufsSlotpattern) so the leakedBoxis reclaimed when a short-lived bundler thread exits.uWS::Loop::get()with a non-null native-loop hint leavescleanMefalse, soon_thread_exit()would skip the free. AddeduWS::Loop::freeLoopWrapperAtThreadExit()and a Rust binding; the overflow thread calls that and thenuv::Loop::shutdown()to release the per-threadus_loop_twrapper (recv/send/cork buffers) and theuv_loop_t's IOCP handle.Verification
New tests in
test/bundler/bun-build-api.test.tsunderBun.build concurrency:nested Bun.build inside an onLoad plugin does not deadlock(and a follow-up independent build still works)nested Bun.build inside an onResolve plugin does not deadlockindependent Bun.build is not blocked behind another build's slow onLoadlater Bun.build whose plugin awaits an earlier build still completes(guards the pre-existing ordering guarantee)The first three hang on
mainand pass with this change; the fourth passes on both.bun-build-api.test.ts,bundler_plugin.test.ts,bundler_plugin_chain.test.ts, and theserve plugins > concurrent requests to multiple routes during plugin loadtest pass under debug+ASAN on Linux; the four concurrency tests also pass on a Windows debug build including a 3x20 concurrent-build stress loop.rust:check-allpasses on all 10 targets.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts