Bun.build({ compile }): produce the executable on the bundle thread instead of blocking the event loop - #37507
Bun.build({ compile }): produce the executable on the bundle thread instead of blocking the event loop#37507robobun wants to merge 8 commits into
Conversation
The bundle completion task ran do_compilation (to_executable: copying and rewriting the bun binary, and downloading it first for a cross target) on the JS thread before settling the promise, so the event loop stalled for the whole write. Run it on the bundle thread right before the completion is posted back; on_complete now only reports the result. to_executable and download_to_path only read the env loader, so they take it by shared reference. The bundle thread gets the same stack size as bun's other threads since this path now runs there too.
WalkthroughChangesBun.build executable compilation now runs on the bundle thread before VM completion dispatch. Dotenv access is shared, TLS rejection caching is atomic, and a regression test checks event-loop responsiveness during cross-target compilation. ChangesBundle compilation flow
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced, fixed, CI green on the current revision (build 92438, 190/190), ready for review. Reproduced with a debug build: a 1ms |
download_to_path now runs on the bundle thread for Bun.build({ compile })
cross targets, while fetch and TLS sockets on the JS thread consult the
same memo, so it can no longer be a plain Cell.
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 `@src/standalone_graph/StandaloneModuleGraph.rs`:
- Line 1657: Update the progress-rendering path around Progress::refresh to
serialize all writes to the shared stderr descriptor across threads. Add and
reuse process-wide synchronization around fd_write_all_quiet, or ensure progress
rendering is dispatched exclusively on the JS thread; do not rely on the
thread-local Source for descriptor safety.
In `@test/bundler/bun-build-compile.test.ts`:
- Around line 108-119: Move proc.send("ping") into the existing try block in the
ping/timeout flow so any IPC send failure reaches the finally block and clears
giveUp. Preserve the current Promise.race behavior and outcome handling.
🪄 Autofix
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: 9f29f409-2705-4e80-a989-dd514d99d695
📒 Files selected for processing (7)
src/bundler/BundleThread.rssrc/dotenv/env_loader.rssrc/options_types/compile_target.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/cli/build_command.rssrc/standalone_graph/StandaloneModuleGraph.rstest/bundler/bun-build-compile.test.ts
|
Updated 2:16 PM PT - Aug 11th, 2026
✅ @robobun, your commit 73cce2b2825ec1a93f055be0a389e2ce5c9bba04 passed in 🧪 To try this PR locally: bunx bun-pr 37507That installs a local version of the PR into your bun-37507 --bun |
…() is called download_to_path read NODE_TLS_REJECT_UNAUTHORIZED and the proxy variables from the VM's env loader, and handed the HTTP client a proxy URL borrowing the loader's bytes for the whole download. Now that the download runs on the bundle thread, a process.env.HTTP_PROXY assignment on the JS thread could free those bytes under it. CompileTarget::download_options copies the two values out on the calling thread (Config::from_js for the JS API, right before to_executable for the CLI) and to_executable takes that instead of the loader, so the compile step no longer touches the loader at all and the env memo can stay a plain Cell.
Both new tests 404 the download or reject the template up front, so neither copies the bun binary.
There was a problem hiding this comment.
I re-reviewed after 3342892: the DownloadOptions snapshot supersedes my earlier Cell race note and also removes the borrowed proxy-URL held across the download, so the bundle-thread compile step no longer touches the VM's env loader at all. No further issues found. Because this moves file I/O, the cross-target download, and executable patching onto the bundle thread and leans on the embedded_work / stop_for_vm_teardown ordering for teardown safety, a human pass on the concurrency reasoning would still be worthwhile.
Checked: do_compilation now reads only task-owned data (config, log, output_files) and DownloadOptions; self.env is no longer dereferenced there. compile_on_bundle_thread preserves the prior result/log shape (CompileResult::Err → log entry + CompilationFailed) and skips on cancelled, matching the old on_complete early-return. The Output::flush() after compile is covered by the new stderr test; the CLI path is unchanged apart from building DownloadOptions at the call site.
Extended reasoning...
Overview
Moves Bun.build({ compile })'s executable-production step (do_compilation → to_executable → possible cross-target download + binary patch + sourcemap writes) from on_complete on the JS thread to compile_on_bundle_thread, which runs inside complete_on_bundle_thread before the completion is posted back. To make that step thread-safe, to_executable/download_to_path now take a DownloadOptions (reject_unauthorized + owned proxy href) that CompileTarget::download_options copies out of the env loader on the calling thread — Config::from_js for the JS API, immediately before to_executable for the CLI. bun_standalone_graph drops its bun_dotenv dependency, exe_path loses its unused _env param, the bundle thread gets DEFAULT_THREAD_STACK_SIZE, and an Output::flush() is added after the compile so bundle-thread stderr reaches the fd. Three new tests cover the event-loop responsiveness, the proxy-snapshot semantics (before/after), and the stderr flush.
Security risks
None identified. DownloadOptions::default() keeps reject_unauthorized: true; the real value is filled in from the loader on the JS thread before the config leaves it (standalone-HTML / browser-target builds that skip the fill-in also clear config.compile, so the default is never consulted for a download). The proxy href is now an owned Box<[u8]> rather than a borrow into the env map, closing a use-after-free window if user JS reassigned process.env.HTTP_PROXY mid-download.
Level of scrutiny
High. This is a thread-affinity change to a path that does blocking file I/O, a synchronous HTTP download, and executable patching, and it interacts with VM-teardown ordering (cancelled, bundle_loop, embedded_work_finished). The PR description enumerates each concern (env-loader access, send_sync off-thread, per-thread stderr buffering, stop_for_vm_teardown concurrency, HTML/browser-target builds, stack size) and the diff addresses each; my earlier data-race finding was resolved by removing the loader access entirely. That said, the correctness argument depends on invariants scattered across BundleThread, LoopHandle, and the HTTP thread's shutdown ordering, which is exactly the kind of reasoning a maintainer should confirm.
Other factors
All prior review threads (my Cell race note, CodeRabbit's timer-cleanup and stderr-interleaving comments, the comment-cop long-comment flags) are resolved in the current revision. The new tests are hermetic (local 404 server via BUN_COMPILE_TARGET_TARBALL_URL, fresh BUN_INSTALL_CACHE_DIR, proxy env cleared), await observable conditions rather than sleeping, and the PR states the "after" proxy case and the ping test fail on the unfixed build. CI on the previous revision was green apart from a known unrelated flake.
What
While
await Bun.build({ compile: { ... } })is pending, the calling thread's event loop stops once bundling finishes and does not resume until the output executable has been fully written: no timers fire, no child exits or pipe reads are observed. Under a debug build (1 GB binary) that is a 3 to 4 second stall; with a release binary it is about 100ms on NVMe and grows with binary size and disk/antivirus speed. For a cross-compile target the download of the target binary happens on the same synchronous path. Observed in #37496, where every test running concurrently with oneBun.build({ compile })reported the compile's duration.Before (debug build):
build took 4294ms, one timer gap144 -> 4304, 381 interval ticks in total. After: no gaps, about 4200 ticks during the build, same executable written.Cause
JSBundleCompletionTask::on_completeruns on the JS thread when the bundle thread posts the finished build back. Forcompileit calleddo_compilationright there, i.e.collect_compile_assets,StandaloneModuleGraph::to_executable(cross-target download, copy of the running binary, read back, ELF/Mach-O/PE patch, full rewrite, rename) and the external sourcemap writes, and only then settled the promise.Fix
complete_on_bundle_threadnow callscompile_on_bundle_threadbefore posting the completion, so the executable is produced on the bundle thread andon_completeonly reports the result. The result/log mutations are the same as before (CompileResult::Erris appended to the log and the result becomesErr(CompilationFailed), soto_js_errorand thethrow: falseshape are unchanged); they just happen before the hand-off. A build whose VM is tearing down (cancelled) skips the step, matching the old code, whereon_completereturned before compiling.What the moved step may touch, and how that was made true:
download_to_pathlooked upNODE_TLS_REJECT_UNAUTHORIZEDand the proxy variables and handed the HTTP client a proxy URL borrowing the loader's bytes for the whole download. Off the JS thread that is a race withprocess.env.HTTP_PROXY = ...(which replaces the map entry and frees the old bytes). SoCompileTarget::download_optionsnow copies those two values out on the calling thread (Config::from_jsfor the JS API, so the decision is made whenBun.build()is called, likefetch(); immediately beforeto_executablefor the CLI, i.e. exactly when it was read before), andto_executable/download_to_pathtake thatDownloadOptionsinstead of aLoader. The step now touches task-owned data and the file system only;self.envis used solely by the pre-existingTranspiler::initcall, andbun_standalone_graphno longer depends onbun_dotenv.AsyncHTTP::send_sync, a blocking wait on the HTTP thread, which works from any thread (it is how the CLI uses it). On main-thread exit the HTTP thread is parked only afterwait_for_embedded_work, andembedded_work_finishedis reported after the compile, so a teardown during a download waits for it rather than deadlocking. That wait is not new: before, nothing could even callprocess.exit()until the download returned.inject()and a failed sourcemap write report their specific cause to stderr.Outputbuffers stderr per thread and nothing else ever flushes the bundle thread's buffer, so the step flushes after compiling (pool workers flush after each task for the same reason). The new stderr test fails if the flush is removed.stop_for_vm_teardowncan still run concurrently with the compile; it only touches atomics and the plugin cell, neither of whichdo_compilationuses. The loop pointer it may wake is the bundle thread's thread-lifetime uws loop, not the per-buildAnyEventLoop.compile+target: "browser"builds never reach the step (config.compileisNoneby then), exactly as before.DEFAULT_THREAD_STACK_SIZElike bun's other threads instead of Rust's 2 MiB default. On Windowsdo_compilation+to_executable+injectdeclare well over half a megabyte ofPathBuffer/WPathBufferlocals between them; that fits in 2 MiB, this just removes the question.bun build --compilecallsto_executablesynchronously on its main thread, which is fine since there is no event loop to block; apart from building theDownloadOptionsat the call site it is untouched. #33621 (mmap the source binary instead of copying it) is complementary: it makes the step cheaper, this takes it off the JS thread. #34353 (stop continuing past aninject()failure) is also compatible: it keeps the stderr line the new test asserts and still produces one log entry.Tests
All in
test/bundler/bun-build-compile.test.ts, in a concurrentdescribe. None of them copies the bun binary: the download is answered with a 404 by a server inside the test (BUN_COMPILE_TARGET_TARBALL_URLplus a freshBUN_INSTALL_CACHE_DIR, target OS different from the host), or the template is rejected up front. About 0.5s each under a debug build.the event loop keeps running meanwhile: the server withholds the 404 until the building child has answered an IPC ping. A child whose JS thread is inside the compile step cannot answer, so the unfixed build deterministically reportsoutcome: "did not answer the ping"(the 4s bound only decides how long to wait before saying so; the unblocked child answers in about 50ms under a debug build, 50 to 170ms with every core busy). Also asserts the error produced on the bundle thread comes back throughlogsand that the child exits on its own.the download uses the proxy settings from when Bun.build() was called,HTTP_PROXYset before / after the call: a second server plays proxy; the request must arrive at the proxy in the first case and at the registry in the second. The "after" case fails on the unfixed build and on the first revision of this PR (both read the environment when the download starts).a template that cannot be patched reports the cause on stderr:executablePathpointing at a text file with a Linux target, so the ELF code rejects it on every host; asserts the exact stderr line, one log entry, and no outfile. Passes on the unfixed build (the JS thread's buffer was flushed at exit) and fails if the bundle-thread flush is removed.Also run on this branch: the rest of
bun-build-compile.test.ts,bun-build-compile-sourcemap.test.ts(sourcemap writes now on the bundle thread),compile-asset-bunfs.test.ts(asset collection and the compile error path),bundler_compile.test.ts -t HelloWorldWithProcessVersionsBunAPIand its--compile-executable-pathcase (plugins + compile, CLI cross target),compile-windows-metadata.test.ts,standalone.test.ts(compile+ browser target),bun-build-api.test.ts; all pass. The only CI failure on the previous revision wasworker-transfer-list.test.tson the x64-asan lane, a known worker-termination flake (#37267, #34655, #34644).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-compile.test.ts