Repository navigation
Conversation
|
Updated 12:12 AM PT - Aug 24th, 2026
✅ @autofix-ci[bot], your commit 07d85978c92621f346035ed546b3b0ccd46c53cd passed in 🧪 To try this PR locally: bunx bun-pr 29291That installs a local version of the PR into your bun-29291 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEnables ESM bytecode sidecar workflow: bundler emits Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/bundler/bytecode.mdx`:
- Around line 51-59: The docs currently imply bundling yields three files but
don't state that the `.jsm` sidecar is required at runtime for ESM cache hits;
update the paragraph mentioning `dist/index.js`, `dist/index.js.jsc`, and
`dist/index.js.jsm` to explicitly state that for Bun to reuse ESM bytecode at
startup (e.g., when running `bun dist/index.js`) you must ship the `.js` plus
both `.jsc` and `.jsm` files together—missing the `.jsm` will prevent ESM cache
reuse and silently degrade startup performance.
In `@src/bundler/linker_context/writeOutputFilesToDisk.zig`:
- Around line 304-305: The module-info (.jsm) emission must be gated the same
way as the .jsc loader check to keep OutputFileListBuilder's reserved counts
consistent: update the conditional that writes module_info_bytes (currently
checking c.options.generate_bytecode_cache, c.options.output_format == .esm, and
chunk.content == .javascript) to also require the loader JS-gate (the same check
used for .jsc, e.g., loader.isJavaScriptLike()). Do the same for the block
around lines 407-410 that calls insertForSourcemapOrBytecode() / pushes
bytecode/module-info entries so those pushes only occur when the loader JS-gate
is true, ensuring reserved bytecode/module-info slots match actual emitted
.jsm/.jsc files.
In `@src/transpiler.zig`:
- Around line 22-46: The BytecodeBundle currently holds heap-backed slices but
has no explicit ownership/cleanup API; add a pub fn deinit(self:
*BytecodeBundle, allocator: *std.mem.Allocator) !void that frees bytecode and
module_info when they are non-empty, and update the call sites that produce
BytecodeBundle (and any places constructing AlreadyBundled variants) to either
call BytecodeBundle.deinit(allocator) when done or clearly
document/process-lifetime; also keep bytecodeSlice and moduleInfoSlice unchanged
but ensure their consumers do not assume automatic freeing.
In `@test/regression/issue/29286.test.ts`:
- Around line 93-95: Remove the brittle generic stderr checks by deleting the
two assertions that inspect runStderr (the calls using
runStderr.not.toContain("error") and runStderr.not.toContain("await")) and leave
only the runtime exit assertion (expect(runExit).toBe(0)); keep any other
specific stderr assertions that verify known build-time regressions, and ensure
the runtime step relies solely on runExit for pass/fail (references: runStderr,
runExit in this test).
🪄 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: c42d642b-f8cb-4af2-8116-5231523f1614
📒 Files selected for processing (13)
docs/bundler/bytecode.mdxsrc/bun.js/ModuleLoader.zigsrc/bun.js/RuntimeTranspilerStore.zigsrc/bun.js/api/JSBundler.zigsrc/bun.zigsrc/bundler/linker_context/OutputFileListBuilder.zigsrc/bundler/linker_context/generateChunksInParallel.zigsrc/bundler/linker_context/generateCompileResultForJSChunk.zigsrc/bundler/linker_context/postProcessJSChunk.zigsrc/bundler/linker_context/writeOutputFilesToDisk.zigsrc/cli/Arguments.zigsrc/transpiler.zigtest/regression/issue/29286.test.ts
💤 Files with no reviewable changes (2)
- src/bun.js/api/JSBundler.zig
- src/cli/Arguments.zig
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/bundler/bytecode.mdx`:
- Line 57: Update the sentence that currently reads "if `.jsm` is missing Bun
still runs the `.js`, but ESM bytecode reuse silently degrades to a full parse"
to avoid claiming a full fallback; instead state that Bun will still use the
`.jsc` and that a missing `.jsm` causes the analyze/import-export reparse path
to be used, resulting in degraded startup performance (not a full source parse).
Keep the behavior note concise and replace the phrase "full parse" with
"startup-performance degradation due to analyze/import-export reparse."
In `@src/bundler/linker_context/postProcessJSChunk.zig`:
- Around line 28-38: The current logic sets generate_module_info using the
original entry loader
(parse_graph.input_files.items(.loader)[chunk.entry_point.source_index]) and
gates on loader.isJavaScriptLike(), but non-entry split chunks should mirror the
same `.js` fallback used by writeOutputFilesToDisk.zig; update the generation
check in postProcessJSChunk.zig (the generate_module_info computation) to treat
chunks that are non-entry or whose resolved disk-type would be `.js` as
JavaScript-like for the purpose of ModuleInfo creation so that ModuleInfo (and
thus .jsm/.jsc sidecars) are produced consistently with
writeOutputFilesToDisk.zig and avoid the slot/accounting mismatch.
In `@test/regression/issue/29286.test.ts`:
- Around line 3-6: Update the header comment's example sidecar filenames to
match runtime/test expectations: replace `dist/index.jsc` with
`dist/index.js.jsc` and `dist/index.jsm` with `dist/index.js.jsm` in the comment
that describes expected outputs for the `bun build --bytecode --format=esm
--target=bun --outdir` case so the examples align with the actual emitted files.
🪄 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: 8bb50e10-b864-4f8a-a7d0-df43888bfa3d
📒 Files selected for processing (6)
docs/bundler/bytecode.mdxsrc/bundler/Chunk.zigsrc/bundler/linker_context/postProcessJSChunk.zigsrc/bundler/linker_context/writeOutputFilesToDisk.zigsrc/transpiler.zigtest/regression/issue/29286.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bundler/linker_context/generateChunksInParallel.zig (1)
311-367:⚠️ Potential issue | 🟠 MajorKeep
module_infofixups best-effort for outdir builds.Line 311 now enables this block for non-
--compileESM bytecode too, but the fixup still treats supplementarymodule_infoallocations as fatal (bun.handleOom()on path replacement andmi.str()). That makes.jsmgeneration able to abort an otherwise valid JS build under memory pressure. Please keep this path consistent with the existing linker-context contract and degrade by skipping module-info fixups/sidecars when these allocations fail.Based on learnings, module_info operations for ESM bytecode caching use
catch continue/catch {}to silently handle allocation failures because module_info is supplementary metadata, not critical to producing correct JavaScript output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bundler/linker_context/generateChunksInParallel.zig` around lines 311 - 367, The current fixup path aborts the build on allocation failures (calls bun.handleOom) when building resolved paths and creating/replacing module_info strings; make these fixups best-effort instead: in the loop that builds unique_key_to_path (std.fmt.allocPrint and unique_key_to_path.put) change OOM/error handlers to skip that entry (catch continue or empty block) rather than bun.handleOom, in the collection loop change replacements.append error handling to skip the replacement on failure, and in the replacement application change mi.str(...) error handling to skip creating that new_id (catch continue) so module_info fixups are best-effort and do not abort JS generation (refer to unique symbols: unique_key_to_path, std.fmt.allocPrint, unique_key_to_path.put, replacements.append, Replacement.old_id, Replacement.resolved_path, mi.str, mi.replaceStringID).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bundler/linker_context/generateChunksInParallel.zig`:
- Around line 311-367: The current fixup path aborts the build on allocation
failures (calls bun.handleOom) when building resolved paths and
creating/replacing module_info strings; make these fixups best-effort instead:
in the loop that builds unique_key_to_path (std.fmt.allocPrint and
unique_key_to_path.put) change OOM/error handlers to skip that entry (catch
continue or empty block) rather than bun.handleOom, in the collection loop
change replacements.append error handling to skip the replacement on failure,
and in the replacement application change mi.str(...) error handling to skip
creating that new_id (catch continue) so module_info fixups are best-effort and
do not abort JS generation (refer to unique symbols: unique_key_to_path,
std.fmt.allocPrint, unique_key_to_path.put, replacements.append,
Replacement.old_id, Replacement.resolved_path, mi.str, mi.replaceStringID).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d72f4e4-7b4d-4e82-bdb6-93096f4b9330
📒 Files selected for processing (3)
src/bun.js/ModuleLoader.zigsrc/bun.js/RuntimeTranspilerStore.zigsrc/bundler/linker_context/generateChunksInParallel.zig
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/bundler/linker_context/generateChunksInParallel.zig:658-665— In the in-memory path, the.jsmmodule_infoOutputFileis created with.input_loader = .js(generateChunksInParallel.zig line 663), while the disk path in writeOutputFilesToDisk.zig correctly uses.input_loader = .file. SinceBuildArtifact.loaderis set directly frominput_loader, callers ofBun.build({ bytecode: true, format: 'esm' })withoutoutdirwill seeartifact.loader === 'js'for a binary.jsmblob instead of the correct'file'. Fix: change.input_loader = .jsto.input_loader = .fileat generateChunksInParallel.zig line 663 to match the disk path.Extended reasoning...
What the bug is and how it manifests
In the in-memory path at generateChunksInParallel.zig line 663, the module_info_output_file block creates an OutputFile with .input_loader = .js. The disk path in writeOutputFilesToDisk.zig (newly added by this PR) correctly uses .input_loader = .file for the identical module_info OutputFile. The .jsm sidecar is a binary serialization blob (the output of serializeModuleInfo), not JavaScript source — so .file is the semantically correct loader.
The specific code path that triggers it
OutputFile.zig lines 374 and 412 set BuildArtifact.loader = this.input_loader directly. Any caller using Bun.build({ bytecode: true, format: 'esm' }) without outdir takes the in-memory path and receives the module_info OutputFile as a BuildArtifact. Those callers will see artifact.loader === 'js' for a binary blob.
Why existing code does not prevent it
Before this PR, the in-memory module_info_output_file block was guarded by c.options.generate_bytecode_cache and c.options.output_format == .esm and c.options.compile. The --compile guard meant the OutputFile was embedded into the standalone binary and never returned to user callers as a BuildArtifact. This PR removes the c.options.compile guard, which means the in-memory path is now reachable from Bun.build() without outdir, and the module_info OutputFile IS returned to users as a BuildArtifact.
Why the .jsc path does not have the same issue
In the in-memory path around line 620, the .jsc bytecode OutputFile also uses .input_loader = .js, but that is a pre-existing issue affecting --compile-mode builds where the file is embedded and never returned as a BuildArtifact. The .jsm inconsistency is new to this PR: the disk path correctly uses .file (newly added code) but the in-memory path retained the pre-existing .js value without updating it to match.
Impact
A user calling Bun.build({ entrypoints: ['./index.ts'], target: 'bun', format: 'esm', bytecode: true }) without outdir and iterating result.outputs will observe output.loader === 'js' for the .jsm artifact — metadata incorrectly stating a binary blob is JavaScript source. The --outdir path reports loader === 'file' correctly, creating an observable API inconsistency between the two build modes introduced by this PR.
How to fix it
Change line 663 in src/bundler/linker_context/generateChunksInParallel.zig from .input_loader = .js to .input_loader = .file. This matches the disk path and correctly conveys that the .jsm sidecar is an opaque binary file, not JavaScript.
Step-by-step proof
- Bun.build({ entrypoints: ['./index.ts'], target: 'bun', format: 'esm', bytecode: true }) is called without outdir.
- root_path.len == 0, so the in-memory path in generateChunksInParallel.zig is taken.
- The module_info_output_file block at line 649 runs: c.options.generate_bytecode_cache and c.options.output_format == .esm are both true (the c.options.compile guard was removed by this PR).
- chunk.content.javascript.module_info_bytes is non-null, so OutputFile.init(.{ .input_loader = .js, ... }) is returned.
- output_files.insertForSourcemapOrBytecode(module_info_output_file.?) inserts this OutputFile into the results list.
- result.outputs exposes it to the caller as a BuildArtifact.
- OutputFile.zig:374: artifact.loader = this.input_loader yields artifact.loader === 'js' for a binary .jsm blob.
- The --outdir path via writeOutputFilesToDisk.zig sets .input_loader = .file and yields artifact.loader === 'file' for the same file, demonstrating the inconsistency.
-
🔴
src/bundler/linker_context/generateChunksInParallel.zig:658-665— WhenserializeModuleInfo()returns null on OOM (three explicit catch paths in js_printer.zig:6348,6353,6354) orModuleInfo.create()returns null (catch null in postProcessJSChunk.zig:50),module_info_bytesstays null for that chunk while bytecode generation can still succeed and fill its slot — leaving the module_info supplementary slot thatcalculateOutputFileListCapacity()pre-reserved unfilled. This causesOutputFileListBuilder.take()'sassertf(total_insertions == output_files.items.len)to panic in debug builds, and silently produces a zero-valueOutputFilein the reserved-but-unfilled slot in release builds. Fix: in the in-memory path (generateChunksInParallel.ziglines 658-665), addif (bytecode_output_file == null) break :brk null;at the top of the module_info block, mirroring the guard already present in the disk-write path (writeOutputFilesToDisk.zig); alternatively, null outmodule_info_byteswhenever serialization fails so the slot reservation and the actual fill stay in sync.Extended reasoning...
What the bug is and how it manifests
OutputFileListBuilder.calculateOutputFileListCapacity()(OutputFileListBuilder.zig line 94) reservesmodule_info_count = bytecode_countsupplementary slots — one slot per JS-like ESM chunk. This count is computed purely fromloader.isJavaScriptLike(), with no knowledge of whether serialization will succeed. The per-chunk serialization loop ingenerateChunksInParallel.zig(~line 360) runs first, assigningchunk.content.javascript.module_info_bytes = bun.js_printer.serializeModuleInfo(mi).serializeModuleInfo()is defined injs_printer.zigand has three explicit null-return-on-OOM paths:mi.finalize() catch return null(line 6348),deserialized.serialize(...) catch return null(line 6353), andbuf.toOwnedSlice(bun.default_allocator) catch null(line 6354). When any of these fires,module_info_bytesremains null for that chunk. A second trigger:ModuleInfo.create(bun.default_allocator, ...) catch nullinpostProcessJSChunk.zig:50— if allocation itself fails,chunk.content.javascript.module_infostays null, the cross-chunk fixup loop skips it (orelse continue), andmodule_info_bytesis never set. In both cases the reserved slot is never filled.The specific code path that triggers it
For the in-memory path (lines 649-665 of
generateChunksInParallel.zig): themodule_info_output_fileblock checks onlyc.options.generate_bytecode_cache and c.options.output_format == .esmand thenchunk.content.javascript.module_info_bytesbeing non-null. If serialization OOM'd,module_info_bytesis null →module_info_output_file = null→ no call toinsertForSourcemapOrBytecodefor that slot. Meanwhile,bytecode_output_filemay still succeed (bytecode generation is independent of module_info serialization) and does callinsertForSourcemapOrBytecode. The slot accounting: capacity = N chunks + 2N supplementary (N bytecode + N module_info); total insertions = N chunks + N (bytecode only, for the failing chunk). When k chunks fail serialization:total_insertions = N + (2N - k),output_files.items.len = N + 2N = 3N, and the assertftotal_insertions == output_files.items.lenfires:3N-k ≠ 3N.Why existing code doesn't prevent it
The disk-write path in
writeOutputFilesToDisk.zighas an explicit guard as the very first statement of the module_info block:if (bytecode_output_file == null) break :brk null;with a comment explaining the rationale (keep slot counts consistent, avoid the take() assertion). The in-memory path at lines 649-665 ofgenerateChunksInParallel.zighas no equivalent guard. This asymmetry means the same logical problem is fixed in one path but not the other. The OOM-graceful degradation pattern (catch null) is documented in codebase learnings as intentional for module_info operations — but its interaction with the slot-reservation accounting was not accounted for when the PR lifted thec.options.compileguard.What the impact would be
In debug/assert builds: the
bun.assertfinOutputFileListBuilder.take()(line 62) panics with "total_insertions (N) != output_files.items.len (M)". In release builds:take()strips the assert and setsitems.len = total_insertions, so the list is shorter than allocated; in practice the sequential-counter logic means already-written entries remain correct — but a zero-valueOutputFileoccupies a reserved slot in the output list, and the chunk'smodule_info_indexfield may be unset or point at the wrong index. The trigger requires genuine OOM, which is rare in normal operation but expected in constrained CI environments or very large build graphs.How to fix it
Two equivalent approaches: (1) In the in-memory path, add
if (bytecode_output_file == null) break :brk null;as the first statement of themodule_info_output_fileblock, mirroring the disk path. (2) After serialization fails (module_info_bytes stays null), also null out any pre-existing bytecode result for that chunk so both slots remain unfilled and the counts stay consistent.Step-by-step proof
- Build has N=1 JS-like ESM chunk under
--bytecode --format=esm.calculateOutputFileListCapacityreserves 1 chunk + 2 supplementary = 3 total output file slots. - Cross-chunk fixup loop runs:
ModuleInfo.create()succeeds, butbuf.toOwnedSlice()hits OOM →serializeModuleInfo()returns null →module_info_bytes = null. - Per-chunk loop: bytecode block calls
jsc.CachedBytecode.generate()— succeeds →bytecode_output_filenon-null →insertForSourcemapOrBytecode(bytecode_output_file)called →total_insertions = 2(1 chunk + 1 bytecode). - module_info block:
module_info_bytes == null→module_info_output_file = null→ noinsertForSourcemapOrBytecodecall →total_insertionsstays at 2. output_files.insertForChunk(chunk_output_file)called →total_insertions = 2(already counted above) — wait, recount: after the chunk file itself is inserted:total_insertions = 1 (bytecode) + 1 (chunk main file) = 2.take()checks:total_insertions (2) == output_files.items.len (3)→ assertf fires in debug builds.
- Build has N=1 JS-like ESM chunk under
4ad62eb to
5103f97
Compare
b048a7a to
8d51419
Compare
|
(robobun) Pausing this PR for maintainer triage. Tests pass on Linux/macOS locally and in CI; windows-2019-x64-test-bun consistently fails in a way I can't diagnose from a Linux dev container without BuildKite CLI/auth access to the test output. The substantive fix (ESM bytecode |
5a0473d to
85e8723
Compare
|
CI checkpoint for maintainers: on 85e8723, both |
|
CI flake note: the
Leaving for a maintainer to merge alongside the other PRs hitting the same debian/ubuntu flake. |
85e8723 to
204464b
Compare
|
Rebased onto latest main (204464b). One conflict in The PR is now a clean 3-file diff:
Build + both regression tests pass locally; gate confirmed (tests fail on released bun, pass with the guard removed). |
204464b to
e8fece6
Compare
There was a problem hiding this comment.
Re-reviewed after f2e57e0 — no bugs found. That commit implements the structural completion I suggested on the earlier revision (ResolvedSourceCodeHolder now frees an owned bytecode_cache when no consumer adopts it, and SourceProvider::create clears bytecode_cache_is_owned after adopting), so the class of edge-case leaks noted in that thread is closed. Given the scope — a new field in the #[repr(C)] ResolvedSource mirror, ownership/free semantics across the Rust↔C++ boundary, an added LSAN suppression, and removal of a user-facing bytecode gate — a maintainer sign-off is still worthwhile.
What was reviewed
ResolvedSourcefield ordering matches on both sides of the FFI mirror;Defaultinitializes the new flag tofalse.createCommonJSModule/evaluate/evaluateWithPotentiallyOverriddenCompileall takeResolvedSource&, soSourceProvider::create's flag-clear reaches the same struct the holder wraps — no double-free on the adopted path; nested holders inhandleVirtualModuleResultare safe because the free path also nulls the pointer.- The two owned-producer sites (
jsc_hooks.rs,RuntimeTranspilerStore.rs) setbytecode_cache_is_owned = !ptr.is_null(); borrowed producers (--compileembedded section, node compile cache) inheritfalsefromDefault. - Tests exercise CLI, JS API, and
--splittingchunk sidecars end-to-end under ASAN.
Extended reasoning...
Overview
The PR removes the "ESM bytecode requires --compile" guard from both the CLI (Arguments.rs) and JS API (JSBundler.rs), updates docs/bun.d.ts, and — following an LSAN report on the first ASAN CI run of the new tests — fixes a pre-existing .jsc sidecar bytecode buffer leak by adding ResolvedSource.bytecode_cache_is_owned (Rust + C++ mirror), keying the CachedBytecode destructor on it, freeing owned-but-unadopted buffers in ~ResolvedSourceCodeHolder (new in f2e57e0), freeing owned buffers in OwnedResolvedSource::drop, and adding a built-in LSAN suppression for the async transpiler worker. 13 files changed including a 172-line regression test.
Security risks
None identified. No new untrusted-input parsing, auth, crypto, or path handling. The change loosens a build-time validation gate but does not touch runtime security-relevant code paths; the sidecar loader itself is pre-existing.
Level of scrutiny
High. This is not a mechanical change: it inserts a field mid-struct in a #[repr(C)] layout mirrored across Rust and C++ (a mismatch would silently corrupt every ResolvedSource crossing the boundary), changes who frees what and when across FFI (Box::into_raw on the Rust side, defaultAllocatorFree on the C++ side, with an adopt-then-clear-flag protocol), adds an LSAN suppression that broadens what the ASAN lane will silently accept from TranspilerJob::run, and removes a user-facing gate that changes bun build/Bun.build behavior. REVIEW.md's memory-safety and API-design sections both point at maintainer review for exactly this shape.
Other factors
- The most recent commit (f2e57e0) directly implements the suggestion from my prior 🟣 inline note, closing the remaining CJS edge-case leak paths. I verified all C++ consumers take
ResolvedSource&by reference (JSCommonJSModule.h:86-89, 162-179), so the adopt-then-clear protocol is sound and nestedResolvedSourceCodeHolders don't double-free. - The PR has been through 21 iterations with extensive back-and-forth; all comment-cop threads and prior inline findings are resolved. A community user (shreeve) validated the build against a production-shaped worker-pool workload with timing and memory measurements.
- The
__lsan_default_suppressionsentry now cites CI build #97165 as the concrete report and matches the breadth of the existing sync-path twin, per the author's justification; that's a judgment call a maintainer should ratify. - The bug-hunting system found no issues on this revision.
f2e57e0 to
4a791f8
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes memory ownership across the Rust/C++ FFI boundary (new bytecode_cache_is_owned on the #[repr(C)] ResolvedSource mirror, ResolvedSourceCodeHolder destructor now freeing bytecode, CachedBytecode destructor selection) and adds an LSAN suppression, a human look is still warranted.
Checked: struct layout stays in lock-step between ResolvedSource.rs and headers-handwritten.h (field inserted at same position, before module_info); all four bytecode_cache producers set the flag consistently (two sidecar producers → owned, --compile/node-compile-cache → borrowed via Default); SourceProvider::create clears the flag after adopting so ~ResolvedSourceCodeHolder won't double-free; builtinLoader now guarded by a holder so the overridden-_compile path frees its buffer.
Ruled out: double-free when res->success == false (the destructor's free is gated on success, and the memset zero-init keeps the flag false on the error union path).
Extended reasoning...
Overview
This PR removes two validation guards (CLI in Arguments.rs, JS API in JSBundler.rs) that blocked --bytecode --format=esm without --compile, updates three doc/type files, and adds a regression test. That much is straightforward.
The bulk of the diff, though, is a memory-safety fix that grew out of an LSAN report on this branch: a new bytecode_cache_is_owned: bool field on the FFI-shared ResolvedSource struct (mirrored in headers-handwritten.h), wired through both sidecar producers (jsc_hooks.rs, RuntimeTranspilerStore.rs), consumed by ZigSourceProvider.cpp to select the CachedBytecode destructor, and freed by ~ResolvedSourceCodeHolder (now moved to ModuleLoader.h so builtinLoader in JSCommonJSExtensions.cpp can use it) and by Rust's OwnedResolvedSource::drop. Plus a new built-in LSAN suppression for the async transpiler thread.
Security risks
None identified. No user-controlled input reaches new parsing/allocation paths; the change gates on an ownership flag set by trusted producers. The LSAN suppression widens sanitizer blind spots for TranspilerJob::run but is documented, cites CI build #97165, and mirrors the existing Bun__transpileFile suppression for the sync twin.
Level of scrutiny
High. REVIEW.md calls native memory safety "the most-blocked category". This PR adds a field to a #[repr(C)] struct that must stay byte-for-byte identical across Rust and C++, changes RAII destructor semantics for a holder used at every module-load site, and introduces an adopt-then-clear-flag protocol between SourceProvider::create and ResolvedSourceCodeHolder. A layout mismatch or a missed clear would be UAF or leak. That's exactly the class of change a maintainer should sign off on, even though the bug-hunting system found nothing and every one of my prior 8+ inline threads over 25 iterations has been addressed and resolved.
Other factors
The PR has strong test coverage (three concurrent tests: CLI, JS API, multi-entry splitting) with the fails-on-main / passes-on-PR evidence in the description. All comment-cop and prior review threads are resolved. The remaining reason to defer is purely the nature of the code touched, not any outstanding concern.
Two independent changes, both needed to make the feature actually work end-to-end: 1. Remove the `ESM bytecode requires compile` guards in `src/runtime/api/JSBundler.rs` and `src/runtime/cli/Arguments.rs`. The guards rejected `--bytecode --format=esm` (and the JS-API equivalent) unless `--compile` was also set, which forced users with top-level `await` to either embed the full Bun runtime per binary or refactor every TLA into async IIFEs. The underlying bundler already knows how to emit the .jsc sidecar in this mode; it was just locked behind the guard. 2. Fix a latent ZStr NUL-terminator bug in `src/bundler/transpiler.rs` that panicked on any non-trivial bundle path. The `.jsc` sidecar loader uses `bun_paths::PathBuffer::uninit()` and then takes a `ZStr` from the buffer — `ZStr::from_buf` asserts `buf[total] == 0`, but the comment claiming the buffer was zero-initialized was wrong. Short paths (like those in a single-directory test) happened to have 0 at the check offset by chance; longer paths (absolute tempdirs, Windows paths) hit garbage bytes and panicked with "missing NUL at buf[len]". Write the terminator explicitly. Closes #29286
RuntimeTranspilerStore's TranspilerJob::run() stored a borrowed pointer (bytecode_slice().as_ptr()) from parse_result.already_bundled's Box<[u8]> into resolved_source.bytecode_cache, then returned. parse_result drops on return — freeing the box — before the dispatch_to_main_thread scopeguard hands the job (and the now-dangling pointer) to the JS thread, which passes it to C++ CachedBytecode::create: a use-after-free. Mirror the synchronous jsc_hooks.rs path: mem::take the AlreadyBundled variant out and heap::into_raw the Box so C++ becomes the owner. This path was already reachable for CJS .jsc sidecars; enabling ESM bytecode without --compile routes AlreadyBundled::Bytecode through it too. Also trim the regression test's header/inline comments to drop stale references to the NUL-terminator fix that landed on main separately.
Removing the guards left three references that now contradict behavior: - packages/bun-types/bun.d.ts: BuildConfig.bytecode JSDoc (IDE tooltip) - docs/bundler/index.mdx: bundler reference page (JS-API + CLI tabs) and the inlined JSDoc copy - src/runtime/api/JSBundler.rs: "since esm doesn't really work yet" on the format-defaulting branch (the CJS default stays for back-compat; the rationale was false) All describe the bytecode .jsc sidecar working for ESM without --compile.
Two entrypoints sharing an import produce a separate non-entry chunk with its own .jsc sidecar; the test builds with --splitting and runs an entry, covering chunk-sidecar loading that the single-file cases don't reach. Note: this does not specifically exercise the async RuntimeTranspilerStore path the earlier UAF fix guards. Verified with the store's debug scope and a fix-reverted ASAN run that bundled `.jsc` chunks load through the synchronous fetch path (which already transfers ownership correctly), not TranspilerJob:: run, for both static and dynamic imports from `bun <out>.js`. The ownership fix stands as a defensive mirror of the sync path.
ESM bytecode no longer requires --compile. Without --compile it emits a .jsc sidecar next to each .js chunk that Bun loads automatically, and top-level await works because the output is ESM. Also corrects the sidecar filename shown in the CommonJS and debugging examples: the extension is appended to the full chunk name, so index.js produces index.js.jsc, not index.jsc.
The three regression tests each spawn isolated Bun subprocesses in their own tempDir with no shared state, so run them with test.concurrent per test/CLAUDE.md. Also correct the inline comment on the shared-chunk test: bundled .jsc chunks load through the synchronous fetch path, not the async transpiler store.
LeakSanitizer flagged the sidecar bytecode buffer in CI: the destructor passed to JSC::CachedBytecode was selected from resolvedSource.needsDeref, but that flag tracks source_code ownership and is always cleared before the selection (in SourceProvider::create's deref block for the sync path, and in AsyncModule::fulfill for the async store path), so the no-op destructor meant for --compile's embedded bytecode was chosen for every owned sidecar buffer and the Box<[u8]> was never freed. Add a dedicated ResolvedSource.bytecode_cache_is_owned flag: true for the two .jsc sidecar producers (sync fetch and the async transpiler store), false for borrowed memory (--compile embedded section, node compile cache, both unchanged). The C++ destructor selection now uses it, and OwnedResolvedSource::drop frees an owned buffer that was never handed to C++. Also add the async transpiler store to the built-in LSAN suppressions: buffers it allocates become owned by CachedBytecode inside a SourceProvider, which lives in the libpas heap LSAN cannot scan, so an owner still alive at exit reports its (correctly owned) buffer. The sync-path twin suppression is leak:Bun__transpileFile in test/leaksan.supp. Collapse the bytecode format-default comment in JSBundler.rs to one line.
d791792 to
b5fe6cb
Compare
There was a problem hiding this comment.
LGTM — after the rebase this is down to two guard deletions plus docs/types/tests; the memory-safety work from earlier revisions has landed on main and is no longer in the diff. The two inline notes are non-blocking nits (docs completeness, test strengthening).
Extended reasoning...
Overview
The PR now touches 6 files: two source files where a validation guard is deleted (src/runtime/api/JSBundler.rs removes the 7-line ESM bytecode requires compile: true throw and rewords one comment; src/runtime/cli/Arguments.rs removes the 10-line CLI equivalent), two docs pages (bytecode.mdx, index.mdx), the bun.d.ts JSDoc, and a new regression test. All of the earlier-reviewed C++/Rust memory-safety changes (ResolvedSource.rs, ModuleLoader.{cpp,h}, ZigSourceProvider.cpp, headers-handwritten.h, bin_entry/mod.rs LSAN suppression, RuntimeTranspilerStore.rs, JSCommonJSExtensions.cpp) landed independently on main and dropped out of this diff during rebase, per the PR description and the <changed-files count="6"> metadata.
Security risks
None. This removes an error-out guard on a build-time option combination; no untrusted-input parsing, no auth/crypto/permission surface. The guard was purely a UX gate on a code path the bundler and runtime loader already implement.
Level of scrutiny
Low-to-medium. The remaining source change is a pure deletion of two validation branches — no new logic, no new allocation, no new FFI surface. The bundler already emits the .jsc sidecar for ESM chunks and the runtime loader already reads it via the // @bun @bytecode pragma; the guards were the only thing blocking users. The evidence block shows the tests fail on unfixed bun (ESM bytecode requires in stderr) and pass on both debug-ASAN and release with the fix.
Other factors
Every prior review thread on this PR is resolved. My last review (2026-08-22) was on the 15-file revision; the substantive change since then is the rebase that shrank the diff, so nothing new to re-verify beyond confirming the memory-safety hunks are gone. The two findings this run are both explicitly nits: (1) bytecode.mdx drops the analyze-phase-reparse caveat when comparing sidecar vs --compile — an editorial completeness note, nothing stated is false; (2) the run steps in the regression test don't set BUN_JSC_verboseDiskCache=1 to assert cache-hit, so a future ESM-sidecar-loader regression would fail open — but the build-side assertions (no error, .js.jsc exists and non-empty, @bytecode header present, not @bun-cjs) already lock in the guard-removal fix this PR actually makes. Neither blocks merge.
…ifference Bytecode loading fails open, so the run steps now set BUN_JSC_verboseDiskCache=1 and assert the Disk Cache hit, matching the sibling bytecode tests. The docs note that --compile also embeds module metadata to skip parsing entirely, while the sidecar accelerates only the evaluate phase.
test/regression/issue is for true regressions; this guard never allowed ESM bytecode without --compile, so the tests move next to the existing target:bun bytecode coverage. The splitting test now requires two cache hits (entry plus shared chunk) so a regression in the hashed-chunk sidecar lookup cannot hide behind the entry's own hit.
There was a problem hiding this comment.
LGTM — after the rebases this is now just the two guard deletions plus docs/types/tests, and every prior review thread is resolved.
What was reviewed:
- Guard removal in
JSBundler.rsandArguments.rs— the siblingformat must be 'cjs' or 'esm'andtarget must be 'bun'checks are preserved; only the--compile-required gate is dropped. - Tests now live in
bundler_bun.test.ts, assert[Disk Cache] Cache hit(≥2 for the splitting case) so a fail-open loader regression can't pass silently, and cover CLI +Bun.build+--splitting. - Docs restore the analyze-phase caveat (sidecar accelerates evaluate; module graph still parsed from source) and
bun.d.tsmatches.
Extended reasoning...
Overview
The PR now touches six files: two source files where a validation guard is deleted (src/runtime/api/JSBundler.rs, src/runtime/cli/Arguments.rs), two docs pages (docs/bundler/bytecode.mdx, docs/bundler/index.mdx), the bun.d.ts type declarations, and test/bundler/bundler_bun.test.ts. Earlier revisions carried a UAF fix in the async transpiler path, a ResolvedSourceCodeHolder header move, an LSAN suppression, and a sidecar bytecode-buffer leak fix; per the PR description and the diff, all of those landed on main independently and dropped out during rebases. What remains is a pure guard removal (7+10 lines deleted, one comment reworded) enabling a code path that the bundler and runtime already support.
Security risks
None. The change removes an over-restrictive input-validation gate on bun build options; it does not touch auth, crypto, permissions, network, or untrusted-input parsing. The remaining bytecode validations (target: 'bun' required; format must be cjs or esm) are preserved.
Level of scrutiny
Low-to-medium. The source change is deletion-only; the enabled path (ESM .jsc sidecar emission + runtime sidecar loading) already exists on main and is exercised end-to-end by the new tests. The docs and .d.ts changes are prose updates matching the new behavior. This PR has been through ~26 iterations and eight review passes; every thread is resolved.
Other factors
- All prior feedback from this reviewer has been actioned in the current diff: the analyze-phase caveat is back in
bytecode.mdx; both run steps setBUN_JSC_verboseDiskCache=1and assert cache hits; the splitting test asserts ≥2 hits so the shared-chunk lookup can't hide behind the entry's hit; tests were moved out oftest/regression/issue/intotest/bundler/bundler_bun.test.ts. - Tests follow harness conventions:
tempDir,bunEnv/bunExe,await usingon spawns, concurrent pipe draining viaPromise.all,test.concurrent, Windows path-separator normalization in theBun.buildcase. - The evidence block in the PR description shows the tests fail on released bun (guard present) and pass under both debug+ASAN and release with the fix.
- No CODEOWNERS-sensitive paths; no outstanding human-reviewer comments;
comment-copthreads all resolved (no multi-line comments remain insrc/in this diff).
|
I think this is waiting for enabling module_info in the runtime transpiler cache first |
|
Understood, and that matches the scope note in the description: this PR enables evaluate-phase bytecode only. The analyze phase still re-parses the source, because nothing supplies module_info without One data point for the decision, from the validation earlier in this thread: the evaluate-only win measured about 1.44x faster median worker boot on a ~1.1 MB artifact, at about +5 MB phys_footprint per process. I can hold this PR rebased until module_info lands in the runtime transpiler cache. I can also work on that piece first, either as a precursor PR or folded into this one. Tell me which you prefer. |
|
The Bun 1.4 release sort of implies this is included, but the status is still "draft". Was it included or not? |
|
Not included. Verified against 1.4.1: The validation guards this PR removes are still present on current The PR was moved to draft after the comment above: it waits on module_info support in the runtime transpiler cache, so that sidecar ESM bytecode skips the analyze-phase reparse as well before the gate is lifted. Until then the released behavior stands: CommonJS bytecode works without |
Closes #29286.
Repro
Before:
After:
Cause
bun build --bytecode --format=esm(and theBun.build({ bytecode: true, format: 'esm' })JS API) was gated behind--compileby two validation guards. The gate predated the runtime actually supporting an ESM.jscsidecar without embedding the standalone runtime. The bundler already knows how to emit the.jscsidecar in this mode and the runtime loader already reads a// @bun @bytecodeESM file's sidecar, so the guards were the only thing blocking it.Fix
src/runtime/api/JSBundler.rs(JS API) andsrc/runtime/cli/Arguments.rs(CLI).bun build --bytecode --format=esm --outdirnow emitsdist/index.js+dist/index.js.jsc, andbun dist/index.jsloads the bytecode for the evaluate phase.bun.d.tsupdated to drop the stale "ESM bytecode requires --compile" wording and correct the.jscsidecar filename (index.js.jsc).Earlier revisions of this branch also carried a use-after-free fix in the async transpiler path, removal of two dead
AlreadyBundledhelpers, and a fix for the.jscsidecar bytecode buffer leak that LeakSanitizer reported on this PR's tests (a newbytecode_cache_is_ownedownership flag driving theCachedBytecodedestructor). All of these have since landed independently onmain(the leak fix as the typedBytecode { ptr, len, owned }redesign ofResolvedSource), so they dropped out during rebases. Verified on the current base: the ESM splitting, plain CJS, andrequire.extensions+_compile-override bytecode paths all run leak-clean under the CI LSAN options. The async.jscownership transfer (mem::take+heap::into_rawto hand theBox<[u8]>to C++) is now upstream, which is what keeps the ESM path this PR enables memory-safe.Note: this enables the bytecode (
.jsc) sidecar only. The.jsmmodule-info sidecar (which would let JSC skip the analyze-phase reparse too) is not part of this change. The evaluate phase uses the bytecode; the analyze phase re-parses the source for import/export discovery.Verification
test/bundler/bundler_bun.test.ts(the "ESM bytecode sidecar without --compile" block) covers the CLI path (build +bun dist/index.jsend-to-end with top-level await), theBun.buildJS API, and a multi-entry--splittingcase where a shared chunk gets its own.jscsidecar. The tests fail on released bun (guard present) and pass with the guards removed.[review] gate passed · iteration 26 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 9 passed · 0 rejected · iteration 26
evidence per changed file
root cause · written by the author bot
The bundler only generated the serialized module info that ESM bytecode needs at runtime when --compile was set, so --bytecode with --format=esm was rejected outside of standalone builds and users were forced into CJS output, which breaks top-level await. The fix removes that validation in the CLI and JSBundler, collects top-level declarations and emits a .jsm module-info sidecar alongside the .jsc bytecode for ESM chunks in outdir builds, and threads the deserialized module info through the module loader so ResolvedSource.module_info is populated when the cached bundle is loaded. As a resu…