Repository navigation
Conversation
|
Updated 8:05 PM PT - Sep 7th, 2026
❌ @robobun, your commit 75f1b95 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32392That installs a local version of the PR into your bun-32392 --bun |
|
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 an ChangesEntry-point directory enumeration optimization
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/resolver/resolver.rs (1)
6179-6187:⚠️ Potential issue | 🟠 MajorFix bin-folder discovery to use the same direct-stat path when entries are incomplete.
The
marker_kind!macro correctly handles incomplete entries by falling back to directstat(line 6137–6144). However, the bin-folder discovery at line 6187 requiresentries!().has_comptime_query(b"node_modules"), which will always fail when the lazy leaf's entry map is incomplete — even thoughmarker_kind!on line 6179 succeeded via direct stat and setHasNodeModulestrue.When
entries_completeis false (incomplete lazy leaf),node_modulescan be proven to exist by direct stat but the subsequenthas_comptime_querycheck skips the.bindiscovery anyway, losing the capability that the first check proved.Replace the check at line 6187 with the same condition-and-fallback logic
marker_kind!uses, or extract the direct-stat path into a shared helper.🤖 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 6179 - 6187, The bin-folder discovery check uses a condition that fails for incomplete lazy leaf entries, even when marker_kind! on the preceding check successfully proved node_modules exists via direct stat. Replace the has_comptime_query check in the bin-folder discovery block with the same condition-and-fallback logic that marker_kind! uses (the direct stat path for incomplete entries), or extract that fallback logic into a shared helper function to avoid the duplication and ensure consistent behavior when entries are incomplete.
🤖 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/resolver/resolver.rs`:
- Around line 5756-5779: The RealFS::kind() call in the entry_point_hint fast
path is passed self.store_fd which allows it to acquire and cache a file
descriptor, but the returned LoadResult hardcodes file_fd: FD::INVALID instead
of propagating cache.fd like the normal resolution path does. Replace the
hardcoded FD::INVALID in the LoadResult with cache.fd to properly propagate the
file descriptor that RealFS::kind() may have opened and cached when
store_fd=true.
---
Outside diff comments:
In `@src/resolver/resolver.rs`:
- Around line 6179-6187: The bin-folder discovery check uses a condition that
fails for incomplete lazy leaf entries, even when marker_kind! on the preceding
check successfully proved node_modules exists via direct stat. Replace the
has_comptime_query check in the bin-folder discovery block with the same
condition-and-fallback logic that marker_kind! uses (the direct stat path for
incomplete entries), or extract that fallback logic into a shared helper
function to avoid the duplication and ensure consistent behavior when entries
are incomplete.
🪄 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: b51b1b9f-82ba-405d-8a19-bac661a6dcd4
📒 Files selected for processing (5)
src/jsc/VirtualMachine.rssrc/resolver/dir_info.rssrc/resolver/fs.rssrc/resolver/resolver.rstest/js/bun/resolve/bun-main-entry-point.test.ts
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 (1)
src/resolver/resolver.rs (1)
627-633:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftScope
entry_point_hintto the actual entry path.The hint is a boolean, so while the VM guard is active, any nested
load_as_file()call can take this stat-only path. Directory entry points can re-enter here viapackage.jsonmain/browser/index resolution, wherepathis package metadata rather than the canonical CLI specifier; on case-insensitive filesystems this can return the metadata spelling withdiff_case: Noneand bypass the normal directory-entry lookup for a non-entry file. Store the hinted file/dir path and requirepath == hint_pathfor the file shortcut, and the matching parent directory for the lazy-leaf shortcut.Also applies to: 4435-4436, 5761-5767
🤖 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 627 - 633, Replace the boolean `entry_point_hint` field with a field that stores the actual canonicalized entry path (or None if not set). At the anchor location (resolver.rs 627-633), change the field to store the hinted file/directory path instead of just a boolean. At the sibling location (resolver.rs 4435-4436), update the `load_as_file` stat-only shortcut to only apply when the current `path` parameter exactly matches the stored hint_path. At the other sibling location (resolver.rs 5761-5767), update the lazy-leaf shortcut logic to additionally verify that the current path's parent directory matches the hint's parent directory before applying the optimization.
🤖 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/resolver.rs`:
- Around line 627-633: Replace the boolean `entry_point_hint` field with a field
that stores the actual canonicalized entry path (or None if not set). At the
anchor location (resolver.rs 627-633), change the field to store the hinted
file/directory path instead of just a boolean. At the sibling location
(resolver.rs 4435-4436), update the `load_as_file` stat-only shortcut to only
apply when the current `path` parameter exactly matches the stored hint_path. At
the other sibling location (resolver.rs 5761-5767), update the lazy-leaf
shortcut logic to additionally verify that the current path's parent directory
matches the hint's parent directory before applying the optimization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52922a2b-7921-4469-bc0f-b37225d3ef14
📒 Files selected for processing (1)
src/resolver/resolver.rs
|
Addressed the entry-point hint scoping in f57a249. The hint is now the canonicalized entry path (was a boolean). The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 (1)
src/resolver/resolver.rs (1)
5777-5802:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t let symlinked entry points take the stat-only shortcut.
A symlink to a regular file can satisfy this
EntryKind::Filecheck and return before the normal directory-entry path records symlink metadata. Later symlink finalization depends on finding the basename in the directory entries around Line 1746, but the lazy leaf stores an incomplete empty listing, so a symlinked main can resolve relative imports from the symlink directory instead of the real target. Fall through for symlinks, or populate/return the realpath state here and add a symlinked-entry regression test.🤖 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 5777 - 5802, The current code takes a fast path optimization when finding the entry-point file, but this shortcut incorrectly handles symlinks. When the entry kind is determined to be a File, a symlink to a file will also match this check, causing the code to return early without recording proper symlink metadata. Later symlink finalization depends on finding the basename in directory entries but gets an incomplete listing. Add an additional check after verifying that cache.kind equals EntryKind::File to ensure the entry is not a symlink. If it is a symlink, skip the dec_ret! early return and fall through to the normal directory-entry path that properly handles symlink resolution and metadata.
🤖 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/resolver.rs`:
- Around line 5777-5802: The current code takes a fast path optimization when
finding the entry-point file, but this shortcut incorrectly handles symlinks.
When the entry kind is determined to be a File, a symlink to a file will also
match this check, causing the code to return early without recording proper
symlink metadata. Later symlink finalization depends on finding the basename in
directory entries but gets an incomplete listing. Add an additional check after
verifying that cache.kind equals EntryKind::File to ensure the entry is not a
symlink. If it is a symlink, skip the dec_ret! early return and fall through to
the normal directory-entry path that properly handles symlink resolution and
metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1552e2c7-1900-40c5-bc34-89676c4e9e95
📒 Files selected for processing (2)
src/jsc/VirtualMachine.rssrc/resolver/resolver.rs
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 (1)
test/js/bun/resolve/bun-main-entry-point.test.ts (1)
33-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin the dropped-privilege child to an accessible cwd.
setprivdrops to uid/gid65534, but the child still inherits the test runner’s cwd. If the checkout/workdir is not traversable bynobody, Bun can fail before exercising the entry script. Setcwd: "/tmp"alongsideHOME: "/tmp"so this stays hermetic.As per coding guidelines, tests must be hermetic and subprocess tests should isolate setup from host-specific state.
🧪 Proposed fix
await using proc = Bun.spawn({ cmd: ["setpriv", `--reuid=${NOBODY}`, `--regid=${NOBODY}`, "--clear-groups", bunExe(), scriptPath], // HOME points at a world-writable dir so `nobody` can reach any cache/config. env: { ...bunEnv, HOME: "/tmp" }, + cwd: "/tmp", stdout: "pipe", stderr: "pipe", });🤖 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 `@test/js/bun/resolve/bun-main-entry-point.test.ts` around lines 33 - 40, The runAsNobody function spawns a child process with dropped privileges but doesn't set the cwd for the child process, causing it to inherit the test runner's current working directory which may not be traversable by the nobody user. Add cwd: "/tmp" to the options object passed to Bun.spawn() alongside the existing HOME: "/tmp" environment variable to ensure the child process runs from a directory accessible to the unprivileged user and remains hermetic.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.
Inline comments:
In `@test/js/bun/resolve/bun-main-entry-point.test.ts`:
- Around line 82-88: The multi-line comment block describing the symlink test
behavior (starting at line 82 in bun-main-entry-point.test.ts) describes the old
behavior where the stat fast path sees through the symlink. Update this comment
to reflect the new intended behavior where symlinked entry points fall through
to the normal directory-listing path instead. The test code itself is correct;
only the documentation comment needs to be updated to accurately describe what
the test validates.
---
Outside diff comments:
In `@test/js/bun/resolve/bun-main-entry-point.test.ts`:
- Around line 33-40: The runAsNobody function spawns a child process with
dropped privileges but doesn't set the cwd for the child process, causing it to
inherit the test runner's current working directory which may not be traversable
by the nobody user. Add cwd: "/tmp" to the options object passed to Bun.spawn()
alongside the existing HOME: "/tmp" environment variable to ensure the child
process runs from a directory accessible to the unprivileged user and remains
hermetic.
🪄 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: ea734922-ea78-47ee-bf38-cbbc4a4b48a0
📒 Files selected for processing (2)
src/resolver/resolver.rstest/js/bun/resolve/bun-main-entry-point.test.ts
There was a problem hiding this comment.
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/bun-main-entry-point.test.ts`:
- Around line 36-39: The test currently uses the shared global /tmp directory
for both the cwd and HOME environment variable when spawning the nobody child
process, which creates a dependency on host-local state and violates test
hermeticity. Instead of hardcoding "/tmp", create a disposable temporary
directory that is world-accessible (with appropriate permissions so the nobody
user can traverse and access it) before the test runs, use this directory path
for both the cwd property and the HOME environment variable in the env object,
and ensure the temporary directory is cleaned up after the test completes or in
a cleanup handler to leave no artifacts behind.
🪄 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: 2eeeb687-dd9f-4215-a869-1ece0ebf7e11
📒 Files selected for processing (1)
test/js/bun/resolve/bun-main-entry-point.test.ts
There was a problem hiding this comment.
No bugs found and all prior feedback is addressed, but this threads a new DirEntry.complete invariant and a lifetime-erased entry_point_hint through the core resolver hot path — worth a human pass on the cache-completeness contract and the get_entries_const re-read behavior change.
Extended reasoning...
Overview
This PR changes how Bun resolves the process entry point so that bun <script> no longer enumerates the script's parent directory. It touches four production files: src/jsc/VirtualMachine.rs (sets/clears a new entry_point_hint around _resolve via a scopeguard over a raw *mut Self), src/resolver/resolver.rs (~250 lines: new entry_point_hint field, lazy_leaf gate in dir_info_cached_miss, a marker_kind! macro in dir_info_uncached, and a stat-only fast path in load_as_file), src/resolver/fs.rs (new DirEntry.complete flag plus re-read of incomplete entries in entries_at and read_directory_with_iterator), and src/resolver/dir_info.rs (get_entries_const rerouted through get_entries_ref(0) so incomplete entries are upgraded before iteration). Three new tests cover a 0711 non-listable directory under a setpriv-dropped nobody child, a listable control, and a symlinked entry point.
Security risks
None that stand out. The fast path is gated on source == MAIN_FILE_NAME with an exact byte-equality match against a canonicalized absolute path, and only short-circuits when kind == File with no symlink involved; everything else falls through to the existing path. No auth, crypto, or permission surfaces are touched.
Level of scrutiny
High. The resolver is the production-critical path that runs on every bun <script> startup, and this change introduces a new cross-cutting invariant: DirEntry.complete == false means the cached listing is a lie, and every consumer that iterates data must upgrade it first. The PR routes the known iteration entry points (entries_at, read_directory_with_iterator, get_entries_const) through the upgrade, but a human should confirm no other consumer reads DirEntry.data directly. Separately, get_entries_const previously did a pure index lookup and now (via entries_at) can issue a filesystem re-read — a behavior change worth a glance at its callers. The entry_point_hint uses unsafe { bun_ptr::detach_lifetime(...) } to erase a borrow to 'static, cleared by a scopeguard holding a raw *mut Self; the SAFETY comments are convincing, but unsafe lifetime erasure in a long-lived struct field is exactly the kind of thing that benefits from a second pair of eyes.
Other factors
The author has addressed every prior review comment (CodeRabbit's fd-propagation and hermetic-tempdir notes; my three nits on _stderr, reserve(64) placement, and .concurrent), and the bug-hunting system found nothing on the latest revision. The PR description is thorough, the code is well-commented, and the scoping fix in f57a249 (hint as exact path rather than a flag) is the right shape. The two CI failures in robobun's report are pre-existing build-rust linker warnings and musl build-cpp infra failures, unrelated to this diff. This looks correct to me, but the combination of a new cache invariant, a semantic change to a "const" accessor, and unsafe lifetime erasure across ~300 lines of resolver internals puts it outside what I'd approve without a human reviewer.
There was a problem hiding this comment.
No issues found on this pass, but this reworks the resolver's directory-cache completeness contract (new DirEntry.complete flag, lazy-leaf skip, marker_kind! stat fallback) plus an unsafe lifetime-erased hint in VirtualMachine::_resolve, and Build #62801 is still showing failures — worth a maintainer look before merge.
Extended reasoning...
Overview
This PR optimizes bun <script> startup by resolving the process entry point via a single stat instead of readdir-ing its parent directory. It touches five Rust files in the resolver/VM (src/resolver/{resolver,fs,lib,dir_info}.rs, src/jsc/VirtualMachine.rs) and adds four tests to test/js/bun/resolve/bun-main-entry-point.test.ts. The core mechanism is a new DirEntry.complete: bool flag marking cache entries created without a full readdir, an entry_point_hint: Option<&'static [u8]> on Resolver that gates a load_as_file short-circuit and a dir_info_cached_miss lazy-leaf skip, and a marker_kind! macro in dir_info_uncached that resolves package.json/tsconfig.json/jsconfig.json/node_modules by direct stat when the listing is incomplete. Three live-read paths (entries_at in both fs.rs and lib.rs, read_directory_with_iterator) and get_entries_const were updated to upgrade an incomplete entry to a full read before any consumer iterates it.
Security risks
None identified. The change is a read-path optimization; it does not touch auth, crypto, network, or permissions. The new setpriv-as-nobody test helper drops privileges (does not escalate) and is gated to Linux+root CI lanes. The unsafe { detach_lifetime(...) } in VirtualMachine::_resolve is a soundness concern rather than a security one — the scopeguard clears the hint on every exit path, and the comment documents why the borrow cannot outlive _resolve.
Level of scrutiny
High. The module resolver is on the hot path of every bun invocation and every import/require, and this PR changes its directory-cache completeness invariant: any consumer that iterates a DirEntry's map without going through one of the upgraded re-read paths would now silently miss files when the entry-point's own directory is involved. The PR appears to cover the known iteration sites (and the new "incomplete-cache upgrade" test exercises one), but verifying that coverage is complete — including the bundler, glob walker, router, shell completions, and test scanner mentioned in the description — is exactly the kind of cross-cutting check a maintainer familiar with the resolver should do. The care_about_bin_folder interaction, case-insensitive-FS handling, and the raw-pointer scopeguard in _resolve each add a dimension that benefits from human review.
Other factors
- CI: robobun reports failures in Build #62801 on commit 7bcc1b1; the only commit since (d1f00de) adds failure-path stderr surfacing to two tests and would not fix a substantive failure. The red build should be understood before merge.
- Review history: All seven inline threads (CodeRabbit's fd-propagation, comment-accuracy, hermetic-tempdir notes; my four test-hygiene nits) are resolved, and the author has been responsive and precise in each fix. The
entry_point_hintwas already tightened from a boolean to an exact-path match (f57a249) to keep nested resolutions on the normal path. - Tests: Good coverage of the new behavior (0711 non-listable dir, listable control, symlinked entry, incomplete-cache upgrade), though the two privilege-drop tests only run on Linux-as-root lanes.
- Not simple/mechanical: ~300 lines of resolver logic change with a new cache invariant and unsafe lifetime handling — outside the bar for bot auto-approval.
There was a problem hiding this comment.
All prior review feedback is addressed and the bug hunter found nothing, but this threads a new DirEntry.complete invariant through the resolver's directory cache and adds an unsafe lifetime-erased hint in VirtualMachine::_resolve — worth a maintainer's eyes on the cache-completeness contract and the scopeguard before merge.
Extended reasoning...
Overview
This PR changes how bun <script> resolves its entry point: instead of readdir-ing the script's parent directory (O(siblings)), it stats the single canonicalized path and probes the four config markers (package.json, tsconfig.json, jsconfig.json, node_modules) directly. Touches src/resolver/resolver.rs (~250 lines: entry_point_hint field, lazy_leaf gating in dir_info_cached_miss, marker_kind! macro in dir_info_uncached, fast path in load_as_file), src/resolver/{fs.rs,lib.rs,dir_info.rs} (new DirEntry.complete flag and re-read-on-incomplete in entries_at / read_directory_with_iterator / get_entries_const), src/jsc/VirtualMachine.rs (sets/clears the hint around the entry resolve), and adds four tests.
Security risks
None identified. The change narrows when a directory is enumerated; it does not relax any permission check, parse untrusted input, or expose data. The new unsafe in VirtualMachine::_resolve (detach_lifetime on normalized_specifier + a raw *mut Self captured by a scopeguard) is scoped to the duration of _resolve and the guard clears the hint on every exit, so the erased borrow cannot outlive its backing — but that argument is exactly what a maintainer should verify.
Level of scrutiny
High. The module resolver is on the hot path of every process start, and this introduces a new cache invariant: a DirEntry may now be stored with complete = false and an empty data map, and every consumer that iterates or does name lookups must upgrade it first. The PR routes get_entries_const through entries_at, guards the .bin-folder probe on entries_complete, and re-reads in read_directory_with_iterator / entries_at — but confirming there is no other read path that bypasses these (bundler, glob walker, test scanner, hot-reload watcher) is the kind of whole-program check a maintainer is better placed to do. The fast path is also gated on subtle conditions (!care_about_bin_folder, exact path equality with the canonicalized hint, regular-file-and-not-symlink) whose interaction with case-insensitive filesystems and bun run is reasoned about in comments but not mechanically enforced.
Other factors
All seven prior review threads (mine and CodeRabbit's) are resolved; the author was responsive to each. The bug-hunting system found nothing on the current revision. Test coverage is good for the new behaviour (0711 directory under nobody, listable control, symlinked entry, incomplete-cache upgrade via require("../pkg")), and the PR description states the existing resolver suites pass. The one CI failure (autobahn.test.ts on macOS) is unrelated to this change. Given the scope — resolver internals, a new cache-completeness contract, and new unsafe — this should get a human sign-off rather than a bot approval.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Remove comments. Remove unsafe.
|
Done in a2224e9.
Tests still pass (7/7 in |
a2224e9 to
3aa8374
Compare
|
Rebased on main (3aa8374). One conflict, in VirtualMachine::_resolve: main moved the function to the typed crate::CrateError instead of bun_core::err!. Resolved by keeping the typed error and this PR's loop structure, so the error arms are now break Err(crate::CrateError::ModuleNotFound) / break Err(e.into()) and the entry_point_hint is still cleared at the single point after the loop. Tests pass (7/7 in bun-main-entry-point.test.ts). |
There was a problem hiding this comment.
No bugs found, but this touches the resolver's directory-cache invariants (new DirEntry.complete state, incomplete-entry upgrade paths, entry-point fast path in load_as_file/dir_info_cached_miss) and reroutes get_entries_const through the re-reading entries_at — worth a human look given how many consumers depend on the cache's completeness contract.
Checked: the entry_point_hint set/clear in _resolve — every loop exit now converges on the post-loop = None, including the retry-after-cache-bust continue. The lazy_leaf gate requires !care_about_bin_folder, so the && entries_complete guards on the bin-folder scan are defensive rather than reachable in the same resolve. The load_as_file fast path falls through on symlinks and non-regular files, and passes store_fd=false so no fd is acquired to leak.
Extended reasoning...
Overview
The PR adds a stat-based fast path for resolving the process entry point, avoiding an O(n) readdir of its parent directory. It touches five resolver files: a new entry_point_hint: Option<Box<[u8]>> on Resolver, a complete: bool flag on DirEntry, a lazy_leaf branch in dir_info_cached_miss that skips the directory open/iterate for the entry's parent, a marker_kind! macro in dir_info_uncached that stats config markers directly when the listing is incomplete, an early-return in load_as_file, and control-flow restructuring in VirtualMachine::_resolve (early returns → break Err so the hint is always cleared). get_entries_const is rerouted through get_entries_ref(0) so incomplete entries are upgraded before any consumer iterates them. Four new tests cover the 0711 directory case (via setpriv/nobody), a listable-directory control, symlinked entry points, and the incomplete-cache upgrade path.
Security risks
None identified. The change is a read-path optimization gated to the main entry point's canonicalized absolute path; it does not touch auth, crypto, network, or user-controlled parsing. The setpriv privilege drop is test-only.
Level of scrutiny
High. The module resolver is on the critical path of every bun <file> invocation, and this change introduces a new cache state (complete = false) with an implicit contract that every consumer that iterates a DirEntry must go through a path that upgrades it first. The PR handles the known consumers (entries_at, read_directory_with_iterator, get_entries_const), but validating that the upgrade is exhaustive across shell completions, routers, glob walker, test scanner, bundler, --hot, and workers is exactly the kind of cross-cutting invariant a maintainer familiar with the resolver's clients should confirm. Rerouting get_entries_const through entries_at also means a formerly pure lookup can now perform I/O, which is a behavior change worth a maintainer's eye.
Other factors
All prior inline review threads (fd propagation, hint scoping, unsafe lifetime erasure, test hygiene) are resolved. The final revision replaced the lifetime-erased &'static [u8] hint with an owned Box<[u8]> and dropped the scopeguard, which removes the last new unsafe beyond the pre-existing &mut *rfs reborrow idiom. The bug-hunting pass found nothing. The change is well-scoped and well-tested for its stated goal, but its blast radius (resolver cache semantics) puts it outside what I'd approve without a human reviewer.
|
Windows confirmation of the diagnosis, plus one measurement that I think shows the fix is scoped too narrowly. Short version: the cost is not specific to the entry point's own parent directory. A large directory anywhere above the script costs exactly the same, and this PR's fast path is gated to the entry point ( I have not built this branch — this is black-box measurement on 1.4.0 read against the PR description, so treat the conclusion as a prediction to falsify, not a review finding. The falsifying test is cheap and I've named it at the end. Environment
The measurement that mattersOne directory holding 50,000 files. I vary only how far above the script it sits:
Same shape with every directory small, as a control:
The penalty is ~+9.5 ms and flat. Being the entry point's own parent is worth nothing extra — row 1 and row 4 are within noise of each other. Only row 1 looks like what this PR fast-paths. Rows 2–4 cost the same and appear to go through the ancestor No project boundary stops the walkI expected
So a normal, well-formed project nested anywhere under a large directory pays the full cost. Why I noticed
node and deno are flat; only bun moves, 8.4×. Scaling is roughly linear in sibling count, ~0.2 µs/entry:
Path depth on its own is free (a six-level-deep path under small parents measures 11.6 ms vs 11.4 ms at the top), and a different volume changes nothing. It is sibling count in some ancestor, full stop. This inverted my benchmark result completely: bun looked ~6× slower than node at running a file, when in a clean directory on the same machine it is 2.2× faster (11.4 vs 25.0 ms), with a free TypeScript path (11.5 ms The cheap falsifying testYour synthetic check ("60k entries ≈ 3× a 1-entry dir") puts the script inside the 60k directory. Re-run it with the script one level down: If the patched build still shows the full 3× there, the fast path is not covering the case that actually bites, and the fix likely needs to extend to the ancestor If it comes out flat, then I've misread the gating and this is just corroboration — say so and I'll withdraw the concern. Happy to run the patched build against the 263k-entry directory on Windows if someone points me at an artifact. |
…ctory Resolving the process entry point (`bun <script>`) read the whole parent directory to find the one file, so startup scaled with the sibling count (slow under /nix/store) and failed outright when the directory was searchable but not listable (mode 0711). Hint the resolver with the entry point's canonicalized path while that single specifier resolves. load_as_file then stats the one file, and dir_info_cached_miss skips enumerating its parent, storing that directory's listing as incomplete (DirEntry.complete == false). dir_info_uncached probes the config markers (package.json, tsconfig.json, jsconfig.json, node_modules) by direct stat when the listing is incomplete. The live read paths (read_directory_with_iterator, entries_at_locked) re-read an incomplete listing before any lookup, so a later import of that same directory still finds its files. Bin-folder discovery in dir_info_uncached is guarded on a complete listing. The hint is an owned Box<[u8]> cleared at the single point after the resolve loop, with the loop's error paths converging via break. Fixes #32389
3aa8374 to
75f1b95
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// `false` marks a listing created without a full `readdir` (entry-point | ||
| /// fast path); consumers that iterate must re-read it first. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A permission-denied ancestor (or the lazy entry-point leaf) | ||
| // has no fd to enumerate; its entry set stays empty. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Pre-size `data` so the per-entry inserts below skip the | ||
| // 1→2→4→…→N hashbrown rehash cascade from an empty table. 64 | ||
| // covers a typical node_modules package dir; larger dirs | ||
| // still rehash from there (cheap relative to starting at 0). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // NOTE: store the raw `NonNull` field (not the | ||
| // `&'static` accessor result) so mut-provenance flows | ||
| // through to `enqueue_dependency_to_resolve`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Rebased on main again (75f1b95), squashed into one commit. Main moved a lot under this PR, so a few resolutions are worth noting:
Verified: 7/7 in |
| // SAFETY: BSSMap-owned slot; uniquely held under `entries_mutex`. | ||
| if let EntriesOption::Entries(existing) = unsafe { &mut *result_ptr } { | ||
| if existing.generation < generation { | ||
| if existing.generation < generation || !existing.complete { |
There was a problem hiding this comment.
🔴 The !existing.complete gate here makes entries_at_locked re-read the entry-point directory during the very resolve this PR is optimising: finalize_result unconditionally calls dir.get_entry(self.generation, name.filename) (resolver.rs:1588), which routes to entries_at_locked on the just-created complete=false slot and performs a full open+readdir — so a 60k-entry /nix/store still costs O(entries) at startup, contrary to the PR's stated fix for #32389. Fix: the incomplete-entry upgrade must not fire on the symlink probe inside the same entry-point resolve — e.g. skip the re-read while entry_point_hint names a child of dir, or resolve the entry's symlink via the stat result already obtained in load_as_file instead of get_entry.
Extended reasoning...
Trace on the base's stated failure case (bun /nix/store/…-pkgs.js): _resolve sets entry_point_hint; resolve_and_auto_install → load_as_file takes the new stat fast-path (resolver.rs:5791) and returns without touching read_directory. Control reaches finalize_result (resolver.rs:1404) → read_dir_info(name.dir) misses the DirInfo cache → dir_info_cached_miss sees lazy_leaf (resolver.rs:4415), stores a DirEntry{complete:false} in rfs.entries (4627-4698) and builds the DirInfo via marker_kind! stats. Back in finalize_result, line 1588 calls dir.get_entry(self.generation, name.filename) → DirInfo::get_entry → entries_at_locked(index, generation) (dir_info.rs:266). At lib.rs:1611 existing.generation == self.generation but !existing.complete, so it opens /nix/store with open_dir_for_iteration and runs self.readdir(...) over the whole listing — the exact O(entries) work the PR claims to remove. The 0711 test still passes because the open fails with EACCES and get_entry returns None, so it never observes that the readdir was attempted; there is no test…
Verification: normal — the fast path this PR adds is immediately undone within the same resolve, so the O(entries) readdir the PR exists to remove (issue #32389, the /nix/store case) still runs. Trace on bun /nix/store/…-pkgs.js: 1. load_as_file takes the new stat short-circuit (src/resolver/resolver.rs:5791-5811) and returns without touching rfs.entries. 2. resolve_and_auto_install then calls…
|
Closing: #48148 covers this case together with the other startup listings (the cwd, every ancestor, the entry point's directory, and the .env lookup) in one mechanism. |
What
Running
bun <script>resolves the entry point by reading its parent directory's entire listing to (1) confirm the file exists and (2) locate the enclosingpackage.json/tsconfig.json. That work is O(entries in the directory), so startup scales with the number of siblings.Fixes #32389: a script under a large immutable tree such as
/nix/storestarts 10-16x slower than the same script in a small directory. The same directory read also makesbunfail on a directory that is searchable but not listable (mode0711), even though the file itself is readable and Node runs it fine.Repro
Synthetic confirmation: startup grows linearly with the sibling count (60k entries ≈ 3x a 1-entry dir on a release build), independent of script content.
Cause
Entry-point resolution goes
VirtualMachine::_resolve→resolve_and_auto_install→load_as_file(read_directoryon the parent) →finalize_result→dir_info_cached_miss(builds the parentDirInfo). Both steps only need a handful of named lookups (the entry file,package.json,tsconfig.json,jsconfig.json,node_modules), but the esbuild-derived cache reads and interns the whole directory to answer them.Fix
The entry point's specifier is a canonicalized absolute path (from
get_fd_path), so its exact basename is the correct on-disk name. When resolving it:load_as_filestatsdir/baseonce and returns if it is a regular file (anything else falls through to the normal directory read).dir_info_uncachedresolves the four config markers by directstatinstead of a map lookup.A new
DirEntry.completeflag marks an entry created without areaddir;read_directory_with_iteratorandentries_atre-read it to a full listing before any consumer iterates it (shell completions, routers, the glob walker, the test scanner), so the cache's completeness invariant is preserved. The fast path is gated to the main entry viaResolver.entry_point_hint, so ordinary relative/package imports and the bundler are unchanged, and case-insensitive filesystems are unaffected (the canonical entry path already carries the on-disk case).Verification
test/js/bun/resolve/bun-main-entry-point.test.tsruns an entry point under a0711(searchable, non-listable) directory. As root the directory read is bypassed by the kernel, so the child is dropped tonobodyviasetpriv(Linux-only, skipped otherwise):error: Module not found(the resolver tries toreaddirthe directory)A listable-directory control under the same privilege drop isolates the assertion from setup failures. With the fix, a 60k-entry directory starts in the same time as a 1-entry directory.
Existing resolver suites (
resolve,resolve-ts,require,resolve-error, tsconfig/long-path overflow) pass, and entry points whose directory carries atsconfig.json(path aliases) orpackage.json(#imports) still pick up that config via the stat path.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/resolve/bun-main-entry-point.test.ts