Skip to content

bake: fail the build when a component throws during static prerender - #33216

Open
robobun wants to merge 8 commits into
mainfrom
farm/d58ebc7a/fix-bake-prerender-hang
Open

robobun wants to merge 8 commits into
mainfrom
farm/d58ebc7a/fix-bake-prerender-hang

Conversation

@robobun

@robobun robobun commented Jul 1, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

test/bake/dev/production.test.ts > "works with sourcemaps - error thrown in React component" times out on every POSIX CI build. The test runs bun build --app on an app whose page throws during static generation and expects the build to print the sourcemapped error and exit 1. Instead the build prints the error and then spins at 100% CPU forever.

Reproduction (fresh fixture; stock release bun and a debug build both hang):

// pages/index.tsx
export default function IndexPage() {
  throw new Error("oh no!");
  return <div>Hello World</div>;
}
$ timeout -s KILL 40 bun build --app ./src/index.tsx; echo "exit=$?"
Rendering routes
error: oh no!
      at r (pages/index.tsx:2:13)
error: An error occurred in the Server Components render. ...
exit=137   # still running, SIGKILL at timeout

Cause

The static prerender path in bun-framework-react has no error handling, unlike the streaming path used by the dev server:

  • renderToStaticHtml (ssr.tsx) passes only onAllReady to react-dom's renderToPipeableStream. On a render error onAllReady never fires, so pipe(stream) is never called and StaticRscInjectionStream.result never settles. There is no onError and no rscPayload.on("error") listener.
  • prerender (server.tsx) calls the flight renderToPipeableStream with no onError and no abort, so the renderer is never stopped.

Because the result promise never settles and the renderers keep the loop busy, vm.wait_for_promise in production.rs loops forever. The streaming sibling (render + renderToHtml + RscInjectionStream) already handles all of this.

It went red fleet-wide with no Bun change because the bake harness installs react/react-dom on a floating @experimental range while react-server-dom-bun is pinned older; a newly published React stopped settling the error-path render on its own. The missing error handling was always latent.

Fix

Mirror the streaming path in the static path:

  • prerender wires a MiniAbortSignal + onError into the flight renderer that records the error, aborts the flight render, and destroys the RSC stream.
  • renderToStaticHtml takes the signal, passes onError to react-dom (abort the HTML render + reject the result), and StaticRscInjectionStream rejects when the RSC stream errors.
  • The error propagates out of prerender, so the build fails with the underlying sourcemapped error instead of hanging.

This also uncovered a sibling gap in the caller: for routes generated from getStaticPaths (the { paths: [...] } form), renderRoutesForProdStatic in src/js/builtins/Bake.ts mapped with a block-body arrow that dropped the callRouteGenerator promise, so the rejection became an unhandled rejection and the build still exited 0. The arrow now returns the promise so Promise.all awaits the render (and the per-page writes).

With the fix bun build --app exits 1 and prints:

2 |   throw new Error("oh no!");
            ^
error: oh no!
      at pages/index.tsx:2:13

Verification

  • bun bd test test/bake/dev/production.test.ts -> 9 pass, 0 fail (includes a new test for a getStaticPaths route that throws -> exit 1).
  • Reverting only the src/ change makes the target tests hang/exit 0 and fail (fail-before holds).

The test gets an explicit ASAN-aware timeout because a debug+ASAN bun build --app takes several seconds and the default 5s test timeout is too tight.

The static prerender path (`renderToStaticHtml`) had no error handling,
unlike its streaming sibling `renderToHtml`. When a component throws during
server-side generation, React never calls `onAllReady`, so the result
promise never settles and neither renderer is aborted. `bun build --app`
printed the error and then spun at 100% CPU instead of exiting.

Mirror the streaming path: wire an abort signal through `prerender`, pass
`onError` to the flight and HTML renderers so a render error aborts both and
rejects the result promise, and reject `StaticRscInjectionStream` when the
flight payload errors. The build now surfaces the thrown error and exits 1.
@robobun

robobun commented Jul 1, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:52 AM PT - Jul 2nd, 2026

❌ @robobun, your commit c928d96 has 1 failures in Build #67814 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33216

That installs a local version of the PR into your bun-33216 executable, so you can run:

bun-33216 --bun

@coderabbitai

coderabbitai Bot commented Jul 1, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Static rendering error handling now propagates failures through a shared abort signal, the production route-generation fallback now awaits all generated pages, and related tests use ASAN-aware timeouts.

Changes

Static rendering and route generation

