Repository navigation
bake: report a server module that JSC rejects as a build error instead of aborting the dev server - #46643
bake: report a server module that JSC rejects as a build error instead of aborting the dev server#46643robobun wants to merge 8 commits into
Conversation
…d of aborting the dev server
|
Status. How I reproduced it:
The 19 new tests in |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Syntax error detection src/ast/lib.rs, src/runtime/bake/BakeSourceProvider.cpp, src/runtime/bake/dev_server/server_hmr_ffi.rs |
The FFI now parses server source without executing it and returns syntax-error details. The source-position conversion returns byte offset zero when the requested position matches the starting position. |
Chunk isolation and recovery src/runtime/bake/dev_server/incremental_graph.rs, src/runtime/bake/dev_server/finalize.rs |
The loader evaluates the combined chunk, identifies rejected modules, removes their code and source-map entries, and reevaluates remaining modules. It maps available syntax-error locations to original sources. |
Failure indexing and regression coverage src/runtime/bake/dev_server/finalize.rs, test/bake/dev/bundle.test.ts |
The server update callback runs only when chunk loading returns modules. Failure changes are indexed after loading. Tests cover error reporting, updates to other routes, recovery, and client-component manifest changes. |
Suggested reviewers: alii, dylan-conway
Priority:
Severity of issue fixed: Medium
Merge Risk: 
fde96
-
The server can recover from a rejected chunk, but subsequent errors in previously loaded code may show generated locations. This is a bounded diagnostics issue rather than a blocker to recovery.
Pre-merge checks: 
-
Check name Status Explanation Title check The title is concise, specific, and accurately describes the main change: reporting JSC-rejected server modules as build errors instead of aborting the dev server. Linked Issues check Issue #15530requires the dev server to stop aborting when JSC rejects generated server code, such as an invalid regular expression, and to identify the source module.load_server_chunkevaluates th…Out of Scope Changes check The changed code supports issue #15530. Source-index tracking and source-map lookup support file and position attribution. JSC syntax-error FFI supports non-executing module checks. Incremental-graph …Description check The description is complete and relevant. It explains the problem, cause, fix, design decisions, limitations, test coverage, verification results, and performance measurements. It does not use the tem… Full details: Linked Issues check
-
Explanation
Issue
#15530requires the dev server to stop aborting when JSC rejects generated server code, such as an invalid regular expression, and to identify the source module.load_server_chunkevaluates the combined chunk, checks each module withbake_find_server_code_syntax_errorafter rejection, records a file-specific build error, removes the rejected module, and reloads the remaining modules. The tests cover JSC parser mismatches, source positions, recovery, multiple modules, and hot updates.
Full details: Out of Scope Changes check
-
Explanation
The changed code supports issue
#15530. Source-index tracking and source-map lookup support file and position attribution. JSC syntax-error FFI supports non-executing module checks. Incremental-graph updates, failure indexing, and client-component handling support recovery and runtime consistency. Tests cover the added behavior. No unrelated change is demonstrated.
Full details: Description check
-
Explanation
The description is complete and relevant. It explains the problem, cause, fix, design decisions, limitations, test coverage, verification results, and performance measurements. It does not use the template headings exactly, but it provides the required information in equivalent sections.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
A second branch for this issue exists: The branch has the same per-module
Tests: 8 new tests in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 note, I also checked: the source-map JSON handed to BakeLoadServerHmrPatchWithSourceMap is owned by the DevServerSourceProvider whether or not executeProgram throws, so the first failed evaluation does not leak it; the +1 ref message out-param from BakeFindServerCodeSyntaxError is released by bun_core::String's Drop; take_js_bundle_server/take_source_map do not consume current_chunk_code/current_chunk_source_maps, so the re-evaluation after remove_current_chunk_part sees exactly the remaining parts, and current_chunk_len is decremented by the same per-part length receive_chunk added; JSC::checkSyntax returns through ParserError without touching the VM exception slot.
Extended reasoning...
The change replaces the dev server's abort-on-JSC-rejection with per-module checkSyntax triage in src/runtime/bake/dev_server/finalize.rs, adds a new C++ FFI entry in src/runtime/bake/BakeSourceProvider.cpp, deletes the dead BakeLoadServerHmrPatch, and adds ten devTest cases; it touches no auth, crypto, or injection surface. A human look is still worthwhile because it is a ~500-line native change across the Rust/C++ boundary with ownership transfers, an index-failures reordering, and process-abort paths, and because the one inline finding concerns untested separateSSRGraph coverage of the new remove_current_chunk_part guard.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the updated PR and didn't find any bugs; the separateSSRGraph test added in af1d25e covers the two-parts-per-file case from my earlier inline comment. Because this rewires the server-chunk load path (a new JSC FFI entry, abort paths, source-map position mapping) and changes Source::line_col_to_byte_offset for every caller, a human look would still be worthwhile.
What was reviewed:
- Ownership across the new FFI: the
ManuallyDropsource-map JSON inevaluate_server_chunkreaches C++ on every call including the retry, and the +1 message ref fromBakeFindServerCodeSyntaxErroris released bybun_core::String's Drop whenServerCodeSyntaxErrorgoes out of scope. BakeLoadServerHmrPatchdeletion: no declaration or caller remains undersrc/; theclone_latin1chunk path it served is gone with it.- The
index_failures()move and theclient_components_addedretraction inremove_current_chunk_part: traced against the empty-chunk and SSR-graph tests, which exercise both the removed-component and proxy-kept outcomes.
Extended reasoning...
The change touches the Bake dev server's server-chunk load (src/runtime/bake/dev_server/finalize.rs, incremental_graph.rs, server_hmr_ffi.rs), replaces a C++ executor with a JSC checkSyntax entry in BakeSourceProvider.cpp, adds an early return to Source::line_col_to_byte_offset in src/ast/lib.rs, and adds ten dev-server tests. It touches no auth, crypto, or injection surface; the untrusted input is the user's own source text fed to JSC's parser. Since the prior review the only change is the test commit addressing my inline nit, and the bug hunt ran dry with no findings. Approval is withheld because the diff is about 530 lines of non-mechanical FFI, abort and source-map logic plus a shared-helper semantics change, which is beyond what an automated review should sign off alone; CODEOWNERS covers only *.d.ts, so no owner-gated path is involved.
Each comment is now one line.
…loads A chunk that JSC rejects no longer takes the place of the source map of the last chunk that loaded, so the stack traces of the code that runs stay mapped.
…server-chunk-syntax-error
|
Changes since the last review (557ebb1 to 06c4f4f):
On a debug build of 06c4f4f, |
… the source of rejected code from its chunk part - A server chunk that throws while JSC accepts its syntax fails each of its files. The abort stays for a chunk that JSC parses module by module and not as a whole. - Each part of a server chunk carries the bundler's index of its source. A search by file gave the wrong source when a plugin gives the client and the server different text. - The position of an invalid regular expression is the last mapping before the token where JSC stopped, which JSC reads before it rejects the expression. - The note says that JSC rejected the code that Bun generated. - Every parser error type is a rejection. - Tests: inputs with no regular expression, a valid file, a parser stack overflow, and a plugin that gives each side its own text.
…server-chunk-syntax-error
|
Changes after a second self-review (06c4f4f to fde964f):
The 19 new tests fail on the release build 1.4.3-canary.1 and pass on a debug build of the branch. Build 131860 had two failed jobs, and neither ran code of this change. |
| while !dev.server_graph.current_chunk_code.is_empty() { | ||
| let module = dev.server_graph.remove_current_chunk_part(0); | ||
| let mut log = Log::init(); | ||
| log.add_error_fmt( | ||
| Some(&sources[module.source_index.get() as usize]), | ||
| bun_ast::Loc::EMPTY, | ||
| format_args!( | ||
| "The server code of this bundle did not load: {}", | ||
| bstr::BStr::new(&message) | ||
| ), | ||
| ); | ||
| fail_server_file(dev, module.file_index, &log)?; | ||
| } |
There was a problem hiding this comment.
🟡 (optional) Under BUN_ASSUME_PERFECT_INCREMENTAL=1 (the debug-build default), a server chunk that throws at load leaves every one of its files failed until each is re-saved, node_modules included. fail_server_chunk at finalize.rs:297-309 records a file-level failure for every part when the chunk throws for a reason no file explains. With perfect-incremental on, a request to a failed route stops at ensure_route.rs:246-251 instead of rebuilding, and only a watcher edit of that exact file clears its failure. Fix: on a chunk-level runtime failure, mark the chunk's files stale so the next request rebuilds and re-evaluates, rather than inserting per-file failures that only an edit of each file removes.
Why this was flagged
A dev server runs with BUN_ASSUME_PERFECT_INCREMENTAL=1, which lifecycle.rs:104-108 makes the default when IS_DEBUG. A bundle's server chunk parses but executeProgram throws (the PR's own test trigger is a Proxy in the global prototype chain). load_server_chunk finds no part that fails alone, so fail_server_chunk (finalize.rs:297-309) calls fail_server_file for every part, inserting a failure keyed by that file into bundling_failures and setting its stale bit. On the next request, ensure_route.rs:246-251 calls check_route_failures; with assume_perfect_incremental_bundling true it returns Stop (ensure_route.rs:448-449), so the chunk is never re-evaluated even after the condition that threw is gone. The only thing that clears a file's failure is receive_chunk of that file (incremental_graph.rs:765-776). On the base branch the process aborted; with =0 (release default) each request rebuilds and recovers, so the trap exists only in the =1 state, which the new test for this path (bundle.test.ts:2148) does not exercise.
Verification: fail_server_chunk (src/runtime/bake/dev_server/finalize.rs:297-309) calls fail_server_file for every part, and insert_failure sets stale_files, failed = true and records a per-file entry in bundling_failures. On the next request, ensure_route.rs:246-251 calls check_route_failures, which under assume_perfect_incremental_bundling returns Stop (ensure_route.rs:448-449).
There was a problem hiding this comment.
This is right, and this push does not change it.
With BUN_ASSUME_PERFECT_INCREMENTAL=1, check_route_failures answers a route that has a failed file with the failures and returns Stop (ensure_route.rs:431-449). The files of such a chunk then stay failed until each one is saved. With 0, the release default, the next request builds the route again, and the test for this path runs in that mode. Before 878019b this input aborted the process in both modes.
The failure does not come from the text of a file, so it does not fit a failure that belongs to a file. The fix needs a failure that a request can clear, one that belongs to a route or to a bundle. serialized_failure::Owner::Route has no producer, and index_failures treats it as unreachable (failures.rs:237). I did not add that state to this PR. The description now lists the limit under "Not in this change".
| if server_hmr_ffi::bake_find_server_code_syntax_error(global, BunString::clone_utf8(&chunk)) | ||
| .is_some() | ||
| { | ||
| // JSC parses each module alone, so the text that Bun puts around the modules is wrong. | ||
| abort_on_server_chunk_error(dev, exception); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: a server file nested a few levels short of JSC's parser stack limit still aborts the dev server, while one level deeper gets a clean build error. Each part is checked as 0,{…} (finalize.rs:218) but the chunk runs as ({…}), about 7 more parser frames, and executeProgram parses deeper still. In that band no part is rejected, so the whole-chunk check at finalize.rs:283-287 sees a StackOverflow and calls abort_on_server_chunk_error. Fix: make the per-part check use at least as much stack as the chunk evaluation (extra wrapper nesting, e.g. 0,[[[{…}]]]) and never abort on a StackOverflow-typed chunk rejection, which also stops the shallower band from failing every file with "The server code of this bundle did not load".
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A developer saves a server file whose block or expression nesting is just below the depth at which JSC's parser overflows with the dev server's BUN_JSC_maxPerThreadStackUsage. JSC's parser checks the absolute stack pointer at every recursive parse call (VM::isSafeToRecurse), so the same text overflows at one call depth and parses at a shallower one. The chunk is evaluated by executeProgram, then parsed as ({ ... }); each part alone is parsed by checkSyntax as 0,{ ... } (incremental_graph.rs:1799-1810), which reaches the module body with fewer frames. For depths where the chunk overflows and the parts do not, the loop at finalize.rs:217-254 rejects nothing, rejected_any stays false, and fail_server_chunk at finalize.rs:283 re-parses the whole chunk, gets a StackOverflow error from BakeFindServerCodeSyntaxError (BakeSourceProvider.cpp:91-99), and aborts the process at finalize.rs:287, as the base branch does for every depth past the limit.
Verification: pre-existing. Trigger: a server file whose nesting depth falls in the band where JSC's parser overflows when the whole chunk is parsed as ({…}) but not when the single part is parsed alone as 0,{…}. The per-part loop at finalize.rs:217-225 finds no part rejected, so fail_server_chunk at :278-287 re-parses the whole chunk, overflows again, and abort_on_server_chunk_error panics. The base branch already aborts here.
There was a problem hiding this comment.
Correct. The chunk adds parser frames to each module (({ against 0,{, and the frames of executeProgram). In a band of a few nesting levels JSC rejects the chunk and accepts each module alone, and fail_server_chunk then takes the abort. Main aborts there too, as it does for every depth past the limit. The test for the parser's stack uses twice the limit, so it does not reach the band.
A wrapper for the per-module parse that nests deeper than the chunk (0,[[[[{) removes the abort in that band. It can then reject a module that the chunk loads, but only when the chunk already failed for another reason. I have no deterministic test for a depth inside the band, and CI runs on the current head, so I did not push that change alone. The description lists the band under "Not in this change". The change goes in with the next push to this branch.
Fixes #15530
Problem
panic: Error thrown while evaluating server code. This is always a bug in the bundler.An invalid regular expression (/+/) in a route does it, and so does any other early error that Bun's parser does not check.finalize_bundlepanics on any error from the chunk load (src/runtime/bake/dev_server/finalize.rs:729).Fix
load_server_chunkparses each module of a rejected chunk alone with JSC. A rejected module is a build error at its source position, and the rest load.(a = /+/) => ait names the wrapper.Proxyin the global prototype chain) is a build error of each of its files.test/bake/dev/bundle.test.tsfail on main.test/bakepasses. Self-reviewed twice: 19 concerns raised, 18 addressed (Notes).Background
Downsides
export * as await from "node:path"with top-level await, until bundler: reserveawait,evalandargumentsin esm output and for bundler-made bindings #34361..textgrew by 3,072 bytes at 557ebb1.Notes
Repro. A framework route (
Bun.serve({ app })orbun bake) that imports a file with/+/.source. On main the first request printsSyntaxError: Invalid regular expression: nothing to repeatand the panic above, exit 134. A save of the same text into a loaded module does the same.What the error looks like now (terminal output. The overlay shows the same message, position and code line):
JSC never parses the file. It parses the code that Bun printed for it, so the note says that. For a valid file the note is the signal that main gave with its panic text.
Which inputs. JSC has early errors that Bun's parser does not check. These abort the release build 1.4.3-canary.1 and are build errors here, each with a test: an invalid regular expression,
new.targetoutside a function,usingdirectly in acaseclause, two constructors in a class, a parser stack overflow, and one valid file (a binding that Bun namesawaitin a module with top-level await, the renamer gap of #34361). A check of regular expressions in Bun's lexer would cover only the first.How a module is judged. The chunk
({ <modules> })is evaluated once, as on main. Only if that throws, each module is parsed alone as0,{ <module> }withJSC::checkSyntax(no leading(, nothing is run, nothing is registered). JSC reports the token where it stopped. The position is the last source mapping at or before that token. JSC reads the token after a regular expression before it rejects the expression, so for that message the position is the last mapping before the token. That is the expression, also when the next token is on another line.The source of a module. Each part of the chunk now carries the bundler's index of its source. A plugin can give the client and the server different text for one file (
bun-plugin-sveltedoes). The file then has more than one source, and a search by file gave the wrong one: the error had no position.A chunk error that no module explains. JSC does not run a program while a
Proxyis in the prototype chain of the global object. User code can set one. Each file of that chunk now fails withThe server code of this bundle did not load: Proxy is not allowed in the global prototype chain., and the files load after the prototype is restored. The abort stays for one case: JSC's parser accepts each module alone and rejects the chunk. Then the text that Bun puts around the modules is wrong, which no file can take.Client component lists.
receive_chunklists a"use client"file for the runtime before JSC sees its code. A rejected"use client"file now leavesclient_components_added, so the runtime does not try to load it. If no module is left and a file stopped being a client component in that save,registerUpdatestill runs with an empty object, so the manifests lose the entry.index_failuresmoved below the chunk load, so a failure recorded there reaches the sockets and the waiting requests in the same round. Nothing between the two places reads what it writes.Deleted.
BakeLoadServerHmrPatchand its Rust wrapper had one caller: the arm for a chunk with code and no source map entry. Every part of a chunk has an entry, so the arm was unreachable.Source::line_col_to_byte_offsetreturnedNonefor the start position. It now returns 0. The other caller (snapshot.rs) handles the equal case before the call.Tests. Three of the new tests were checked against a build with one change undone each. With the lookup for regular expressions undone, the expression at the end of a line is reported at
lib.ts:5:1and notlib.ts:4:3. With the search by file back, the plugin case printsComp.ts: error:with no position. With the abort back, theProxycase panics.Measurements (main 5c6d4e2 and this branch at 557ebb1, linux-x64. The later commits add about 90 lines of Rust and were not measured again):
bundle.test.ts(64 tests): every accepted chunk is evaluated once and no module is parsed alone (69 chunks). A rejected chunk of n modules: 2 evaluations and n module parses, for any number of rejected modules. 1 evaluation if no module is left.with,delete name,var arguments, and so on): 44 answer 200 on main and 44 answer 200 here, with the same body. This change does not touch code that works..text82,231,705 to 82,234,777 (+3,072),.rodata+0,.data+0.strace,perfandvalgrindare not installed in the build container. The accepted path adds no call.test/bake, debug build, with--timeout 120000(a React production build takes 5 to 11 s under ASAN on this machine, soproduction.test.tsand tworeact-spa.test.tstests time out at the default 5 s with and without the change): 289 tests in 26 files pass at 878019b. Release build at 557ebb1: 288 pass, 0 fail.incremental-graph-edge-deletion.test.tsfailed once in four runs on this branch and once in two on main.BUN_ASSUME_PERFECT_INCREMENTAL0 and 1. With 0 (the release default) each request for a failed route bundles the route again, so the error prints again.Not in this change.
/+/in client code the page is the static HTML, with no overlay and no error on the terminal. On a hot update the overlay and the terminal show the browser'sSyntaxErrorwith no file or line.bake://server.patch.js, so a chunk that registers replaces the source map of the chunks before it, also a rejected one. After a second chunk loads, the frames of a module from the first chunk print asbake://server.patch.js:45:22(release build 1.4.3-canary.1 and this branch). bake: give every server HMR patch its own source URL so older patches keep remapping #37440 gives each chunk its own URL."use client"file that re-exports a rejected file withexport *. After the fix of the rejected file the runtime still printsFailed to load bundled moduleuntil the"use client"file is saved. A parse error of Bun in the same place does the same on main.BUN_ASSUME_PERFECT_INCREMENTAL=1, the default of debug builds, with a chunk that JSC did not run. In that mode a request does not build a failed route again (ensure_route.rs:431), so the files of that chunk stay failed until each is saved. With 0, the release default, the next request builds the route again. The failure does not come from the text of a file, and the dev server has no failure that belongs to a route or a bundle.ParserError(ZigSourceProvider.cpp:250).Self-review. The first review raised 10 concerns. Addressed: blame by the position of the chunk-level error (each module is now parsed alone), line counting that differed from JSC's (no line lookup is left), the client component lists, a parser stack overflow in a module, tests for both incremental modes, a note on the failure, a test with a connected client, a test for a later chunk with accepted and rejected modules, and packaging (strict mode for server modules and a renamer change are not part of this PR). Not taken then: the abort for a chunk error that no module explains.
The second review, of the PR as it stood at 06c4f4f, raised 9. Addressed: a commit that moved the registration of a chunk's source map is reverted (#37440 removes the cause), the source of a part is carried and not searched, the note no longer says that JSC rejected the file, rows without a regular expression and the valid file are tested, the parser stack overflow is tested, an unknown parser error type is a rejection, and the body names the inputs, the late position and the client behaviour as measured. The abort that the first review left is now a build error when the error is not from the parser. Not done: no issue is filed for client chunks. They are described under "Not in this change".