resolver: close the store_fd directory handle when readdir fails

[robobun]

Repro

Under any mode that caches directory fds (bun --hot, bun --watch, bun install, bun test), resolve a path whose parent directory can be opened but whose getdents64 fails:

FAILED=16 LEAKED=32   # bun --hot: 16 directories, 2 opens each via the bust-and-retry in VirtualMachine resolve

getdents64 failing after a successful open(O_DIRECTORY) is rare but reachable: a filesystem that returns EIO mid-iteration, a FUSE mount that disappears between the two syscalls, or a directory the process may search but not read. The runtime's NotFound retry in VirtualMachine also busts the cached Err and re-opens once per require.resolve, so an unresolvable directory leaks one fd on every call rather than once total.

Cause

read_directory_with_iterator opens the directory (or accepts one from the caller), then sets up a scopeguard to close it. The guard was gated on !had_handle && (!store_fd || need_to_close_files()): the "we never intended to cache this fd" condition. When store_fd is set and need_to_close_files() is false (always on Windows; on POSIX once the raised rlimit puts the process well under its fd budget), that condition is false and the guard never closes.

On the success path that is what we want: entries.fd = handle publishes the fd into the cached DirEntry. On the early-return paths it leaks: DirnameStore::append_slice(...)? and readdir(...) returning Err both return before the fd is published, and DirEntry has no Drop.

Fix

The guard now holds Cell<Option<Fd>> initialized from maybe_handle and is armed before any exit path. It closes whenever the handle has not been published, with the pre-existing close-after-publish behaviour for a freshly-opened fd we never intended to cache kept under !had_handle. open_dir writes into the same cell on a cache miss. handle_published is set when readdir returns Ok with e.fd == handle, so the success path is unchanged.

One exception to "close when not published": when the iterator is non-void (only the bun test scanner), readdir dispatches the fd to the iterator for every entry it yields, so a later iteration failure leaves queued ScanEntry { relative_dir: fd } values that the scan loop will still use. Closing there would be a use-after-close against those queued entries, so the Err arm with a non-void iterator keeps the pre-PR hold-and-leak behaviour. Every read_directory caller (load_as_file, install, lockfile) uses the void iterator and gets the close.

RealFS::kind() shares the same gate expression in its symlink branch; it is excluded here because its failure points (fstat, get_fd_path) route through rustix's linux_raw backend and cannot be driven by the LD_PRELOAD shim, so there is no fail-before proof available. It has been handed off separately.

Verification

test/js/bun/resolve/resolver-readdir-error-fd-leak.test.ts builds an LD_PRELOAD shim that makes syscall(SYS_getdents64, fd, ...) fail with EIO when the fd's /proc/self/fd target contains a marker string, then:

• runs bun --hot inner.ts (so resolver.store_fd = true, maybe_handle = None, void iterator), resolves one path per marker directory, and asserts FAILED=16 LEAKED=0 (FAILED proves the shim fired);
• runs bun test . over a tree with marker subdirectories so the scanner opens each one and hands the fd in with its non-void iterator, and asserts HELD=16 (the fds stay valid for queued work; this guards the use-after-close regression).

Before: LEAKED=32. After: LEAKED=0. The scanner test passes on both, by design.

test/js/bun/resolve/resolve.test.ts, test/cli/test/bun-test.test.ts, test/js/bun/resolve/import-meta.test.js, and test/cli/hot/hot.test.ts are unchanged.

---

no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/resolve/resolver-readdir-error-fd-leak.test.ts

[coderabbitai]

Walkthrough

The resolver now tracks directory handle publication and closes handles on failed reads. A Linux/glibc regression test forces getdents64 failures and checks both hot resolution and bun test scanner paths.

Changes

Resolver file descriptor cleanup

Layer / File(s)	Summary
Handle ownership and cleanup src/resolver/lib.rs	RealFS::read_directory_with_iterator tracks handle ownership and publication. It closes unpublished handles when cleanup is required and preserves handles after successful publication.
Failure-path regression coverage test/js/bun/resolve/resolver-readdir-error-fd-leak.test.ts	The test builds an LD_PRELOAD shim that forces getdents64 failures. It verifies descriptor cleanup for hot resolution and the bun test scanner path.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the primary fix: closing the directory handle when readdir fails.
Description check	✅ Passed	The description explains the problem, cause, fix, scope, and verification results in sufficient detail.

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — targeted fd-leak fix on the resolver's readdir error path.

What was reviewed:

• Traced all exit paths of read_directory_with_iterator against the new guard condition: error paths now close, and the store_fd success path is byte-for-byte unchanged (should_close_handle=false ∧ handle_published=true ⇒ no close).
• Checked drop order: handle_published: Cell<bool> is declared before _close_guard, so it's still live when the guard closure runs; the closure borrows it shared, which composes with the later .set(true).
• Confirmed no double-close on the readdir Err path — DirEntry has no Drop, so the partial value dropping does not close dir.fd.
• Test: skip gating (!isLinux || isMusl || !cc) matches the LD_PRELOAD/procfs requirements; beforeAll early-return + dir?. in afterAll is safe; pipes drained concurrently; process.exit(0) terminates --hot.

Extended reasoning...

Overview

The PR fixes an fd leak in src/resolver/lib.rs::read_directory_with_iterator. The function opens a directory, arms a scopeguard to close it, then interns the dirname and calls readdir. The old guard condition was !had_handle && (!store_fd || need_to_close_files()) — i.e. "close only if we never intended to cache this fd". When store_fd=true and the fd budget is comfortable, that condition is false, so the guard is inert. On the success path that's correct (the fd is published into the cached DirEntry), but on the two early-return paths (DirnameStore::append_slice? and readdir → Err) the fd is never published and leaks.

The fix adds a Cell<bool> handle_published that starts false and flips to true only after readdir returns Ok. The guard now closes when !had_handle && (should_close_handle || !handle_published). I walked every combination:

• had_handle — never closes (unchanged; caller owns it).
• fresh handle, error before readdir Ok — !handle_published ⇒ closes (the fix).
• fresh handle, store_fd, budget OK, readdir Ok — false || !true ⇒ keeps (unchanged; fd lives in e.fd and is moved into the BSSMap slot / leaked Box before the next ?).
• fresh handle, !store_fd or over-budget, readdir Ok — should_close_handle ⇒ closes (unchanged).

The readdir body (lines 1080–1110) sets dir.fd = handle when store_fd, but DirEntry has no Drop, so on Err the partial dir dropping does not close the handle — no double-close with the guard. Drop order is correct: handle_published outlives _close_guard, and Cell::get/set both take &self so the shared capture composes with the later mutation.

Security risks

None. This is a resource-leak fix on an error path; no new inputs are parsed and no trust boundary is crossed.

Level of scrutiny

Moderate. The resolver is hot, but this change is confined to the guard closure and one match arm — the success-path condition reduces to exactly the old expression once handle_published is true. The failure mode being fixed (getdents64 failing after a successful open) is rare, so regression risk on the common path is the main concern, and that path is provably unchanged.

Other factors

The test is well-constructed for a hard-to-reach syscall failure: an LD_PRELOAD shim fails SYS_getdents64 for marker directories, the inner process runs under --hot (so store_fd=true), and leaked fds are counted via /proc/self/fd. It's correctly gated to Linux+glibc with a compiler present, drains subprocess pipes concurrently, uses tempDir/bunEnv/bunExe, and asserts a combined {stdout, stderr, exitCode} object. The PR description shows before=32/after=0, and the neighboring resolve/hot suites are reported unchanged.

[claude]

LGTM — targeted fd-leak fix; success path is provably unchanged, error paths now close the freshly-opened handle.

Checked: guard condition when handle_published=true reduces to exactly the old should_close_handle, so no behaviour change on success and no new double-close. On the readdir Err / DirnameStore::append_slice Err paths the local DirEntry (no Drop) is discarded and the guard now closes — verified readdir sets dir.fd = handle only under store_fd, so the !store_fd close was already single. Drop order (_close_guard before handle_published: Cell) is correct. Test asserts FAILED=16 so it self-invalidates if getdents64 stops routing through libc syscall().

Extended reasoning...

Overview

Two files: a ~10-line change to the scopeguard in read_directory_with_iterator (src/resolver/lib.rs) and a new Linux-glibc-only regression test that LD_PRELOADs a shim forcing getdents64 to fail with EIO for marker directories, then counts leaked fds via /proc/self/fd.

Correctness of the guard change

The old guard computed !had_handle && (!store_fd || need_to_close_files()) once and closed on that. The new guard computes the same expression as close_even_if_published, and ORs in !handle_published. handle_published flips to true only after self.readdir(...) returns Ok. So on success the condition collapses to the old one — byte-for-byte identical behaviour, no new double-close and no regression in the store_fd && need_to_close_files() corner. On the two early-return paths (DirnameStore::append_slice(...)? and readdir Err), handle_published is still false, so any freshly-opened handle (!had_handle) is closed. I confirmed readdir sets dir.fd = handle before iteration only when store_fd, and DirEntry has no Drop, so the Err-path close is single. handle_published is a Cell declared before _close_guard, so it is still live when the guard drops; the non-move closure borrows it, and handle/had_handle outlive the guard as well.

Security risks

None. This is resource-lifetime bookkeeping on a directory fd the resolver already opened; no new inputs are trusted, no new syscalls are added.

Level of scrutiny

Moderate — resolver fd handling is load-bearing for --hot/--watch/bun test, but the change is a strict tightening of an existing guard whose success-path semantics are mechanically preserved. The one remaining ? after handle_published.set(true) (self.entries.put) already leaked the fd into a leaked Box<DirEntry> in the pre-PR code under store_fd, so no regression there.

Other factors

The comment-cop bot's three notes were addressed in follow-up commits (guard comments trimmed; variable names now carry the intent) and are marked resolved. The test is properly gated (skipIf(!isLinux || isMusl || !cc)), spreads bunEnv, drains stdout/stderr/exited concurrently, uses tempDir, and asserts FAILED=DIR_COUNT so it fails loudly rather than passing vacuously if the interposition stops working. The PR notes resolve.test.ts, import-meta.test.js, and hot.test.ts are unchanged.

[claude]

Beyond the inline finding, I also checked: drop order of handle_published vs _close_guard (Cell outlives the guard, so the borrowed .get() is valid); the success-path close condition is byte-identical to the old should_close_handle, so no new double-close; and the ? at self.entries.put(...) fires only after *entries_ptr = entries has moved the fd into persistent storage, so handle_published=true is accurate there.

Extended reasoning...

The scopeguard rewrite is sound for the !had_handle path it targets, and the LD_PRELOAD test proves the shim fired via FAILED=DIR_COUNT. The inline comment covers the one same-class gap (caller-supplied handle from Scanner). Deferring rather than approving because this is fd-lifecycle logic in a shared resolver path and the sibling-arm leak is exactly the "fix the whole class" case REVIEW.md flags.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@test/js/bun/resolve/resolver-readdir-error-fd-leak.test.ts`:
- Around line 144-176: Add a test file inside each marker directory that would
produce a detectable success signal if executed, then assert in the parent test
that this signal is absent. Update the fixture and assertions around the
generated `files` map and `LEAKED` result while preserving the existing leak and
process checks, so the test proves marker directories could not be enumerated
and the shim exercised the intended cleanup path.

🪄 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: c8585c7d-dd7d-425a-913d-2e0a2bf68478

📥 Commits

Reviewing files that changed from the base of the PR and between 2190ef1 and e3694ab.

📒 Files selected for processing (2)

• src/resolver/lib.rs
• test/js/bun/resolve/resolver-readdir-error-fd-leak.test.ts

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/resolver/lib.rs:1233-1239 — RealFS::kind() (~250 lines below, lib.rs:1481-1491) has the byte-identical (!store_fd || need_to_close_files) && !existing_fd.is_valid() gate and the same leak: with store_fd and a fresh open, when fstat? or get_fd_path? early-return the guard's else-if writes file into the stack-local cache (EntryCache is #[derive(Clone, Copy)], no Drop) and the fd is orphaned. This is pre-existing, but REVIEW.md is explicit that same-class sibling sites are one concern for the same PR — the same handle_published-style flip applies directly there.

  Extended reasoning...

  What the bug is

  The scopeguard in RealFS::kind()'s symlink branch (src/resolver/lib.rs:1481-1488) uses the same success-path-only gate this PR just removed from read_directory_with_iterator:

  let _guard = scopeguard::guard(file, move |file| {
      if (!store_fd || need_to_close_files) && !existing_fd.is_valid() {
          let _ = bun_sys::close(file);
      } else if bun_core::feature_flags::STORE_FILE_DESCRIPTORS {
          unsafe { (*cache_ptr).fd = file };
      }
  });
  let file_stat = bun_sys::fstat(*_guard)?;
  symlink = bun_sys::get_fd_path(*_guard, &mut outpath)?;

  When store_fd = true and need_to_close_files() = false (always on Windows per lib.rs:1534-1541; usually on POSIX after the rlimit raise per 1546-1547), the first branch is false regardless of whether the fd was ever published. The else-if writes the fd into *cache_ptr, which points at a stack-local EntryCache — and EntryCache is #[derive(Clone, Copy)] (src/resolver/fs.rs:116-117), so it has no Drop. On the ? early-return the stack local is discarded and the fd leaks.

  Step-by-step proof

  Under bun --hot / bun test (so store_fd = true), resolving through a symlink where the open succeeds but the follow-up syscall fails:

  1. lstat at 1449 reports SymLink; existing_fd is invalid (fresh entry).
  2. Line 1460-1464: open_file_absolute_z(...)?.into_raw() opens the target and disarms Drop — file is now a raw Fd owned by nobody.
  3. need_to_close_files() is false (fd budget not exceeded).
  4. Line 1490: bun_sys::fstat(*_guard)? returns Err(EIO) — e.g. a FUSE mount vanished between open and fstat, or the underlying device returned EIO. (Or line 1491: get_fd_path, i.e. readlink("/proc/self/fd/N") on Linux, fails with ENAMETOOLONG or EIO.)
  5. The ? unwinds. _guard drops first (reverse declaration order): (!store_fd || need_to_close_files) && !existing_fd.is_valid() = (false || false) && true = false. Else-if fires: (*cache_ptr).fd = file writes the fd into the stack-local cache.
  6. cache drops — but EntryCache is Copy, so nothing runs. kind() returns Err. The fd is orphaned.

  This is the exact mechanism the PR description names for read_directory_with_iterator: "the guard is gated on 'we never intended to cache this fd' … On the success path that is what we want … On the early-return paths it leaks."

  Why existing code doesn't prevent it

  The guard was written assuming the else-if's "store" branch always publishes into a value that survives the return. On the Ok path that's true — cache is returned by value and the caller stashes it in the entry map. On the Err path cache never leaves the stack frame, and EntryCache deliberately has no Drop (it's Copy; the fd is meant to be owned by the long-lived cache, not the struct). Nothing else in kind() touches file after the guard is armed.

  Impact

  Same rarity class as the bug this PR fixes: open succeeds, then the very next syscall on that fd fails (EIO on flaky media, a FUSE mount disappearing between the two calls, or readlink of /proc/self/fd/N hitting ENAMETOOLONG). One fd leaked per failing symlink resolve. Under --hot/--watch the resolver's bust-and-retry can re-hit it repeatedly, same as the readdir case. Low absolute impact, but it's the identical bug class in the same file, ~250 lines below, using the byte-identical (!store_fd || need_to_close_files) gate expression.

  Why this belongs in this PR

  REVIEW.md, under Correctness: the bug class, not the bug:

  Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep). Grep for every sibling site sharing the pattern … If a site is intentionally excluded, say so in the PR.

  Grepping (!store_fd || need_to_close_files) finds exactly this site. The PR already extended the fix to the had_handle sibling arm after the earlier review round; kind() is the remaining sibling of the same gate.

  Fix

  The same shape as the PR's own fix: track a published flag that flips only after the fd has been written into a value the Ok path returns, and close on drop whenever !published (regardless of store_fd). Concretely, move (*cache_ptr).fd = file out of the guard to just before the Ok(cache) tail (after get_fd_path? succeeds), and have the guard close whenever the fd was freshly opened and not yet stored — e.g.:

  let published = core::cell::Cell::new(false);
  let _guard = scopeguard::guard((), |()| {
      if !existing_fd.is_valid() && (!published.get() || !store_fd || need_to_close_files) {
          let _ = bun_sys::close(file);
      }
  });
  let file_stat = bun_sys::fstat(file)?;
  symlink = bun_sys::get_fd_path(file, &mut outpath)?;
  file_kind = kind_from_mode(file_stat.st_mode as bun_sys::Mode);
  if store_fd && bun_core::feature_flags::STORE_FILE_DESCRIPTORS {
      cache.fd = file;
      published.set(true);
  }

  (Note FilenameStore::instance().append_slice(symlink)? at 1504 is another ? after the guard has dropped — moving the store to just before the return, or flipping published right after get_fd_path succeeds, keeps that path safe too.)

[robobun]

On the kind() sibling at lib.rs:1481-1491: it shares the same gate expression and the same leak shape, but the failure points there are fstat and get_fd_path, both of which route through rustix's linux_raw backend (inline asm, no libc), so the LD_PRELOAD shim cannot reach them and I have no way to produce a fail-before proof for that change in this PR. I have handed it off separately so it gets its own targeted reproduction; this PR stays scoped to read_directory_with_iterator, whose failure point (getdents64 via libc::syscall) is the one the tests here can actually drive.

[coderabbitai]

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)

1268-1270: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Do not mark an unstored handle as published.

When store_fd is false, readdir does not assign handle to DirEntry.fd. Line 1269 still marks a caller-supplied handle as published. The guard then skips close(), so each successful call leaks that descriptor.

Set publication only when entries.fd retains the handle. Add a regression case that passes Some(Fd) with store_fd: false.

Proposed fix

 Ok(e) => {
-    handle_published.set(true);
+    handle_published.set(store_fd && e.fd.is_valid());
     e
 }

As per coding guidelines, “Pair every resource acquisition with release at the acquisition site, including all early-return, error, success, and lifecycle paths.”

🤖 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 1268 - 1270, Update the success branch
around the visible handle-publication logic so `handle_published` is set only
when `DirEntry.fd` actually retains the supplied handle; when `store_fd` is
false, leave it unset so the descriptor is closed. Add a regression test
covering `Some(Fd)` with `store_fd: false` and verify the descriptor is
released.

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 1268-1270: Update the success branch around the visible
handle-publication logic so `handle_published` is set only when `DirEntry.fd`
actually retains the supplied handle; when `store_fd` is false, leave it unset
so the descriptor is closed. Add a regression test covering `Some(Fd)` with
`store_fd: false` and verify the descriptor is released.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0b168859-1b1a-49e4-a6d6-ca5fa4b80f32

📥 Commits

Reviewing files that changed from the base of the PR and between e3694ab and 3889f07.

📒 Files selected for processing (2)

• src/resolver/lib.rs
• test/js/bun/resolve/resolver-readdir-error-fd-leak.test.ts

[robobun]

CI at 4502d27: all 9 failures are marked flaky by the runner (fastutf8stream-reopen, run-crash-handler, napi GC-condition, and six parallel-batch-only failures on Windows/alpine). None touch the resolver, the new test, or any store_fd path. The new test passed on every Linux glibc lane; the filesystem_router.test.ts aarch64 segfault from the previous build did not recur (that test deliberately races router.reload() against Bun.build on the shared directory-entry cache; this change is behavior-neutral for Bun.build's store_fd=false path, and it passed 5/5 locally).

The diff is ready. All review threads are resolved; the last finding was marked pre-existing by the reviewer.