Layer / File(s) Summary
Static render signal support
src/runtime/bake/bun-framework-react/ssr.tsx
renderToStaticHtml passes a MiniAbortSignal into StaticRscInjectionStream, and the injection stream rejects using the abort reason when the RSC payload errors.
Prerender abort wiring
src/runtime/bake/bun-framework-react/server.tsx
prerender creates a PassThrough and MiniAbortSignal, forwards render errors into abort and stream destruction, and awaits renderToStaticHtml without swallowing failures.
Route generation await fix
src/js/builtins/Bake.ts
The sync-iterator-less static route branch now returns the route generator promise values directly so Promise.all waits for every generated page.
ASAN test timeouts
test/bake/dev/production.test.ts
isASAN is imported and used to switch the sourcemaps error and SSG failure test timeouts to 60_000 or 30_000, with a comment referencing Bun issue 33214.

Sequence Diagram(s)

sequenceDiagram
  participant prerender
  participant renderToPipeableStream
  participant rscPayload
  participant renderToStaticHtml
  participant StaticRscInjectionStream
  prerender->>renderToPipeableStream: render(page, serverManifest, onError, filterStackFrame)
  renderToPipeableStream->>prerender: onError(err)
  prerender->>rscPayload: destroy(err)
  prerender->>renderToStaticHtml: await renderToStaticHtml(rscPayload, modules, signal)
  renderToStaticHtml->>StaticRscInjectionStream: construct with signal
  StaticRscInjectionStream->>StaticRscInjectionStream: reject(signal.aborted ?? err)
  renderToStaticHtml-->>prerender: reject/propagate error
Loading

Compact metadata

  • Estimated code review effort: High
  • Lines changed: +68/−36 across 4 files

Related issues: Bun issue 33214 is referenced in the test comments.

Related PRs: None identified.

Suggested labels: bake, react, bug-fix, tests

Suggested reviewers: Reviewers familiar with the bun-framework-react runtime, static rendering, and bake test harness.

Poem
Streams now fail in ways that show,
Not tucked away, nor soft and low.
Routes await each promised page,
And tests adapt for ASAN stage.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: failing the build when static prerender throws.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description covers the issue, fix, and verification clearly, even though it uses Problem/Cause/Fix instead of the template headings.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/runtime/bake/bun-framework-react/server.tsx`:
- Around line 185-188: The static RSC payload header in server.tsx is writing
the number of styles instead of the serialized stylesheet string length. Update
the logic around the RSC payload assembly to use the length of
meta.styles.join("\n") for the Uint32Array header, matching the live RSC path so
index.rsc advertises the correct CSS metadata length. Keep the fix localized to
the rscChunks/header construction near the rscPayload.on("data") flow.
- Around line 172-179: The RSC metadata written by prerender() is using
meta.styles.length, but client.tsx expects that header to be a byte count before
reading the CSS payload, so static navigations can get out of sync. Update the
header generation in the prerender/index.rsc path to write the same byte length
used by the live response, or centralize the encoder so both paths share the
exact format; use the existing server.tsx renderToPipeableStream flow as the
reference point. Also, if signal.abort(err) remains in the onError handler,
widen MiniAbortSignal.abort to accept the error reason and remove the current
suppression so the types and call site match.
🪄 Autofix (Beta)

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: 7ed218be-ccab-476e-8831-948ea14a1afc

📥 Commits

Reviewing files that changed from the base of the PR and between 5b55beb and 9a8f2db.

📒 Files selected for processing (3)
  • src/runtime/bake/bun-framework-react/server.tsx
  • src/runtime/bake/bun-framework-react/ssr.tsx
  • test/bake/dev/production.test.ts

Comment thread src/runtime/bake/bun-framework-react/server.tsx
Comment thread src/runtime/bake/bun-framework-react/server.tsx
robobun added 2 commits July 1, 2026 23:53
The real abort function (from renderToPipeableStream) takes a reason, and
three call sites already pass one. Widen the type to match and drop the
@ts-expect-error suppressions.
Comment thread src/runtime/bake/bun-framework-react/server.tsx
Comment thread src/runtime/bake/bun-framework-react/server.tsx
robobun and others added 2 commits July 2, 2026 00:24
The Promise.all over paramGetter.pages used a block-body arrow that dropped
the callRouteGenerator promise, so a render error on a parameterized route
became an unhandled rejection and the build still exited 0. Return the
promise so Promise.all awaits the render (and the per-page writes).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/runtime/bake/bun-framework-react/server.tsx (1)

102-109: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Normalize thrown render values before storing them in signal.aborted. renderToHtml and renderToStaticHtml treat that field as a truthy abort sentinel, so a thrown undefined, 0, or "" can leave the render looking live and let the static build hang. Convert to an Error (or split the sentinel from the reason) in both onError handlers.

🤖 Prompt for 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.

In `@src/runtime/bake/bun-framework-react/server.tsx` around lines 102 - 109,
Normalize the abort state in both `renderToHtml` and `renderToStaticHtml`
`onError` handlers so `signal.aborted` is always a truthy sentinel and not the
raw thrown value. Update the `signal.aborted` assignment in `server.tsx` to
store an `Error` (or separate the boolean sentinel from the error reason) before
calling `signal.abort(err)` and `rscPayload.destroy(err)`, so thrown values like
`undefined`, `0`, or "" still mark the render as aborted.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@src/runtime/bake/bun-framework-react/server.tsx`:
- Around line 102-109: Normalize the abort state in both `renderToHtml` and
`renderToStaticHtml` `onError` handlers so `signal.aborted` is always a truthy
sentinel and not the raw thrown value. Update the `signal.aborted` assignment in
`server.tsx` to store an `Error` (or separate the boolean sentinel from the
error reason) before calling `signal.abort(err)` and `rscPayload.destroy(err)`,
so thrown values like `undefined`, `0`, or "" still mark the render as aborted.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 761eb02b-f16b-4b8a-881e-d2b95f6ae678

