bake: fail bun build --app when an output file cannot be written - #39295
bake: fail bun build --app when an output file cannot be written#39295robobun wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Limit details: You’ve used all 5 included reviews currently available under your plan. 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 selected for processing (2)
Comment |
|
Status: reworked and stacked on #39302, waiting on CI. The PR base is #39302's branch, so the diff here is only this change; it merges after that one. Reproduced on the 1.4.0 canary (eabb96d) and on a debug build of main: with a regular file where the output directory goes, The four new tests in Earlier state: the first version ended the build with |
There was a problem hiding this comment.
LGTM — small, well-scoped fix that turns silent write failures into a nonzero exit.
What was reviewed:
OutputDir::writepreserves the exact error-reporting path (samehandle_error_return_trace+Output::errmessage) and only adds a counter; verified no otherroot_diruses remain.Global::crash()onfailed_writes > 0matches how sibling failures in this function exit and mirrorsbuild_command.rs'shad_errreport-all-then-exit pattern.- Tests cover each of the three call sites plus the all-fail case, assert exact failed-write sets and
prerendered: false, and use per-test temp dirs sodescribe.concurrentis safe.
Extended reasoning...
Overview
The PR changes src/runtime/bake/production.rs to make bun build --app exit 1 when any output file cannot be written to dist/, instead of printing an error and continuing to prerender broken pages. Three duplicated file.write_to_disk(...) → Output::err blocks are folded into a small OutputDir helper that also counts failures; after the last write the build calls Global::crash() if the count is nonzero. Four tests are added to test/bake/dev/production.test.ts, one per call site plus the all-fail case.
Security risks
None. This is CLI exit-code / control-flow behavior in the static-site build path. No parsing of untrusted input, no auth/crypto, no new syscalls beyond what was already there.
Level of scrutiny
Low-to-moderate. The Rust change is a mechanical dedup plus a single new guard; the error message and handle_error_return_trace call are byte-identical to before, so the only observable difference is the new Global::crash() when writes fail — which is unambiguously the correct behavior (the old exit-0-with-broken-output is the bug). Global::crash() is the same exit path used by every other user-facing failure in build_with_vm (unresolved framework imports, missing prerender export), and the report-all-then-exit-1 shape matches build_command.rs's had_err pattern the PR cites. I grepped for remaining root_dir references — none, so the rename is complete. OutputDir owns the bun_sys::Dir for the same scope the local did, so drop timing is unchanged.
Other factors
The tests are thorough: each of the three write sites (client chunk in the loop, server dump under --debug-dump-server-files, runtime chunk after the loop) is exercised in isolation, plus a case where every write fails. Each asserts the exact sorted set of failed paths via expect.stringMatching, exitCode: 1, and that dist/index.html is not written. describe.concurrent is safe because each test gets its own tempDirWithBakeDeps. No CODEOWNERS cover the touched paths. No prior review comments to address.
|
Updated 2:19 AM PT - Aug 16th, 2026
✅ @robobun, your commit 84d7b7bfe53c79e9887f0aae1de11da2606e8cbd passed in 🧪 To try this PR locally: bunx bun-pr 39295That installs a local version of the PR into your bun-39295 --bun |
There was a problem hiding this comment.
LGTM — straightforward fix that turns silently-ignored write failures into a nonzero exit.
What was reviewed:
- The three
write_to_diskcall sites are consolidated intoOutputDir::writewith identical error reporting; only the counter and post-loopGlobal::crash()are new behavior. - Confirmed
root_dirhad no other uses;root_dir_path(passed to prerendering) is a separate variable and untouched. - The
had_err→ exit-1 pattern mirrorsbuild_command.rs:756-1157as the description states;Global::crash()matches the other user-facing failures in this function. - Tests cover each of the three call sites plus the all-fail case, assert exit 1 / no
index.html/ the specific failed paths, and usedescribe.concurrentwith isolated temp dirs.
Extended reasoning...
Overview
The PR fixes bun build --app (bake static build) exiting 0 when client chunks, the runtime chunk, or dumped server files fail to write to dist/. Previously each of the three write_to_disk call sites in build_with_vm printed an error and continued, so prerendering ran anyway and produced index.html referencing missing chunks. The fix introduces a small OutputDir struct that wraps the bun_sys::Dir handle and a failed_writes counter; the three call sites now go through OutputDir::write (identical error reporting, plus += 1), and after the last write the build calls Global::crash() if any write failed, before PerThread::init and prerendering. Four new tests in test/bake/dev/production.test.ts cover each call site and the all-fail case.
Security risks
None. This is CLI-side error-handling for local disk writes during a build; no untrusted input parsing, no network, no auth/crypto. The change only adds a failure counter and an early exit.
Level of scrutiny
Low-to-medium. The Rust change is a mechanical dedup of three copy-pasted error blocks into one helper, plus a counter and a Global::crash() — the same exit mechanism already used throughout build_with_vm for user-facing failures (unresolved framework imports, missing prerender export). I verified root_dir has no remaining references after the rename to output_dir.dir, that root_dir_path (the string passed to BakeRenderRoutesForProdStatic) is a separate binding and untouched, and that the report-all-then-exit-1 pattern matches build_command.rs (had_err) as claimed. The bun_sys::Dir ownership is unchanged — moved into the struct, dropped at end of scope as before.
Other factors
The tests are well-constructed per the repo review guidelines: one test per call site (client chunk in the loop, runtime chunk after the loop, server file under --debug-dump-server-files) plus the all-fail case; they assert the exact set of failed paths (sorted, via toMatchObject with expect.stringMatching), that dist/index.html was not written, and exit 1. describe.concurrent is safe since each test has its own tempDirWithBakeDeps. The Windows mkdir hang was worked around by blocking the leaf output directories rather than dist/_bun itself, with the underlying bun_sys bug noted separately. The comment-cop bot feedback (long doc comment) was addressed in 9b5175b and the thread is resolved. The PR description documents that all four tests fail on the unfixed build on Linux and Windows and pass with bun bd.
The three write_to_disk call sites in the production build (client chunks, server files under --debug-dump-server-files, and the runtime chunk) printed the error and carried on, so the routes were still prerendered against chunks missing from dist/ and the process exited 0. Route the writes through one helper that reports and counts the failures, and return BakeBuildFailed after the last write, before prerendering, so build_command exits 1 through the build VM like the other failures the build reports.
9b5175b to
6fdb587
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The Rust change is a small, correct refactor (extract the three write_to_disk sites into OutputDir::write, count failures, return BakeBuildFailed before prerendering), and the four new tests cover each call site. Because it's stacked on #39302 and changes the user-facing exit code of bun build --app, a human look would still be worthwhile before it lands.
What was reviewed:
OutputDir::writepreserves the original error message andhandle_error_return_tracecall;root_diris only moved into the struct (no other use sites), so the fd lifetime is unchanged.- The
failed_writes > 0check is placed after the last write (runtime chunk) and beforePerThread::init/prerendering, so the success path is unchanged. - Tests:
failedWritessort order matches thestringMatchingmatcher order;describe.concurrent,bunEnvspread, and timeout scaling match the siblingfailures reported by the buildblock from #39302.
Extended reasoning...
Overview
The PR touches two files: src/runtime/bake/production.rs (adds an OutputDir helper struct and replaces three inline write_to_disk error-handling blocks with calls to it, then checks the failure count after the last write) and test/bake/dev/production.test.ts (adds a four-test describe.concurrent block, one test per write call site plus an all-fail case). It is stacked on #39302, which introduces Error::BakeBuildFailed and the build_command handling of it that this PR relies on.
Security risks
None. The change only affects the exit code and whether prerendering runs after a disk write failure in bun build --app. No new inputs are parsed, no auth/crypto/permissions code is touched, and the error path already existed — it just didn't fail the build.
Level of scrutiny
Medium. The Rust diff is small (~40 net lines, mostly deduplication of three copy-pasted blocks) and the fix is clearly correct — exit 0 after Failed to write ... to output directory is unambiguously a bug. However, it changes user-facing CLI exit behavior, is stacked on an unmerged PR, and the tests depend on debug-only flags (--debug-no-minify, --debug-dump-server-files) and work around a separately-reported Windows mkdir bug. That's enough moving parts that a human should confirm the stack ordering and CI results across platforms.
Other factors
The comment-cop bot flagged the OutputDir doc comment twice; both were addressed (the comment was removed in 84d7b7b). The tests follow the conventions of the sibling failures reported by the build block in the same file (from #39302): BUN_DESTRUCT_VM_ON_EXIT=1, exit event assertion via a process.on('exit') handler in the config, toMatchObject on a combined result, and 30_000 * WAIT_MULTIPLIER timeouts. The failedWrites extraction sorts the matched paths and the expected matchers are listed in the resulting alphabetical order, so the toMatchObject array comparison is stable. I did not find any leaked resources (output_dir.dir has the same lifetime root_dir had before) or other uses of root_dir past the point it is moved into the struct.
Stacked on #39302 (the PR base is its branch, so the diff here is only this change); it uses the
BakeBuildFailedreturn that PR introduces.Problem
bun build --appexits 0 when a client chunk or the runtime chunk cannot be written todist/. It printsENOTDIR: Failed to write "_bun/<hash>.js" to output directoryfor each file, prerenders the routes anyway, anddist/index.htmlends up referencing chunks that do not exist, so a CI build publishes a broken site. Same for the server files written by--debug-dump-server-files."use client"component and a regular file where the output directory goes: threeFailed to writelines,dist/index.htmlwritten, exit 0.write_to_diskcall sites inbuild_with_vm(src/runtime/bake/production.rs: client chunks, server files under--debug-dump-server-files, and the runtime chunk after the loop) each callOutput::errand continue. Nothing records that a write failed, so the function goes on toBakeRenderRoutesForProdStaticand returnsOk(()).Fix
OutputDir::write, which reports the failure as before and counts it. After the runtime chunk, the last write,build_with_vmreturnsError::BakeBuildFailedif anything failed, beforePerThread::initand prerendering;build_command(from bake: return bun build --app failures instead of exiting; name the output directory it cannot open #39302) then exits 1 through the build VM, the same way as the failures the build already reports.test/bake/dev/production.test.ts, newoutput files that cannot be writtendescribe, one test per call site plus the all-fail case. Every test puts a file or directory where an output goes, runs withBUN_DESTRUCT_VM_ON_EXIT=1like the bake: return bun build --app failures instead of exiting; name the output directory it cannot open #39302 tests, and asserts the exact list of reported files, exit 1,exit event: 1from anexithandler in the config (the build returned instead of exiting in place), and nodist/index.html:dist/_bun/bun-framework-reactanddist/_bun/components: the client entry, the runtime chunk and the client component are all reporteddist/_bun/components: only the client component chunk (loop call site)dist/_bun/pageswith--debug-dump-server-files: only the server page module (server call site)index.htmlwritten) on Linux and on Windows x64, and pass withbun bd; the whole file (including the bake: return bun build --app failures instead of exiting; name the output directory it cannot open #39302 tests) passes withbun bd;cargo clippy -p bun_runtimeis clean.--debug-no-minifyso the output files are named after their sources instead of by hash. Like--appitself, the two debug flags exist on canary and debug builds, which is what CI runs. They carry the same explicit timeout as the bake: return bun build --app failures instead of exiting; name the output directory it cannot open #39302 tests in this file because each one bundles a react app (the runtime chunk test twice), which takes several seconds on the debug ASAN build.dist/_bun: with a file in place of an ancestor directory, bun's recursive mkdir on Windows alternates between ENOENT for the child and EEXIST for the file forever (make_path_withinsrc/paths/component_iterator.rswith the Windows step insrc/sys/lib.rs), which is how the first version of the test hung on the Windows lanes. That is a separate, pre-existingbun_sysbug, reported separately; on POSIXmkdirreturns ENOTDIR and both shapes behave the same.Background
bun build --app(bake's static build,production.rs) bundles the app, writes the client-side output files todist/_bun/, keeps the server-side output files in memory, and then prerenders every route todist/<route>/index.htmlfrom JS (BakeRenderRoutesForProdStatic). Server files are only written to disk with the debug flag--debug-dump-server-files.__esm,__commonJS, ...). It comes out of the server bundle, but the client chunks import it, so it is also written todist/_bun, after the loop over the other outputs; that is why it is a separate call site.Error::BakeBuildFailed(bake: return bun build --app failures instead of exiting; name the output directory it cannot open #39302) means "the build already printed why it failed".build_commandcatches it afterbuild_with_vmreturns and exits through the VM (on_exit, which runsprocess'sexithandlers, thenglobal_exitwith exit code 1), instead of the build code callingexititself.Earlier version
The first version (b3c6274 to 9b5175b) was based on main and ended the build with
Global::crash()at the check, matching the other failure sites in the file at the time, and described that as the same report-all-then-exit shape asbun build. Review of #39302 asked that bake's code return its failures instead of exiting, and that PR converts the existing sites, so this one was rebased onto it and returnsBakeBuildFailedinstead. Thebun buildcomparison was also dropped: bundledbun build --outdirstops at the first write it cannot do (writeOutputFilesToDisk); only--no-bundlereports every file.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bake/dev/production.test.ts