Clear JSC CodeCache on --hot reload and balance ref_strings refcount - #35588
Clear JSC CodeCache on --hot reload and balance ref_strings refcount#35588robobun wants to merge 5 commits into
Conversation
GlobalObject::reload() cleared the module loader and requireMap but not the VM's CodeCache. CodeCache is keyed by source text, so every reload of an edited file inserted a new UnlinkedModuleProgramCodeBlock entry holding a Strong<> to the old SourceProvider (and its source string). Its prune only fires after ~10s elapsed or ~16 MB accumulated; a tight edit-save loop hits neither, so entries piled up unbounded (heapStats shows UnlinkedModuleProgramCodeBlock climbing exactly +1 per reload). Separately, ref_counted_resolved_source() returned with source_code_needs_deref=false so the +1 from create_external() on a fresh RefString was never balanced. The ref_strings map is designed to be weak (entries self-remove via free_ref_string -> RefString::destroy -> clear_ref_string when the impl refcount hits zero), but with that leaked +1 the duped source bytes and map slot survived forever even after the owning SourceProvider was collected. reload() now calls vm.codeCache()->clear(), and ref_counted_resolved_source now hands off exactly +1 (taking an extra ref on cache hit) with needs_deref=true so SourceProvider::create consumes it. Fixes #11083
|
Warning Review limit reached
Next review available in: 2 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)
Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this touches refcount balance across the Rust↔C++ FFI boundary (ref_strings / ExternalStringImpl / SourceProvider::create's needsDeref gate) plus a VM-wide CodeCache::clear() on every hot reload — worth a human pass on the ownership reasoning.
What was reviewed:
- Traced the impl refcount on both fresh-insert and cache-hit paths through
adopt_wtf_impl(no inc) →toWTFString(ZeroCopy)(+1) →needsDerefderef →~SourceProvider; both now net to zero when the last provider dies. - Checked the other
ref_counted_stringcaller (filesystem_router.rs) — it uses the wrapper that discardswas_new, so its behavior is unchanged. - Confirmed both
ref_counted_resolved_sourcecall sites use::<false>, so theADD_DOUBLE_REFbranch's interaction with the newneedsDeref=trueis dead. - Test buffers stderr to newline framing before
JSON.parse, kills the child, and asserts a heapStats-derived count with a wide margin (2 observed vs <10 threshold vs ~50 unfixed).
Extended reasoning...
Overview
Two changes on the --hot reload path: (1) GlobalObject::reload() now calls vm.codeCache()->clear() after clearing the module registry, and (2) ref_counted_resolved_source() now sets source_code_needs_deref = true and takes an extra ref on cache hits so both fresh and hit paths hand exactly +1 to the C++ consumer. A new regression test drives 50 hot reloads with unique source each time and asserts UnlinkedModuleProgramCodeBlock count stays bounded.
Security risks
None identified. This is a leak fix on a dev-only (--hot) path; no untrusted-input parsing, no auth/crypto.
Level of scrutiny
High. Per the repo's review guidance, native memory safety — specifically "reference counts provably balanced on every terminal path" — is the most-blocked category. The refcount flow here spans three layers (create_external's +1 leaked into impl_, adopt_wtf_impl which does not inc, and SourceProvider::create's toWTFString(ZeroCopy) which does inc followed by the needsDeref-gated deref). My trace says the new balance is correct on both was_new branches, but a wrong-by-one here is a UAF once a SourceProvider is collected while another holder still has the impl, and that only shows under GC pressure. The CodeCache::clear() is straightforward but is a VM-wide behavioral change on every reload.
Other factors
- The PR re-applies #30553, which was closed unmerged during the Rust rewrite rather than approved-then-ported.
- The unused
ADD_DOUBLE_REF=truepath now interacts differently withneedsDeref=true(previously all three refs leaked; now one is balanced) — dead code today, but a human may want it audited or removed. - Test quality is good: newline-framed stderr parsing,
await usingon the subprocess, per-platform timeout branch onisDebug, and a threshold (<10) well separated from both the fixed value (2) and the unfixed value (~50). PR description showsUSE_SYSTEM_BUN=1fails andbun bdpasses, plushot.test.tsandfilesystem_router.test.tsremaining green.
The original test only asserted on heapStats().UnlinkedModuleProgramCodeBlock, which is the JSC-cell side of the leak and goes to ~2 with the CodeCache clear alone. The ref_strings leak is native-heap (Box<[u8]> + ExternalStringImpl + map slot), invisible to heapStats(), so reverting the refcount balance fix in VirtualMachine.rs would have silently passed. Expose VirtualMachine::ref_strings.count() via bun:internal-for-testing and assert it stays <10. Verified: with only the CodeCache clear, maxRefStrings climbs to ~45 and the test fails; with both fixes it sits at 1. Also surface child crash lines instead of silently skipping non-JSON stderr, and drop the stale TODO in hot.test.ts that referenced this leak.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
ref_counted_string previously returned +1 on a fresh insert and +0 on a cache hit, which no caller can consume correctly without also knowing was_new. Move the cache-hit ref into the helper so it always hands back exactly +1. This lets ref_counted_resolved_source drop its was_new special-case, and fixes the identical leak in FileSystemRouter where claim() took an unconditional +1 on top of the fresh-insert +1 and finalize() only released one. Also: trim explanatory comments per review, drop the defensive typeof guard in the test (a missing binding now fails loudly via the crash regex + toBeGreaterThan(0) instead of silently passing).
There was a problem hiding this comment.
Thanks — all three points from the earlier pass are addressed: the +1-always invariant now lives in ref_counted_string itself (so claim correctly drops its extra ref), the test calls refStringsCount() unconditionally with a > 0 floor, and the comments are down to the durable one-liners. I traced the refcount at both call sites (fresh vs. cache-hit, including MatchedRoute::init's independent +1) and it balances. Deferring to a maintainer for the contract change on ref_counted_string and the per-reload codeCache()->clear() tradeoff rather than bot-approving a refcount change on the module-load path.
What was reviewed:
- Confirmed
ref_counted_stringhas exactly two callers (ref_counted_resolved_source,filesystem_router::claim); both consume the new always-+1 contract correctly. adopt_wtf_impladopts (no inc), sosource_code_needs_deref = trueis the single balancing deref — no double-free on cache hit.MatchedRoute::initstill takes its own refs on origin/base_dir/asset_prefix, so droppingclaim'sref_()doesn't under-count when a route outlives its router.
Extended reasoning...
Overview
Re-review after the author addressed prior feedback. The PR fixes two coupled leaks on the --hot reload path: (1) ref_counted_string returned +1 on fresh insert but +0 on cache hit, and callers didn't distinguish, leaking the create_external +1; (2) JSC's CodeCache accumulates one UnlinkedModuleProgramCodeBlock per unique source and its prune heuristic never fires under a tight edit loop. The fix pushes the +1-always invariant into ref_counted_string (so both callers — ref_counted_resolved_source and FileSystemRouter::claim — consume it uniformly) and adds vm.codeCache()->clear() to GlobalObject::reload(). A new refStringsCount() internal-for-testing binding lets the regression test observe the native-heap map size independently of heapStats().
Prior feedback resolution
All three earlier inline comments are resolved: (a) the sibling filesystem_router.rs leak is fixed at the shared-helper layer per REVIEW.md's whole-class rule; (b) the typeof refStringsCount === "function" guard is gone and expect(maxRefStrings).toBeGreaterThan(0) guards against silent rot; (c) the multi-line bug-history comments are squeezed to single-line invariant/SAFETY notes and comment-cop's re-fires are on the pre-squeeze commit (all threads resolved).
Security risks
None identified. This is a leak fix; no user-controlled input reaches new parsing or allocation paths, and the new binding is gated behind bun:internal-for-testing.
Level of scrutiny
High. Changing the return contract of a refcount helper on the module-source interning path is exactly the class where an off-by-one is a UAF or a permanent leak. I verified there are only two call sites of ref_counted_string in the tree (both updated), that adopt_wtf_impl adopts without incrementing (so source_code_needs_deref = true is the single balancing deref, not a double-free on cache hit), and that MatchedRoute::init still takes its own +1 on the router's BackRefs so a matched route outliving its router stays sound after claim dropped its ref_(). The codeCache()->clear() addition is placed after moduleLoader->clearAll() / requireMap()->clear() so every module re-fetches anyway; the re-parse cost rationale in the PR description holds.
Other factors
Deferring rather than approving because (1) refcount-contract changes on the module-load path are the canonical "maintainer should look" category in this repo, and (2) clearing the entire JSC CodeCache on every reload is a deliberate perf/memory tradeoff (vs. e.g. a targeted delete or letting prune run) that a maintainer should ratify. Test coverage is solid: the regression test drives 50 unique-content reloads and asserts both counters stay bounded, with the PR's own table showing each half of the fix independently fails when reverted.
StatusThe diff is green. Seven failures on build #80919, all tagged flaky by the CI helper and unrelated to this change:
This PR only touches Ready for a maintainer pass on the refcount contract change and the |
|
Superseded by #36675, which combines this with the other |
…odeCache clear, ref_strings balance) Fixes #11083 Fixes #14734 Supersedes and combines #36251 and #35588. Three independent per-reload leaks under --hot: Resolver DirEntry orphaning (the ~5x canary-vs-1.3.14 regression): bust_entries_cache() only dropped the hash->index BSSMap mapping, orphaning the boxed DirEntry, its .data StringHashMap, its open fd, and all Entry slots it referenced in the append-only EntryStore slab. The next dir_info_cached_miss() allocated everything fresh. The Rust port added reserve(64) on the fresh map so every orphaned DirEntry leaked a fixed 3.2 KB hashbrown table regardless of actual directory size (Zig 1.3.14 started from zero capacity and leaked only what it touched). Fix: bust_entries_cache() now marks the DirEntry .stale instead of removing the mapping; the refresh sites (dir_info_cached_miss, dir_info_for_resolution, read_directory_with_iterator, entries_at_locked) see the existing slot and rewrite it in place, carrying the fd forward and passing prev_map so Entry slots are reused. The unadopted freshly-opened dir fd is released. JSC CodeCache accumulation: CodeCache is keyed by source text, so every edited reload inserted a fresh UnlinkedModuleProgramCodeBlock holding a Strong<> to the old SourceProvider and source string; its prune only fires after ~10s / ~16 MB / 2000 entries, which a fast edit loop does not hit. Fix: GlobalObject::reload() clears it alongside the module registry. ref_strings refcount imbalance: ref_counted_string() returned +1 on insert, +0 on hit; ref_counted_resolved_source() set source_code_needs_deref=false, so the fresh-insert +1 was never balanced and the ExternalStringImpl + duped source bytes could never reach zero even after the SourceProvider was collected. FileSystemRouter's claim() had the same shape for origin/base_dir/asset_prefix. Fix: ref_counted_string() now always returns exactly +1 (extra ref on hit); ref_counted_resolved_source() sets source_code_needs_deref=true, and FileSystemRouter::claim drops its extra ref. Watcher::add_file's already-watched branch on Linux closes the descriptor it just replaced (gated on st_nlink>0 so releasing an unlinked inode cannot deliver a deferred IN_DELETE_SELF on the item's still-registered watch). Adds hotReloadDiagnostics() to bun:internal-for-testing (refStrings, sourceMappings, watchlistLen, hotReloadCounter) for the regression test. Adds an optional per-size-bucket live-byte histogram wrapper around the Rust global allocator (BUN_TRACK_ALLOC=1 at build time) for future investigations. Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
Fixes #11083
Fixes #14734
Re-applies #30553 (closed unmerged when the Rust rewrite moved the files) against the current tree, with the refcount invariant pushed into the shared helper so it also covers
FileSystemRouter.Reproduction
heapStats().objectTypeCounts.UnlinkedModuleProgramCodeBlockclimbs exactly +1 per reload with no upper bound; RSS grows linearly.Cause
Two leaks on the
--hotreload path whenever the edited file's content changes. They are not independent: the refcount fix only drains once the SourceProvider is released, which needs the CodeCache entry gone.ref_stringsleaks its initial refref_counted_string()returned +1 on a fresh insert (fromString::create_external()) but +0 on a cache hit.ref_counted_resolved_source()setsource_code_needs_deref = false, so the fresh-insert +1 was never balanced. Theref_stringsmap is weak (entries self-remove viafree_ref_string -> RefString::destroy -> clear_ref_stringwhen the impl refcount hits zero), but with that leaked +1 theExternalStringImplrefcount could never reach zero, so the duped source bytes and the map slot survived forever even after the owningSourceProviderwas collected.FileSystemRouterhad the same bug:claim()took an unconditional +1 on whateverref_counted_stringreturned, so a fresh origin/base_dir/asset_prefix leaked +1 afterfinalize()released its single hold.JSC CodeCache accumulates stale entries
JSC::CodeCacheis keyed by source text (SourceCodeKey= hash + length + flags). Every reload of an edited file produces a different transpiled string, sogetUnlinkedModuleProgramCodeBlockmisses and inserts a fresh entry holding aStrong<UnlinkedModuleProgramCodeBlock>, whose key keeps the oldZig::SourceProvider(and its sourceStringImpl) alive viaRefPtr.The cache's
prune()only runspruneSlowCase()when ~10s have elapsed, or the working set grew by ~16 MB of source text, or it hit 2000 entries. A tight edit-save loop hits none of these, so one entry is added per reload and none are evicted. The cap is in source bytes, not code-block overhead, so 16 MB of pinned source can hold substantially more inUnlinkedModuleProgramCodeBlock+SourceProviderobjects.Fix
ref_counted_string()now always returns exactly +1 (takes an extra ref on cache hit). Callers consume it uniformly:ref_counted_resolved_sourcesetssource_code_needs_deref = true, andFileSystemRouter::claimdrops its extra ref.GlobalObject::reload()callsvm.codeCache()->clear()alongsidemoduleLoader()->clearAll()/requireMap()->clear(). Every module already goes through fetch + transpile afterclearAll(), so the added JSC re-parse is a fraction of the existing per-reload work; it trades that for not pinning up to ~2000 stale code blocks between prunes.Verification
refStringsCount()is exposed viabun:internal-for-testingso the test can observe the native-heap side (ref_strings entries areBox<[u8]>+ExternalStringImpl, invisible toheapStats()).test/regression/issue/11083.test.tsdoes 50 reloads with unique content each time and asserts both the max liveUnlinkedModuleProgramCodeBlockcount and theref_stringscount stay under 10. With the fix both sit at 1-2; reverting either half drives its counter to ~50.test/cli/hot/hot.test.ts(12 tests) andtest/js/bun/util/filesystem_router.test.ts(29 tests, including the UAF guard) remain green.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/hot/hot.test.ts