Skip to content

js_parser: store string enum members flat so folds over them stay linear and cannot corrupt them - #41973

Merged
Jarred-Sumner merged 6 commits into
mainfrom
robobun/1ac63323/enum-string-fold-linear
Sep 8, 2026
Merged

Jarred-Sumner merged 6 commits into
mainfrom
robobun/1ac63323/enum-string-fold-linear

Conversation

@robobun

@robobun robobun commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • S.A + "k" + S.A + "k" + ... over an inlined string enum member folds in quadratic memory: 8 192 terms (49 KB) take 1.4 GB in bun run. Every A.B reference shares the member's rope, so join_strings (src/ast/fold_string_addition.rs) deep-cloned both ropes when either operand was an E::InlinedEnum: each enum term re-cloned the accumulator.
  • Template::fold (src/ast/e.rs) and members named with */ (left unwrapped by wrap_inlined_enum) had no guard and appended onto the shared rope. enum A { B = "1" + "2", T = `t${B}t` } makes A.B print "12t". A second use panics on 1.4.3: called Option::unwrap() on a None value, Crashed while visiting.

Fix

Background

  • String folding copies no bytes. "a" + "b" links the right EString node onto the left through next/end (a rope). EString::push writes to the rope's last node, so a rope needs one owner.
  • The visit turns A.B into an E::InlinedEnum around a copy of the member's root node. Later folds see a string literal.
Notes
  • Fuzz-ledger finding test(serve-http3): attach the fetch handlers before the wait for STOPPED #44146. Curve before: 1 024 pairs 62 MB, 4 096 700 MB, 8 192 2.76 GB, 16 384 OOM-killed at 3 GB (x2 terms, x4 memory). bun build --minify-syntax behaves the same. Not a regression: 1.3.14 behaves the same, and the Zig code this was ported from had the same shape.
  • The */ case on 1.4.3: enum A { "*/" = "s" + "t" }; console.log(A["*/"] + "u", A["*/"] + "v") panics the same way. With members stored flat the unwrapped value is a single node, so it is safe too.
  • After the fix, debug+ASAN build, both chains of the test (top level and inside an enum body): 1 024 pairs 7 MB, 4 096 9 MB, 16 384 22 MB, 65 536 70 MB. bun run of a 16 384-pair file peaks 8 MB over an empty script.
  • I first narrowed the clone to the side that is the enum member (also linear). Storing members flat is simpler: it removes the clone, covers Template::fold, the */ names and the bundler's cross-module substitution at the one place the sharing starts, and costs one O(len) copy per rope-valued member. esbuild stores enum string values flat too.
  • resolve_rope_if_needed leaves rope_len (still the length) and a stale end behind. end is only read from a node whose next is set, and a push onto a copy of a flat node assigns both.
  • The runtime transpiler cache is keyed on the source and the features hash, not on bun's version. A file that hit the template corruption without the panic was cached with the wrong output, so EXPECTED_VERSION goes to 30.
  • Out of scope, tracked separately: a + chain of template literals that each have a substitution (`a${x}b` + `a${x}b` + ...) is also quadratic under syntax minification (4 096 terms: 526 MB). That is concat_parts re-copying the accumulated parts array at each step, a different path that this change does not touch.
  • Suites run on the debug build: bundler_edgecase.test.ts, bundler_minify.test.ts, bundler_string.test.ts, esbuild/ts.test.ts, esbuild/default.test.ts (enum/template/string filter), transpiler/transpiler.test.js, transpiler-comma-chain-oom.test.ts, cli/run/transpiler-cache.test.ts. cargo clippy on bun_ast and bun_js_parser is clean.

no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/transpiler/transpiler-enum-concat-chain-oom.test.ts

…append to them

An enum member initialized with a concatenation was stored as a rope, and
every inlined use of it copied only the rope's root node. Template::fold
pushes the following template text onto whatever it inlined, which walked
to the end of the shared chain and appended there, so the member's value
changed in its declaration and in every other use. Using the member in a
second template literal then hit a chain node without an end pointer
(unwrap panic), and using it twice in one literal linked the chain onto
itself (infinite loop when resolving).

Resolve the rope when the enum is visited, so the stored string is a
single node and copies of it share nothing. This makes the inlined-enum
cloning in fold_string_addition redundant, so it is removed, and
wrap_inlined_enum asserts the invariant in debug builds.
Covers the fuzz finding that `S.A + "k" + S.A + "k" + ...` over an
inlined string enum member used quadratic memory while folding, plus the
ways a folded enum member can meet another `+` or template literal,
including a member name that contains `*/`.
The transpiled output of a file that uses a folded string enum member inside
a template literal changes with the previous commits (it was wrong before),
and the cache is not keyed on the bun version, so a stale entry would keep
serving the old output after an upgrade.
@coderabbitai

coderabbitai Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file.

Or wait 6 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: d7887052-f639-4b46-9f91-18fe4d2b2788

📥 Commits

Reviewing files that changed from the base of the PR and between d745f03 and ffcb911.

📒 Files selected for processing (8)
  • src/ast/e.rs
  • src/ast/fold_string_addition.rs
  • src/js_parser/p.rs
  • src/js_parser/visit/visit_stmt.rs
  • src/jsc/RuntimeTranspilerCache.rs
  • test/bundler/bundler_edgecase.test.ts
  • test/bundler/transpiler/transpiler.test.js
  • test/js/bun/transpiler/transpiler-enum-concat-chain-oom.test.ts

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Sep 8, 2026
@robobun

robobun commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. CI on ffcb911 is green for this diff: 180 of 181 jobs passed, and the one red lane (debian x64-asan) is test/js/node/test/parallel/test-crypto-dh-leak.js, which fails on main too and is tracked separately. The new tests and the carried #38998 tests pass on every lane that ran.

Reproduced on bun 1.4.3 with the fuzz input (enum S { A = "value" } then S.A + "k" repeated): 4 096 pairs take 1 545 MB in an in-process Bun.Transpiler transform, 8 192 terms 1.4 GB under bun run, doubling the terms quadruples the memory. With this branch the same inputs take 9 MB and 22 MB (debug build), and 65 536 pairs take 70 MB.

While reducing it I hit two more symptoms of the same shared rope, both on 1.4.3: enum A { B = "1" + "2", T = `t${B}t` } prints A.B as "12t" and a second template use panics with called Option::unwrap() on a None value, and a member named with */ panics the same way after two + uses. #38998 already had the flat-storage fix for the template case, so this PR carries its two commits and adds the memory regression test and the transpiler cache version bump.

@robobun

robobun commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 7:12 AM PT - Sep 8th, 2026

❌ @robobun, your commit ffcb911 has 1 failures in Build #112876 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 41973

That installs a local version of the PR into your bun-41973 executable, so you can run:

bun-41973 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread test/js/bun/transpiler/transpiler-enum-concat-chain-oom.test.ts
Comment thread src/ast/e.rs Outdated
Comment thread src/ast/fold_string_addition.rs Outdated
Comment thread src/js_parser/visit/visit_stmt.rs Outdated
Comment thread src/jsc/RuntimeTranspilerCache.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants