bake: release the strings production.rs returns to the module loader hooks

[robobun]

Problem

• Strings that bake's Rust code creates for its C++ entry points, and a few it creates for itself, are never released. LeakSanitizer (with Malloc=1, see Background) reports them on a two page bun build --app as, for example:

  Direct leak of 294 byte(s) in 7 object(s) allocated from:
      ... BunString__fromBytes -> bun_core::string::String::clone_utf8 -> create_format
      BakeProdResolve                  src/runtime/bake/production.rs:1371
      Bake::bakeModuleLoaderResolve    src/runtime/bake/BakeGlobalObject.cpp:71
  Direct leak of 80 byte(s) in 2 object(s) allocated from:
      ... BunString__createExternal -> OutputFile.rs to_bun_string_ref
      BakeProdLoad                     src/runtime/bake/production.rs:1629
      Bake::bakeModuleLoaderFetch      src/runtime/bake/BakeGlobalObject.cpp:135
  Direct leak of 48 byte(s) in 2 object(s) allocated from:
      ... BunString__fromBytes -> clone_utf8
      build_with_vm                    src/runtime/bake/production.rs:1107
  

  (all 23 records from the two test scenarios are in the details block below; every leaked string in a production build is one of these.)
• Cause, same mistake at every site: a bun_core::String (BunString) holding a WTF::StringImpl is a plain struct whose reference has to be released by exactly one consumer. These consumers used the borrowing conversions instead, which take a reference of their own and leave the one they were handed:
  • src/runtime/bake/BakeGlobalObject.cpp: the results of BakeProdResolve (lines 48 and 74), BakeProdLoad (140) and, on Windows, BakeToWindowsPath (162) read with toWTFString(). One leak per import edge resolved and per chunk loaded while prerendering. The referrer-less branch at line 86 already used transferToWTFString().
  • src/runtime/bake/BakeSourceProvider.cpp: BakeLoadServerHmrPatch and BakeLoadServerHmrPatchWithSourceMap (lines 69 and 93) read the patch source with toWTFString(). DevServer.rs (finalize_bundle, lines 4257 and 4275) hands them a fresh clone_utf8/clone_latin1 copy of the whole server bundle and never touches it again, so the dev server leaked one copy of the server bundle per server-side hot update.
  • src/runtime/bake/production.rs: the config path (line 336), client entry URLs (828), CSS chunk URLs (932) and route patterns (1107) were converted with the borrowing to_js() (compare the transfer_to_js() directly below at 1124), and the module keys (746) were kept in a plain Vec<BunString>, which frees nothing when dropped.

Fix

• C++ consumers of Rust-created strings consume them with transferToWTFString() (six sites in the two files above). production.rs converts its temporaries with transfer_to_js() and holds the config path and PerThread.module_keys in OwnedString, the RAII wrapper the rest of bake (DevServer.rs, bake_body.rs, FrameworkRouter.rs) already uses.
• Correct because transferToWTFString() / transferToJS() (src/jsc/bindings/BunString.cpp) build the WTF::String or JSString and then drop the BunString's own reference, so the string ends up owned only by the module key, SourceProvider, or JS string it was converted into; OwnedString drops the reference when the owner goes away. The non-owned values these paths can carry are unaffected: transferToWTFString() on the static builtin alias BakeProdResolve returns takes the same toStringStatic path line 74's toWTFString(ZeroCopy) already took, transferToJS() and toJS() treat Empty/Dead identically, and the thrown cases return before any conversion.
• The DevServer.rs wrappers for the two HMR patch entry points now take an OwnedString and hand its reference to C++ with into_inner(), so the type says the callee releases the string; finalize_bundle, their only caller, wraps the copies it makes. BakeLoadInitialServerCode keeps a plain BunString because it receives a static string.
• Verified with the two tests added to test/bake/dev/production.test.ts (ASAN builds only; LSan does not exist on Windows). Each builds a two page app under LeakSanitizer, checks that the leak scan ran (log_threads=1 makes it announce itself), and fails on any leak record that has bake's string machinery (BunString__* / bun_core::string) on its stack, reduced to the frames that created the string:
  • "after a successful build": pages rendered; fails before with 4 records (both module loader hooks, route patterns), passes after.
  • "after a failed build tears the VM down": one page throws, BUN_DESTRUCT_VM_ON_EXIT=1. Only that exit path tears the VM down (production.rs build_command), which is what makes the chunk sources, config path and client entry URL visible to LSan. Fails before with 19 records, passes after.
  • bun bd test --timeout 90000 test/bake/dev/production.test.ts: 11/11 (the pre-existing tests need more than the 5s local default on a debug build; CI passes its own timeout). On CI's x64-asan lane the file passes 11/11 in about 12s (builds 97734 and 98233).
  • The dev server hunks are not leak-tested: observing a server hot update under LSan needs a dev server driven outside the bake harness, which treats sanitizer output as a crash. test/bake/dev/server-sourcemap.test.ts (5/5 under the ASAN debug build) exercises BakeLoadServerHmrPatchWithSourceMap across repeated reloads and the provider's destruction at exit, so the string now being owned solely by the provider is exercised; the plain variant differs only in the provider class, which the production tests cover.
