resolver: reuse BSSMap slots and PackageJSON/TSConfigJSON on bustDirCache#29922
resolver: reuse BSSMap slots and PackageJSON/TSConfigJSON on bustDirCache#29922robobun wants to merge 11 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds reclaimable backing-store slots to the allocator; changes resolver cache repopulation to reuse prior ChangesAllocator: reclaimable backing-store slots
Resolver: reuse parsed DirInfo JSON + regression test
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 9:51 AM PT - May 4th, 2026
❌ @robobun, your commit 3baec36 has some failures in 🧪 To try this PR locally: bunx bun-pr 29922That installs a local version of the PR into your bun-29922 --bun |
|
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}
robobun
force-pushed
the
farm/533bc3a0/fix-bustdircache-leak
branch
from
5ed258c to
0c53fab
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/allocators.zig`:
- Around line 697-700: The code is silently dropping errors from
reclaimable.put() (using catch {}), which can leak orphaned slots; replace the
silent catch with explicit error handling: call bun.handleOom() if the error is
error.OutOfMemory, otherwise propagate the error (or convert to a fast-fail)
instead of swallowing it. Update the block around self.index.fetchRemove(...)
and the reclaimable.put(self.allocator, _key, kv.value) call to handle the
returned error (e.g., catch |err| { if (err == error.OutOfMemory)
bun.handleOom(); else return err; }) so the function fails fast on allocation
failures and doesn’t ignore other errors.
In `@src/resolver/resolver.zig`:
- Around line 2702-2729: The reuse branch currently frees some PackageJSON
fields but skips releasing the exports/imports trees and scripts/config, causing
leaks; implement and call a recursive deallocator (e.g.,
PackageJSON.deinitRecursive or freePackageJSON(prev, alloc)) that walks and
frees all owned substructures (prev.exports, prev.imports, prev.scripts,
prev.config and any nodes in their tree/map types, plus the already-handled
main_fields, browser_map, dependencies.map and side_effects variants) before
doing prev.* = pkg; accept an allocator parameter (use bun.default_allocator)
and use defer inside the helper where appropriate to guarantee cleanup on error.
In `@test/js/bun/resolve/bust-dir-cache-leak.test.ts`:
- Around line 1-72: Move the new "bustDirCache reuses DirInfo and EntriesOption
slots across repeated reloads" test into the existing resolve test file instead
of creating a new standalone file; copy the entire test body (including imports
expect/test and bunEnv, bunExe, tempDir usage and the spawn/run.ts logic that
exercises Bun.FileSystemRouter.reload()) into the existing resolve tests, remove
the new file, ensure the test name and timeout (30_000) remain unchanged, and
verify imports don't duplicate existing ones in that file so the test compiles
and runs as part of the existing resolve test suite.
🪄 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: 9c9e3355-6d39-4a0c-a38c-5d1b21b4a00e
📒 Files selected for processing (3)
src/allocators.zigsrc/resolver/resolver.zigtest/js/bun/resolve/bust-dir-cache-leak.test.ts
robobun
force-pushed
the
farm/533bc3a0/fix-bustdircache-leak
branch
from
fb17145 to
f221d3a
Compare
robobun
force-pushed
the
farm/533bc3a0/fix-bustdircache-leak
branch
from
786f3db to
a2f2234
Compare
There was a problem hiding this comment.
All prior feedback looks addressed and the latest pass found nothing new, but this is a non-trivial memory-lifecycle change in the resolver's core BSSMap/DirInfo caching with cross-entry pointer aliasing invariants (and CI is showing build-zig failures on a2f2234), so it warrants a human sign-off.
Extended reasoning...
Overview
This PR changes BSSMap in src/allocators.zig to park freed slots in a per-key reclaimable map so remove() → getOrPut() → put() reuses the same backing slot, and threads previous *PackageJSON / *TSConfigJSON heap pointers through dirInfoUncached / parsePackageJSON / parseTSConfig in src/resolver/resolver.zig so they are overwritten in place instead of leaked on every bustDirCache(). It also adjusts the tsconfig extends merge loop to preserve the reused allocation's address, gates two rfs.entries cache-hit checks on status == .exists, and adds an RSS-growth regression test.
Security risks
None identified. No auth, crypto, network, or untrusted-input handling is touched. The risk class here is memory safety (UAF via enclosing_package_json / enclosing_tsconfig_json aliasing), not security exposure — and one such UAF regression (tsconfig extends + reuse) was caught and fixed during review in bd4dfed.
Level of scrutiny
High. BSSMap is a foundational data structure backing both dir_cache and fs.entries, and the resolver is on the critical path for every import resolution in --hot / --watch / DevServer / bundler. The change relies on a subtle invariant — child DirInfo entries hold raw pointers into parent slots, so reuse must preserve addresses and never free aliased allocations. The PR went through eight rounds of inline review that surfaced and fixed a real UAF regression, several incomplete-cleanup leaks, and a put()-after-markNotFound() slot-orphaning edge case. That iteration history alone suggests this is not a mechanical change.
Other factors
- The robobun CI status comment reports build-zig failures on six platforms for commit a2f2234 (the most recent substantive commit before the
retrigger gatepush). I can't verify whether a3a600d cleared them. - All prior inline comments (mine and CodeRabbit's) are marked resolved, and the author has documented the accepted residual gaps (ExportsMap nodes, tsconfig file-text buffer, jsx.import_source, scripts/config) as follow-ups.
- The new test exercises the slot-reuse path but does not cover the package.json / tsconfig.json reuse branches or the
extendsmerge fix — the author noted ASAN verification for the latter, but a maintainer should confirm that's sufficient. - The change interacts with companion PR #29919 (DirEntry/EntryStore side); a human should confirm the two land in a compatible order.
…ache bustDirCache() calls BSSMap.remove() which only dropped the hash→index mapping; the backing_buf slot (and its *PackageJSON / *TSConfigJSON heap pointers) stayed orphaned, and the next getOrPut()+put() for the same key allocated a fresh slot plus fresh heap objects. Every --hot/--watch file change, DevServer hot-reload, and FileSystemRouter.reload() burned one DirInfo slot and one EntriesOption slot per affected directory, plus a new PackageJSON/TSConfigJSON when the directory had one — unbounded growth over a dev session. Fix: BSSMap.remove() now stashes the old slot in a per-key reclaimable map. getOrPut() returns that slot with status=.unknown so the caller re-resolves, and put() writes back to the same slot instead of incrementing backing_buf_used. Before overwriting a reclaimed DirInfo, the resolver captures the old package_json/tsconfig_json and passes them through to parsePackageJSON/parseTSConfig, which overwrite the existing heap struct in place rather than calling .new(). Child DirInfo.enclosing_* pointers keep working because the address is preserved. The rfs.entries read path now checks result.status so a reclaimed slot is treated as stale (re-iterate the directory, reuse the DirEntry in place) rather than returning the old listing.
… on markNotFound - parsePackageJSON now frees the previous value's source.contents, name/version, main_fields/browser_map backing, dependencies map, and side_effects map/glob before overwriting the reused struct, so each hot-reload of a directory containing package.json no longer leaks the prior file text and hash-map arrays. Matches the paths.deinit() already done in parseTSConfig's reuse path. - markNotFound no longer drops the reclaimable entry: getOrPut early-returns on NotFound before consulting reclaimable, so keeping it lets a delete→recreate→bust cycle reuse the original slot instead of permanently orphaning it.
…st timeout The captured reuse_package_json/reuse_tsconfig_json is dropped when the underlying file was deleted (or fails to parse) between busts. That's a one-off leak per delete event; freeing would UAF through child enclosing_* aliases, and parking it back on the DirInfo would make the resolver think the file still exists. Documented the tradeoff instead.
Spawning a debug+ASAN subprocess has several seconds of startup overhead before the reload loop runs; the default 5s per-test budget wasn't enough, so the test timed out on both with- and without-fix lanes. 50+1000 reloads still gives ~14-18 KB/iter without the fix vs ~1-2 KB/iter with it.
When a tsconfig has "extends", the merge loop sets info.tsconfig_json to the deepest-base allocation and bun.destroy()s every intermediate — which now includes the reused pointer that child DirInfo.enclosing_tsconfig_json still aliases. Skip destroying the reused allocation in the loop and copy the merged result back into it afterward so the address invariant holds for the extends case too. Also free the []string value slices in prev.paths before deinit(), matching the extends-merge cleanup pattern.
getOrPut() early-returns on NotFound without consulting reclaimable, so a caller that proceeds past .not_found (dirInfoForResolution does) reaches put() with result.index == NotFound. put() would then allocate a fresh slot and discard the parked one that markNotFound intentionally kept. Pull the reclaimable.fetchRemove up front so the parked slot is used instead.
Each pattern in SideEffects.glob/.mixed.globs is separately duped by normalizePathForGlob(); ArrayListUnmanaged.deinit() only frees the list backing, not the item strings. Matches the paths.values() fix already in parseTSConfig.
browser_map has dupe_keys=false but each key is caller-duped via allocator.dupe(u8, fs.normalize(...)) in PackageJSON.parse, so StringMap.deinit() skips them. main_fields keys are static slices from r.opts.main_fields and must not be freed.
…alloc gaps parseMemberExpressionForJSX heap-allocates factory/fragment as []string when set in JSON; guard on jsx_flags since defaults are static slices. Remaining per-field sub-allocations (jsx.import_source, tsconfig file text, ExportsMap nodes, scripts/config) need a centralized deinitOwnedFields() helper and are tracked as follow-ups.
Jarred-Sumner
force-pushed
the
farm/533bc3a0/fix-bustdircache-leak
branch
from
a3a600d to
3baec36
Compare
There was a problem hiding this comment.
No new issues found in this revision, but given the pointer-aliasing invariants this depends on (child enclosing_package_json/enclosing_tsconfig_json must stay valid across in-place overwrites, the extends-merge UAF that was fixed mid-review, the changed hasCheckedIfExists() semantics affecting all BSSMap callers) and the acknowledged residual gaps, this warrants a human pass before merge.
Extended reasoning...
Overview
This PR reworks resolver cache lifecycle to stop leaking one DirInfo BSSMap slot (plus its heap *PackageJSON/*TSConfigJSON) per bustDirCache() call. It touches three areas: src/bun_alloc/bun_alloc.zig (adds a per-key reclaimable map to BSSMap and changes getOrPut/put/remove/hasCheckedIfExists semantics), src/resolver/resolver.zig (threads reuse_package_json/reuse_tsconfig_json through dirInfoUncached→parsePackageJSON/parseTSConfig to overwrite allocations in place, with manual cleanup of ~10 owned sub-allocations, plus reworks the tsconfig extends-merge loop to preserve the reused address), and a new RSS-growth regression test.
Security risks
None identified. This is internal memory management; no parsing of untrusted input was added, no auth/crypto/permissions surface. The risk class here is correctness (UAF / heap corruption) rather than security per se — and one real UAF regression (tsconfig extends + reused allocation being bun.destroy()'d while child DirInfos still aliased it) was caught and fixed during review.
Level of scrutiny
High. This is core resolver infrastructure on the hot path of every --hot/--watch/DevServer reload. The correctness of the fix rests on a subtle invariant — that overwriting a heap struct in place keeps child DirInfo.enclosing_* raw-pointer aliases valid — and that invariant interacts non-trivially with the tsconfig extends-merge loop, the markNotFound→put path, and the changed meaning of hasCheckedIfExists() (which has a third caller in fs.zig that was deliberately left for #29919). The PR went through ~9 rounds of fixes during review (UAF, several incomplete-cleanup leaks, slot orphaning on the NotFound path), which is a signal that the surface is delicate.
Other factors
- All inline review comments are resolved; the current bug-hunting pass found nothing new.
- There are acknowledged, documented residual gaps left for follow-up (ExportsMap tree nodes, tsconfig file-text buffer,
jsx.import_source,scripts/config) — a human should sign off on that scoping decision. - The latest CI build (#50592) shows a
build-zigfailure on Windows x64 plus three test failures; even if unrelated/flaky, that should be confirmed before merge. - This PR is designed to compose with #29919 (the
*DirEntryside); a reviewer familiar with both should confirm the interaction at the two shared generation checks in resolver.zig and the untouchedfs.zigsite.
This is well outside the "simple/mechanical" bar for auto-approval.
1 participant
What
Resolver.bustDirCache()leaked oneDirInfoBSSMap slot (and the heap-allocated*PackageJSON/*TSConfigJSONit owned) per call. Triggered on every file change in--hot/--watch, every DevServer hot-reload event, everyFileSystemRouter.reload(), and the runtime ENOENT-retry path — so growth was unbounded over a dev session.Cause
BSSMap.remove()only doesself.index.remove(hash); the backing slot stays inbacking_bufat its old index with live*PackageJSON/*TSConfigJSONpointers. The nextgetOrPut()for the same key finds nothing, returnsUnassigned, andput()takes a fresh slot (backing_buf_used += 1).dirInfoUncached()then heap-allocates a brand-newPackageJSON/TSConfigJSON. The old slot and its heap objects are never reclaimed.The same applies to the
EntriesOptionmap (bustEntriesCachegoes through the sameBSSMap.remove()).Fix
BSSMap.remove()now stashes the old slot in a per-keyreclaimablemap instead of dropping it on the floor.getOrPut()returns a reclaimed slot withstatus = .unknownso callers still re-resolve, butput()writes back to the same slot instead of incrementingbacking_buf_used.DirInfo, the resolver captures the oldpackage_json/tsconfig_jsonheap pointers and threads them through toparsePackageJSON/parseTSConfig, which overwrite the existing allocation in place rather than calling.new(). ChildDirInfo.enclosing_package_json/enclosing_tsconfig_jsonpointers keep working because the address is preserved.rfs.entries.atIndex(...)checks inresolver.zignow gate onstatus == .existsso a reclaimed slot is treated as stale (re-iterate the directory, reuse theDirEntryvia thein_placepath) rather than returning the old listing.hasCheckedIfExists()now testsstatus != .unknownso a reclaimed-but-unresolved slot reads as "never looked up".Because reclaim is per-key (not a generic free list), a directory always goes back to the slot it came from. Child
DirInfo.parentindices therefore remain valid — this is the concern that #29919 raised when it skipped thedir_cachehalf of this leak.Verification
FileSystemRouter.reload()over a 9-directory tree, 4000 iterations:The residual is unrelated
DirnameStoreappends along the re-resolve path.New test:
test/js/bun/resolve/bust-dir-cache-leak.test.tsRelated: #29919 handles the
*DirEntry/EntryStoreside of the same bust cycle with astaleflag onDirEntry; this PR handles theDirInfo/PackageJSON/TSConfigJSONside at theBSSMaplayer. They touch the same two generation checks inresolver.zigbut are otherwise independent.