install: keep a bundled file: dependency bundled when installing from bun.lock - #42884
Conversation
… bun.lock The bundle-root pre-scan in the bun.lock parser read the info object at index 2 of every package entry. That index is right only for an npm resolution. A file:, git, tarball or workspace resolution has no registry string, so its info object is at index 1 and the `"bundled": true` marker was never seen. The installer then installed the bundled dependency a second time on top of the copy the tarball shipped. That second install went through the symlink method. When a file already existed, the fallback linked the entry to its own basename, which produced self-referencing symlinks such as `package.json -> package.json`. The retry now links to the same cache path as the first attempt. Fixes #42882
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughThe installer now detects bundled metadata across package-info layouts and retries replacement symlink creation correctly. Registry fixtures and regression tests cover bundled and unbundled ChangesBundled file dependency installation
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The reported self-referencing symlink failure is the behavior this change targets, and no additional concrete regression is established by the available evidence. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked two things and ruled them out: the find_map pre-scan in bun.lock.rs cannot pick the wrong element, since every lockfile package tuple shape has exactly one object (resolution, registry and integrity are all strings); and the new Windows EEXIST arm in PackageInstall.rs calls sys::unlink_w(&WStr) and sys::symlink_w(dest, src, options) with signatures that match src/sys/lib.rs, so it should type-check on the Windows target.
Extended reasoning...
Three confirmed findings are already posted inline (discarded unlinkat result masking EISDIR on the retry path, an unchecked head2 write for long cache paths on macOS, and the pre-scan not recovering bundled: true markers that older bun versions already dropped from re-saved lockfiles), so this note only records what else was examined. The lockfile pre-scan's first-object selection was checked against the tuple layouts the writer emits and is unambiguous. The Windows EEXIST branch was checked for symbol existence and argument types against src/sys/lib.rs (symlink_w(dest: &WStr, target: &WStr, options) and unlink_w(from: &WStr)), and the unix retry now uses the target ZStr built at line 1793 that is in scope at the retry site. This is informational; a human should still weigh the inline findings before merging.
2 verified lower-impact observations (convention, logging or cleanup points) were not posted.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/install/PackageInstall.rs— macOS users with a deep file: dependency path get a Rust index-out-of-bounds panic and a crash report from bun install instead of an ENAMETOOLONG error. PackageInstall.rs:1788-1791 writes cache path plus entry.path into head2 with no length check; the buffer is a path_buffer_pool buffer of MAX_PATH_BYTES, which is 1024 on macOS (bun_core/util.rs:685). The Windows arm at :1817-1821 has the bounds check and returns ENAMETOOLONG. Fix: add the sameentry.path.len() > head2.len() - to_copy_into2_offsetcheck to the unix arm before the copy so the failure is a recoverable install error on every platform.Extended reasoning...
install_with_symlink at :1735 takes buf2 from bun_paths::path_buffer_pool::get(), sized MAX_PATH_BYTES. On Linux that is 4096, on macOS 1024 (bun_core/util.rs:676-686). :1740 writes the absolute cache directory of the file: dependency into it, e.g.…
Verification: pre-existing. Triggering condition: a
file:dependency installed via Method::Symlink whose absolute cache path plus a relative entry path exceeds MAX_PATH_BYTES (1024 on macOS, 4096 on Linux per /home/claude/bun/src/bun_core/util.rs:676-686). Mechanism verified: /home/claude/bun/src/install/PackageInstall.rs:1735 takesbuf2frombun_paths::path_buffer_pool::get()(a `PathBuffer(pub [u8;…
|
On the macOS path length note: the unix arm of the symlink copy loop writes the cache path plus the entry path into a pool buffer with no length check. That is pre-existing and not part of this bug, so I am leaving it out of this PR to keep the diff at the two reported causes. The two inline threads are answered in place. |
Problem
bun installfrom an existingbun.lockinstalls a bundledfile:dependency a second time. The second copy lands as self-referencing symlinks (package.json -> package.json) inside the bundling package'snode_modules, and the import fails withCannot find module. The firstbun addworks. Fixes Frozen install creates self-referencing symlinks for a bundled file #42882.src/install/lockfile/bun.lock.rs:2545read the info object at index 2 of every package entry. That index is right only for an npm resolution ([res, registry, {info}, integrity]). Afile:, git, tarball or workspace resolution has no registry string, so its info object is at index 1 and the"bundled": truemarker was never seen.src/install/PackageInstall.rs:1803linked an entry to its own basename when the destination file already existed. That is where thepackage.json -> package.jsonlinks come from.Fix
EEXISTlinks to the same cache path as the first attempt.parse_append_dependenciessetsBehavior::BUNDLEDon the edge and the installer skips it, as it does on a fresh resolve. The bundled copy that the tarball shipped stays in place.test/cli/install/bun-install-registry.test.ts(bundledDependenciesblock). Both fail on the released bun. Also ran all ofbun-install-registry.test.ts,bun-lock.test.tsandisolated-install.test.ts.Background
node_modules/. The installer must not install it again.bun.lockrecords this with"bundled": trueon the dependency's own entry, and the parser marks the parent's edgeBUNDLEDfrom that entry.file:dependency of an npm package is installed with the symlink method: each file in the destination is a symlink to the cached source (PackageInstall.rs:2316). Whennode_modulesis new, the installer skips the delete before install, so files the tarball already shipped are still there and theEEXISTpath runs.Notes
New fixture
bundled-file(generated bytest/cli/install/registry/packages/create-bundled-file-packages.ts):1.0.0depends onbundled-file-depviafile:vendor/bundled-file-depand bundles it. The tarball shipsvendor/bundled-file-dep/andnode_modules/bundled-file-dep/. The test installs, checks the lockfile carries"bundled-file/bundled-file-dep": ["bundled-file-dep@file:vendor/bundled-file-dep", { "bundled": true }], deletesnode_modules, installs with--frozen-lockfile, and checks the install reports 1 package and the bundled files are regular files. Released bun reports 2 packages and leaves self-referencing links.2.0.0ships the same files but does not bundle the dependency. The test checks thefile:dependency is linked to the vendor copy andrequire("bundled-file")works. Released bun producespackage.json -> package.json.The same two bugs exist in the Zig source before the Rust port, which matches the report that 1.2.19 also fails.
no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-install-registry.test.ts