Repository navigation
install: fail a download that a non-optional peer needs, whichever dependency owns the slot - #43219
Conversation
|
Comment |
|
Updated 9:07 PM PT - Sep 17th, 2026
✅ @robobun, your commit 7fc3fd843c1eff58c2f8dced9c3244ef41d7845f passed in 🧪 To try this PR locally: bunx bun-pr 43219That installs a local version of the PR into your bun-43219 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the plumbing: Bitset in PackageInstaller.rs is an alias for DynamicBitSet, so the bitset from Lockfile::filter is stored without conversion; it is sized to packages.len() at hoist time, matching the bit_length() guards in mark_required and is_required_download; and the isolated linker and auto-installer pass the same per-dependency behavior.is_required() the enqueue functions computed before, so their behavior is unchanged.
Extended reasoning...
Findings are present, so this is the brief ruled-out note only. Verified from the diff: type Bitset = DynamicBitSet at src/install/PackageInstaller.rs:43 and the DynamicBitSet as Bitset import in src/install/hoisted_install.rs:5; required_packages is DynamicBitSet::init_empty(slice.len()) in Lockfile::hoist only for Filter, and mem::take in clean() returns it intact; the three enqueue functions now take is_required as a parameter and the two non-hoisted callers compute it exactly as the removed inline code did. The substantive semantic questions (optional+peer duplicates, optional-only reachability, optional peers losing required status) are covered by the inline findings and need a human decision on intended semantics.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/install/PackageInstaller.rs— pre-existing: on a fresh install (no lockfile) orbun add, a user still gets exit 0 with the package a non-optional peer needs missing from node_modules, in both name orders. The resolve phase downloads the tarball for the optional edge first and logs a warning on 404; the install phase then knowsis_requiredis true fromis_required_downloadbut theAlreadyFailedarm at PackageInstaller.rs:1732 only counts the tree and drops that knowledge. Fix: when an enqueue returnsAlreadyFailedand the caller'sis_requiredis true but the dedupe entry was recorded as not required, log the failure as an error (or bumpsummary.fail) so the install exits 1, at all threeAlreadyFailedarms (1660, 1693, 1732). … [also at: src/install/PackageInstaller.rs:1623 - Pre-existing, partial fix: on a fresh install (no lockfile, cold cache) a 404 for a package that a non-optional peer needs still printswarn:and exits 0 whichever parent sorts first.]Extended reasoning...
…The PR notes #43197 changes this path; until it lands the title's promise holds only when a lockfile exists.
Repro from the issue with no bun.lockb and a cold cache: root depends on aaa (peerDependencies baz) and zzz (optionalDependencies baz), baz-1.0.0.tgz answers 404. Resolve phase: peers of non-root packages are deferred (PackageManagerEnqueue.rs:1763-1767, peer_dependencies.write_item) so zzz's optional edge resolves baz first and generate_network_task_for_tarball at PackageManagerEnqueue.rs:2469 records is_required false (behavior.is_required() of the optional edge). The 404 arrives: runTasks.rs:860 reads is_required false, marks the task failed, logs 'warn: GET ... - 404'. aaa's peer later resolves to the existing baz package, is_first_time false, so no has_created_network_task call ORs the flag. Install phase: filter() sets baz's bit (aaa's peer edge is non-optional), is_required_download returns true at…
Verification: pre-existing (the base fails the same way by the same route; this PR fixes only the lockfile-present path and its own description concedes the fresh-install path: "a peer's tarball that fails first for an optional dependency is a warning, in both name orders. The install phase then gets AlreadyFailed"). Triggering condition: a first install (no lockfile, cold cache) where the package a…
|
Pushed 159affb30c for the sibling-group finding: a peer deduplicated onto its own parent's entry of the same name no longer marks the package, so a name in both |
…ndency is bound to, peers included The tree builder records the package that each linked dependency is placed as or deduplicated onto, for the install tree only. A peer that the hoister binds to an ancestor's other version marks that version. A peer deduplicated onto its own parent's entry of the same name does not mark: that entry decides. The hoisted install gives that bit set to RequiredPackages, which answers from it for every package the hoist saw and walks the lockfile only for a package appended later.
fdade78 to
0fe89f1
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Reworked after the self-review: this PR is now stacked on #43197 (its base is that branch) and feeds the hoisted tree builder's bit set into |
There was a problem hiding this comment.
Beyond the inline finding, I also checked the reorder of the input_dep_range.contains / res_id == package_id checks in hoist_dependency (src/install/lockfile/Tree.rs:1177-1185): both results reach a continue arm in process_subtree, so control flow is unchanged and only the marking differs. The third hoist::<Filter> caller in src/install/prune.rs:783 discards the new HoistResult, so the bit set is allocated there but unused, which is harmless.
Extended reasoning...
A confirmed finding on the sibling-group marking rule is already posted inline, so this body only records what else was examined. The HoistDependencyResult::Hoisted split was audited at every match site (Tree.rs:951, 990-991, 1179, 1184, 1197) and both new variants map to the same continue path as the old single variant; the hoist() return-type change was checked at all three callers (resolve, filter, and prune.rs), each compiling against the new struct with no dropped behavior.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
…s an optional peer The parent's own dependency list decides, not the dependency that owns the slot. This replaces HoistedOntoSibling, which only saw the case where the parent's optional entry owned the slot.
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.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/install/lockfile/Tree.rs— pre-existing: projects on the isolated linker (the default for a fresh workspace lockfile) still get exit 0 or exit 1 for the same failed download depending on which parent sorts first, the exact shape of #43214.RequiredPackages::newat Tree.rs:669 hands the isolated linker an empty tree bitset, socontainsfalls to the walk, and the walk skips every peer edge at Tree.rs:750. The store entry asks with thedep_idof whichever node was created first (isolated_install.rs:2118). Fix: record peer bindings for the isolated store as well (mark the package a non-OPTIONAL peer resolves to), socontainsgives one answer per package under both linkers; the PR calls this exclusion intentional, but isolated users keep the reported bug.Extended reasoning...
The base (#43197) and this PR both leave the isolated linker on the lazy walk; nothing in the diff changes isolated_install.rs, so this is pre-existing, but it is the same class the PR title claims to close.
Shape from the issue, isolated linker: root package.json dependencies { aaa, zzz }; one of them has peerDependencies { baz: "1.0.0" }, the other optionalDependencies { baz: "1.0.0" }. Lockfile present, cold cache, registry answers 404 for baz-1.0.0.tgz.
isolated_install.rs:688 walks root deps in sorted order and pushes nodes; line 860 reverses so the first sibling is popped first (DFS).
If aaa holds the optional edge: aaa's subtree creates the baz node with dep_id = the optional edge (line 711-717). zzz's peer baz later auto-installs (line 821, 846-854) a second baz node; early dedupe at line 535-550 merges it onto the first node (same pkg_id, same name). The store entry for baz keeps node_dep_ids = the optional edge.
isolated_install.rs:2103 builds RequiredPackages::new, sotreeis DynamicBitSet::default() with bit_length 0. Line 2419 calls contains(dep_id = optional…Verification: pre-existing — acknowledged in diff: the PR description says "The isolated linker still walks."; that note is accurate about the code but does not bound the consequence (the isolated linker keeps the exact order-dependent exit code #43214 describes). Triggered on the isolated linker (the default when a V1 lockfile has workspaces, /home/claude/bun/src/install/lockfile.rs:470) when one parent…
|
On the isolated linker finding: I ran the script from #43214 with |
This branch has not been deployed
1 participant
Fixes #43214. Stacked on #43197: the base is that PR's branch, and this PR feeds its
RequiredPackages.Problem
peerDependenciesentry onbaz, the other hasbazinoptionalDependencies. On a cold cache, the install printserror: GET .../baz-1.0.0.tgz - 404and exits 1 when the parent with the peer sorts first. It printswarn:and exits 0 when the other parent sorts first.RequiredPackages(src/install/lockfile/Tree.rs) whether a non-optional dependency resolves to the package, and leaves peers out: the hoister can bind a peer to another package than the one it resolves to, and the tree does not record that binding.Fix
OPTIONALbit is placed as or deduplicated onto.HoistDependencyResult::Hoistednow carries the package of the slot. A peer bound to an ancestor's other version marks that version. A peer whose parent lists the same name inoptionalDependenciesis an optional peer and does not mark, whichever dependency owns the slot.Lockfile::filterreturns that bit set. The hoisted install buildsRequiredPackages::from_treewith it.containsanswers from the bit set for every package the hoist saw and walks the lockfile only for a package appended later. The isolated linker still walks.test/cli/install/bun-install-retry.test.ts(five new tests, two fail on install: fail when an optional and a required dependency share a failed download #43197 alone). Alsobun-install-offline,bun-install-cpu-os,isolated-install,bun-workspaces, andbun-install-lifecycle-scripts(the same PATH failures as noted in install: fail a lifecycle script of a package that a required and an optional dependency share #43218).Background
node_modulesfolder where its name is free (Tree::hoist_dependency). A later dependency on the same package is deduplicated and gets no slot. A peer is deduplicated onto any version that satisfies its range, or onto any version the root declares.Tree.dependency_idis the one dependency that owns a slot. Dependencies sort by behavior then by name, so which dependency owns a slot follows from the names.RequiredPackages(install: fail when an optional and a required dependency share a failed download #43197) decides whether a failed download is an error. It walks the lockfile from the root with the linker's filters and skips peers.Notes
Why the builder and not the walk. The walk sees the lockfile resolution of a peer. The hoister can bind it to another version (
hoist_dependency: a version in range, or any version the root declares). Only the builder knows the binding, so it records it. This is the hoisted-linker answer forRequiredPackages::contains. #43197's walk stays for the isolated linker and for a package the install appends after the hoist.Optional peers. An optional peer has the
OPTIONALbit, so it does not mark. That is the same predicate as #43197.A required edge from an optional parent marks the package. That is the rule on main when the root does not list the package, and #43197 keeps it. An optional subtree that fails as a unit, as npm does, would be a separate change.
Fresh install. With no lockfile, root peers resolve last. #43197's
download_already_failedlets the install phase download again for the required request, so the fresh install reports the error too.Review. Self-reviewed twice. The first review asked to fold the change into #43197 instead of a standalone mechanism, which this revision does. 4 concerns from the bot review: 2 addressed (a name in both
optionalDependenciesandpeerDependenciesof one package.json, first when that package's optional entry owns the slot, then when the root's does), 2 kept as described in the two notes above.Repro from the issue with the debug build, second install, both orders:
[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The hoisted linker decided whether a failed tarball download was an error from the single dependency that happened to own the node_modules slot, so a non-optional peer owning the slot made the failure fatal while an optional dependency owning it made the same failure a warning, and the owner was chosen purely by parent name order. The fix records which package each peer is actually bound to during hoisting and marks that package as required from every non-optional edge that binds to it, rather than only from the slot owner. A peer is treated as optional when its own parent also lists the sa…