Repository navigation
bake: report a server module that JSC rejects as a build error instead of aborting the dev server #46643
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,6 +149,211 @@ pub(super) fn drain_current_bundle_requests(current_bundle: &mut CurrentBundle) | |
| } | ||
| } | ||
|
|
||
| /// Runs the current server chunk. The inner `Err` is what JSC threw. | ||
| fn evaluate_server_chunk(dev: &mut DevServer) -> JsResult<JsResult<JSValue>> { | ||
| let source_map_json = { | ||
| // Create a temporary source map entry to render | ||
| let mut source_map_entry = source_map_store::Entry { | ||
| ref_count: 1, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| // Fill the source map entry | ||
| dev.server_graph.take_source_map(&mut source_map_entry)?; | ||
| // Note: erase to raw ptr so `render_json` can borrow the entry while the cleanup guard is armed. | ||
| let entry_ptr: *mut source_map_store::Entry = &raw mut source_map_entry; | ||
| scopeguard::defer! { | ||
| // SAFETY: `source_map_entry` is a stack local that outlives this guard. | ||
| unsafe { | ||
| (*entry_ptr).ref_count = 0; | ||
| (*entry_ptr).deinit(); | ||
| } | ||
| }; | ||
|
|
||
| source_map_entry.render_json(dev, ChunkKind::HmrChunk, bake::Side::Server)? | ||
| }; | ||
|
|
||
| let server_bundle = | ||
| dev.server_graph | ||
| .take_js_bundle_server(&incremental_graph::TakeJSBundleOptionsServer { | ||
| kind: ChunkKind::HmrChunk, | ||
| })?; | ||
|
|
||
| // This memory will be owned by the `DevServerSourceProvider` in C++ | ||
| let source_map_json = ::core::mem::ManuallyDrop::new(source_map_json); | ||
|
|
||
| Ok(server_hmr_ffi::bake_load_server_hmr_patch_with_source_map( | ||
| dev.global(), | ||
| BunString::clone_utf8(&server_bundle), | ||
| source_map_json.as_ptr(), | ||
| source_map_json.len(), | ||
| )) | ||
| } | ||
|
|
||
| fn abort_on_server_chunk_error(dev: &mut DevServer, exception: JSValue) -> ! { | ||
| // SAFETY: vm is JSC_BORROW — valid for DevServer lifetime; | ||
| // `print_error_like_object_to_console` needs `&mut`. | ||
| dev.vm_mut().print_error_like_object_to_console(exception); | ||
| // Note: `panic!()` would unwind through the `extern "C"` boundary above (`nounwind` UB), so abort. | ||
| bun_core::Output::panic(format_args!( | ||
| "Error thrown while evaluating server code. This is always a bug in the bundler." | ||
| )); | ||
| } | ||
|
|
||
| /// Loads the current server chunk. A module that JSC rejects becomes the build error of its file, and the rest load. | ||
| fn load_server_chunk( | ||
| dev: &mut DevServer, | ||
| sources: &[bun_ast::Source], | ||
| ) -> JsResult<Option<JSValue>> { | ||
| let global = dev.global(); | ||
| let mut exception = match evaluate_server_chunk(dev)? { | ||
| Ok(modules) => return Ok(Some(modules)), | ||
| Err(err) => global.take_exception(err), | ||
| }; | ||
|
|
||
| // JSC's error for the chunk can name a place in another module, so each module is parsed alone. | ||
| let mut rejected_any = false; | ||
| let mut part = 0; | ||
| while part < dev.server_graph.current_chunk_code.len() { | ||
| let alone = dev.server_graph.current_chunk_part_alone(part); | ||
| let Some(syntax_error) = server_hmr_ffi::bake_find_server_code_syntax_error( | ||
| global, | ||
| BunString::clone_utf8(&alone), | ||
| ) else { | ||
| part += 1; | ||
| continue; | ||
| }; | ||
| rejected_any = true; | ||
|
|
||
| let module = dev.server_graph.remove_current_chunk_part(part); | ||
| let source = &sources[module.source_index.get() as usize]; | ||
| let message = syntax_error.message.to_utf8(); | ||
| // JSC reads the token after a regular expression before it rejects the expression. | ||
| let token_is_rejected = | ||
| !strings::has_prefix_comptime(message.slice(), b"Invalid regular expression"); | ||
| let loc = original_loc( | ||
| source, | ||
| &module.source_map, | ||
| (syntax_error.line, syntax_error.column), | ||
| token_is_rejected, | ||
| ); | ||
| let mut log = Log::init(); | ||
| log.add_range_error_fmt_with_notes( | ||
| Some(source), | ||
| bun_ast::Range { | ||
| loc: loc.unwrap_or(bun_ast::Loc::EMPTY), | ||
| len: 0, | ||
| }, | ||
| Box::new([bun_ast::range_data( | ||
| None, | ||
| bun_ast::Range::NONE, | ||
| "JavaScriptCore rejected the code that Bun generated for this file. If the file is valid, this is a bug in Bun's bundler.", | ||
| )]), | ||
| format_args!("{}", bstr::BStr::new(message.slice())), | ||
| ); | ||
| fail_server_file(dev, module.file_index, &log)?; | ||
| } | ||
|
|
||
| if dev.server_graph.current_chunk_len > 0 { | ||
| if rejected_any { | ||
| exception = match evaluate_server_chunk(dev)? { | ||
| Ok(modules) => return Ok(Some(modules)), | ||
| Err(err) => global.take_exception(err), | ||
| }; | ||
| } | ||
| fail_server_chunk(dev, sources, exception)?; | ||
| } | ||
| // No module is left. The runtime must still hear of a client component that this round removed. | ||
| let removed = !dev.incremental_result.client_components_removed.is_empty(); | ||
| Ok(removed.then(|| JSValue::create_empty_object(global, 0))) | ||
| } | ||
|
|
||
| /// Makes what the load of the current server chunk threw the build error of each file that the chunk holds. | ||
| fn fail_server_chunk( | ||
| dev: &mut DevServer, | ||
| sources: &[bun_ast::Source], | ||
| exception: JSValue, | ||
| ) -> JsResult<()> { | ||
| let global = dev.global(); | ||
| let chunk = | ||
| dev.server_graph | ||
| .take_js_bundle_server(&incremental_graph::TakeJSBundleOptionsServer { | ||
| kind: ChunkKind::HmrChunk, | ||
| })?; | ||
| 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); | ||
| } | ||
|
|
||
| let message = if exception.is_termination_exception() { | ||
| b"JavaScript execution was terminated".to_vec() | ||
| } else { | ||
| let mut holder = jsc::zig_exception::Holder::init(); | ||
| exception.to_zig_exception(global, holder.zig_exception()); | ||
| holder.zig_exception().message.to_owned_slice() | ||
| }; | ||
| 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)?; | ||
| } | ||
|
Comment on lines
+297
to
+309
Contributor
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 flaggedA 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:
Collaborator
Author
There was a problem hiding this comment. This is right, and this push does not change it. With 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. |
||
| Ok(()) | ||
| } | ||
|
|
||
| fn fail_server_file( | ||
| dev: &mut DevServer, | ||
| file: incremental_graph::ServerFileIndex, | ||
| log: &Log, | ||
| ) -> JsResult<()> { | ||
| // The server and SSR graphs share one record per file, and so one failure. | ||
| if !dev.server_graph.bundled_files.values()[file.get() as usize].failed { | ||
| dev.server_graph | ||
| .insert_failure(incremental_graph::InsertFailureKey::Index(file.get()), log)?; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Maps a 0-based (line, column) of a module's text in a chunk back to `source`, through the last mapping before it. | ||
| fn original_loc( | ||
| source: &bun_ast::Source, | ||
| source_map: &packed_map::Shared, | ||
| position: (u32, u32), | ||
| or_at_position: bool, | ||
| ) -> Option<bun_ast::Loc> { | ||
| // The mappings of one module name one source. | ||
| let parsed = | ||
| bun_sourcemap::mapping::parse(source_map.get()?.vlq(), None, 1, Default::default()).ok()?; | ||
| let position = ( | ||
| i32::try_from(position.0).ok()?, | ||
| i32::try_from(position.1).ok()?, | ||
| ); | ||
| let after = parsed.mappings.generated().partition_point(|mapping| { | ||
| let mapping = (mapping.lines.zero_based(), mapping.columns.zero_based()); | ||
| mapping < position || (or_at_position && mapping == position) | ||
| }); | ||
| let original = parsed.mappings.original()[after.checked_sub(1)?]; | ||
| let offset = bun_ast::Source::line_col_to_byte_offset( | ||
| &source.contents, | ||
| 1, | ||
| 1, | ||
| u64::try_from(original.lines.zero_based()).ok()? + 1, | ||
| u64::try_from(original.columns.zero_based()).ok()? + 1, | ||
| )?; | ||
| Some(bun_ast::Loc { | ||
| start: i32::try_from(offset).ok()?, | ||
| }) | ||
| } | ||
|
|
||
| /// Called at the end of BundleV2 to index bundle contents into the `IncrementalGraph`s | ||
| /// This function does not recover DevServer state if it fails (allocation failure) | ||
| pub(super) fn finalize_bundle( | ||
|
|
@@ -644,14 +849,6 @@ pub(super) fn finalize_bundle( | |
| )?; | ||
| } | ||
|
|
||
| dev.client_graph.ensure_stale_bit_capacity(false)?; | ||
| dev.server_graph.ensure_stale_bit_capacity(false)?; | ||
|
|
||
|
|
@@ -667,88 +864,11 @@ pub(super) fn finalize_bundle( | |
| } | ||
|
|
||
| // Load all new chunks into the server runtime. | ||
| if !dev.frontend_only | ||
| && dev.server_graph.current_chunk_len > 0 | ||
| && let Some(server_modules) = load_server_chunk(dev, ctx.sources)? | ||
| { | ||
| let global = dev.global(); | ||
| // Note: `make_array_for_server_components_patch` needs `&mut self` | ||
| // while reading `self.incremental_result.client_components_{added,removed}`. | ||
| // It only touches `server_graph.bundled_files.keys()`, so reborrow the | ||
|
|
@@ -784,6 +904,14 @@ pub(super) fn finalize_bundle( | |
| let _ = errors; // TODO: | ||
| } | ||
|
|
||
| // Index all failed files. The server chunk load above can add failures, so this comes after it. | ||
| if !dev.incremental_result.failures_removed.is_empty() | ||
| || !dev.incremental_result.failures_added.is_empty() | ||
| { | ||
| had_sent_hmr_event.set(true); | ||
| } | ||
| dev.index_failures()?; | ||
|
|
||
| let mut route_bits = DynamicBitSet::init_empty(dev.route_bundles.len())?; | ||
| let mut route_bits_client = DynamicBitSet::init_empty(dev.route_bundles.len())?; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 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, andexecuteProgramparses 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 as0,{...}(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 as0,{…}. The per-part loop at finalize.rs:217-225 finds no part rejected, sofail_server_chunkat :278-287 re-parses the whole chunk, overflows again, andabort_on_server_chunk_errorpanics. The base branch already aborts here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. The chunk adds parser frames to each module (
({against0,{, and the frames ofexecuteProgram). In a band of a few nesting levels JSC rejects the chunk and accepts each module alone, andfail_server_chunkthen 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.