Repository navigation
bun test: close scanner directory fds once their children are opened - #40016
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 10 minutes Limit details: You’ve used the included review currently available. Your 65 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (1)
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 (3)
💤 Files with no reviewable changes (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughChangesDirectory scanning now uses reference-counted Scanner directory ownership
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:53 PM PT - Aug 21st, 2026
⏳ @Jarred-Sumner, your commit b8bf054 is still building in
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The test-file scanner opened every directory with openat(parent_fd, name) and handed the fd to the resolver with store_fd = true, so each scanned directory stayed open for the life of the process. In a large repo that is thousands of fds, and on macOS a stdio pipe numbered >= 10240 makes libc's posix_spawn_file_actions_adddup2 fail with EBADF. The scanner now owns each directory fd (Rc<Dir>): a parent stays open only until its last child has been opened. The resolver still caches the listing, but not the fd. Entry kinds are resolved without keeping a file fd for symlinks. Fixes #39852 Fixes #39783
f84185e to
4e9857d
Compare
Jarred-Sumner
changed the title
There was a problem hiding this comment.
The PR has been scoped down since my earlier pass — the bun-spawn.cpp / spawn.test.ts changes my previous inline comments addressed are no longer part of this diff, so those two findings can be disregarded here.
On the current 3-file change I checked for double-close and fd escape: read_directory_with_iterator with a caller-supplied handle sets should_close_handle = false and, with store_fd = false, never caches handle into entries.fd — so the Rc<Dir> is the sole owner and its Drop is the only close. The Windows path now goes through open_dir_at_windows_a's relative-to-RootDirectory handling, which already supports a real dir_fd.
Extended reasoning...
The PR was reduced from ~11 files to 3 (Scanner.rs, test_command.rs, bun-test.test.ts) after my previous review; my earlier findings on bun-spawn.cpp (maxTarget missing the Close arm) and the undrained stdout in spawn.test.ts targeted code that is no longer in this PR. On the reduced diff I traced fd ownership through Rc<Dir>: Dir::drop closes the fd, read_directory_with_iterator neither closes nor caches a caller-supplied handle when store_fd = false, and the borrow of entry.name/parent.fd ends before drop(entry). This is still a native fd-lifetime refactor with a cross-platform behavior change (Windows now opens relative to the parent fd instead of by absolute path), so it warrants a human look; the only new finding this run is the store_fd: true nit posted inline.
| // in the future we should investigate if refactoring this to not | ||
| // rely on the dir fd yields a performance improvement | ||
| store_fd: true, | ||
| store_fd: ctx.debug.hot_reload != jsc::virtual_machine::HotReload::None, |
There was a problem hiding this comment.
🔴 Under --watch/--hot this sets resolver.store_fd = true, but the scanner now caches every walked directory with entries.fd = INVALID (Scanner.rs passes store_fd = false). When the resolver later walks a test file's directory chain in dir_info_cached_maybe_log, it finds the cached DirEntry with an invalid fd, opens a fresh fd (resolver.rs:4460), tracks it in open_dirs[], then hits needs_iter = false (cached generation >= self.generation) so the fd is never stored into entries.fd — and the defer! at resolver.rs:4423 skips the close because store_fd = true and need_to_close_files() is false. One fd leaks per test-file directory (and its uncached ancestors) on the first watch run. Fix: at resolver.rs:4634-4643, when needs_iter = false && self.store_fd and the cached entries.fd is invalid, store open_dir into it (or close it).
Extended reasoning...
What the bug is
The follow-up commit gates store_fd on hot-reload: store_fd: ctx.debug.hot_reload != HotReload::None (test_command.rs:2279). Under bun test --watch/--hot, resolver.store_fd is therefore true. Meanwhile the scanner now unconditionally passes store_fd = false to read_directory_with_iterator (Scanner.rs:235), so every directory the scanner walks is cached in the resolver's DirEntry map with entries.fd = Fd::INVALID and entries.generation = 0 (lib.rs:1318 is skipped when store_fd is false).
That combination — a scanner-cached DirEntry with an invalid fd, plus resolver.store_fd = true — trips a latent hole in dir_info_cached_maybe_log (resolver.rs:4326+): a freshly opened directory fd is neither stored into the cache nor closed by the cleanup guard.
Step-by-step trace
Take a test file at <cwd>/sub0/probe.test.ts under bun test --watch, with resolver.generation = 0 (resolver.rs:931).
- Scan phase. The scanner walks
sub0/and callsread_directory_with_iterator(path, Some(fd), 0, /*store_fd*/ false, iter). The resolver cachesDirEntry { dir: "<cwd>/sub0", fd: INVALID, generation: 0, data: {…} }. - Load phase. Loading
probe.test.tscallsdir_info_cached("<cwd>/sub0"). TheDirInfocache misses (only theDirEntrycache is populated), sodir_info_cached_maybe_logbuilds the ancestor queue. For thesub0slot, resolver.rs:4347-4353 finds the scanner-cachedDirEntryand setsslot.fd = entries.fd = INVALID,slot.safe_path = entries.dir. - resolver.rs:4460 —
queue_top.fdis invalid, so a freshopen_dirfd is opened viaopenat. - resolver.rs:4573-4577 —
!queue_top.fd.is_valid() && open_dir.is_valid()→open_diris written intobufs!(open_dirs)[open_dir_count++]. - resolver.rs:4626-4638 —
entries.get_or_put(dir_path)returns the scanner-cached index (dir_pathisentries.dir, the same interned key the scanner stored under).at_index()returns the cachedEntries(entries);entries.generation (0) >= self.generation (0)→needs_iter = false. - resolver.rs:4645-4698 — the entire
if needs_iter { … new_entry.fd = if self.store_fd { open_dir } … }block is skipped.open_diris passed to the innerdir_info_uncached()(resolver.rs:6099+) but only used read-side (openat(fd, ".bin"),fstat); nothing there stores or closes it. - resolver.rs:4421-4428
defer!— the guard closesopen_dirs[0..n]only whenn > 0 && (!close_dirs_store_fd || need_to_close_files()). Hereclose_dirs_store_fd = trueandneed_to_close_files()(lib.rs:1565) returnsfalsewhilefile_limit > 254 && file_limit > (max_fd+1)*2, which holds after Bun raisesRLIMIT_NOFILE. The condition is!true || false = false→ the fd is never closed.
bufs!(open_dirs) is a threadlocal scratch buffer whose live prefix is reset by open_dir_count on the next call, so open_dir is orphaned: opened, never stored anywhere, never closed.
Why existing code doesn't prevent it
The defer! at resolver.rs:4423 assumes that when store_fd = true, every fd in open_dirs[] was written into some DirEntry.fd at line 4698 and will be reused/closed later. That holds when needs_iter = true. When needs_iter = false — a DirEntry cache hit — line 4698 never runs, and the assumption is wrong. Before this PR that state was unreachable from bun test: the scanner cached with store_fd = true, so entries.fd was valid, so line 4460 took the queue_top.fd fast path and no fresh fd was opened or tracked. The PR creates the exact combination (DirEntry cached with fd = INVALID + resolver.store_fd = true) that reaches the hole.
Impact
One fd leaks per directory that (a) the scanner cached and (b) later appears in a module-resolution DirInfo walk — i.e. every test-file directory and each of its ancestors down to the first already-DirInfo-cached dir, on the first --watch run. The need_to_close_files() safety valve (~file_limit/2) caps it, so the OPEN_MAX failure the PR fixes cannot recur, but these are truly untracked fds — an unpaired acquisition per REVIEW.md's "Pair every acquisition with its release" rule — introduced in a mode this PR explicitly special-cases at line 2279 and does not test (the new test does not run --watch).
How to fix
Fix the resolver at the layer that owns the fd: at resolver.rs:4634-4643, when needs_iter = false and self.store_fd and the cached entries.fd is invalid, store open_dir into it (so the fd is cached and reused exactly as store_fd intends). Alternatively, close open_dir on that branch, or force the defer! to close by tracking per-slot whether the fd was stored. Storing it is the correct fix for store_fd = true semantics.
2 participants
What
Fixes #39852
Fixes #39783
bun testkept one directory fd open for every directory the test-file scanner visited. The scanner opened each directory withopenat(parent_fd, name)and passed the fd to the resolver withstore_fd = true, which cached it inDirEntry.fdforever.In a large repo that is thousands of fds. On macOS, libc's
posix_spawn_file_actions_adddup2rejects any fd number ≥OPEN_MAX(10240) withEBADF. Once the process holds that many fds, a new stdio pipe gets a number past 10240 and every pipedBun.spawn/child_processcall fails. That is theEBADF: bad file descriptor, posix_spawnin the issue.Change
The scanner owns each directory fd. Children hold an
Rc<Dir>of their parent, so the parent fd closes as soon as its last child has been opened. The resolver still caches the directory listing, just not the fd. The tree is still walked withopenat(parent_fd, name), so deep paths do not hitENAMETOOLONGin the open.Symlinked entries no longer keep a file fd open either (
Entry::kindwithstore_fd = false), which is thelinker = "isolated"case in #39783.600 sibling directories, fds open inside the test:
Test
test/cli/test/bun-test.test.ts: scans 257 directories and asserts the open-fd count stays below 64. Fails on 1.4.1 (271), passes here (13).