Conversation
Zig::SourceProvider::create picked the CachedBytecode destructor from resolvedSource.needsDeref, but the same function clears needsDeref a few lines earlier for every non-builtin source, so the destructor was always the no-op one and the Box<[u8]> read from a `.jsc` sidecar leaked on every load of a `// @Bun @bytecode` module. needsDeref cannot be used for this anyway: it is also true for the Node compile cache paths, whose blobs are owned by the cache's entry map and must not be freed. Add an explicit bytecode_cache_needs_free bit to ResolvedSource (Rust and C mirrors), set it in the two sidecar producers, and make every consumer honor it: SourceProvider::create hands an owned blob to the CachedBytecode destructor and clears the bit; ResolvedSourceCodeHolder and the overridden module._compile path free a blob that never reached a SourceProvider; OwnedResolvedSource::drop frees it on the Rust side. Compile cache and standalone executable blobs leave the bit clear and keep no destructor.
|
Updated 12:41 PM PT - Aug 13th, 2026
❌ @robobun, your commit 9e80222 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38138That installs a local version of the PR into your bun-38138 --bun |
|
Status
|
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
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 (9)
WalkthroughThe change adds bytecode cache ownership metadata to resolved sources, releases owned Rust allocations across module-loading paths, and adds regression coverage for repeated sidecar loading and garbage collection. ChangesBytecode cache lifetime
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also checked: the Node compile cache producer (jsc_hooks.rs:3322) and the standalone-executable producer (jsc_hooks.rs:4036) leave bytecode_cache_needs_free at its false default, so borrowed blobs still get no destructor; and SourceProvider::create clears the flag on the by-reference resolvedSource before the constructor copies it into m_resolvedSource, so neither the provider's stored copy nor the caller's ResolvedSourceCodeHolder can free the blob a second time.
Extended reasoning...
Verified the two borrowed-blob producers do not set the new flag and that the flag-clear in SourceProvider::create happens before the struct is copied into the provider, ruling out double-free on both the provider-owned copy and the holder. The inline nit already covers the one uncovered early-return; nothing further to add.
A non-callable module._compile returned before reaching the free. Cover it with a test variant, and size each variant's threshold by how many sidecars a load reads.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Overlaps with #39405, which maps the sidecar (and the module text) instead of reading it into a heap buffer. That PR needs the same three release points as this one (CachedBytecode destructor, ResolvedSourceCodeHolder, the overridden module._compile branch, plus OwnedResolvedSource::drop), so the needs-free bit here becomes the owning file pointer there. If #39405 lands first this PR becomes unnecessary; if this one lands first, #39405 rebases on top of it. |
This branch has not been deployed
1 participant
Problem
bun build --target=bun --bytecodeleaks its.jscsidecar: RSS grows by exactly the sidecar size perrequire()/import()(3.5 MB sidecar, 100 loads withdelete require.cachein between: +340 MB; the same module built without--bytecodestays flat). Long-lived processes that reload such modules (require.cachebusting, repeated workers) never get that memory back.Zig::SourceProvider::create(src/jsc/bindings/ZigSourceProvider.cpp) chose theCachedBytecodedestructor withresolvedSource.needsDeref ? free : noop, but the same function setsneedsDeref = falseabout 30 lines earlier for every non-builtin source (that is thesource_codederef). So the destructor was always the no-op and theBox<[u8]>thattranspile_source_code_inner(src/runtime/jsc_hooks.rs) andTranspilerJob(src/jsc/RuntimeTranspilerStore.rs) hand over viaheap::into_rawwas never freed.needsDerefcannot be the signal even when read at the right time: it is alsotrueon the Node compile cache paths (node_compile_cache::fetch), whose blobs belong to the cache's entry map for the life of the process. The compile cache currently relies on the destructor being a no-op (this was called out and dismissed for that reason in the review of node compat batch: callback-throw dispatch, Assert class + native deep-equality parity, Intl gate + URL/buffer fallout, compile cache, watch kill-signal, profilers (+98 tests) #34660), so the two cases need a dedicated bit.SourceProvider(theimport()of a file thatrequire()already put in the require map re-reads the file and then reuses the existing module; an overriddenmodule._compileonly receives the source text) was dropped on the floor too, andOwnedResolvedSource::drop(src/jsc/ResolvedSource.rs) did not free it either.Fix
bytecode_cache_needs_freetoResolvedSource(Rust struct and the C mirror in headers-handwritten.h; it lands in existing padding next toneedsDeref/already_bundled, so the layout is otherwise unchanged). The two sidecar producers set it; everything else leaves the defaultfalse.SourceProvider::createinstalls the free destructor only when the bit is set and clears the bit as it hands the blob to theCachedBytecode, so neither the copy stored in the provider nor the caller's struct can free it again. Borrowed blobs (compile cache, standalone executables) get no destructor, which is what they got before.Zig::freeOwnedBytecodeCache()releases a still-owned blob fromResolvedSourceCodeHolder(every fetch path in ModuleLoader.cpp) and from the overridden_compilebranch ofevaluateWithPotentiallyOverriddenCompile;OwnedResolvedSource::dropdoes the same on the Rust side.Box<[u8]>from Rust's global allocator, soBun::defaultAllocatorFree(the destructor already used here) and reconstructing theBox<[u8]>frombytecode_cache/bytecode_cache_sizeare the matching frees; the bit travels with the struct and is cleared by whoever takes the blob, so each blob is freed exactly once; and ownership is now stated by the producer instead of being inferred from an unrelated string refcount flag, which is what let the compile cache and sidecar cases collide.test/bundler/bun-build-api.test.ts("bytecode output does not leak the .jsc sidecar when the module is released"): five variants covering both producers and all three release points, includingmodule._compileoverridden with a non-function, which returns early from that branch. Each variant loads a module with a 0.61 MB sidecar 60 times and requires RSS growth to stay under two thirds of one sidecar per load (24.5 MB): it grows 2 to 12 MB with this change and 40 to 80 MB without it (debug build; 41 to 44 MB on the 1.4.0 release for the plain variants). TheOwnedResolvedSource::droppath is only reachable when a transpiler job is torn down before reaching C++, so it has no test.test/regression/issue/26298.test.ts(standalone executables with embedded bytecode) and the compile cache tests intest/js/node/module/node-module-module.test.json the debug (ASAN) build to confirm the borrowed-blob paths still get no destructor.bytecode_cachefield; the two changes are independent.Background
bun build --target=bun --bytecodeemitsout.jsstarting with// @bun @bytecode @bun-cjsplusout.js.jsc, JSC's serialized bytecode for it. When the runtime loadsout.js, the parser stops at the pragma and the transpiler reads the sidecar into aBox<[u8]>; the bytes are returned to C++ inResolvedSource.bytecode_cache.ResolvedSourceis the plain#[repr(C)]struct the Rust module loader fills in for C++ (source text, specifier, bytecode, flags). It isCopy, so it carries no destructor;OwnedResolvedSourceis the Rust-side RAII wrapper used while one is in flight, andResolvedSourceCodeHolderis the C++ scope guard that releases what C++ did not consume.Zig::SourceProvideris Bun'sJSC::SourceProviderfor a module. When bytecode is present it wraps the bytes in aJSC::CachedBytecode, which JSC decodes instead of parsing;CachedBytecodetakes a destructor callback that runs when it is released, and a null callback means the bytes belong to someone else. Bun has three sources of such bytes: the.jscsidecar (a fresh heap allocation per load), the Node compile cache (process-lifetime entries), and the bytecode section of abun build --compileexecutable (part of the binary image). Only the first one is the provider's to free.Reproduction used to measure the leak
bun 1.4.0, 3.5 MB sidecar:
340.7 MB(3.4 MB per load); the same module built without--bytecode: 15 to 22 MB of allocator noise at every checkpoint, no growth.import()instead ofrequire()leaks the same way (222 MB over 60 loads), andBUN_DEBUG_AsyncModule=1confirms those loads go through the RuntimeTranspilerStore producer.