diff --git a/src/runtime/cli/test/Scanner.rs b/src/runtime/cli/test/Scanner.rs index bd79d0ea47b4..30698f15555e 100644 --- a/src/runtime/cli/test/Scanner.rs +++ b/src/runtime/cli/test/Scanner.rs @@ -1,18 +1,17 @@ use std::collections::VecDeque; +use std::rc::Rc; use bun_alloc::AllocError; use bun_bundler::Transpiler; use bun_bundler::options::BundleOptions; use bun_collections::index_sort; -#[cfg(not(windows))] -use bun_core::ZStr; use bun_core::{StringOrTinyString, strings}; use bun_output::{declare_scope, scoped_log}; use bun_paths::resolve_path::{join_abs_string_buf_checked, platform}; use bun_paths::{self, PathBuffer}; use bun_ptr::Interned; use bun_resolver::fs::{self as fs, DirEntryIterator, EntriesOption, FileSystem}; -use bun_sys::{self, Fd}; +use bun_sys::{Dir, Fd}; declare_scope!(jest, hidden); @@ -33,13 +32,16 @@ pub struct Scanner<'a> { pub(crate) options: &'a BundleOptions<'a>, pub(crate) has_iterated: bool, pub(crate) search_count: usize, + /// The directory being iterated; its fd closes once every child `ScanEntry` has been opened. + current_dir: Option>, } // FIFO queue of scan entries (pop_front / push_back). pub(crate) type Fifo = VecDeque; pub struct ScanEntry { - pub(crate) relative_dir: Fd, + /// `None` for children of the root, which are opened by absolute path. + pub(crate) relative_dir: Option>, // `'static` is sound here: borrows from FileSystem.dirname_store, a // process-lifetime arena that is never reset. pub(crate) dir_path: &'static [u8], @@ -61,11 +63,11 @@ bun_core::oom_from_alloc!(ScanError); #[repr(transparent)] struct ScannerDirIter<'a>(*mut Scanner<'a>); impl<'a> DirEntryIterator for ScannerDirIter<'a> { - fn next(&self, entry: &mut fs::Entry, fd: Fd) { + fn next(&self, entry: &mut fs::Entry, _fd: Fd) { // SAFETY: `self.0` is `&mut Scanner` for the duration of // `read_directory_with_iterator`; no other live `&mut` alias exists // while the resolver walks entries. - unsafe { (*self.0).next(entry, fd) } + unsafe { (*self.0).next(entry) } } } @@ -86,6 +88,7 @@ impl<'a> Scanner<'a> { open_dir_buf: PathBuffer::uninit(), has_iterated: false, search_count: 0, + current_dir: None, }) } @@ -161,8 +164,6 @@ impl<'a> Scanner<'a> { // you typed "." and we already scanned it if !self.has_iterated { if let EntriesOption::Entries(entries) = root { - let fd = entries.fd; - debug_assert!(fd != Fd::INVALID); // Collect first so `self.next(…)` doesn't overlap the // `entries.data` borrow. // this branch is taken when the resolver already has @@ -183,87 +184,55 @@ impl<'a> Scanner<'a> { for entry_ptr in entry_ptrs { // SAFETY: `EntryMap` stores `*mut Entry` into the // process-static `EntryStore`; valid for `'static`. - self.next(unsafe { &mut *entry_ptr }, fd); + self.next(unsafe { &mut *entry_ptr }); } } } while let Some(entry) = self.dirs_to_scan.pop_front() { - debug_assert!(entry.relative_dir.is_valid()); + let parts2: [&[u8]; 2] = [entry.dir_path, entry.name.slice()]; + let Some(path2) = self.fs().abs_buf_checked(&parts2, &mut scan_dir_buf) else { + continue; + }; + let (parent, rel_path): (Fd, &[u8]) = match &entry.relative_dir { + Some(parent) => (parent.fd, entry.name.slice()), + None => (Fd::cwd(), path2), + }; #[cfg(not(windows))] - { - let dir = entry.relative_dir; - - let parts2: [&[u8]; 2] = [entry.dir_path, entry.name.slice()]; - let buf_len = self.open_dir_buf.len(); - let Some(path2) = self - .fs() - .abs_buf_checked(&parts2, &mut self.open_dir_buf[..buf_len - 1]) - else { - continue; - }; - let path2_len = path2.len(); - self.open_dir_buf[path2_len] = 0; - let name_len = entry.name.slice().len(); - // SAFETY: open_dir_buf[path2_len] == 0 written immediately above - let path_z = unsafe { - ZStr::from_raw( - self.open_dir_buf.as_ptr().add(path2_len - name_len), - name_len, - ) - }; - // bun.openDir → sys.openat(dir, pathZ, O.DIRECTORY|O.CLOEXEC|O.RDONLY, 0).stdDir() - let Ok(child_fd) = bun_sys::open_dir_at(dir, path_z.as_bytes()) else { - continue; - }; - let child_dir = bun_sys::Dir::from_fd(child_fd); - let path2 = self - .fs() - .dirname_store - .append_slice(&self.open_dir_buf[..path2_len]) - .map_err(|_| ScanError::OutOfMemory)?; - FileSystem::set_max_fd(child_dir.fd.native()); - let _ = self - .read_dir_with_name(path2, Some(child_dir)) - .map_err(|_| ScanError::OutOfMemory)?; - } + let opened = bun_sys::open_dir_at(parent, rel_path); #[cfg(windows)] - { - let fs = self.fs(); - let parts2: [&[u8]; 2] = [entry.dir_path, entry.name.slice()]; - let Some(path2) = fs.abs_buf_checked(&parts2, &mut self.open_dir_buf) else { - continue; - }; - let Ok(child_fd) = - bun_sys::open_dir_no_renaming_or_deleting_windows(Fd::INVALID, path2) - else { - continue; - }; - let child_dir = bun_sys::Dir::from_fd(child_fd); - let stored = fs - .dirname_store - .append_slice(path2) - .map_err(|_| ScanError::OutOfMemory)?; - let _ = self - .read_dir_with_name(stored, Some(child_dir)) - .map_err(|_| ScanError::OutOfMemory)?; - } + let opened = bun_sys::open_dir_no_renaming_or_deleting_windows(parent, rel_path); + // Dropping `entry` releases the parent fd once its last child is opened. + drop(entry); + let Ok(child_fd) = opened else { + continue; + }; + let child_dir = Rc::new(Dir::from_fd(child_fd)); + let path2 = self + .fs() + .dirname_store + .append_slice(path2) + .map_err(|_| ScanError::OutOfMemory)?; + self.current_dir = Some(Rc::clone(&child_dir)); + let result = self.read_dir_with_name(path2, Some(child_dir.fd)); + self.current_dir = None; + result.map_err(|_| ScanError::OutOfMemory)?; } Ok(()) } + /// `handle` stays owned by the caller; the resolver caches the listing but not the fd. fn read_dir_with_name( &mut self, name: &[u8], - handle: Option, + handle: Option, ) -> crate::Result<&'static mut EntriesOption> { let fs_ptr = self.fs; let iter = ScannerDirIter(std::ptr::from_mut::>(self)); - let raw = handle.map(bun_sys::Dir::into_raw); // SAFETY: borrows only the `fs` field; re-entrant access is serialised by `RealFS.entries_mutex`. unsafe { &mut (*fs_ptr).fs } - .read_directory_with_iterator(name, raw, 0, true, iter) + .read_directory_with_iterator(name, handle, 0, false, iter) .map_err(Into::into) } @@ -359,13 +328,13 @@ impl<'a> Scanner<'a> { && !self.matches_path_ignore_pattern(name) } - pub(crate) fn next(&mut self, entry: &mut fs::Entry, fd: Fd) { + pub(crate) fn next(&mut self, entry: &mut fs::Entry) { let name = entry.base_lowercase(); self.has_iterated = true; // SAFETY: `self.fs` is the process singleton. let real_fs = unsafe { &raw mut (*self.fs).fs }; // SAFETY: caller holds `entries_mutex`; the direct path is single-threaded. - match unsafe { entry.kind(real_fs, true) } { + match unsafe { entry.kind(real_fs, false) } { fs::EntryKind::Dir => { if (!name.is_empty() && name[0] == b'.') || name == b"node_modules" { return; @@ -402,7 +371,7 @@ impl<'a> Scanner<'a> { self.search_count += 1; self.dirs_to_scan.push_back(ScanEntry { - relative_dir: fd, + relative_dir: self.current_dir.clone(), // SAFETY: StringOrTinyString is repr(C) POD ([u8;31] + u8) with // no Drop. Upstream type lacks Clone/Copy, so bitwise-copy here. name: unsafe { core::ptr::read(&raw const entry.base_) }, diff --git a/src/runtime/cli/test_command.rs b/src/runtime/cli/test_command.rs index a6927faaf87a..6104233d3715 100644 --- a/src/runtime/cli/test_command.rs +++ b/src/runtime/cli/test_command.rs @@ -2276,12 +2276,7 @@ impl TestCommand { debugger: core::mem::take(&mut ctx.runtime_options.debugger), log: core::ptr::NonNull::new(ctx.log), env_loader: core::ptr::NonNull::new(&raw mut *env_loader), - // we must store file descriptors because we reuse them for - // iterating through the directory tree recursively - // - // 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, smol: ctx.runtime_options.smol, is_main_thread: true, ..Default::default() diff --git a/test/cli/test/bun-test.test.ts b/test/cli/test/bun-test.test.ts index 21ec163324af..49a4905e9b14 100644 --- a/test/cli/test/bun-test.test.ts +++ b/test/cli/test/bun-test.test.ts @@ -2061,4 +2061,34 @@ describe.concurrent("test file discovery (scanner)", () => { }, ); } + + // https://github.com/oven-sh/bun/issues/39852 + test.skipIf(isWindows)("does not keep a directory fd open per scanned directory", async () => { + const N = 64; + const files: Record = {}; + for (let i = 0; i < N; i++) { + files[`sub${i}/a/b/c/.gitkeep`] = ""; + } + files["sub0/probe.test.ts"] = /* ts */ ` + import { test } from "bun:test"; + import { readdirSync } from "node:fs"; + test("probe", () => { + console.log("OPEN_FDS=" + readdirSync(process.platform === "linux" ? "/proc/self/fd" : "/dev/fd").length); + }); + `; + using dir = tempDir("scanner-dir-fds", files); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", "probe"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // 4N+1 directories are scanned; none of them may stay open. + expect(Number(stdout.match(/OPEN_FDS=(\d+)/)?.[1])).toBeLessThan(N); + expect(stderr).toContain(" 1 pass"); + expect(exitCode).toBe(0); + }); });