📥 Commits

Reviewing files that changed from the base of the PR and between 9a8f2db and c0b474c.

📒 Files selected for processing (3)
  • src/js/builtins/Bake.ts
  • src/runtime/bake/bun-framework-react/server.tsx
  • test/bake/dev/production.test.ts

@robobun

robobun commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Re: normalizing the signal.aborted sentinel for falsy thrown values.

I checked this empirically on the static path. A component throwing a falsy value still fails the build (no hang) in every case:

thrown result
throw undefined exit 1
throw null exit 1
throw 0 exit 1
throw "" exit 1

The static path has an independent, truthy error source: when the flight render aborts, createFromNodeStream rejects, React.use throws, and react-dom's onError fires with a real Error that rejects stream.result regardless of the falsy signal.aborted. So a falsy sentinel does not strand the render.

The signal.aborted-as-sentinel pattern is pre-existing and shared with the streaming render / renderToHtml path (server.tsx:102-109, which this PR does not modify); the prerender path mirrors it for consistency. Splitting the sentinel from the reason across both paths is a reasonable hardening, but since it does not affect the hang this PR fixes, I am leaving it out of scope here.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the static prerender error path now mirrors the existing streaming path, and the Bake.ts dropped-promise fix + regression test address my earlier feedback.

Extended reasoning...

Overview

This PR fixes a fleet-wide CI hang where bun build --app spins forever when a component throws during static prerender. It touches four files: server.tsx and ssr.tsx in bun-framework-react (wire onError/abort into the static prerender path, mirroring the existing streaming render()/renderToHtml() pattern in the same files), src/js/builtins/Bake.ts (one-token fix: expression-body arrow so Promise.all actually awaits getStaticPaths-generated renders), and test/bake/dev/production.test.ts (ASAN-aware timeouts + a new regression test for the parameterized-route case).

Security risks

None. This is error-propagation plumbing in the bake framework's React integration — no auth, crypto, permissions, or untrusted-input parsing is involved. The only behavioral change is that a render error now rejects the build promise instead of being swallowed (try/catch → return undefined removed), which is strictly safer: builds fail loudly instead of hanging or exiting 0.

Level of scrutiny

Moderate. The change involves stream lifecycle and abort-signal wiring, but it is a near-verbatim mirror of the working render() path directly above it in the same file — same MiniAbortSignal shape, same onError structure, same rscPayload.destroy(err) teardown. The renderToStaticHtml signature change has exactly one caller (verified via grep), which is updated. The abort?.() optional-chaining in ssr.tsx defensively handles the (theoretical) sync-onError case, and stream.destroy() rejects the result regardless. The Bake.ts change is trivially correct.

Other factors

I previously left two inline comments on this PR. The first (dropped promise for getStaticPaths routes in Bake.ts) was fixed in 2d05f99 with a dedicated regression test. The second (pre-existing meta.styles.length header bug) is unchanged by this diff and now tracked in #33218. All CodeRabbit threads are resolved (the MiniAbortSignal.abort type was widened in 03a5d18). The bug-hunting system found no issues. No CODEOWNERS cover these paths. The PR description documents fail-before verification (USE_SYSTEM_BUN hangs / exits 0; debug build exits 1) and 9/9 bake production tests passing.

@robobun

robobun commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Retriggered CI. A breakdown of the previous run (build 67792) for whoever merges this, since the red it shows is not caused by this diff.

