install: resolve file: and link: dependencies on the same directory as separate packages - #38114
install: resolve file: and link: dependencies on the same directory as separate packages#38114robobun wants to merge 1 commit into
Conversation
…t its path The folder-resolution map was keyed only by the hash of the target's package.json path. A link: dependency parses that file with Features::LINK and stores a Symlink resolution, a file: dependency parses it with Features::FOLDER and stores a Folder resolution, so when one install referenced the same directory both ways, whichever resolved first was handed out for the other: the file: consumer lost its dependencies, or the link: consumer was recorded and installed as a folder. Add the parse kind (folder/workspace, link, cache folder) to the key so the same directory gets one entry per kind. Folders and workspaces keep sharing an entry, including the root package seeded by PackageManager::init.
|
Updated 7:46 AM PT - Aug 13th, 2026
✅ @robobun, your commit ba749637d12381bae798f5ee2593010aed7a8044 passed in 🧪 To try this PR locally: bunx bun-pr 38114That installs a local version of the PR into your bun-38114 --bun |
|
Status: reproduced on bun 1.4.0 with a project declaring both |
|
Warning Review limit reached
Next review available in: 4 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 (3)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The cache-key widening is the canonical fix for the bug class and every .folders accessor and GlobalOrRelative caller lines up with the new Kind mapping. Because it changes package-manager resolution semantics — in particular the deliberate choice to keep Folder and Workspace under one Kind while splitting Link out — a human look from someone familiar with bun install would still be worthwhile.
What was reviewed
GlobalOrRelative::kind()matches the resolver dispatch inget_or_putarm-for-arm; theRelative(_)unreachable!()mirrors the existing one.- All
.foldersmap accesses (init seed + three inget_or_put) migrated; the removedhash()helper has no remaining callers. - Root-package seed uses
Kind::Folder, sofile:deps on the root still hit it; the hash-collisionabs_pathguard is preserved. - Test covers both alias orders, drains pipes concurrently, and asserts the exact lockfile
packagesshape.
Extended reasoning...
Overview
The PR widens the folder-resolution cache key in src/install/resolvers/folder_resolver.rs from a bare u64 path hash to a Key { kind: Kind, abs_hash: u64 } struct, where Kind is Folder (file:/workspace), Link (link:), or CacheFolder (npm cache reads). PackageManager::init is updated to seed the root package under Kind::Folder, the FolderResolutionMap typedef in PackageManager.rs is updated to the new key type, and two parametrized regression tests are added to bun-install.test.ts. The old free hash() function is removed.
Security risks
None. This is an in-process cache-key change during dependency resolution; no untrusted input handling, path traversal, or network surface is introduced. The abs_path byte-comparison collision guard from #32745 is retained unchanged.
Level of scrutiny
High — this is bun install resolution logic, where a wrong cache hit silently produces an incorrect lockfile (as the bug being fixed demonstrates). I verified: (1) kind() agrees with the resolver get_or_put picks for every GlobalOrRelative variant, including the Relative(_) fallthrough which is unreachable!() in both places; (2) grepping for .folders. and folder_resolver::hash shows every map accessor was migrated and nothing still calls the removed helper; (3) the four get_or_put call sites in PackageManagerEnqueue.rs and PackageManagerResolution.rs only pass Global, Relative(Folder), Relative(Workspace), and CacheFolder, all of which kind() handles.
Other factors
The one non-mechanical choice is merging Folder and Workspace into a single Kind::Folder. The PR description justifies this well — both parse dependencies, and a file: pointing at a workspace member or the seeded root must reuse that package (covered by an existing isolated-install.test.ts case). Since the previous key had no kind dimension at all, this preserves the prior Folder↔Workspace sharing while newly separating only Link and CacheFolder, which is exactly the reported bug. Still, it is a semantic decision about what counts as "the same package" in the lockfile, and a maintainer who owns src/install/ is better placed to confirm there's no edge case (e.g. workspace-member-also-referenced-via-link:, or lockfile migration) that this reasoning misses. No CODEOWNERS entry covers this path. The tests follow harness conventions (tempDir, bunExe, concurrent pipe drain, exact toEqual on lockfile structure) and the --lockfile-only rationale is sound.
|
On the two edge cases raised in the review:
|
Problem
"a": "link:../shared"and"z": "file:../shared"records whichever alias resolves first for both of them. Withlink:first,zis written to bun.lock asshared@link:../sharedand its dependencies are dropped, so nothing gets installed for thefile:consumer. Withfile:first, thelink:consumer is written asshared@file:../sharedand installed as a folder copy (exit 0, so this direction is silent).folder_resolver::get_or_put(src/install/resolvers/folder_resolver.rs) caches resolutions inPackageManager.folderskeyed only by the hash of<dir>/package.json. The key does not record how the package.json was read:file:parses it withFeatures::FOLDERand stores aFolderresolution,link:parses it withFeatures::LINK(no dependencies) and stores aSymlinkresolution. Alink:value starting with.is resolved against the project root (normalize_package_json_path), solink:../sharedandfile:../sharedproduce the same key and the second lookup is served the first one's package.Fix
folder_resolver::Key { kind, abs_hash }.Kindis derived from theGlobalOrRelativevariant, i.e. from the resolverget_or_putis about to run:Folder(file:folders and workspace members),Link(link:targets),CacheFolder(npm packages read back from the cache). Same directory, different kind: separate entry. Theabs_pathcomparison that guards same-kind hash collisions (install: compare stored scope name and folder path, not just their hash #32745) is unchanged.file:pointing at a workspace member, or at the root package thatPackageManager::initseeds asPackageId(0), is meant to resolve to that existing package (isolated-install.test.ts"can install folder dependencies on root package" covers the seed). The seed is now inserted underKind::Folder.Featuresand store a differentResolutiondo not produce the same package, so they must not share an entry. Once both consumers reach their own parse,Lockfile::get_package_idalso keeps the two packages apart, because aFolderand aSymlinkresolution never compare equal.GlobalOrRelative::kindhas anunreachable!()arm forRelativetagsget_or_putcannot resolve today, so a future variant (for example the project-relativelink:form in install: support path-form link: dependencies #35461) has to pick a kind explicitly instead of silently sharing with folders.bun update, or the lockfile is removed; the fix takes effect whenever the dependency is actually resolved.test/cli/install/bun-install.test.ts("resolves file: and link: to the same directory as separate packages"), one case per alias order. The tests runbun install --lockfile-onlyand compare thepackagessection of bun.lock: on the released binary thefile:alias comes back asshared@link:../sharedwith noextraentry (or thelink:alias asshared@file:../sharedwith one); with this change each alias gets its own package and only thefile:one hasextra.--lockfile-onlyis used because materializing alink:../xtarget is a separate, pre-existing limitation that would otherwise decide the exit code.file:tests inbun-install.test.ts(14, including the abs-path hash collision test),bun-workspaces.test.ts(68), the folder dependency tests inisolated-install.test.ts, andbun-link.test.ts(3 of 4; "should link dependency without crashing" fails on debug builds regardless of this change because the debug-only install-failure trace ends up on stdout, which is tracked separately).cargo clippy -p bun_installis clean.Background
PackageManager.folders): during resolution, everyfile:, workspace andlink:dependency becomes a lockfile package by reading the target directory's package.json. The map remembers, per package.json path, which package that produced, so later dependencies on the same directory reuse it instead of appending a duplicate.Features: the optionsPackage::parseuses when reading a package.json.FOLDER/WORKSPACEread the dependency sections so those get installed;LINKreads none of them, because alink:dependency is only a symlink to a directory the user manages themselves.Resolution: the lockfile's record of where a package comes from (Folder,Symlink,Workspace,Npm, ...). The installer decides how to materialize a package from this tag, which is why alink:consumer handed aFolderpackage ends up with a copy instead of a symlink.GlobalOrRelative: the argument that selects the resolver insideget_or_put:Globalforlink:(joined onto the global link dir unless the value starts with.),Relative(Folder | Workspace)forfile:and workspaces,CacheFolderfor the offline auto-install path.