resolver: find packages installed after a failed runtime resolution - #37372
resolver: find packages installed after a failed runtime resolution#37372robobun wants to merge 1 commit into
Conversation
…time resolution When a runtime resolution comes back not found, the module loader busts the directory cache once and retries. The bust only covered the parent of join(source_dir, specifier), which is the right directory for a relative specifier but names nothing for a package path, so the not-found markers recorded for node_modules/<pkg> at every level (and the importing directory's "has no node_modules" flag) stayed in place. A package installed at runtime after a failed require.resolve() kept failing for the rest of the process, while Node finds it. Move the bust policy into Resolver::bust_dir_cache_for_not_found. It still evicts the candidate path's parent, now also the candidate itself (a directory created later for "./lib" was missed too), and for package paths it walks the same levels load_node_modules searched (or the require.resolve paths roots) plus NODE_PATH: a level where the package directory now exists gets the package directory, the literal subpath chain, and, if its listing predates the package, the node_modules listing evicted; a level whose DirInfo predates its node_modules directory is rebuilt along with the levels below it. Levels where the package is still missing cost one access() and evict nothing. Both copies of the retry loop now call the resolver method, which also removes the per-thread path buffer they kept for building the key.
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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 (4)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Not adding
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the resolver's cache-bust-on-miss policy (a hot path every failed require/import goes through) with new parent-chain walks and per-level access() probes, a human look at the eviction scope and miss-path cost would be worthwhile.
What was reviewed:
bufs!aliasing across the new helpers —node_modules_checkholdsbasewhileesm_absolute_package_pathis scratch, anddir_info_cached_maybe_logtouches neither.DirInfoRefvalidity acrossbust_dir_cache/read_dir_info_ignore_errorduring the parent-chain walk —BSSMapInner::removeonly drops the key→index entry; slots (and.abs_path/.parent) stay live.node_path_entries()refactor —strings::tokenizefilters empty fields, matching the old.split().filter(!is_empty()).custom_dir_pathsiteration while calling&mut selfmethods — it'sOption<&'a [String]>(Copy), same pattern as the existingcheck_packageloop.
Extended reasoning...
Overview
This PR moves the runtime resolver's "bust-then-retry-once" policy for not-found resolutions out of the two call sites (VirtualMachine::_resolve and the jsc_hooks.rs copy) into a new Resolver::bust_dir_cache_for_not_found. The old code busted only the parent of join(source_dir, specifier), which is meaningless for package paths; the new code evicts the candidate path and its parent for every search root, then for package paths walks the DirInfo parent chain (and NODE_PATH entries) and, at each level where <node_modules>/<package> exists on disk now, evicts the package directory, every directory on the literal specifier chain below it, and the node_modules listing itself if it predates the package. Levels that gained a node_modules since their DirInfo was built are rebuilt together with everything below them so the retry's cached-parent-link walk sees the new state. ~220 lines of new resolver logic, ~80 lines removed from the two call sites (including a per-thread PathBuffer TLS cell), and a node_path_entries() helper factored out of load_node_modules. 11 new subprocess tests cover the variant matrix (require/import/resolve, scoped/bare/subpath, new-node_modules in importer/ancestor, symlink, NODE_PATH, paths option, relative directory, still-missing negative).
Security risks
None identified. The new code reads user-controlled specifiers and issues access() on paths derived from them, but that is exactly what the resolver already does; no new trust boundary is crossed. .. in the specifier is guarded by the starts_with(base) containment check in bust_package_dir_cache so evictions stay under the search root.
Level of scrutiny
High. Module resolution is on the critical path of every Bun program, and this changes what happens on every miss. The PR argues the added cost is one access() per ancestor for packages that stay not-installed (no new evictions triggered), which sounds right, but the interaction between the rebuild_through rebuild and load_node_modules' cached-parent-link walk is subtle enough that a maintainer who owns the resolver should confirm the eviction set exactly mirrors what the failed lookup consulted. The bufs! thread-local scratch discipline is documented in-line and I traced it through, but it's the kind of invariant that benefits from a second set of eyes.
Other factors
Test coverage is thorough and follows repo conventions (tempDir, bunEnv spread, concurrent subprocess tests, exact-output assertions, a negative case). The PR description enumerates what is deliberately not covered (exports-mapped targets, package.json edits) and cross-references the neighboring open work (#36675, #36678, #36928). The bug-hunting pass found nothing. Net: well-executed, but the scope and the layer it touches put it outside what an automated approval should sign off on.
|
Updated 1:05 AM PT - Aug 11th, 2026
❌ @robobun, your commit 71b957f has some failures in 🧪 To try this PR locally: bunx bun-pr 37372That installs a local version of the PR into your bun-37372 --bun |
Repro
Once a package path has failed to resolve, it keeps failing for the rest of the process no matter what gets installed. The same happens for
require(),import(), a barerequire("pkg"), a file added to an already resolved package (pkg/generated.js), a package created under aNODE_PATHentry or under arequire.resolve(id, { paths })root, anode_modules/directory created after the fact in the importing directory or one of its ancestors, and for a relative specifier whose target is a directory created later (require("./lib")withlib/index.js). Node resolves all of these on the second call. Thenode_modules-created-later case is the shape of thebunnpm package's own postinstall (packages/bun-release/src/npm/install.ts:require.resolvefails, the platform package is downloaded into a newnode_modules/next to the script,require.resolveagain), which is why it currently has to run under node.Cause
VirtualMachine::_resolve(and its copy injsc_hooks.rs) retries a not-found resolution once after busting the directory cache, but the only thing it busted was the parent ofjoin(source_dir, specifier). For./xthat is the directory whose listing is consulted, so files created later were already found. For a package path it is<source_dir>/@scope/pkg/bin, which names nothing, so everything the failed walk cached stayed in place:dir_info_cached(<dir>/node_modules/@scope/pkg)and theload_as_file_or_directoryprobes record not-found markers fornode_modules/@scope,.../pkg,.../pkg/binin both theDirInfocache and the listing cache; the next lookup is answered from those markers.DirInfo::has_node_modules()is computed from the listing when theDirInfois built, andload_node_modulesskips levels without it and walks levels through cached parent links, so anode_modules/created later in the importing directory is never looked at../lib, the target directory itself had a not-found marker from the load-as-directory probe; only the parent was busted.Fix
The bust policy moves into the resolver as
Resolver::bust_dir_cache_for_not_found(source_dir, specifier), and both retry loops call it (this also retires the per-thread path buffer each loop kept for building the key). It evicts:./libdirectory case), against each search root: the source directory, or thepathsroots whenrequire.resolvewas given some;DirInfoparent chain, starting from the nearest existing directory likecheck_package_pathdoes) and for everyNODE_PATHentry: if<base>/<package name>exists on disk now, the package directory, every directory on the literal<base>/<specifier>chain below<base>(the not-found markers), and<base>itself when its cached listing does not contain the package's top-level entry (that listing is where the package entry, including a symlink target, is read from, so a package symlinked in later resolves to its real path like in Node). A level whoseDirInfosays it has nonode_moduleswhile the package directory exists under it is rebuilt together with every level below it, since the retry reaches it through the cached parent links.The existence check is what makes this cheap to have on the miss path: a level where the package still does not exist costs one
access()and evicts nothing, sotry { require("optional-dep") } catch {}for something that is not installed behaves exactly as before (the pre-existing parent-directory bust included) plus a handful of failedaccess()calls, and no retry is triggered by the new logic. A miss for a subpath of a package that is installed does evict and re-read that package directory once per miss, which is the same thing the existing retry already does to the parent directory of a missed relative specifier. The per-retry retention of the bust mechanism itself is a separate, already tracked matter (#36675, #36928); this change adds no evictions in the not-installed steady state.Why this is the right layer: the resolver is the only place that knows which directories a package-path lookup consulted (
node_moduleslevels,pathsroots,NODE_PATH), and the invalidation has to mirror exactly that search space; the two runtime loops only know about the source directory. #36678 fixes the neighboring tsconfigpathscase of the same retry and is independent of this.Not covered, deliberately: a package whose
exportsmap points at a file that is created later (the consulted directory is the exports target, not the specifier path), and edits to an existingpackage.json. Neither is part of the install-then-require pattern this fixes.Verification
New tests in
test/js/bun/resolve/resolve.test.ts(files created after a failed lookup resolve on the next lookup): scoped subpath viarequire.resolve, bare package viarequire, package viaimport(), file added to an already resolved package,node_modules/created in the importing directory and in an ancestor of it, symlinked package resolving to its real path,NODE_PATH,require.resolvepaths, relative directory created later, and a check that packages that stay missing keep failing. 10 of the 11 fail on the unfixed binary (after: MODULE_NOT_FOUND), all pass with the fix.test/js/bun/resolve/(352 pass; the only failure is the pre-existing debug-build timeout inload-same-js-file-a-lot.test.ts, owned by #36929),test/js/node/module/(97 pass) and theNODE_PATHbundler tests pass on the debug build. WithBUN_DEBUG_Resolver=1, misses for packages that are not installed show no new evictions; a subpath miss inside an installed package evicts only that package directory and the missed leaf.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/resolve/resolve.test.ts