bundler: fix a segfault in bun build --sourcemap when the link step fails - #44502
Conversation
…hose link step failed
|
Updated 8:22 PM PT - Oct 2nd, 2026
@dylan-conway, your commit 8f4e351 is building: |
… fold the two arms of init_and_run
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. WalkthroughBundler teardown now waits for source-map line-offset and quoted-content tasks before running finalizers. Completion-task cleanup deinitializes the bundle after successful and failed builds. Regression tests check repeated CLI and Bun.build runs for the transitive top-level-await require error. ChangesSource-map task lifecycle
Possibly related PRs
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change adds regression tests and a clarifying comment for the source-map teardown fix. No actionable merge-blocking risk was found. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the --watch branch of generate_from_cli (it leaks the Box<BundleV2> instead of tearing down, so in-flight source-map tasks keep a live graph and the missing wait there is not a use-after-free), the folded Ok/Err match in js_bundle_completion_task.rs (set_result still runs before deinit_without_freeing_arena on success, and the error path now waits via the shared teardown instead of locally), and the cost of the unconditional waits (WaitGroup::wait on the default zero count is one lock, one atomic load and one unlock, so builds without source maps do not block).
Extended reasoning...
The change moves the two source-map WaitGroup waits into the shared BundleV2 teardown so every caller (CLI, bake, DevServer, bun test --changed, Bun.build) joins the pool tasks before finalizers and worker teardown, deletes the JS API's now-redundant local waits, tightens three fields to pub(crate), and adds a 40-run CLI stress test. It touches no security-sensitive surface. The inline findings (a SAFETY comment overstating the wait_for_parse invariant, and no Bun.build sibling coverage) already signal a human look; the remaining candidates were ruled out from the code as noted above.
…eue error exception in the Worker SAFETY comment
What does this PR do?
bun buildwith source maps on sometimes crashes when the build fails in the link step. It should print the error and exit with code 1.Expected, and what the other runs print:
The crash is not tied to top-level await.
import { nope } from "./b.js"(No matching export) crashes the same way. It needs--sourcemap; without it there were 0 crashes in 40 runs.Cause
LinkerContext::linkcallscompute_data_for_source_map, which puts two tasks per reachable file on the worker pool. The only waits for them are ingenerate_chunks_in_parallel.?andreturn Errinlinkafter that point returns with the tasks still on the pool. The quickest isreturn Err(ImportResolutionFailed)in step 4 ofscan_imports_and_exports. It comes before the blockingworker_pool.eachof step 5, which otherwise gives the tasks time to start.generate_from_clithen callsdeinit_without_freeing_arena, which walksworkers_assignmentsand readsWorker.threadof every entry.SourceMapDataTask::run_*→Worker::get→get_worker_slow. It has inserted itsBox::<Worker>::new_uninit()pointer into the map and has not reachedworker.write(...).So the main thread reads a
Workerthat was never written:threadreads asNone,Worker::deinitruns on the main thread, and the drop glue ofOption<WorkerData>follows a nullBox<Define>. That is the fault at0x18.0xbe…be.threadreads asSome(0xbebebebebebebebe)andQueue::pushfaults at0xbebebebebebebee6.A debugger stopped at the ASAN fault shows the main thread in
deinit_soonand pool threads atThreadPool.rs:448and:451, inside theWorker { .. }literal ofget_worker_slow, underrun_line_offsetandrun_quoted_source_contents.Bun.builddoes not crash because its caller waits on the two groups on its error path.generate_from_cliandgenerate_from_bake_production_clirun the same teardown without that wait.Fix
deinit_without_freeing_arenafrees what the tasks use, so it now waits for them, before anything else. The first thing it did was to run the finalizers of native plugins, which free source text that these tasks read. The two waits inBun.build's caller are now redundant and are removed. That leaves the two arms of itsmatchthe same, so they are folded into one call.source_mapsand the two groups have no user outside the crate any more and becomepub(crate). Waiting on a group that is already at zero takes a lock and returns.The fixing lines are the two
wait()calls insrc/bundler/bundle_v2.rs.Which build errors this covers
Okreturns also skip the waits: the dependency scanner branch ofgenerate_from_cli, andchunks.is_empty()ingenerate_from_bake_production_cli. They are covered too.Not changed
enqueue_entry_points_*(...)?ingenerate_from_cli,generate_from_bake_production_cliandrun_from_js_in_new_threadreturns beforewait_for_parse, so an error there would reach the teardown with parse tasks on the pool. Every error I traced on that path is an allocation failure. I found no input that reaches it, so there is no test to write. Waiting there is not a drop-in fix either:enqueue_entry_itemincrementspending_itemsbefore its two fallible steps, so after such a failurewait_for_parsewould never return. It is left as it is, and theSAFETYcomment inget_worker_slownames the exception.How did you verify your code works?
New test
default/TopLevelAwaitForbiddenRequireSourceMapCLIintest/bundler/esbuild/default.test.ts, next to the existing test for this error. That test uses theBun.buildbackend without source maps, which is why it never saw the crash. The new test runs the CLI build 40 times, four at a time, and expects the error line and exit code 1 from each. It checks the error line because an ASAN crash also exits with code 1.bun bd test)USE_SYSTEM_BUN=1)The whole file run against the parent release build five times: 152 pass and this test fails, each time. With
bun bd testand both new tests: 154 pass, 0 fail.default/TopLevelAwaitForbiddenRequireSourceMapAPIcoversBun.build, which now relies on the shared wait: 40 builds of the same two files in one subprocess. It passes on main, where the caller has its own wait, so its "before" is this branch with the two sharedwait()calls removed. There ASAN reports a heap-use-after-free incompute_quoted_source_contents, which reads the freed linker graph.Crashes of 40 runs. The two release builds are of the same commit with the same toolchain and differ only by the fix:
--sourcemap=inlineNo matching export, two files,--sourcemap=inline--target=node --sourcemap=inline--splittingCrashes of 10 runs on the debug + ASAN builds, all with
No matching export:export *--splitting --sourcemap=external--compile --sourcemap=inline--minify --target=bun --sourcemap=linkedReleased versions, crashes of 40 runs of the ten-file graph: 1.3.0: 0, 1.4.0: 15, 1.4.2: 10.
Bun.build, which lost its own wait: 540 failing builds with source maps in three processes, six at once, under ASAN. Before and after, every build rejects with the expected message and there is no report.bun build --appwith a link error, the other caller without the wait: 15 crashes of 40 runs before and 0 of 50 after on release, 10 of 10 before and 0 of 30 after under ASAN.An independent A/B of the two release builds and the two ASAN builds:
taskset -c 0), undernice -n 19, under CPU load, or with a graph of 10,000 files.