Fix bun --hot per-reload memory retention (DirEntry reuse, ref_strings balance) - #36675
Fix bun --hot per-reload memory retention (DirEntry reuse, ref_strings balance)#36675robobun wants to merge 13 commits into
Conversation
|
Updated 1:02 AM PT - Aug 5th, 2026
✅ @robobun, your commit d9bf5846e4e09d73b5467340ede6438c6289717b passed in 🧪 To try this PR locally: bunx bun-pr 36675That installs a local version of the PR into your bun-36675 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Not adding
|
|
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:
WalkthroughHot reload now exposes allocation diagnostics, uses owned reference strings, refreshes stale resolver entries in place, manages directory descriptors safely, and adds regression tests for descriptor and native-memory growth. ChangesHot reload leak handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/resolver/lib.rs (1)
1587-1640: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
DirEntry::staleinentries_at_locked. Hot-reload invalidation marks the cached entry stale without advancingResolver::generation, and durableDirInfoRefvalues can still reach that indexed slot. Add|| existing.staleto the refresh condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/resolver/lib.rs` around lines 1587 - 1640, The refresh condition in Resolver::entries_at_locked currently checks only whether existing.generation is older than generation. Also refresh the cached DirEntry when existing.stale is true by including that flag in the condition, while preserving the existing directory reopening and replacement flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/rust.ts`:
- Around line 494-496: Update the Config type and resolveConfig() to add and
populate a flat trackAlloc field from BUN_TRACK_ALLOC once during configuration
resolution. In the Rust flag construction shown, replace the direct
process.env.BUN_TRACK_ALLOC check with cfg.trackAlloc so subsequent builds use
the resolved configuration rather than mutable process state.
In `@src/bun_bin/track_alloc.rs`:
- Around line 39-54: Update alloc, alloc_zeroed, and realloc in the allocator
implementation to adjust allocation counters only after their underlying calls
return a non-null pointer; preserve the old realloc accounting when it fails.
Add failure-path tests covering null results and verifying histogram bytes and
allocation counts remain correct for all three methods.
In `@src/watcher/Watcher.rs`:
- Around line 879-889: Update the descriptor replacement logic around fds[index]
in Watcher to close every valid previous descriptor when fstat fails or reports
st_nlink == 0, while retaining the existing behavior that avoids closing
descriptors for linked inodes unless their watch is removed first. Ensure
cleanup removes the associated watch before closing where necessary, preserving
the no-spurious-reload behavior.
In `@test/cli/hot/hot.test.ts`:
- Around line 983-1011: Make the ref-string diagnostic assertion in the hot
reload test fail when the diagnostic hook is unavailable: require maxRefStrings
to be nonnegative and below the limit, removing the maxRefStrings < 0 fallback.
Preserve the existing failure-context fields and ensure the invariant explicitly
validates that hotReloadDiagnostics() resolved under the configured test
environment.
---
Outside diff comments:
In `@src/resolver/lib.rs`:
- Around line 1587-1640: The refresh condition in Resolver::entries_at_locked
currently checks only whether existing.generation is older than generation. Also
refresh the cached DirEntry when existing.stale is true by including that flag
in the condition, while preserving the existing directory reopening and
replacement flow.
🪄 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: e1126d54-7038-40a3-b068-e613c9982ddb
📒 Files selected for processing (16)
Cargo.tomlscripts/build/rust.tssrc/bun_bin/lib.rssrc/bun_bin/track_alloc.rssrc/js/internal-for-testing.tssrc/jsc/VirtualMachine.rssrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/hot_reloader.rssrc/jsc/virtual_machine_exports.rssrc/resolver/fs.rssrc/resolver/lib.rssrc/resolver/resolver.rssrc/runtime/api/filesystem_router.rssrc/runtime/dispatch_js2native.rssrc/watcher/Watcher.rstest/cli/hot/hot.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this PR reworks memory/fd ownership across the resolver, watcher, hot reloader, and VM ref-string interning — including a semantic change to ref_counted_string (now always +1) and in-place DirEntry slot rewriting under entries_mutex. Worth a human pass on the refcount balance and fd carry-forward paths.
Checked: both ref_counted_string callers (ref_counted_resolved_source via source_code_needs_deref = true, FileSystemRouter::claim) updated for the new +1 contract.
Checked: bust_entries_cache now marks stale instead of removing; all four refresh sites (read_directory, entries_at_locked, dir_info_for_resolution, dir_info_cached_miss) honor .stale and carry prev_fd forward.
Checked: the new entries_mutex lock in hot_reloader.rs — bust_dir_cache is called before the locked block, so no re-entry on the non-recursive mutex.
Checked: Watcher::add_file fd-close is Linux-only (ATOMIC_FILE_WATCHER) and the st_nlink > 0 gate's known atomic-rename leak is documented in the PR body.
Extended reasoning...
Overview
This PR fixes three stacked per-reload memory leaks in bun --hot (~7.6 KB/reload → flat), touching 17 files across the resolver (src/resolver/{fs,lib,resolver}.rs), hot reloader (src/jsc/hot_reloader.rs), watcher (src/watcher/Watcher.rs), VM ref-string interning (src/jsc/VirtualMachine.rs), FileSystemRouter, and ZigGlobalObject.cpp. It also adds a cfg(bun_track_alloc) global-allocator histogram, a hotReloadDiagnostics() internal-for-testing hook, and two regression tests (fd-count and native-memory) in test/cli/hot/hot.test.ts.
Security risks
None identified. No untrusted-input parsing, no auth/crypto. The changes are internal lifecycle/ownership plumbing.
Level of scrutiny
High. Every substantive change here is in the most-blocked review category (native memory safety): a refcount-contract change on ref_counted_string (fresh insert and cache hit now both return +1, with source_code_needs_deref flipped to true and FileSystemRouter::claim dropping its explicit extra ref); fd-ownership rework where bust_entries_cache marks DirEntry.stale instead of orphaning, and four refresh sites carry an existing .fd forward while a scopeguard closes any freshly-opened-but-unadopted directory handle; a new cross-thread invariant where the hot reloader now takes entries_mutex around DirEntry reads because slots can be rewritten in place by a JS-thread resolve; and a Linux-only Watcher::add_file close of the replaced fd gated on st_nlink > 0 (with a documented residual leak on atomic-rename saves). Each of these is exactly the shape REVIEW.md flags as needing careful human audit — refcount balance on every terminal path, fd released exactly once with a named owner, and mutex non-re-entry.
Other factors
The PR description is unusually thorough (allocation-histogram evidence, gdb backtrace, before/after tables, explicit "Not fixed here" section), the CodeRabbit findings were all addressed in the follow-up commit, and the two new tests target the specific leak signatures (fd delta via /proc/<pid>/fd, UnlinkedModuleProgramCodeBlock count and ref_strings count via the diagnostic hook). The RSS bound was dropped from the test as too noisy across CI lanes, which is reasonable. That said, the interaction surface — resolver × watcher × hot-reloader thread, plus a public-ish refcount API contract change — is large enough that a maintainer familiar with the resolver's BSSMap/EntryStore lifetime and the dev-server's DirectoryWatchStore fd sharing should confirm the carry-forward reasoning and the open_dir_adopted scopeguard placement in dir_info_cached_miss.
|
CI on 7029a35 (#86979): The only Ready for review. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
We don't want to clear the codeCache. The code cache is important for hot reloading speed.
|
Dropped the It turns out the clear isn't needed for the leak fix anyway: CodeCache's own prune policy (2000 entries / 16 MB / 10 s) bounds the accumulation, and with the
The test now asserts |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/resolver/lib.rs (2)
1283-1299: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not cache a descriptor that the close guard will close.
When
store_fdis true andneed_to_close_files()is true,readdirstoreshandleinentries.fd, whileshould_close_handleremains true. If the previous descriptor is invalid, this block does not clearentries.fd. The cache then storeshandle, and_close_guardcloses it on return. Later users receive a valid-looking but closed descriptor.Clear
entries.fdbefore caching when ownership is not transferred, or disarm the guard only after the cache takes ownership.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/resolver/lib.rs` around lines 1283 - 1299, Update the descriptor ownership handling around the `store_fd` assignment and `in_place` block so `entries.fd` never retains `handle` while `should_close_handle` remains true. When ownership is not transferred and no valid previous descriptor is restored, clear the cached descriptor or disarm the close guard only after caching takes ownership; preserve reuse of a valid `prev_fd`.
1204-1207: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftRetain the cached entry when refresh fails.
When a stale refresh fails, both refresh paths call
read_directory_errorafter selecting an existingDirEntry. That operation replaces the cachedEntriesOption::Entriesvalue or marks it not found. The stored reference does not own or drop the leakedBox<DirEntry>, and its cached descriptor remains open. A failed reload can therefore leak one directory entry and descriptor per attempt.Retain the existing slot and return a temporary error, or add lock-safe cleanup that explicitly releases the descriptor before changing the cached variant.
Also applies to: 1602-1613, 1630-1634
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/resolver/lib.rs` around lines 1204 - 1207, Update the stale-entry refresh flows around the EntriesOption::Entries selection and the read_directory_error calls to preserve the existing cached entry when reloading fails. Avoid replacing the cached EntriesOption or marking it not found while the selected DirEntry remains referenced; instead return a temporary error with the slot intact, or perform lock-safe cleanup that explicitly releases the descriptor before changing the cached variant. Apply the same handling to both refresh paths identified near the related read_directory_error logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/resolver/lib.rs`:
- Around line 1283-1299: Update the descriptor ownership handling around the
`store_fd` assignment and `in_place` block so `entries.fd` never retains
`handle` while `should_close_handle` remains true. When ownership is not
transferred and no valid previous descriptor is restored, clear the cached
descriptor or disarm the close guard only after caching takes ownership;
preserve reuse of a valid `prev_fd`.
- Around line 1204-1207: Update the stale-entry refresh flows around the
EntriesOption::Entries selection and the read_directory_error calls to preserve
the existing cached entry when reloading fails. Avoid replacing the cached
EntriesOption or marking it not found while the selected DirEntry remains
referenced; instead return a temporary error with the slot intact, or perform
lock-safe cleanup that explicitly releases the descriptor before changing the
cached variant. Apply the same handling to both refresh paths identified near
the related read_directory_error logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3470746d-fc70-4589-8ab6-ee94cf436566
📒 Files selected for processing (5)
scripts/build/config.tsscripts/build/rust.tssrc/bun_bin/track_alloc.rssrc/resolver/lib.rstest/cli/hot/hot.test.ts
|
@robobun rebase |
…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>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/resolver/resolver.rs (2)
4535-4540: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate directory iteration errors.
Line 4539 converts an
iterate_dir(open_dir)error into a successful partial scan. The code then cachesnew_entry, so later resolutions can treat incomplete directory contents as complete until invalidation. Return the iterator error instead of caching partial results.As per coding guidelines, “Never swallow failures or signal success after failure; propagate I/O, syscall, cleanup, and requested-operation errors explicitly.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/resolver/resolver.rs` around lines 4535 - 4540, Update the directory iteration loop around dir_iterator.next() to propagate Err values instead of breaking and caching partial results. Preserve the existing handling for Ok(Some(v)) and Ok(None), and return the iterator error through the enclosing resolver operation.Source: Coding guidelines
4224-4228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReopen or rewind the descriptor before generation-only refreshes.
When
entries.generation < self.generationandentries.staleis false,queue_top.fdcan point at a descriptor whose POSIX directory offset is already at EOF.bun_sys::iterate_dirresets only iterator state, not the descriptor offset. The refresh can therefore cache an empty or partial map. Open a fresh descriptor or reset the offset before enumeration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/resolver/resolver.rs` around lines 4224 - 4228, The generation-only refresh path must reset the directory descriptor before enumeration when reusing a non-stale entry. Update the logic around entries.generation, entries.stale, and slot.fd to reopen the descriptor or rewind its POSIX offset so bun_sys::iterate_dir starts from the beginning and produces a complete map.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/api/filesystem_router.rs`:
- Around line 100-109: Make base_dir a non-optional OwnedRefString in both
FileSystemRouter and MatchedRoute, preserving the constructor and
MatchedRoute::init assignments without wrapping the value in Some. Update all
base_dir reads to remove unwrap calls, including the sites near the constructor
and route matching logic, while leaving genuinely optional origin and
asset_prefix unchanged.
---
Outside diff comments:
In `@src/resolver/resolver.rs`:
- Around line 4535-4540: Update the directory iteration loop around
dir_iterator.next() to propagate Err values instead of breaking and caching
partial results. Preserve the existing handling for Ok(Some(v)) and Ok(None),
and return the iterator error through the enclosing resolver operation.
- Around line 4224-4228: The generation-only refresh path must reset the
directory descriptor before enumeration when reusing a non-stale entry. Update
the logic around entries.generation, entries.stale, and slot.fd to reopen the
descriptor or rewind its POSIX offset so bun_sys::iterate_dir starts from the
beginning and produces a complete map.
🪄 Autofix
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: 78565092-be07-49f2-ae56-933e5d865e9b
📒 Files selected for processing (6)
src/jsc/AsyncModule.rssrc/jsc/RefString.rssrc/jsc/VirtualMachine.rssrc/resolver/resolver.rssrc/runtime/api/filesystem_router.rssrc/runtime/jsc_hooks.rs
…fd only for cache hits base_dir is assigned unconditionally in both FileSystemRouter's constructor and MatchedRoute::init, so the Option (and its unwraps) only existed for the old take()-based finalizers. The queue-seed gate now also skips generation-outdated entries: their stored descriptor offset is at EOF like a stale entry's, so re-enumerating through it would cache an empty listing; the re-scan opens a fresh handle instead and the stored fd still carries forward into the refreshed entry.
|
feb261e addresses the review feedback on c695426:
Not changing the
|
The carry-forward gate in read_directory_with_iterator skipped the close when the caller passed the handle in (had_handle), but ownership of maybe_handle transfers to this function (Scanner passes Dir::into_raw()), so overwriting entries.fd with prev_fd orphaned it. Close whatever entries.fd holds unless _close_guard owns it. Not reachable today (Scanner runs before anything can mark an entry stale), so this corrects the ownership contract rather than a live leak.
There was a problem hiding this comment.
No new issues found on d9bf584. The three concerns raised on earlier revisions — the entries_mutex ↔ Watcher.mutex AB-BA ordering in load_as_file, the dead open_dirs defer in dir_info_cached_miss, and the !had_handle fd-ownership gap in read_directory_with_iterator — are each addressed. Deferring to a human reviewer given the scope: cross-thread in-place mutation of resolver DirEntry slots under entries_mutex, the RefString refcount-balance change (now source_code_needs_deref = true), and fd carry-forward across three refresh sites.
What was reviewed:
- Lock ordering: the four new
entries_mutexacquire sites drop before any path that reachesWatcher.mutex;bust_dir_cache_recursivesnapshots entry pointers under the lock and releases before recursing. OwnedRefStringbalance:ref_counted_stringreturns exactly +1 on both arms;into_raw()transfers to C++ matched bysource_code_needs_deref;FileSystemRouter/MatchedRouterelease via field drop.- fd lifecycle:
prev_fdcarry-forward closes the displaced handle unless_close_guardowns it;open_dir_owned.take()indir_info_cached_misstransfers only the freshly-opened handle.
Extended reasoning...
Overview
Twenty files across the resolver (lib.rs, resolver.rs, fs.rs, dir_info.rs), JSC bindings (RefString.rs, VirtualMachine.rs, hot_reloader.rs, AsyncModule.rs, jsc_hooks.rs), FileSystemRouter, the watcher, plus a new opt-in bun_track_alloc global-allocator wrapper and two regression tests in hot.test.ts. The core changes are (1) bust_entries_cache marks DirEntry slots .stale for in-place refresh instead of orphaning them, with fd carry-forward at four refresh sites; (2) ref_counted_string now returns an RAII OwnedRefString that is always +1, and ResolvedSource sets source_code_needs_deref = true so the external-string finalizer can drain the map; (3) Watcher::add_file closes the fd it displaces on Linux (gated on st_nlink > 0).
Security risks
None identified. No user-controlled input reaches new parsing or allocation-sizing logic; the track_alloc histogram is behind a build-time cfg and the diagnostic hook is gated on bun:internal-for-testing.
Level of scrutiny
High. This is memory-safety-critical native code: manual refcount balance across an FFI boundary, raw-fd ownership transfer, and in-place mutation of a process-global cache slot that other threads hold raw pointers into. The PR itself introduced (and then fixed after review) an AB-BA deadlock and an fd-ownership contract gap, which is exactly the class of bug that benefits from a second set of eyes on the final shape. The get_entries_ref → get_entries_ref_locked migration also changes the aliasing contract for every DirEntry.data reader.
Other factors
The PR has iterated substantially (six+ revisions) in response to review; all prior findings are resolved and CI on the previous head was green with no [new] failures. Test coverage is good (fd-count assertion via /proc/<pid>/fd, refStrings-vs-UnlinkedModuleProgramCodeBlock invariant). One comment-cop bot note on lib.rs:1291 remains open but is cosmetic. Given the breadth of the resolver/watcher interaction and the refcount-semantics change visible to C++ (source_code_needs_deref flip), a maintainer sign-off is appropriate.
|
Heads up: #37050 fixes the EBADF/EISDIR reload race in this same area and touches the |
…#37050) ## What Fixes `test/cli/hot/watch-many-dirs.test.ts` ("handles 129 directories being updated simultaneously"), which has been failing on main and across PR CI runs on Linux with: ``` error: EISDIR reading "/tmp/.../hot-many-dirs_.../dir-0072/index.js" error: EBADF reading "/tmp/.../hot-many-dirs_.../dir-0009/index.js" ``` followed by the 30s test timeout (the failed reload never prints the expected output). Seen red on main builds 86133/86209/86347 and in many PR runs, e.g. [build 89567](https://buildkite.com/bun/bun/builds/89567). The race is timing-sensitive: it reproduced once in 80 local runs of an unmodified release build (with the exact EBADF + EISDIR + timeout signature), while loaded CI machines hit it regularly. ## Cause `bun --hot` re-transpiled a changed module by reading through a file descriptor snapshotted from the watcher's watchlist (`ImportWatcher::snapshot_fd_and_package_json`). The snapshot copies the fd number under the watcher mutex, but the read in `transpiler.rs:read_file_with_allocator` happens after the mutex is released. Concurrently, the watcher thread's `flush_evictions` closes stored descriptors under that same mutex. On Linux, a write to a file inside a watched directory produces both a file event and a directory event; the directory-event arm in `hot_reloader.rs` evicts the watched file's entry (to handle atomic saves that replace the inode). With 129 directories the inotify events span multiple batches, so a reload triggered by batch N snapshots a descriptor that batch N+1's eviction then closes: - fd closed before the read: `EBADF reading "<path>"` - fd number recycled by one of the resolver's many `openat(O_DIRECTORY)` calls: `EISDIR reading "<path>"` The mutex ordering added previously (`flush_evictions` before `enqueue`, snapshot under the mutex) only closed the same-event window; nothing can serialize a later batch's eviction against a read that happens after the snapshot returns. Reading through the stored fd was also wrong after atomic saves (pre-rename inode, stale contents), which is why the entrypoint already had a workaround skipping it. ## Fix Reloads now always open the file by path, and the watchlist keeps sole ownership of its stored descriptor: - `snapshot_fd_and_package_json` becomes `snapshot_package_json`; the stored fd is never handed out for reads. This removes the race structurally and makes the entrypoint's open-by-path workaround universal, so it is deleted. - `Watcher::add_file` no longer replaces a valid stored fd on the already-watched branch (it only upgrades fd-less entries inserted by `add_file_by_path_slow`, e.g. the `--hot` entrypoint, so the directory-event recovery path keeps working). The old overwrite dropped the replaced descriptor without closing it, leaking one fd on the entrypoint per reload. - `add_file` now returns `FdOwnership` saying whether it adopted the caller's descriptor; the transpiler handoff sites, `AsyncModule`, the bundler's plugin watch path, and `add_file_by_path_slow` close the descriptor when the watcher did not take it. The bundler's `watcher_data` path intentionally ignores the outcome and keeps today's behavior, since its descriptor can be borrowed from the resolver's entry cache. The fd-per-reload behavior is locked in by a new deterministic test in `watch-many-dirs.test.ts` that counts `/proc/<pid>/fd` entries for the entrypoint and an edited dependency across 15 reloads. On an unfixed build the entrypoint gains exactly one fd per reload (+15); with the fix both counts are stable. (The remaining per-reload directory-handle growth comes from the resolver's `DirEntry` cache and is addressed separately in #36675.) ## Verification - New test fails on an unfixed build (`entryDelta: 15`) and passes with the fix. - The race fix is structural rather than statistical: after this change the transpiler only ever reads descriptors it opened itself, so the closed-by-eviction read cannot occur. Empirically, "handles 129 directories" failed 1/80 runs on an unfixed release build and passed 40/40 (plus 12/12 debug ASAN) runs on a fixed one. - `test/cli/hot/hot.test.ts` (12), `test/cli/hot/watch.test.ts` (2), `test/cli/watch/` (7), `test/bake/dev/hot.test.ts` (11, including DEV:hot-9), `test/js/bun/util/filesystem_router.test.ts` (33), `test/cli/test/test-changed.test.ts` (20) all pass on a debug ASAN build. - `bun run rust:check-all`: 10/10 target combos clean. The race predates any recent change (the fd-reuse pattern and evicting close shipped with #30412 and were inherited from the original design); CI frequency rose recently with timing shifts. The open #36675 fixes the adjacent resolver-side leaks and includes a different change to the same `add_file` branch; this PR supersedes that hunk by never replacing the stored fd at all. <!-- robobun:evidence:begin --> --- **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/watch-many-dirs.test.ts <!-- robobun:evidence:end -->
2 participants
Fixes #11083
Fixes #14734
Closes #36251, Closes #35588, Closes #29919, Closes #33197
Reproduction
20,000 reloads on a release build, before: RSS 27 MB -> 179 MB (~7.6 KB/reload). After: RSS 27 MB -> 29 MB (flat).
Cause
Two independent per-reload leaks stack. A per-size-bucket allocation histogram (wrapped global allocator, see
BUN_TRACK_ALLOCbelow) attributed the growth on a 40-file project directory over 5000 reloads:DirEntry.data.reserve(64)hashbrown tableEntryStoreBSSList overflow blocksBox<RefString>+ transpiled bytesResolver
DirEntryorphaning (the ~5x canary-vs-1.3.14 regression)bust_entries_cache()only dropped the hash->indexBSSMapmapping, orphaning the boxedDirEntry, its.dataStringHashMapbucket array, its open.fd, and everyEntryslot it referenced in the append-onlyEntryStoreslab. The nextdir_info_cached_miss()allocated everything fresh in a new slot.The mechanism existed in Zig 1.3.14 too (
BSSMap.removethere also only drops the mapping). What turned it into a ~5x regression is that the Rust port addsnew_entry.data.reserve(64)so every orphanedDirEntryleaks a fixed 3216-byte hashbrown table regardless of actual directory size; Zig started from zero capacity and leaked only what the directory needed. On top of that, each orphanedDirEntry's names are re-appended intoEntryStore, so the leak scales with the project directory's file count.The backtrace for the per-reload 3216-byte allocation, captured with gdb on a stripped profile build:
Fix.
bust_entries_cache()now marks theDirEntry.staleinstead of removing the mapping. The four refresh sites (dir_info_cached_miss,dir_info_for_resolution,read_directory_with_iterator,entries_at_locked) see the existing slot viaat_index, rewrite it in place, carry any stored.fdforward, and passprev_mapsoEntryslots are reused. A freshly-opened directory fd that was not adopted intoDirEntry.fdis released.The
.fdcannot simply be closed at bust time: its raw value is also copied into queuedParseTask.contents_or_fd, the dev server'sDirectoryWatchStore, and sometimes the watcher's own watchlist, so a close there would race those readers on another thread. Carrying it forward into the refreshed entry is safe because the same owners still hold the same handle.Because the slot is now rewritten in place instead of orphaned, the hot reloader's directory-event path takes
entries_mutexwhile reading the capturedDirEntry(the previous code relied on the orphaned slot never being written again).ref_stringsrefcount imbalanceref_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: theExternalStringImplrefcount could never reach zero, and the duped source bytes + map slot survived forever even after the owningSourceProviderwas collected.FileSystemRouter::claim()had the same shape fororigin/base_dir/asset_prefix.Fix.
ref_counted_string()now always returns exactly +1 (takes an extra ref on cache hit).ref_counted_resolved_sourcesetssource_code_needs_deref = true, andFileSystemRouter::claimdrops its extra ref.With the balance fixed,
ref_stringsdrains as JSC'sCodeCacheevicts (it pins oneSourceProviderper unique source via theSourceCodeKey). The CodeCache's own prune policy (2000 entries / 16 MB of source / 10 s) bounds that accumulation, so clearing the cache on each reload is not needed and would cost unchanged modules their cache hit: at 3000 reloads of a tiny source,UnlinkedModuleProgramCodeBlockandref_stringsboth cap at 2000 and hold there, RSS 32 -> 40 MB.Watcher entrypoint fd
Watcher::add_file's already-watched branch on Linux overwrote the stored fd with the caller's freshly-opened one and never closed the one it replaced. It now closesprevious, gated onfstat(previous).st_nlink > 0so releasing the last reference to an unlinked inode cannot deliver a deferredIN_DELETE_SELFon the item's still-registered watch (whichDEV:hot-9otherwise turns into a spurious reload; see #33197 for the 0/20-vs-20/20 verification of that gate).Verification
New in
test/cli/hot/hot.test.ts:bun --hot, asserts the set of/proc/<pid>/fdentries pointing into the project is identical before and after (allowing a +/-1 transient per target).ref_stringscount never exceeds liveUnlinkedModuleProgramCodeBlockcount by more than a small constant (i.e. it drains as CodeCache evicts).hotReloadDiagnostics()is added tobun:internal-for-testingso the test can observerefStrings,sourceMappings,watchlistLen, andhotReloadCounterdirectly.test/cli/hot/hot.test.ts(14),test/bake/dev/hot.test.ts(11, includingDEV:hot-9), andtest/js/bun/util/filesystem_router.test.ts(33) all pass on a debug ASAN build.bun run rust:check-allpasses on all 10 targets.Not fixed here
Watcher::append_file_assume_capacity::<true>pathto_vec()).watchlistLenstays constant, so the allocation is being dropped somewhere without theCowbeing freed; it is tiny and separate from the entrypoint-fd path this PR touches.rename(tmp, target)) still leaks one fd per save on Linux because of thest_nlinkgate; fixing it needs the watch re-registered on the new inode first.BUN_TRACK_ALLOCThe per-size-bucket live-allocation histogram that located the
DirEntryleak is kept behindcfg(bun_track_alloc)(build withBUN_TRACK_ALLOC=1). It wraps the Rust global allocator with two atomic adds per alloc/free;hotReloadDiagnostics().allocHistogramreads it out. It surfaces reachable-but-growing native memory that LSAN (unreachable-only) cannot see.no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/hot/hot.test.ts