Fix capacity miscount in ESMConditions.init - #30692
Conversation
The expression 'if (allow_addons) 1 else 0 + conditions.len' parses as 'if (allow_addons) 1 else (0 + conditions.len)', so when allow_addons is true (the default for Bun.build), conditions.len was dropped from the ensureTotalCapacity request. With enough user conditions this overflowed the reserved capacity and tripped the putAssumeCapacity assert.
|
Updated 5:46 AM PT - May 14th, 2026
❌ @robobun, your commit 28128c6 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30692That installs a local version of the PR into your bun-30692 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughExplicitly regrouped additive terms when sizing loader-capacity in bundler options and added two tests that exercise many condition entries/flags to ensure bundling and CLI resolution do not crash. ChangesESM Condition Capacity Refactor and Tests
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/resolve/import-custom-condition.test.ts`:
- Around line 172-188: Replace the manual for-loop inside the "many custom
conditions resolve correctly" test with a parameterized test so each `n` is its
own case: convert the current it(...) block in import-custom-condition.test.ts
to use test.concurrent.each([4,5,6,8,12,20]) (or test.each if concurrency is
undesired) and move the body that builds `flags`, calls `Bun.spawnSync({...})`,
and asserts the result into the per-case callback; keep the same `flags`
construction logic, same `Bun.spawnSync` call and the expect comparing `{ n,
exitCode, stdout, stderr }` to the expected object, but run it once per `n` so
failures report independently.
🪄 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: 82323416-8876-4ed1-ac10-8381bf5d54dc
📒 Files selected for processing (1)
test/js/bun/resolve/import-custom-condition.test.ts
| it("many custom conditions resolve correctly", async () => { | ||
| for (const n of [4, 5, 6, 8, 12, 20]) { | ||
| const flags = []; | ||
| for (let i = 1; i < n; i++) flags.push(`--conditions=c${i}`); | ||
| flags.push("--conditions=first"); | ||
| const { exitCode, stdout, stderr } = Bun.spawnSync({ | ||
| cmd: [bunExe(), ...flags, `${dir}/test.js`], | ||
| env: bunEnv, | ||
| cwd: import.meta.dir, | ||
| }); | ||
| expect({ n, exitCode, stdout: stdout.toString("utf8"), stderr: stderr.toString("utf8") }).toMatchObject({ | ||
| n, | ||
| exitCode: 0, | ||
| stdout: "1\n", | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use parameterized test cases instead of a manual loop.
This should be split into parameterized cases so each n reports independently and can run concurrently across the matrix.
♻️ Suggested rewrite
-it("many custom conditions resolve correctly", async () => {
- for (const n of [4, 5, 6, 8, 12, 20]) {
- const flags = [];
- for (let i = 1; i < n; i++) flags.push(`--conditions=c${i}`);
- flags.push("--conditions=first");
- const { exitCode, stdout, stderr } = Bun.spawnSync({
- cmd: [bunExe(), ...flags, `${dir}/test.js`],
- env: bunEnv,
- cwd: import.meta.dir,
- });
- expect({ n, exitCode, stdout: stdout.toString("utf8"), stderr: stderr.toString("utf8") }).toMatchObject({
- n,
- exitCode: 0,
- stdout: "1\n",
- });
- }
-});
+it.concurrent.each([4, 5, 6, 8, 12, 20])("many custom conditions resolve correctly (n=%i)", async n => {
+ const flags = [];
+ for (let i = 1; i < n; i++) flags.push(`--conditions=c${i}`);
+ flags.push("--conditions=first");
+
+ const { exitCode, stdout, stderr } = Bun.spawnSync({
+ cmd: [bunExe(), ...flags, `${dir}/test.js`],
+ env: bunEnv,
+ cwd: import.meta.dir,
+ });
+
+ expect({ n, exitCode, stdout: stdout.toString("utf8"), stderr: stderr.toString("utf8") }).toMatchObject({
+ n,
+ exitCode: 0,
+ stdout: "1\n",
+ });
+});Based on learnings, in test/js/bun/**/*.test.ts, prefer test.each()/test.concurrent.each() over manual loops when each parameter value maps to a single assertion.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("many custom conditions resolve correctly", async () => { | |
| for (const n of [4, 5, 6, 8, 12, 20]) { | |
| const flags = []; | |
| for (let i = 1; i < n; i++) flags.push(`--conditions=c${i}`); | |
| flags.push("--conditions=first"); | |
| const { exitCode, stdout, stderr } = Bun.spawnSync({ | |
| cmd: [bunExe(), ...flags, `${dir}/test.js`], | |
| env: bunEnv, | |
| cwd: import.meta.dir, | |
| }); | |
| expect({ n, exitCode, stdout: stdout.toString("utf8"), stderr: stderr.toString("utf8") }).toMatchObject({ | |
| n, | |
| exitCode: 0, | |
| stdout: "1\n", | |
| }); | |
| } | |
| }); | |
| it.concurrent.each([4, 5, 6, 8, 12, 20])("many custom conditions resolve correctly (n=%i)", async n => { | |
| const flags = []; | |
| for (let i = 1; i < n; i++) flags.push(`--conditions=c${i}`); | |
| flags.push("--conditions=first"); | |
| const { exitCode, stdout, stderr } = Bun.spawnSync({ | |
| cmd: [bunExe(), ...flags, `${dir}/test.js`], | |
| env: bunEnv, | |
| cwd: import.meta.dir, | |
| }); | |
| expect({ n, exitCode, stdout: stdout.toString("utf8"), stderr: stderr.toString("utf8") }).toMatchObject({ | |
| n, | |
| exitCode: 0, | |
| stdout: "1\n", | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/bun/resolve/import-custom-condition.test.ts` around lines 172 - 188,
Replace the manual for-loop inside the "many custom conditions resolve
correctly" test with a parameterized test so each `n` is its own case: convert
the current it(...) block in import-custom-condition.test.ts to use
test.concurrent.each([4,5,6,8,12,20]) (or test.each if concurrency is undesired)
and move the body that builds `flags`, calls `Bun.spawnSync({...})`, and asserts
the result into the per-case callback; keep the same `flags` construction logic,
same `Bun.spawnSync` call and the expect comparing `{ n, exitCode, stdout,
stderr }` to the expected object, but run it once per `n` so failures report
independently.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/bundler/options.zig:1090-1094— This editssrc/bundler/options.zig, which per CLAUDE.md:141 is a non-compiled porting reference — the shipping implementation issrc/bundler/options.rs, and that file already hoistslet addon_extra = if allow_addons { 1 } else { 0 };(line 832) and uses.insert()instead of assume-capacity inserts (line 838:// PERF(port): was assume_capacity). So the describedputAssumeCapacity/addOneAssumeCapacitydebug-assert crash cannot occur in the current binary, the .zig change has zero runtime effect, and the new test passes withUSE_SYSTEM_BUN=1(which CLAUDE.md:130 says makes it invalid). Suggest re-scoping this as "sync .zig reference with .rs (no behavior change)" and dropping the test/Fuzzilli framing — or, if Fuzzilli really tripped this, identifying where, because it can't beoptions.rs.Extended reasoning...
What this PR actually changes vs. what it claims
The PR description says the capacity miscount in
ESMConditions.initcausedputAssumeCapacityto trip a debug assert inMultiArrayList.addOneAssumeCapacity, found by Fuzzilli. The fix and the test are both framed as a crash repro + fix. But the only source change is tosrc/bundler/options.zig, and per the repo's own contributor docs that file is not compiled and not shipped:CLAUDE.md:141: "You will see
.zigfiles alongside many.rsfiles … These are the original Zig implementation, kept only as a porting reference — they are not compiled and not shipped. New code goes in.rs. … Never add new behavior to a.zigfile."src/CLAUDE.md:15-17: "
.zigsiblings … are the original implementation kept as a porting reference for behavior; they are not compiled and are not where new code goes."There is no top-level
build.zigin the repo and no build configuration referencesoptions.zig. Editing it is a documentation-sync change, not a behavior change.The shipping code already has this fix
The actual implementation is
src/bundler/options.rs:820-875(ESMConditions::init), last touched in #30412 (the Rust rewrite), well before this PR. It already does exactly what this PR adds to the.zig:// src/bundler/options.rs:832-836 let addon_extra = if allow_addons { 1 } else { 0 }; default_condition_amp.reserve(defaults.len() + 2 + addon_extra + conditions.len()); import_condition_map.reserve(defaults.len() + 2 + addon_extra + conditions.len()); require_condition_map.reserve(defaults.len() + 2 + addon_extra + conditions.len());
And critically, the Rust port replaced
putAssumeCapacitywith growable.insert():// src/bundler/options.rs:838-841 // PERF(port): was assume_capacity import_condition_map.insert(b"import".as_slice().into(), ());
So even without the hoist, the Rust build would not assert —
.insert()reallocates on overflow. The crash narrative in the PR description ("putAssumeCapacitycalls tripped a debug assert inMultiArrayList.addOneAssumeCapacity") describes Zig-only machinery that does not exist in the shipped binary.Step-by-step proof the test is non-validating
- The new test calls
Bun.build({ target: "bun", conditions: ["a"…"l"] }). - This flows to
ESMConditions::initinoptions.rs(the only compiled implementation), withallow_addons = true(default peroptions.rs:1955) and 12 user conditions. options.rs:832computesaddon_extra = 1, then reservesdefaults.len() + 2 + 1 + 12— correct.- Even if step 3 under-reserved,
options.rs:838+uses.insert(), which grows the map; no assert can fire. - Therefore
bundle.success === trueon abunbinary built without this PR. - Per CLAUDE.md:130: "Your test is NOT VALID if it passes with
USE_SYSTEM_BUN=1." This test does, so it validates nothing about this change.
Why this matters / what to do
Merging as-is lands a misleading commit ("Fix capacity miscount … Found by Fuzzilli") that future readers will assume describes a real shipped bug, plus a test that can never regress. Two reasonable paths:
- Re-scope as a doc sync: retitle to something like "sync options.zig reference with options.rs", drop the Fuzzilli/crash framing from the description, and drop (or relabel) the test since it doesn't gate any behavior.
- Or investigate the Fuzzilli fingerprint: if
6a3e4efbcd4ab6e8really fired, it wasn't on this code path in the current Rust build — the repro/fingerprint may be stale (pre-#30412 Zig build) or pointing at a different site.
- The new test calls
|
Closing — this bug was already fixed on main by #30412. The Fuzzilli fingerprint |
| (if (target.isBun()) default_loader_ext_bun.len else 0) + | ||
| (if (target == .browser) default_loader_ext_browser.len else 0) + | ||
| default_loader_ext.len, |
There was a problem hiding this comment.
🔴 This PR's only source change is to src/bundler/options.zig, but per src/CLAUDE.md the .zig files are non-compiled porting references — the shipped binary is built from src/bundler/options.rs, where ESMConditions::init already hoists addon_extra and uses growing .insert(), so the bug described in the PR body cannot occur there. On top of that, commit 28128c6 reverted the ESMConditions.init hunk from 1e2c22c, so the net diff doesn't even touch that function — only the cosmetic parenthesization of loadersFromTransformOptions (which uses getOrPutValue and was never UB) survives. Net effect: the .zig edit changes nothing in the built binary and the title/description are misleading; only the two new regression tests add value. Please verify whether #30619 still reproduces on main (post-Rust-rewrite) — if not, retitle as a tests-only PR; if it does, the fix needs to go in .rs.
Extended reasoning...
What this PR actually changes
The PR title is "Fix capacity miscount in ESMConditions.init" and the description claims to fix #30619 by hoisting if (allow_addons) 1 else 0 into a typed local. But the net diff against base for src/bundler/options.zig shows zero changes to ESMConditions.init — only loadersFromTransformOptions (lines 1574-1577) is modified, adding parentheses around two if/else expressions in a capacity hint.
This happened because commit 28128c6 ("Also fix if/else precedence in loadersFromTransformOptions capacity hint") accidentally reverted the ESMConditions.init fix from 1e2c22c. git show 28128c64 -- src/bundler/options.zig shows it removing const addons_extra: usize = if (allow_addons) 1 else 0; and restoring the original buggy expression:
- const addons_extra: usize = if (allow_addons) 1 else 0;
- try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + addons_extra + conditions.len);
+ try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len);At HEAD, src/bundler/options.zig:1090-1092 still reads the unparenthesized form, which Zig parses as defaults.len + 2 + (if (allow_addons) 1 else (0 + conditions.len)).
Why none of this affects the binary
More fundamentally, src/CLAUDE.md (lines 15-17) states:
You will see
.zigsiblings next to many.rsfiles — those are the original implementation kept as a porting reference for behavior; they are not compiled and are not where new code goes.
The base of this PR is one commit after 23427db "Rewrite Bun in Rust (#30412)". There is no top-level build.zig, and all CI build failures in this PR's timeline are in .rs files (Blob.rs, server/mod.rs, h2_frame_parser.rs, JSSecrets.rs, VirtualMachine.rs, process.rs) — there is no Zig build step. The compiled implementation is src/bundler/options.rs.
Inspecting src/bundler/options.rs:832-835, the Rust port of ESMConditions::init already computes capacity correctly:
let addon_extra = if allow_addons { 1 } else { 0 };
default_condition_amp.reserve(defaults.len() + 2 + addon_extra + conditions.len());and at line ~838 it uses .insert() (which grows on demand) rather than an assume-capacity variant — the comment // PERF(port): was assume_capacity confirms this was intentionally changed during the port. So even if the reserve hint were wrong, the Rust code could not exhibit the putAssumeCapacity overflow / debug assert described in the PR body. Similarly, loaders_from_transform_options in options.rs already sums its terms correctly (Rust if/else with braces doesn't have Zig's precedence trap).
Step-by-step proof
- Net source diff:
git diff 19d8ade2..HEAD -- src/bundler/options.zigtouches only lines 1574-1577 inloadersFromTransformOptions.ESMConditions.initis byte-identical to base. - Surviving change is cosmetic:
loadersFromTransformOptionscallsstringHashMapFromArrays(which onlyputAssumeCapacity'sinput_loaders.extensions— always ≤ the hint regardless of precedence) and then usesgetOrPutValuefor the default extensions, which grows the map safely. The added parentheses fix the hint arithmetic but there was never UB here. - File is not compiled:
src/bundler/options.zigis a reference-only sibling ofsrc/bundler/options.rsper project conventions. Editing it cannot change the built binary. - Rust path is already correct:
ESMConditions::initin options.rs already hoistsaddon_extraand uses growing.insert(). With 12 user conditions andallow_addons=true, it reserves2 + 2 + 1 + 12 = 17and inserts via.insert()— no overflow possible. - Therefore: the two new regression tests should pass on
mainwith or without this PR's.zigedit, because they exercise the Rust runtime. The.zigchange has zero runtime effect.
Impact and how to fix
The PR as written does not deliver what its title and "Fixes #30619" claim. The actionable outcomes are:
- Drop the
options.zigedit (or at minimum re-apply the revertedESMConditions.inithunk so the reference file stays internally consistent — but persrc/CLAUDE.md, new code shouldn't go in.zigfiles at all). - Verify whether bun --conditions: export condition matching breaks with 4+ conditions (falls back to default) #30619 reproduces on current
main. Given options.rs is already correct, it almost certainly doesn't — in which case this PR should be retitled to "Add regression tests for bun --conditions: export condition matching breaks with 4+ conditions (falls back to default) #30619" and the description updated to note the bug was fixed by the Rust rewrite. - If bun --conditions: export condition matching breaks with 4+ conditions (falls back to default) #30619 does still reproduce on a Rust build, the real bug is elsewhere in
.rscode and needs to be found and fixed there; editingoptions.zigcannot help.
|
автоботы ролл аут |
2 participants
What
ESMConditions.initcomputed the hash map capacity as:which Zig parses as:
When
allow_addonsistrue(the default),conditions.lenwas dropped from the capacity request entirely. With enough userconditionsto exceed the allocator's rounded-up capacity (8), the subsequentputAssumeCapacitycalls wrote past the reserved slots.MultiArrayList.addOneAssumeCapacityassert (found by Fuzzilli, fingerprint6a3e4efbcd4ab6e8).--conditionsflags, resolution would fall back todefaultor fail with "Cannot find package".Fix
Hoist the
ifinto a typed local so the addition associates correctly.Repro
or at runtime:
Fixes #30619