278 of 286 jobs passed. The 8 that did not:

  • 2x darwin 26 aarch64 - test-bun: infrastructure. Both died on buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun' before running any tests.
  • 4x darwin 14 (aarch64/x64) - test-bun: never ran, stuck in scheduled with no agent.
  • 2x alpine 3.23 (x64 / x64-baseline) - test-bun: test/js/node/test/parallel/test-net-connect-memleak.js fails a GC collection assertion. This is pre-existing and unrelated to bake: the same annotation appears on 9 of the last 19 builds (67767, 67768, 67771, 67780, 67783, 67787, 67794, 67796, 67798), none of which touch this code.

Evidence the fix works beyond the local bun bd test run: test/bake/dev/production.test.ts shows up as a failure annotation on every recent build from 67767 through 67801. Build 67792 is the only one in that window where it does not, and it is the only one carrying this diff.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the Bake.ts dropped-promise gap and adding the parameterized-route regression test — the current diff looks correct to me, but the multi-stream error/abort wiring across the flight and react-dom renderers is subtle enough that someone familiar with bake should give it a final look.

Extended reasoning...

Overview

This PR fixes a fleet-wide CI hang: bun build --app spins forever when a component throws during static prerender. The fix wires error handling into the static path (prerender in server.tsx + renderToStaticHtml / StaticRscInjectionStream in ssr.tsx) to mirror the existing streaming render / renderToHtml path, so a render error aborts both the flight and HTML renderers and rejects the awaited promise instead of leaving it pending. It also fixes a one-token bug in src/js/builtins/Bake.ts where a block-body arrow dropped the callRouteGenerator promise for getStaticPaths routes, and widens MiniAbortSignal.abort to accept a reason (removing two @ts-expect-error suppressions). Tests: the existing hanging test gets an ASAN-aware timeout, and a new test covers the parameterized-route case.

Security risks

None identified. This is build-time error propagation in the React framework adapter; no auth, crypto, permissions, or untrusted-input parsing is touched.

Level of scrutiny

Moderate. The change is well-motivated (fixes a real CI-red hang), closely mirrors the already-working streaming sibling, and has regression tests that were verified fail-before/pass-after. However, it is not a mechanical change: it removes a try/catch that previously swallowed prerender errors, adds a required signal parameter to renderToStaticHtml, and threads error state through three interacting async surfaces (flight onError → rscPayload.destroy → StaticRscInjectionStream rejection, plus react-dom onError → abort() → stream.destroy). The double-reject paths are safe (Promise.withResolvers makes later settles no-ops), and the falsy-thrown-value edge case was empirically checked by the author, but this is the kind of control-flow change where a maintainer who knows bake's render lifecycle should confirm nothing else relies on the old swallow-and-return-undefined behavior.

Other factors

All prior review threads are resolved: CodeRabbit's type-widening request was applied in 03a5d18; my earlier note about the dropped promise in Bake.ts was fixed in 2d05f99 with a regression test; the pre-existing meta.styles.length header bug is intentionally out of scope and tracked in #33218. The bug-hunting pass on the current revision found nothing. renderToStaticHtml has exactly one caller and it was updated. No CODEOWNERS cover these paths. Given this is a user-facing framework build path rather than a config/typo change, I'm deferring rather than auto-approving.

@robobun

robobun commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the red lanes are unrelated to this diff and pre-existing.

This PR only touches src/js/builtins/Bake.ts, src/runtime/bake/bun-framework-react/{server,ssr}.tsx, and the bake test. The failing jobs are:

  • darwin-aarch64 test-bun (builds 67792 and 67814): Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'. The tests never ran; this is an artifact-download/infra timeout that recurred across both builds (including the re-roll).
  • alpine-x64 test-bun (build 67792): node *-connect-memleak.js GC assertions (assert.strictEqual(collected, true) → false !== true). These are pre-existing fleet-wide reds, noted in bake: "works with sourcemaps - error thrown in React component" times out on every CI build #33214 as a separate issue; production.test.ts does not run on that musl lane.

Neither can be caused by changes to the bake React SSR error path. The bake production suite passes locally (9/9 on a debug+ASAN build), and the fix is verified fail-before/after (the unfixed build hangs or exits 0; the fixed build exits 1).

The one CI re-roll (c928d96) hit the same darwin artifact-download timeout, so the infra issue persists independently of this branch. The diff is ready; the remaining red is infra + unrelated flake and needs a maintainer to merge through it.

@robobun

robobun commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator Author

Stale PR review: keep open, rework.

The fix is still wanted. On main, the { pages } arm of renderRoutesForProdStatic maps with a block body and returns nothing (src/js/builtins/Bake.ts:156), so nothing observes a route that throws during prerender. A maintainer's review on #39488 describes the effect: the build prints the error, prints "done", and exits 0 without that page. The maintainers folded this PR into #39488, to close when #39488 lands. #39488 is not merged and conflicts with main, so a small standalone fix is still wanted.

The current diff is not the shape to land:

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant