bundler: give the bundle thread a 16 MiB stack - #38862
Conversation
|
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 (2)
WalkthroughChangesBundler stack configuration
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The PR gives the bundle worker a larger stack to prevent failures on long import chains and adds a focused regression test; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Jarred-Sumner
left a comment
There was a problem hiding this comment.
nooooo lets just not use that much ram?
|
Closing: a bigger stack only moves the threshold (a 3000-file chain still overflows under ASAN). The right fix is to finish what #34554 started and turn the remaining recursive linker walks (find_imported_files_in_css_order, export-star, chunk graph) into explicit-stack DFS, or fail with a BuildMessage via is_safe_to_recurse where that is impractical. |
| test("css @import chain of 1000 files does not overflow the bundle thread stack", async () => { | ||
| const files: Record<string, string> = {}; | ||
| const n = 1000; | ||
| for (let i = 0; i < n; i++) { | ||
| files[`c${i}.css`] = (i + 1 < n ? `@import "./c${i + 1}.css";\n` : "") + `.a${i} { color: red; }\n`; | ||
| } | ||
| const dir = tempDirWithFiles("bun-build-api-css-chain", files); | ||
|
|
||
| const build = await Bun.build({ | ||
| entrypoints: [join(dir, "c0.css")], | ||
| write: false, | ||
| }); | ||
|
|
||
| expect(build.success).toBe(true); | ||
| expect(build.outputs).toHaveLength(1); | ||
| expect(await build.outputs[0].text()).toContain(`.a${n - 1}`); | ||
| }); |
There was a problem hiding this comment.
🔴 This crash-regression test calls Bun.build() in-process, but the failure mode it guards against is AddressSanitizer: stack-overflow on the bundle thread — a process abort, not a catchable error. Per REVIEW.md ("Crash fixes need the crashing input as a spawned fixture"), and matching the "loader map with an empty-string key" test in this same file, wrap the Bun.build() call in a Bun.spawn([bunExe(), "-e", ...]) subprocess and assert on {stdout, stderr, exitCode} so a regression fails just this test instead of killing the whole runner.
Extended reasoning...
What the issue is
The new test "css @import chain of 1000 files does not overflow the bundle thread stack" (test/bundler/bun-build-api.test.ts:46-62) calls Bun.build() directly in the test-runner process. Per the PR description, on an unfixed build this exact input dies with AddressSanitizer: stack-overflow in find_imported_files_in_css_order::Visitor::visit — a hard process abort on the bundle thread. BundleThread.rs itself notes panic = "abort" on that thread, and a stack overflow is not a catchable Rust panic anyway; the whole process is gone.
REVIEW.md's Tests reviewers reject section states explicitly: "Crash fixes need the crashing input as a spawned fixture". This is listed among rules that have blocked merges.
Step-by-step: how it manifests
- The fix in this PR is one line:
.stack_size(16 * 1024 * 1024)on the bundle thread builder. - Suppose a future change drops that line (a refactor of
spawn(), or someone "cleaning up" the builder chain), or per-frame stack usage in the CSS@importDFS grows — the PR description already notes a 3000-file chain still overflows 16 MiB on debug+ASAN, so the margin at 1000 files is finite. - On CI,
bun bd test test/bundler/bun-build-api.test.tsreaches this test and callsBun.build()in-process. - The bundle thread recurses through 1000 CSS
@importedges, blows its stack, ASAN printsstack-overflow, and the process aborts. - Every subsequent test in
bun-build-api.test.ts— the leak tests, the "can be called thousands of times" test, the sourcemap tests — is never run. CI reports an opaque runner crash / signal exit for the whole file rather than one clean✗ css @import chain of 1000 files…failure.
Why nothing prevents it
There is no isolation here. await Bun.build(...) runs on the process's singleton bundle thread; a stack overflow there is not surfaced as a rejected promise, it aborts the process. try/catch cannot help.
In-file precedent
This file already encodes the correct pattern. The "loader map with an empty-string key" test carries this exact comment:
"Run in a subprocess so a crash in the bundler thread surfaces as a test failure instead of taking down the test runner."
and spawns bunExe() -e for its Bun.build() call. The "can be called thousands of times in one process without crashing" test — also a crash regression — likewise runs in a spawned subprocess and asserts on {stdout, stderr, exitCode}.
How to fix
Move the Bun.build() into a spawned subprocess:
test("css @import chain of 1000 files does not overflow the bundle thread stack", async () => {
const files: Record<string, string> = {};
const n = 1000;
for (let i = 0; i < n; i++) {
files[`c${i}.css`] = (i + 1 < n ? `@import "./c${i + 1}.css";\n` : "") + `.a${i} { color: red; }\n`;
}
const dir = tempDirWithFiles("bun-build-api-css-chain", files);
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const build = await Bun.build({
entrypoints: [${JSON.stringify(join(dir, "c0.css"))}],
write: false,
});
if (!build.success) throw new AggregateError(build.logs, "build failed");
const text = await build.outputs[0].text();
console.log(JSON.stringify({ success: build.success, outputs: build.outputs.length, hasLast: text.includes(".a${n - 1}") }));
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(JSON.parse(stdout.trim())).toEqual({ success: true, outputs: 1, hasLast: true });
expect(exitCode).toBe(0);
});A regression then shows up as exitCode !== 0 (SIGSEGV/SIGABRT) with the ASAN report on stderr — one failed test, runner intact.
On the refutation
One verifier flagged bug_002 as a duplicate of bug_001. That is correct and is why these were merged into a single finding; it does not bear on whether the finding itself is real.
|
|
||
| const build = await Bun.build({ | ||
| entrypoints: [join(dir, "c0.css")], | ||
| write: false, |
There was a problem hiding this comment.
🟡 write is not a Bun.build option — it's not in the BuildConfig interface (packages/bun-types/bun.d.ts) and JSBundler.rs never reads it, so this line is a silently-ignored no-op that will fail typecheck against the published types. Bun.build already returns in-memory outputs whenever outdir is omitted (see the neighboring "css works" test), so just drop the write: false line.
Extended reasoning...
What the bug is
The new test at test/bundler/bun-build-api.test.ts:56 passes write: false to Bun.build(), but write is not a recognized BuildConfig option in Bun. This is an esbuild-ism — esbuild's build() API uses write: false to request in-memory output instead of writing to disk, and the author likely copied the convention from there (test/js/third_party/esbuild/esbuild-test.js uses it for esbuild.build calls).
The specific evidence
-
Not in the type definition: The
BuildConfiginterface in packages/bun-types/bun.d.ts (lines ~2899-3292) enumerates every accepted property —entrypoints,outdir,target,format,naming,splitting,plugins,external,define,loader,sourcemap,minify,bytecode,compile, etc. There is nowritefield. -
Not read at runtime: src/runtime/api/JSBundler.rs, which parses the
Bun.buildconfig object, contains ~39config.get*property reads. None of them reads a property named"write". The option is silently ignored — Bun.build does not perform excess-property validation on unknown keys. -
Explicitly documented as n/a: docs/bundler/esbuild.mdx:174 lists esbuild's
writeoption as "n/a" for Bun.build, with the note that output is in-memory wheneveroutdir/outfileis unset.
Why the test still works
Bun.build already returns in-memory BuildArtifact blobs whenever no outdir is provided. The neighboring "css works" test at line 36 of the same file calls Bun.build with only entrypoints and minify, then reads build.outputs[0].text() — no write option needed. So the new test happens to work correctly, but only because the write: false is dead: omitting outdir is what actually produces the in-memory output.
Impact
- Runtime: none — the option is ignored and the test passes.
- Typecheck: this will produce a TypeScript excess-property error against the published
.d.ts(Object literal may only specify known properties, and 'write' does not exist in type 'BuildConfig'). - Maintenance: it's misleading — a future reader may assume
writeis a real Bun.build option and copy it elsewhere.
Per REVIEW.md: "Every line you add must be demonstrably live … parsed-but-never-read is a red flag".
Step-by-step proof
- Call site:
Bun.build({ entrypoints: [join(dir, "c0.css")], write: false }). Bun.buildconfig parsing lives in src/runtime/api/JSBundler.rs. Grepping everyget_truthy/get_optional/ property-name string in that file yields no"write"— the property is never read off the config object.- Since no
outdiris passed,JSBundlerleavesoptions.outdirempty, and the completion path returnsBuildArtifactblobs in memory (the same path the "css works" test on line 36 relies on). - Therefore removing
write: falseproduces byte-identical behavior; keeping it only adds a line that the runtime never reads and that TypeScript will flag againstBuildConfig.
Fix
Delete line 56:
const build = await Bun.build({
entrypoints: [join(dir, "c0.css")],
- write: false,
});No outdir already means in-memory output.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
What does this PR do?
The thread that runs
Bun.build()was spawned with Rust's default 2 MiB stack. The Zig version usedstd.Thread's 16 MiB default, and the linker's graph walks (CSS@importorder, export-star, chunk graph) still recurse per edge, so aBun.build()on a long import chain now overflows where 1.3.14 didn't. The CLI path runs on the main thread and was unaffected. Every other long-lived thread already sets an explicit size (Debugger uses 16 MiB), this one was missed.Out of scope: converting
find_imported_files_in_css_orderand the remaining walks to an explicit stack. On a debug+ASAN build a 3000-file chain still overflows at 16 MiB (release 1.3.14 handles it); that needs the DFS rewrite, not a bigger stack.How did you verify your code works?
Added a
Bun.build()test with a 1000-file CSS@importchain. On main's debug build the same input dies withAddressSanitizer: stack-overflowinfind_imported_files_in_css_order::Visitor::visit; with this change it builds (bun bd test test/bundler/bun-build-api.test.ts -t "chain of 1000", 2.5s).