install: keep linking the remaining bins when one bin or one dependency's bins fail to link - #38972
install: keep linking the remaining bins when one bin or one dependency's bins fail to link#38972robobun wants to merge 5 commits into
Conversation
…bins fail with the isolated linker link_dependency_bins returned on the first bin::Linker error, so every dependency after the failing one was left out of the entry's node_modules/.bin. Attempt every dependency, then fail the task with one error per dependency that could not be linked, naming that dependency.
|
Warning Review limit reached
Next review available in: 5 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 |
|
Status: fix pushed (latest a7b43f2), CI pending. Reproduced on the released build (1.4.0-canary.1) with the two tests in this PR:
Both pass with this branch on Linux and Windows: every linkable bin is linked, each failing package is still reported (by name, per |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused error-aggregation change that mirrors the hoisted linker's continue-on-error behavior; the inline note is a test-hygiene nit, not a correctness issue.
Extended reasoning...
Overview
This PR changes Installer::link_dependency_bins in src/install/isolated_install/Installer.rs so that when linking one dependency's bins into an entry's node_modules/.bin fails, the loop continues through the remaining dependencies instead of returning early. Failures are collected into a new TaskError::DependencyBinaries(Box<[DependencyBinariesError]>) variant, which on_task_fail prints one line per failed dependency, attributed to both the dependency and the parent entry. The step still fails overall (exit code 1, staging dir removed for global-store entries), so the isolated model's invariants are preserved — only aggregation and attribution change. A test in test/cli/install/isolated-install.test.ts covers the root and a workspace, asserts the sibling bins are linked, the five expected error lines, and exit code 1.
Security risks
None. This is error-handling control flow in the local package installer; no untrusted input parsing, no auth/crypto/network surface is touched.
Level of scrutiny
Moderate. It's a small, self-contained change to an error path in the isolated installer. I verified: the only caller of link_dependency_bins is Step::SymlinkDependencyBinaries (updated to pass the TaskError through directly); both match sites on TaskError (on_task_fail and TaskError::clone) handle the new variant; DependencyBinariesError is Copy so Box<[_]>::clone() works; the per-iteration state (node_modules_path, path buffers, seen) is safe to carry across a failed iteration per the same pattern already used in the hoisted PackageInstaller::link_tree_bins. The new error message follows the existing Output::err format used by sibling arms.
Other factors
The one finding is a test-hygiene nit (stdout: "pipe" not drained) — with 4 file: deps + 1 workspace the install summary is far under 64KB, so it can't deadlock in practice; switching to stdout: "ignore" would satisfy the convention. The test handles Windows (.bunx extension, path-separator normalization on stderr). I did not independently verify that Windows surfaces the open-file-as-directory failure as ENOTDIR specifically — CI will catch that if it differs, and it wouldn't affect the runtime fix either way. The PR description confirms the test fails on the released build and passes with the change, and that the related isolated-install test files pass.
|
Updated 11:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit a7b43f2 has some failures in 🧪 To try this PR locally: bunx bun-pr 38972That installs a local version of the PR into your bun-38972 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Review round addressed: the test now drains stdout as well (fee48c9), and the doc comments flagged on the new error types are removed (ba91fc5). No behavior change since the first push; the two red tests in the first CI run (test/bake/deinitialization.test.ts on Windows, test/regression/issue/09041.test.ts under ASAN) are unrelated to the install code touched here and have been reported separately. |
There was a problem hiding this comment.
LGTM — the earlier stdout-drain nit and the comment-cop flags are addressed, and this pass found nothing new.
What was reviewed:
link_dependency_binsloop-continuation safety: each iteration builds a freshbin::Linkerwitherr: Noneand rebuilds both path buffers;seenis meant to persist across deps;node_modules_pathis only read bybuild_destination_dir, so no per-iteration state leaks after a failure.- New
TaskError::DependencyBinariesis wired into bothon_task_fail(per-dep attribution viaentry_node_ids→node_pkg_ids) andclone_for_owner; the task still fails, so staging cleanup and exit-1 semantics are unchanged. - Test cross-platform shape: Windows
open_awithO::DIRECTORYroutes throughNtCreateFile+FILE_DIRECTORY_FILE, which maps toENOTDIR; the test already handles.bunxand normalizes path separators, and sorts stderr lines for parallel task ordering.
Extended reasoning...
Overview
Two files: src/install/isolated_install/Installer.rs changes link_dependency_bins to collect per-dependency bin-link failures into a Vec and continue the loop instead of return Err on the first one, adds a DependencyBinariesError struct and a TaskError::DependencyBinaries(Box<[..]>) variant, handles it in on_task_fail (one line per failed dep, naming both the dep and the parent) and in clone_for_owner, and has Step::SymlinkDependencyBinaries propagate the TaskError directly rather than re-wrapping as TaskError::Binaries. test/cli/install/isolated-install.test.ts gains a test with a-bin / two bad directories.bin packages / z-bin at the root plus a workspace, asserting the sibling bins land in both .bin dirs, all five error lines appear, and exit code is 1.
Security risks
None. This is error aggregation in the isolated-linker bin step; no new inputs are trusted, no new filesystem writes are introduced, and the failure still fails the task and prevents committing an incomplete global-store entry.
Level of scrutiny
Medium-low. The behavioral change is narrow (early-return → collect-and-continue in one loop) and mirrors the hoisted linker's link_tree_bins, which already logs and continues. I traced bin::Linker::link for the Tag::Dir arm to confirm continuing is safe: the linker is constructed fresh each iteration with err: None, build_target_package_dir/build_destination_dir rewrite abs_target_buf/abs_dest_buf from scratch, and node_modules_path is only read (not appended to) by build_destination_dir, so a failed iteration leaves nothing dirty for the next. The shared seen map is intentionally per-.bin-directory state and should carry across deps.
Other factors
- All prior review threads are resolved: my stdout-drain nit was fixed in fee48c9 (the diff now shows
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])), and the three comment-cop doc-comment flags were addressed in ba91fc5 (the diff has no doc comments on the new types). - I checked the Windows path for the test's
ENOTDIRassertion:open_dir_absolute→open_awithO::DIRECTORYroutes to theNtCreateFilepath withFILE_DIRECTORY_FILE(src/sys/lib.rs:6698), which fails on a file and maps toENOTDIR; the test already branches onisWindowsfor the.bunxextension and normalizes backslashes, and sorts the error lines to tolerate parallel task ordering. clone_for_owneris updated for the new variant (errs.clone()on aBox<[DependencyBinariesError]>where the element isCopy), so the match stays exhaustive.- The test brackets the bad packages between
a-binandz-binso it exercises deps both before and after the failing ones, and checks a workspace whose first dep is bad to cover the "nothing linked at all" case from the repro.
…s bins fails bin::Linker::err is reported once per package and was also what decided, after every bin, whether the link just created is kept: once one bin of a bin map or directories.bin failed, each later bin was created, left without the executable bit and unlinked again, and a previously linked bin was deleted on the next install. Scope the cleanup to the bin that failed and keep the first error for reporting. A bin that failed is also dropped from the seen set so a later package providing the same name, or the retry without the native bin link, can still create it; the isolated retries start from a clean err.
There was a problem hiding this comment.
Beyond the inline nit, I also checked: the entry_node_ids[dep_err.dep_entry_id] lookup in the new on_task_fail arm uses the same indexing pattern as link_dependency_bins at line ~2374 where dep.entry_id originates, so no OOB there; and the two new bin_linker.err = None resets (retry-after-native-redirect at ~1881 and the per-dependency loop at ~2448) are both required now that a stale err would otherwise survive into the next link() call — the prior_err.or(err) split in link_bin_or_create_shim and these resets are consistent with each other. Note the comment-cop bot flag on bin.rs:835 is still unresolved.
Extended reasoning...
Verified the new TaskError::DependencyBinaries handling in on_task_fail indexes entry_node_ids / node_pkg_ids with dep_entry_id exactly as link_dependency_bins already does when it iterates entry_deps[parent_entry_id].slice(), so the stored ids are valid indices. Also traced the two added bin_linker.err = None; skipped_due_to_missing_bin = false resets: without them, the loop's continuation past a failure (or the native-binlink retry) would carry a stale Some(err) into the next link(), which the new prior_err.or(err) in link_bin_or_create_shim would then keep as the "first" error and re-push into failed for every subsequent dependency — the resets prevent that. The Windows openat early-return gap is covered by the inline nit; nothing further to add.
robobun
changed the title
|
Second review round, after a closer look at the shared linker: the same first-failure problem also existed one level down in |
There was a problem hiding this comment.
I reviewed this again after a7b43f2 and didn't find any bugs — that commit addresses the Windows openat early-return I flagged (it now falls through to the shared prior_err.or(err) / seen.remove cleanup), and also drops the doc comment that comment-cop flagged on bin.rs. Because 2aa0cdc widened the scope from the isolated linker into bin::Linker::link_bin_or_create_shim itself — first-error-wins across a package's bins, seen rollback on failure, and a restructured Windows shim path that every caller (hoisted included) goes through — a human look at that shared piece is still worthwhile.
What was reviewed:
prior_err.take()/prior_err.or(err)scoping — verified the cleanup keys off this bin'serrwhile the reported error stays first-wins, and that the Windows EISDIR fall-through matches the old early-return behavior.- The two isolated retry sites now reset
err/skipped_due_to_missing_bin, matching the hoisted retry which reconstructs the linker fresh viacontinue. - The 300-byte bin name in the new tests hits the syscall (ENAMETOOLONG/ENOENT) inside
link_bin_or_create_shim, not theabs_dest_bufoverflow guard inTag::Map, so it exercises the new per-bin scope.
Extended reasoning...
Overview
The PR now touches four files across two behavior changes: (1) link_dependency_bins in the isolated installer collects per-dependency bin-link failures into a new TaskError::DependencyBinaries instead of returning on the first one, with a new on_task_fail arm and sound_clone case; (2) bin::Linker::link_bin_or_create_shim in bin.rs — shared by both the hoisted and isolated linkers — now scopes its error-cleanup to the current bin (prior_err = self.err.take() … self.err = prior_err.or(err)), removes the failed dest from seen, and moves the Windows target openat inside that scope so its error path gets the same treatment. Two new tests cover the isolated-linker sibling-dependency case and the hoisted-linker sibling-bins-within-one-package case.
Security risks
None identified. The change is confined to error aggregation and per-bin cleanup ordering; no new inputs are trusted, no path validation is relaxed, and seen.remove on failure only re-opens a slot that was never actually populated on disk.
Level of scrutiny
Medium-high. The isolated-installer half is straightforward (collect-then-report, mirroring PackageInstaller::link_tree_bins). The bin.rs half is subtler: it changes error semantics for every caller, restructures a #[cfg(windows)] block that CI on the latest commit hasn't finished on yet, and interacts with the native-binlink retry (which now explicitly resets err/skipped_due_to_missing_bin at both isolated call sites). I traced the EISDIR-on-Windows fall-through, the chmod_on_ok/try_normalize_shebang gating (both key off this bin's err, not the carried prior), and the Tag::Map ENAMETOOLONG buffer guard (unchanged early-return, but the test's 300-byte name doesn't reach it) and didn't find a regression.
Other factors
The scope grew mid-review — 2aa0cdc added the bin.rs per-bin continuation on top of the original isolated-linker fix, and a7b43f2 reshaped the Windows path in response to feedback. All prior review threads (stdout drain, doc-comment cop ×3, Windows openat) are addressed in the current head; the one remaining unresolved comment-cop on bin.rs is stale against 2aa0cdc's since-removed doc comment. Tests look solid (both linkers covered, Windows-branched expectations, exit code asserted last), but CI on a7b43f2 is still pending. Given the shared-code reach and the Windows restructuring, deferring to a human reviewer rather than auto-approving.
1 participant
Problem
Found by reading the bin linking code while fixing #38954; there is no user report. Two places stop linking bins after the first failure and take the bins that come after it down with the failing one:
Installer::link_dependency_bins(src/install/isolated_install/Installer.rs) runs onebin::Linkerper dependency of an entry and returned on the first linker error, so every dependency after the failing one got no link in that entry'snode_modules/.bin. In the test below the project ends up witha-clionly and the workspace with no.binat all. The error was reported asENOTDIR: failed to link binaries for package: root@, naming the parent and not the dependency whose bins failed. The hoisted linker reports the bad package and links the rest (PackageInstaller::link_tree_bins).bin::Linker::err(src/install/bin.rs) is the error reported for the package, andlink_bin_or_create_shimalso used it, after every bin, to decide whether the link it just created should be kept. Once one bin of abinmap ordirectories.binfailed, each later bin of that package was created, skipped by the chmod, and unlinked again, and on a reinstall a bin that had been linked correctly before was deleted. A package withbin: {<bad>, multi-b, multi-c}links nothing with either linker.Fix
link_dependency_binsattempts every dependency and collects the failures. If there were any it returns the newTaskError::DependencyBinaries, one(dependency entry, error)per failed dependency, andon_task_failprints one line per entry:ENOTDIR: failed to link binaries of dependency bad-bin-dir-1@deps/bad-bin-dir-1 for package: root@. Continuing the loop is safe: the linker restoresnode_modules_pathafter appending to it, the path buffers are rebuilt by everylink()call, and theseenmap is per.bindirectory on purpose. A failure of the.bindirectory itself (say EACCES) is now reported once per dependency that has bins, which is what the hoisted linker prints too..binis incomplete is not committed to the global store (its staging directory is removed inon_task_fail), the error shows up again on the next install instead of being hidden by the warm-hit path, and the install exits 1. Only the aggregation and the attribution change.symlink_dependencies(thenode_modules/<dep>links) keeps failing on the first error: a package missing one of its dependencies is broken either way, and retrying the rest would not make the entry usable.link_bin_or_create_shimtakeserrout of the linker before creating the link and merges it back afterwards (the first error of the package is what stays reported), so the unlink and the chmod only look at the bin that was just attempted (on Windows this scope includes opening the target, whose failure is the other error path of the same bin). A bin that failed is also removed fromseenand its destination unlinked, so a later package providing the same name, or the retry without the native bin link, can still create it. The two isolated retries reseterrandskipped_due_to_missing_binbefore callinglink()again, which is what the hoisted linker already does by building a fresh linker for the retry; without the reset the first attempt's error would now survive a successful retry.returns in thebinmap anddirectories.binloops for a bin name longer than the path buffer (several KiB); those lines are being reworked by install: stop panicking on bin values longer than the path buffer #38954.test/cli/install/isolated-install.test.ts: a root depending ona-bin, two packages whosedirectories.binpoints at a file,multi-bin(first bin name longer than a file name may be, thenmulti-bandmulti-c) andz-bin, plus a workspace depending on one bad package andz-bin. Asserts the full.binlistings of the root, the workspace andmulti-bin's own store entry, the seven error lines, and exit code 1. On the released build the root hasa-clionly and the other two listings are empty.test/cli/install/bun-install-registry.test.ts("binaries"): the samemulti-binpackage with the hoisted linker linksmulti-bandmulti-c, still reportsFailed to link multi-bin, and exits 1. On the released build.binis empty.binariesblock of the registry tests,isolated-relink.test.tsandbun-install-native-binlink.test.ts(the retry paths) pass with the debug build.Background
node_modules/.bun/<name>@<version>/node_modules/, and its dependencies are symlinked into that directory. The root project and each workspace are entries too; their storenode_modulesis their ownnode_modules.TaskwalkingSteps on the thread pool.Step::SymlinkDependencyBinarieslinks the bins of the entry's dependencies into the entry'snode_modules/.bin(for the root entry, the project'snode_modules/.bin);Step::Binarieslinks the entry's own bins into the same directory. A step that fails returns aTaskError; the main thread prints it inon_task_fail, counts the entry as failed, and the entry's later steps (lifecycle scripts, committing a global-store entry) do not run.bin::Linkeris the bin linker shared by both installers,bun linkandbun unlink. One instance links one package's bins (a single file, abinmap, or every file ofdirectories.bin) into one.bindirectory; callers read itserrfield once afterwards.seenis the set of.binnames already claimed in that directory, so the first package providing a name wins, like npm.directories.binpointing at a missing directory is skipped silently (npm does the same); any other failure to open it is an error.Repro on the released build (bun 1.4.0-canary.1)
With this change:
no test proof · iteration 0 · 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 test/cli/install/isolated-install.test.ts