Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Capture copied input sizes src/bundler/LinkerContext.rs |
When metafile generation is enabled, the linker records copied asset sizes by source index. |
Report and validate input sizes src/bundler/linker_context/MetafileBuilder.rs, test/bundler/metafile.test.ts, test/aot/cli/metafile.test.ts |
Metafile generation uses recorded sizes for copied inputs and falls back to source.contents.len() otherwise. Tests check reported sizes across loaders, plugin contents, HTML builds, CLI output, and AOT builds. |
Suggested reviewers: alii, jarred-sumner
Priority:
Merge Risk: 
999b5
-
No actionable merge-blocking issue is established for the copied-input byte reporting change. Merge remains subject to normal build and test checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: reading copied input sizes from asset files for metafile output. Description check The description explains the problem, implementation, scope, tradeoffs, and verification results. It does not use the template headings exactly, but it provides the required information in equivalent … Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the problem, implementation, scope, tradeoffs, and verification results. 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.
Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
|
Updated 5:40 AM PT - Oct 30th, 2026
❌ @robobun, your commit 999b571 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 38341That installs a local version of the PR into your bun-38341 --bun |
|
Status: ready for review. The diff is green on every lane that it touches. The one red test, Reproduced on main (9fe1cd6) and on the releases 1.4.0 and 1.4.1: This PR is now the size of copied inputs only. The two fixes for the records of CSS outputs that its first version had are in #47453. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — metafile-only bugfixes bringing CSS entry points and copied-asset sizes in line with esbuild.
What was reviewed:
- Verified the PR's claim that nothing else consumes
files_with_parts_in_chunkfor CSS chunks:find_all_imported_parts_in_js_orderskipsContent::Css, andcompute_cross_chunk_dependencies::walkcontinues on CSS content — so seeding the map only affects the metafile and the CSS printer'sfetch_add(which previously no-oped on the missing entry). - The extracted
css_files_with_parts_in_chunkhelper is byte-identical to the inline loop it replaces at the JS-entry-imports-CSS site. - The new mutable
copied_contents_lencolumn in theprocess_files_to_copysplit-borrow is disjoint SoA storage, same invariant as the existing three mutable columns; the write reuses the already-computedcontents_lenright beforemem::take. - Gating exports on
Content::Javascriptalso empties HTML-chunk exports;bundler_htmltests still pass per the description and[]is the correct value there.
Extended reasoning...
Overview
Three targeted metafile bugfixes: (1) CSS entry point chunks now seed files_with_parts_in_chunk from the CSS import order via a shared helper (previously left empty, so the metafile listed no inputs), (2) generate_chunk_json writes exports only for JS chunks so CSS/HTML outputs get exports: [] instead of leaking the JS entry's or the stylesheet's JS-side exports, (3) a new InputFile.copied_contents_len SoA column captures the source length before process_files_to_copy moves the bytes out, so copied assets no longer report bytes: 0. Four new test.concurrent cases cover both CSS chunk construction paths and both Cow arms of the asset move.
Security risks
None. This only shapes the metafile JSON (diagnostic build metadata); it does not touch bundle output, resolution, or any user-input parsing path.
Level of scrutiny
Moderate. The bundler linker is core code, but every change here is metafile-observable only — none of the three fixes alters emitted JS/CSS bytes. I traced each consumer of files_with_parts_in_chunk to confirm CSS chunks are skipped outside the metafile/printer paths (findAllImportedPartsInJSOrder.rs:34, computeCrossChunkDependencies.rs:145), so populating the map for CSS entry points cannot change chunk ordering or cross-chunk deps. The unsafe split-borrow extension in bundle_v2.rs adds one more disjoint MultiArrayList column under the same SAFETY invariant already covering three mutable columns; the comment count was updated to seven.
Other factors
The helper extraction deduplicates the two CSS-chunk sites per REVIEW.md's "second time a block appears, extract" rule. copied_contents_len reuses the contents_len local that already fed OutputFile::size, so it cannot diverge from the emitted asset's size. Tests use exact toEqual assertions, Buffer.alloc per repo convention, and the plugin test's on-disk file has a different size than the plugin bytes so the assertion can only pass by reading the plugin's length. The PR ran the full esbuild-compat and bundler test matrix on the debug build.
…].bytes (#42760) ### Problem - In the JSON metafile (`bun build --metafile`, `Bun.build({ metafile: true })`), `outputs[path].bytes` is smaller than the emitted file when the chunk references another output. On 1.4.3 `m0.js` is 130 bytes on disk, 98 in the metafile. `bun.d.ts` documents the field as "Size of the output file in bytes". - The cause is `generate_chunk_json` (`src/bundler/linker_context/MetafileBuilder.rs:75`). It ran before `code()` and wrote `IntermediateOutput::get_size()` (`src/bundler/Chunk.rs:610`), which adds only the literal pieces. It leaves out every path and HTML manifest that `code()` writes, and the `//# debugId` and `//# sourceMappingURL` comments. ### Fix - `Chunk::final_output_size` records the length of the emitted buffer. Each place that makes the final buffer sets it: two in `generateChunksInParallel.rs` (standalone HTML, in-memory) and one in `writeOutputFilesToDisk.rs`. - `metafile_builder::generate` builds each chunk fragment. Both callers run it after `generate_chunks_in_parallel`, so the size is known. esbuild defers this fragment in the same way. - Removed: `get_size()`, `Chunk::metafile_chunk_json`, and the `unsafe detach_lifetime()` in `generate`. The fragment was its only borrow that was not `'static`. - Verified: `test/bundler/metafile.test.ts`, 15 new cases, all fail on 1.4.3. Also the `esbuild/metafile`, `bundler_compile_splitting`, `standalone`, `bundler_html` and `html-import-manifest` suites. ### Background - The bundler prints a chunk before its final path exists, because the path holds a content hash. A reference to another output is a placeholder (a unique key) in the printed text. - `IntermediateOutput::Pieces` holds the printed text, split at each placeholder. `code()` joins the pieces and writes the final path at each split. - The bundler then appends the source map comment and writes the buffer to disk, or returns it as a `BuildArtifact`. <details><summary>Notes</summary> Repro (1.4.3 and main at 5fce36e): ```sh mkdir -p src printf 'import { v1 } from "./m1.ts";\nimport { v2 } from "./m2.ts";\nconsole.log(v1 + v2);\n' > src/m0.ts printf 'import c from "./c3.cjs";\nexport const v1 = "V1" + c.v3;\nconsole.log(v1);\n' > src/m1.ts printf 'import c from "./c3.cjs";\nexport const v2 = "V2" + c.v3;\nconsole.log(v2);\n' > src/m2.ts printf 'module.exports = { v3: "V3" };\n' > src/c3.cjs bun build --splitting --target=bun --outdir=out --metafile=meta.json src/m0.ts src/m1.ts src/m2.ts ``` | output | on disk | `bytes` before | `bytes` after | | --- | --- | --- | --- | | `./m0.js` (2 imports) | 130 | 98 | 130 | | `./m0-5begjve3.js` (1 import) | 195 | 179 | 195 | | `./m1.js` (1 import) | 74 | 58 | 74 | | `./m0-8x1hvvzq.js` (no import) | 1400 | 1400 | 1400 | Each import costs 16 bytes, the length of `./m0-xxxxxxxx.js`. Other cases measured on 1.4.3: - `--sourcemap=linked`: `m0.js` is 207 on disk, the metafile said 98. - `--sourcemap=inline`: 611 on disk, 98 in the metafile. - `--sourcemap=external`: 176 on disk (the `//# debugId` comment, which `code()` writes), 98 in the metafile. - `import logo from "./logo.svg"` (file loader): 98 on disk, 79 in the metafile. - An HTML entry point: `index.html` is 218 on disk, 160 in the metafile. - A server build that imports an HTML file (the manifest placeholder): `server.js` now matches, 889 bytes. esbuild 0.25.1 on a split build with source maps reports the exact file size (`a.js`: 146 on disk, 146 in the metafile). Its linker passes `len(outputContents)` to `jsonMetadataChunkCallback(finalOutputSize)` after it appends the `sourceMappingURL` comment. The new tests compare `outputs[key].bytes` with the size of each emitted file: - Code splitting with each `sourcemap` mode (`none`, `linked`, `inline`, `external`), with an `outdir` (the disk loop) and without (the in-memory loop). - An asset referenced from JS, and a 128 KB asset referenced from CSS. A stylesheet inlines a smaller file as a `data:` URL. - An HTML entry point. - A standalone HTML file (`compile` with `target: "browser"`). This is the only case that reaches the third assignment. The test compares the `bytes` of each inlined chunk with the text between its tags in the document. - The CLI with `--metafile` and `--metafile-md`. The test reads the `[ENTRY: ... (N bytes)]` lines of the report. With any one of the three assignments removed, the chunks of that path report 0 and its cases fail. More builds checked by hand against the fixed binary, all equal to the file on disk: `--banner` and `--footer`, a shebang entry with `--target=bun`, `--minify` with `--sourcemap=linked`, `--format=cjs --bytecode`, `--public-path`, `--entry-naming '[dir]/[name]-[hash].[ext]'` with entry points in subdirectories, and a CSS entry point with an `@import` and a 200 KB `url()` asset. Why a field on the chunk and not `OutputFile.size`: a standalone HTML build gives each inlined chunk an empty placeholder `OutputFile` and then filters those out of the list. The index of a chunk and the index of its output file match only by a `debug_assert`, and not in that mode. What changes for a user: - `bytes` grows by the paths, by the `debugId` comment, and by the `sourceMappingURL` comment. With `sourcemap: "inline"` that comment holds the whole map. The number now equals `BuildArtifact.size`. - The `--metafile-md` report uses the same numbers for "Total output size", "Bundle size", the chunk sizes and the "% of Total" column. - `--compile --target=browser` inlines the JS and CSS chunks into the document. `index.html` now reports its real size (403, was 203). An inlined chunk keeps its `outputs` entry, as before. Its `bytes` is the size of the inlined code. - `--compile` embeds each chunk in the executable. `bytes` is the size of the embedded module. - An error from `generate_chunk_json` now fails `generate` with the existing "Failed to generate metafile" warning. Before, `unwrap_or_default()` left an empty fragment and the JSON was not valid. The only fallible call is a write to a `Vec`. Not changed here, tracked in #42758: - `.map`, `.jsc` and copied asset files have no `outputs` entry. This is a different defect (a missing key, not a wrong value) and it adds keys that users see. - `inputs[..].bytesInOutput` counts a placeholder at its own length. A fix needs a count per compile result after the substitution, as in esbuild's `accurateFinalByteCount`. - A standalone HTML build lists the inlined chunks as outputs. Self-reviewed: 5 concerns raised, 5 addressed. A test for the standalone HTML assignment. The `unsafe` that this change made needless. Stricter test assertions (`statSync`, exact key lists, one combined CLI assertion). The `debugId` wording in the commit message. A tracker for the gaps above. Open PRs that touch `MetafileBuilder.rs` but not this field: #38341, #38624, #36974, #38666. None reads state that is gone when `generate` runs. #38666 has a small text conflict in `Chunk.rs` where this PR deletes `get_size()`. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/metafile.test.ts <!-- robobun:evidence:end -->
`inputs[..].bytes` is 0 for each input that the build copies to an asset: `file`, wasm, napi and embedded sqlite since 1.4.0, text under `--compile` since 1.4.1 (#40177). Bun 1.3.14 wrote the size. `process_files_to_copy` empties `source.contents` of such an input when it makes the asset file: it copies the bytes that were read from disk and moves the bytes that a plugin returned. The metafile read `source.contents.len()` after that. `LinkerContext::link` now reads the size of each asset file, and only when the build asks for a metafile. It reads before the outputs are emitted: a build to disk or in memory then takes the asset files out of the graph, and a standalone HTML build emits none of them. test/aot/cli/metafile.test.ts had an exemption for this ("The bundler says 0 for a file that it loads as text", #45631). It is gone, so that test compares `bytes` of each input.
f659493 to
25674b9
Compare
robobun
changed the title
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 findings, I also checked the new size snapshot in LinkerContext::link: asset.source_index.unwrap() is the IndexOptional -> Option conversion (bun_core/util.rs), not a panicking unwrap, and every index written into copied_input_sizes comes from reachable_files, so it is within input_files.len() after the resize. All three build paths (CLI, JS API, aot page bundles) reach metafile_builder::generate through link, so the snapshot is taken before generate_chunks_in_parallel drains the assets in each of them; the hoisted destinations/outdirFor test helpers are byte-identical to the removed ones.
Extended reasoning...
The new commit adds a per-source-index size vector to LinkerContext filled right after load() when the metafile is enabled, and MetafileBuilder reads it in place of the emptied source.contents; no security-sensitive surface is touched. The two inline findings are a pre-existing duplicate-key issue and a test that pins it, so the fix itself looks correct, but the inline comments already signal a human look.
There was a problem hiding this comment.
LGTM — the latest push addresses the one nit from my earlier review (the two-graphs test now asserts every image.png record is 13 rather than pinning exactly two records), and I found nothing else.
What was reviewed:
asset.source_index.unwrap()inlinkisGenericIndexOptional::unwrap, which returnsOption(no panic), and the index comes fromreachable_filesinprocess_files_to_copy, sosizes[..]is withininput_files.len().- All four
linkcall sites run afterprocess_files_to_copy, andOutputFile::initsetssizefromcontents_lentaken beforemem::take, so the recorded size is the pre-emptied length; thedebug_assertingeneratecompiles out and the release fallback is the oldsource.contents.len(). - Tests cover disk and in-memory for file/wasm/napi/sqlite, an onLoad-returned buffer whose size differs from disk, standalone HTML with no emitted assets, and the AOT text-import guard removal.
Extended reasoning...
The change adds a metafile-gated Vec<Option<usize>> of asset sizes to LinkerContext, filled once in link from parse_graph().additional_output_files, and reads it in MetafileBuilder::generate with a fallback to the existing source.contents.len(); about 30 Rust lines plus tests in test/bundler/metafile.test.ts and test/aot/cli/metafile.test.ts. It touches no security-sensitive surface (no user-path handling, no auth, no network). The bug hunt ran dry, no CODEOWNERS entry covers src/bundler/, no third-party CHANGES_REQUESTED review is present, and the only prior inline nit from this bot was addressed in commit 999b571, which decided approve.
Problem
inputs[..].bytesin the metafile is 0 for each input that the build copies:file, wasm, napi and embedded sqlite since 1.4.0, text under--compilesince 1.4.1. Bun 1.3.14 and esbuild write the size.process_files_to_copy(src/bundler/bundle_v2.rs:4787) emptiessource.contentswhen it makes the asset.generate(MetafileBuilder.rs:290) reads its length after that.test/aot/cli/metafile.test.ts:109has a guard for it.Fix
LinkerContext::linkreads the size of each asset file, only when the build asks for a metafile.generatewrites it.test/bundler/metafile.test.ts(7 new tests, 0 pass on main) andtest/aot/cli/metafile.test.tswithout the guard. Also 13 suites (Notes).Background
--metafile,Bun.build({ metafile: true })) is the JSON build report in esbuild's format.source.contents. The chunk fold weight and the printer's buffer reserve read that length and need the stub's size.Downsides
link,LinkerContext+24 bytes,.text+768 bytes.Notes
What changed since the first push. The first version (2026-08-14) had three fixes in one diff: this size, through a new column on each input file, and two fixes for the records of CSS outputs. A design review rejected the column: it costs 16 bytes for each input file in each build, for a number that only a metafile reads. A self-review then asked for one concern in each PR. This PR is now the size only. The two CSS fixes are in #47453, which does not depend on this PR.
inputs[..].bytesby releaselogo.png(5,000 bytes)mod.wasm(8 bytes)note.txtas text under--compile(7 bytes)1.4.1 copies a text import under
--compile(#40177). That is the case of the guard intest/aot/cli/metafile.test.ts(#45631). This PR removes the guard, so that test comparesbytesof each input with the--aotmetafile. The other two filters of that loop do not change.Why
source.contentsis empty.process_files_to_copytakes it (core::mem::take). It copies bytes that were read from disk into the asset file, and it moves bytes that anonLoadplugin returned. The length is read after that bygenerate, and also by the fold weight of a chunk (mergeSmallChunks.rs:1026), the reserve of the print buffer (js_printer/lib.rs:8049) and a source map hint (:7690). Those three want the size of the stub, so this PR does not put the bytes back.Where the asset files are when
generateruns--outdir--compile--compile --target=browser)So no one list after emit has each asset.
linkruns before emit, with all of them in the graph.A build with two graphs.
bun build ./p.html ./e.js --target=bun --metafile, where the stylesheet of the page and the server code both load a 13-byte image. The file has a record for each graph, under one key (#47083). The stylesheet inlines the image, so only the module of the server code is copied. On main that record says 0 and the other says 13, and the one that a JSON parser keeps changes from build to build. With this PR both say 13 (120 builds). With #47083 merged on an earlier head of this branch, the file is listed once in all 120.What stays as on main
bom.binhas 13 bytes, its asset has 10, andbytesis 10.import page from "./index.html",--target=bun): the stub of the server graph has a record withbytes: 0beside the record of the page, under one key. bundler: give each metafile record of a build with two graphs its own key #47083 removes the record of the stub.outputs(fix(bundler): include emitted files in metafile outputs #43823).Numbers (release builds, main 9fe1cd6 against this diff on it)
.textbytes (size -A).rodatabytessize_of::<LinkerContext>()size_of::<InputFile>()linkruns--metafile: times it runs--metafile: 2,001 TS modules--metafile: 200 copied files--metafile: 5,000 copied files--metafileInstructions are user instructions of the process (
perf_event_open), the median of 21 interleaved runs with 2 pool threads (11 runs for 5,000 files). The spread of the runs is 0.15% to 1.0%, so no row is a change. The run counts are gdb breakpoint hits. Syscalls are ptrace counts. strace, perf, valgrind and bloaty are not on the machine. A sorted list of pairs with a binary search was measured too: +0.85% instructions for the 5,000 files with--metafile, so the sizes are a list by source index.Other open PRs on these lines (trial merges)
[hash]and debugId #47285, bundler: give a shared stylesheet the name of its CSS entry point under CSS chunking #46599, bundler: metafile names the output of a split import() / require() by itsoutputskey #47337, bundler: list the chunks an entry point loads through other chunks in --metafile-md #42755.LinkerContext::link. Both insert afterself.load(..). Keep both.Tests. In
test/bundler/metafile.test.ts: files of thefile, wasm, napi and sqlite loaders, a file whose bytes anonLoadplugin returns (the file on disk has another size), and a standalone HTML file, each to disk and in memory. One CLI build with two graphs reads both records from the text of the metafile.test/aot/cli/metafile.test.tscovers a text import under--compile.Suites run with the debug build.
metafile72 pass,aot/cli/metafile4 pass. All pass:esbuild/metafile12,esbuild/css68,esbuild/loader10,bundler_loader62,bundler_plugin68,bundler_html26,html-import-manifest10,bundler_html_server4,bundler_bun22,compile-asset-bunfs10,bundler_splitting209,esbuild/default155,regression/issue/101392.Self-review. A review of this diff together with the two CSS fixes and a refactor of the JS byte counters raised 28 concerns. Addressed (24): the split, "empties" where the text said "moves", the route of a standalone HTML build, the guard and the releases named above, and the lists above of what stays and of the overlaps. The rest are in #47453. Held (3): the ones about the refactor, which is not in a PR. Rejected (1): a metafile check in
test/bundler/expectBundled.tsfor each fixture that setsmetafile. It changes the shared harness. The loader test here and the AOT test cover the same five loaders. Not done: afileimport in the fixture of the AOT test, because its text import takes the same path.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/metafile.test.ts, test/aot/cli/metafile.test.ts