• Overlap with open PRs: Decode module text that comes from disk or bundler output as UTF-8 #38714 carries the BakeProdLoad hunk and the two BakeSourceProvider.cpp hunks incidentally (identical lines) and also edits the clone_latin1 call in finalize_bundle that this PR wraps in OwnedString; bake: check for exceptions in the production build's module helpers #38949 rewrites the import() line of BakeGlobalObject.cpp while keeping toWTFString() there. Whichever lands second needs a one-line rebase in each case, and the end state should keep the transferring calls everywhere. This PR is the one that is about the ownership bug and carries the test for it.

Background

• BunString (bun_core::String in Rust) is the string type shared across the Rust/C++ boundary. With the WTFStringImpl tag it points at a refcounted WTF::StringImpl, and the value itself is Copy with no destructor: a reference it holds is released only by explicit code. A Rust function returning one by value, or passing one by value to C++, hands its +1 reference to the receiver.
• toWTFString() / to_js() copy a string out by adding a reference (for strings the caller does not own); transferToWTFString() / transfer_to_js() do the same and then release the BunString's reference (for strings it does own). OwnedString is the Rust RAII wrapper that releases on drop.
• Bake::GlobalObject is the JS global only bun build --app uses to prerender routes. Its module loader hooks map bake:/... specifiers onto the server chunks the bundler just produced: bakeModuleLoaderResolve and bakeModuleLoaderImportModule turn a specifier plus referrer into a key via BakeProdResolve; bakeModuleLoaderFetch gets a chunk's source from BakeProdLoad. The dev server instead evaluates each incremental server bundle through the HMR patch entry points in BakeSourceProvider.cpp.
• LeakSanitizer only sees allocations made through the system allocator. WTF strings normally come from bmalloc, which it does not track; the Malloc=1 environment variable makes bmalloc route through the system allocator (bmalloc's Environment.cpp checks it unconditionally). Identifier::fromString interns its string in the per-thread atom table, so the first resolution of each key stays reachable through that table and only repeat resolutions (every shared chunk) show up as records; the tests' two pages import the same chunks for that reason.

Leak records from the unfixed build for the two test scenarios

Successful build (4 records):

Direct leak of 294 byte(s) in 7 object(s)   BakeProdResolve production.rs:1371 <- bakeModuleLoaderResolve BakeGlobalObject.cpp:71
Direct leak of 42 byte(s) in 1 object(s)    BakeProdResolve production.rs:1371 <- bakeModuleLoaderResolve BakeGlobalObject.cpp:71
Direct leak of 42 byte(s) in 1 object(s)    BakeProdResolve production.rs:1371 <- bakeModuleLoaderImportModule BakeGlobalObject.cpp:45
Direct leak of 21 byte(s) in 1 object(s)    build_with_vm production.rs:1107 (route pattern)

Failed build with BUN_DESTRUCT_VM_ON_EXIT=1 (19 records):

Direct leak of 168 byte(s) in 4 object(s)   BakeProdResolve production.rs:1371 <- bakeModuleLoaderResolve BakeGlobalObject.cpp:71
Direct leak of 126 byte(s) in 3 object(s)   BakeProdResolve production.rs:1371 <- bakeModuleLoaderResolve BakeGlobalObject.cpp:71
Direct leak of 42 byte(s) in 1 object(s)    BakeProdResolve production.rs:1371 <- bakeModuleLoaderImportModule BakeGlobalObject.cpp:45   (x2 records)
Direct leak of 80 byte(s) in 2 object(s)    to_bun_string_ref OutputFile.rs:169 <- BakeProdLoad production.rs:1629 <- bakeModuleLoaderFetch BakeGlobalObject.cpp:135
Direct leak of 40 byte(s) in 1 object(s)    same BakeProdLoad stack                                                                      (x5 records)
Indirect leak of 16/32 byte(s)              same BakeProdLoad stack (the ExternalStringImpl's callback storage)                          (x6 records)
Direct leak of 48 byte(s) in 2 object(s)    build_with_vm production.rs:1107 (route patterns)
Direct leak of 45 byte(s) in 1 object(s)    build_with_vm production.rs:336  (config path)
Direct leak of 37 byte(s) in 1 object(s)    build_with_vm production.rs:828  (client entry URL)

With this branch both scenarios report zero records with string machinery on the stack; the remaining LSan output is unrelated process-lifetime bundler state, which is why test/bake/dev/production.test.ts stays on test/no-validate-leaksan.txt and the tests filter instead of asserting an empty report.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 20 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 59a3e2f6-b742-449b-b3a4-7a70479163b0

📥 Commits

Reviewing files that changed from the base of the PR and between 63ffef8 and 2498432.

📒 Files selected for processing (5)

• src/runtime/bake/BakeGlobalObject.cpp
• src/runtime/bake/BakeSourceProvider.cpp
• src/runtime/bake/DevServer.rs
• src/runtime/bake/production.rs
• test/bake/dev/production.test.ts

---

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

[robobun]

^{Updated 12:13 PM PT - Aug 15th, 2026}

🔄 @robobun, the build for your commit 24984324 (Build #98233) was cancelled — waiting for the next build...

[robobun]

Status: fix pushed (2498432); the diff is green on every lane that exercises it, ready for a maintainer.

Reproduced on an unfixed debug (ASAN) build by running bun build --app on a two page react app with Malloc=1 ASAN_OPTIONS=detect_leaks=1. LeakSanitizer reports every string the build created and never released: the bake:/... keys from BakeProdResolve (both module loader hooks), the route patterns, and, once a failing build tears the VM down under BUN_DESTRUCT_VM_ON_EXIT=1, the chunk sources from BakeProdLoad, the config path and the client entry URL (4 and 19 records; listed in the PR body). With this branch both runs report none. The two tests in test/bake/dev/production.test.ts encode those runs and fail on the unfixed build.

Revision history: the first revision fixed the four BakeGlobalObject.cpp sites; the second extended the fix to the same mistake in the dev server's HMR patch entry points and in production.rs's own strings, and widened the tests to the whole class; the third encodes the HMR ownership in the wrapper types instead of comments.

CI for 2498432 (#98233): 176 jobs green, including all 20 x64-asan shards (production.test.ts 11/11 in about 12s). The one red job is test/bake/deinitialization.test.ts segfaulting on Windows 2019 x64 during dev server teardown; that test runs an HTML-only dev server, which never reaches the code this PR touches (the HMR patch path is behind !frontend_only && server_graph.current_chunk_len > 0, the rest is bun build --app only), it passed on this PR's first revision, and it is the same intermittent failure seen on main (build 96743); reported for triage. The two darwin-aarch64 test jobs are again waiting for an agent, as they did for the whole of the first revision's build.

[claude]

LGTM — mechanical leak fix that brings four call sites in line with the existing transferToWTFString() pattern already used in the same file.

What was reviewed:

• Verified transferToWTFString() in BunString.cpp handles all three tags these functions return: WTFStringImpl (adds ref, then derefs the BunString's own — net transfer), StaticZigString (via toStringStatic, same as the old ZeroCopy path, so the builtin-alias case at production.rs:1351 is unaffected), and Dead (guarded before the call in bakeModuleLoaderFetch).
• Confirmed BakeProdResolve/BakeProdLoad/BakeToWindowsPath each return a +1-owned BunString on their non-static/non-dead paths (create_format, to_bun_string_ref, clone_utf16), and that the throw path returns dead() so RETURN_IF_EXCEPTION before the transfer does not leak.
• The referrer-less branch of bakeModuleLoaderResolve already used transferToWTFString(); this makes the other four sites consistent.

Extended reasoning...

Overview

Four one-token substitutions in src/runtime/bake/BakeGlobalObject.cpp (toWTFString() → transferToWTFString()) at the sites consuming BunString results from BakeProdResolve, BakeProdLoad, and BakeToWindowsPath, plus a comment on the extern declarations documenting the ownership contract. Two ASAN-only LSan regression tests in test/bake/dev/production.test.ts that build a two-page app under Malloc=1 + detect_leaks=1 and assert no leak records rooted in BakeProd(Resolve|Load).

Security risks

None. This is a per-import-edge string leak in the bun build --app prerender VM (a build-time tool), not a runtime request path. No user input handling, auth, or crypto is touched.

Level of scrutiny

Medium — refcount management across the Rust/C++ FFI boundary is exactly the class REVIEW.md flags. But the change is narrow and I verified the semantics directly:

• transferToWTFString() (BunString.cpp:820-846) constructs WTF::String(impl.wtf) (which refs) then impl.wtf->deref(), netting a transfer of the caller's +1 into the returned WTF::String. For StaticZigString it takes the same toStringStatic path the previous toWTFString(ZeroCopy) call took, so BakeProdResolve's static builtin-alias return is unchanged.
• BakeProdResolve (production.rs:1339-1379) returns static_(), dead() (on throw, before the transfer runs), or create_format() — the last is the only owned case. BakeProdLoad returns to_bun_string_ref() (owned ExternalStringImpl) or dead() (guarded by source.tag != Dead before the transfer). BakeToWindowsPath returns clone_utf16() (owned).
• The fifth sibling site in the same file (the referrer-less bakeModuleLoaderResolve branch) already used transferToWTFString(), so this is convergence to an established local pattern, not a novel one.

Other factors

The tests are well-constructed per the repo's leak-test conventions: they filter LSan output to only records allocated inside BakeProd* (ignoring unrelated process-lifetime bundler state and the LSan-forced exit code), assert the build actually exercised the code paths (rendered HTML / thrown error present) before checking for zero leaks, run test.concurrent, skip on non-ASAN and Windows, and justify the 60s timeout (llvm-symbolizer against the debug binary). The PR description's analysis matched the code at every point I checked. The noted overlap with #38714/#38949 is a rebase concern, not a correctness one.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes refcount ownership at several Rust↔C++ FFI sites (BunString → WTFString transfer, plus OwnedString/transfer_to_js on the Rust side and the two DevServer HMR entry points), a human look is still worthwhile.

What was reviewed:

• transferToWTFString() handles all three tags BakeProdResolve returns (WTFStringImpl derefs after wrapping, StaticZigString goes through toStringStatic, and the thrown/dead case is short-circuited by RETURN_IF_EXCEPTION).
• OwnedString::drop calls String::deref(), which is a no-op for Tag::Dead, so the placeholder module_keys slots and Vec reassignment don't over-release.
• Both DevServer HMR callers pass clone_utf8/clone_latin1 (a fresh +1), so consuming with transferToWTFString() on the C++ side is balanced.
• The comment-cop flags look like false positives — the added comments are two/three-line FFI ownership contracts (which REVIEW.md asks for), not workaround justifications.

Extended reasoning...

Overview

The PR fixes WTF::StringImpl refcount leaks in bun build --app's prerender module loader. Four call sites in BakeGlobalObject.cpp and two in BakeSourceProvider.cpp are switched from toWTFString() (which adds a ref, leaving the caller's +1 orphaned) to transferToWTFString() (which adopts the caller's +1). On the Rust side, production.rs wraps several long-lived BunStrings in OwnedString so their +1 is released on drop, and switches short-lived ones from .to_js() to transfer_to_js(). DevServer.rs gains doc comments on the HMR wrappers stating the C++ side now consumes the passed string. Two ASAN-only LSan tests are added to test/bake/dev/production.test.ts.

Security risks

None. This is refcount bookkeeping on strings the process itself created; no untrusted input handling or auth surface changes.

Level of scrutiny

High. Refcount ownership across the Rust/C++ boundary is REVIEW.md's "most-blocked category," and the correctness of each site depends on what the producing function actually returns. I verified: transferToWTFString() in BunString.cpp:820-846 refs-then-derefs for WTFStringImpl and uses toStringStatic for StaticZigString, matching the two non-thrown paths of BakeProdResolve; OwnedString in bun_core/string/mod.rs:1172-1222 calls self.0.deref() on drop, which is a no-op for non-WTFStringImpl tags (so the dead() placeholders and index reassignment are safe); the DevServer HMR callers at DevServer.rs:4257,4275 pass clone_utf8/clone_latin1, so the new transferToWTFString() on the C++ side is balanced. The remaining .to_js() reads on module_keys[i] and config_entry_point_string go through Deref to &String and don't consume, so the OwnedString drop is the single release.

Other factors

• Three comment-cop bot comments flag the added code comments. Those comments are 2–3 line FFI ownership contracts, which REVIEW.md explicitly asks for ("comment it when non-local, especially across FFI"), so they read as false positives — but the author may want to trim them to satisfy the linter.
• The new tests parse LSan output with regexes and depend on Malloc=1 + log_threads=1 behavior; that's a somewhat brittle harness a human should sanity-check for CI stability, though the describe.skipIf(!isASAN || isWindows) gate keeps it off non-ASAN lanes.
• The PR description notes overlap with #38714 and #38949; whichever lands second needs a one-line rebase.
• CI is reported green on 177 jobs including the x64-asan lane where the new tests run.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes refcount ownership across the Rust/C++ FFI boundary (the most-blocked category per REVIEW.md), a human look would still be worthwhile.

What was reviewed:

• Verified transferToWTFString() (BunString.cpp:820) handles every tag BakeProdResolve/BakeProdLoad/BakeToWindowsPath can return — StaticZigString for the builtin-alias path, WTFStringImpl for create_format/clone_utf16/to_bun_string_ref's ExternalStringImpl, and Dead/Empty fall through to an empty string — so no borrowed value is over-released.
• Verified OwnedString::into_inner() disarms Drop via ManuallyDrop, so the DevServer.rs → BakeLoadServerHmrPatch* handoff is single-owner with no double-free; from_js_host_call always invokes its closure.
• Verified Vec<OwnedString> for module_keys: the initial dead() slots deref to a no-op, overwriting a slot correctly drops the previous value, and the remaining .to_js() reads go through Deref<Target=BunString> without consuming.
• The comment-cop bot flags look like false positives — the added comments are one-line FFI ownership notes, which REVIEW.md asks for.

Extended reasoning...

Overview

The PR fixes WTF string leaks in bun build --app's prerender module loader by switching four toWTFString() sites in BakeGlobalObject.cpp and two in BakeSourceProvider.cpp to transferToWTFString(), switching three to_js() sites in production.rs to transfer_to_js(), and wrapping module_keys/config_entry_point_string and the DevServer HMR patch sources in OwnedString so their +1 is released on Drop. Two ASAN-gated LSan tests are added that fail on the unfixed build and pass with the fix.

Security risks

None. This is refcount bookkeeping on strings created and consumed inside a single build process; no untrusted input, auth, or network surface is touched.

Level of scrutiny

High. REVIEW.md calls out native memory safety as the most-blocked category, and every change here is about who releases which +1 across the Rust/C++ boundary. I traced each producer (create_format, clone_utf8/16, to_bun_string_ref, static_, dead) through transferToWTFString()'s tag switch and confirmed none over-releases; I checked OwnedString (new/into_inner/Drop) and BunString::deref() (no-op unless WTFStringImpl) so the Vec<OwnedString> initialization with dead() and later slot assignment are safe; and I confirmed from_js_host_call unconditionally calls its closure so into_inner() always runs and the C++ side always reaches transferToWTFString() before anything can throw. Everything checks out, but a mistake here is a UAF, so it merits maintainer sign-off.

Other factors

CI is green on 177 lanes including x64-asan where the new LSan tests run. The comment-cop bot left three "paragraph-long comment" flags, but the added comments are one-liners documenting FFI ownership — exactly what REVIEW.md asks for ("comment it when non-local, especially across FFI") — so they read as false positives; a maintainer should confirm and dismiss. The PR description also notes overlap with #38714 and #38949 that will need a one-line rebase whichever lands second.