Conversation
… that workspace A range of a registry package links to a workspace of the same name through the root's workspaces edge. When a later install drops that edge (a root dependency of the same name replaces it, or the workspace leaves workspaces), the range stayed linked. The workspace stayed in the lockfile through an edge bun.lock does not record, so the next install failed with 'Duplicate package path', installed a different package than the install that wrote the file, or failed on a deleted folder. The differ now resets those ranges and enqueues them again, so the install writes the bun.lock that an install with no lockfile writes.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for one more day.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 2 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 12:34 PM PT - Sep 19th, 2026
✅ @robobun, your commit 1bdca05d7f430bfd7475f7159358f1252dc0a10a passed in 🧪 To try this PR locally: bunx bun-pr 43468That installs a local version of the PR into your bun-43468 --bun |
|
Status: draft, do not merge. How I reproduced the original bug: a loopback registry with What is wrong with this branch (head 1bdca05). All three are reproduced, and all came from review comments, not from my own checks.
Why I stopped: the pass estimates what the new root reaches before resolution has run. Two seed rules in a row were wrong, one in each direction. A correct version has to run after resolution settles, which changes the main install loop, or the fix has to move somewhere else. I want a maintainer to choose that. I did not complete my own review of this diff before I opened the PR, and it shows. What is still useful here: the test matrix (5 histories, both linkers) and the probes for the three failures above. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Rows of the root and of workspaces link by path, with or without the root's edge. | ||
| lockfile | ||
| .packages | ||
| .items_dependencies() | ||
| .iter() | ||
| .zip(pkg_resolutions) | ||
| .filter(|(_, resolution)| { | ||
| !matches!( | ||
| resolution.tag, | ||
| ResolutionTag::Root | ResolutionTag::Workspace | ||
| ) | ||
| }) | ||
| .flat_map(|(owned, _)| owned.begin()..owned.end()) | ||
| .filter(|&row| { | ||
| dropped.is_set_allow_out_of_bound(resolutions[row as usize] as usize, false) | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
🔴 Users who remove a workspace from workspaces can get bun install exit 1 where the base exits 0, when a package reachable only through that workspace has a range on the workspace's own name. install_with_manager.rs:1591-1606 collects rows of every non-root, non-workspace package, including packages the new root no longer reaches and clean will discard. Those rows are re-resolved from the registry; a private workspace name gets a 404, which is an error for a required dep or a non-optional peer. Fix: re-enqueue only rows of packages still reachable from the new root (walk buffers.resolutions from the root rows, skipping dropped workspaces), so unreachable packages are pruned as on the base. [also at: src/install/PackageManager/install_with_manager.rs:1616 - Users whose dropped workspace was the only path to a git/file/tarball package that ranges on the workspace's own (unpublished) name now get GET <registry>/<name> - 404 and exit 1, where the base install succeeded. install_with_manager.rs:1591-1606 selects rows by the owner's resolution tag only,…]
Extended reasoning...
Shape: workspaces packages/host (@ acme/host, private, never published) and packages/app. host depends on a registry package @ acme/host-plugin whose peerDependencies has @ acme/host: ^1. First install links the peer to the workspace. The user moves host out of the monorepo by removing it from workspaces (the folder may stay). Base: the differ drops the root edge, nothing re-enqueues plugin's rows, Lockfile::clean_with_logger (lockfile.rs:1077-1099) clones from the root, so host and the plugin are dropped; exit 0. New: the plugin is ResolutionTag::Npm so its rows pass the filter at install_with_manager.rs:1596-1601; its peer row resolves to the dropped workspace (dropped set at :1583), untag_workspace_link restores ^1, :1616 enqueues it. Phase 1 defers the peer (PackageManagerEnqueue.rs:1142, 1327).…
Verification: normal (narrow trigger, but a hard exit-1 regression where base and a fresh install both succeed). Trigger: a workspace leaves workspaces while a non-workspace package that only that workspace reached has an npm range on the workspace's own (unpublished) name. Mechanism verified in /home/claude/bun/src/install/PackageManager/install_with_manager.rs:1554-1621. The row collection at 1591-1606…
There was a problem hiding this comment.
Confirmed, and thank you. I reproduced it: a workspace the registry does not have depends on a package that has a range on the workspace's name, then the workspace leaves workspaces. main exits 0. d4f850d exited 1 with GET <registry>/<name> - 404, did not update bun.lock, and every later install failed too. It happened with a peer and with a regular dependency. Fixed in 1bdca05: the pass walks from the surviving root rows (reachable::packages_from) and skips owners the walk does not reach. Two new tests cover it. They fail on d4f850d and pass on main and on this commit. The walk is an estimate because it runs before resolution. The PR notes describe the one case it can still miss.
| let mut kept: Vec<PackageNameHash> = lockfile.packages.items_dependencies()[0] | ||
| .get(dependencies) | ||
| .iter() | ||
| .filter(|dep| dep.behavior.is_workspace()) | ||
| .map(|dep| dep.name_hash) | ||
| .collect(); | ||
| index_sort::sort_slice_unstable_by(&mut kept, |a, b| a.cmp(b)); | ||
|
|
||
| let mut dropped = DynamicBitSet::init_empty(pkg_resolutions.len()).unwrap_or_oom(); | ||
| for (previous, &workspace) in previous_root_dependencies | ||
| .get(dependencies) | ||
| .iter() | ||
| .zip(previous_root_resolutions) | ||
| { | ||
| if previous.behavior.is_workspace() | ||
| && (workspace as usize) < pkg_resolutions.len() | ||
| && kept.binary_search(&previous.name_hash).is_err() |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing, left unfixed by this change: users who move a workspace folder (same name, new path) while a registry package's range links to it still get a bun.lock the next install rejects with Duplicate workspace name. kept at install_with_manager.rs:1565-1581 is keyed by name_hash, so the old workspace package keeps its linked rows even though the root's edge now resolves to a new package appended for the new path. Fix: mark a previous workspace package dropped when no workspace row of the new root resolves to that package id (or its version.workspace() path differs), not only when its name is gone from the root.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Extended reasoning...
Setup as in the PR's tests: workspace packages/no-deps (no-deps@ 1.5.0), workspace packages/app depends on peer-deps-fixed, whose peer no-deps@^1.0.0 links to the workspace on the first install. Then the user moves the folder to libs/no-deps and sets workspaces: ["packages/*", "libs/*"]. Diff::generate (lockfile/Package.rs:1361-1436): the old root row no-deps (Behavior::WORKSPACE) is found in to_deps with the same name and behavior, but Dependency::eql fails because Version::eql compares the workspace path (dependency.rs:677); mapping stays invalid, summary.update += 1. install_with_manager.rs:572-591 re-enqueues the root row; PackageManagerEnqueue.rs:2973-3026 calls FolderResolution::get_or_put for libs/no-deps; folder_resolver.rs:361 get_package_id compares Resolution::Workspace paths, packages/no-deps != libs/no-deps, so a new package W2 is appended. Now reresolve_ranges_linked_to_dropped_workspaces: kept = {app, no-deps} by name, the old row's name_hash is in kept, so W1 is never set in dropped (line 1581) and the function returns at 1586. The peer row…
Verification: pre-existing (base fails by the same route; this PR adds the function that is meant to handle exactly this class of dropped root->workspace edges and the moved-workspace variant slips through it). Trigger: a workspace folder is moved to a new path with the same name while a registry package's npm range (e.g. a peer) is linked to that workspace in bun.lock. Mechanism verified: - Root workspace…
There was a problem hiding this comment.
Reproduced on main and on d4f850d. After the move, bun.lock keeps the old workspaces entry and --frozen-lockfile fails with lockfile had changes. I did not see Duplicate workspace name. Fixed in 1bdca05 with the path variant you suggested: a kept name whose version.workspace() path differs counts as dropped. I did not use the package id, because a workspace whose package.json changed has an unresolved root row at that point, and the id rule would drop it by mistake. New history row "the workspace moves to another folder", both linkers. It fails on main.
There was a problem hiding this comment.
Correction to my reply above. The move is fixed only when nothing depends on the moved workspace, which is the shape my test uses. When another workspace depends on it by workspace:*, that workspace is parsed again, the reach walk stops at its unresolved rows, and this branch behaves like main: bun.lock keeps the old workspaces entry and --frozen-lockfile fails with lockfile had changes. I reproduced that. I have reopened this thread.
… a moved workspace as dropped The pass that re-resolves ranges linked to a dropped workspace collected rows of every non-workspace package. A package that only the dropped workspace reached is discarded by clean, but its range on the workspace's name was sent to the registry first. For a name the registry does not have, that failed the install with a 404 and left bun.lock unchanged, so every later install failed too. The pass now walks from the root rows that survive and skips owners the walk does not reach. A workspace that moves to another folder keeps its name and gets a new package. The dropped set was keyed by name, so the old package kept its linked ranges and stayed in bun.lock. The workspace path now decides as well.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// A range of a non-workspace package links to a workspace only through the root's edge to it | ||
| /// (`root_workspace_package_id`). Ranges linked to a workspace whose root edge the differ dropped | ||
| /// resolve again, as they do with no lockfile: bun.lock cannot record the link without the edge. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Shortened to two lines in f0b31300a5. That commit is local for now. CI is running on 1bdca05, which fixes a regression, and I do not want to restart it for a comment. It goes out with the next push. I kept the pointer to root_workspace_package_id, because that is the fact a reader cannot get from this function.
| // Only owners the new root still reaches: `clean` discards the rest, and a range of theirs | ||
| // on a workspace name the registry does not have would fail the install. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Shortened to one line in f0b31300a5 (local, goes out with the next push). I kept it because the walk looks removable without it, and removing it brings back an install that fails with a 404. A test now guards the clause as well.
| dropped.set(resolved as usize); | ||
| } else if names.binary_search(&previous.name_hash).is_ok() { | ||
| survivors.push(resolved); |
There was a problem hiding this comment.
🔴 Users who bump a root dependency's range in the same edit that drops a private workspace can get bun install exit 1 where the base exits 0. The reachability filter seeds survivors from the OLD resolution of every root row whose name is still present (install_with_manager.rs:1601), including rows the differ left unmapped because their range changed. Packages only that stale old version reached count as reached, so their ranges on the dropped workspace's unpublished name are re-enqueued and 404. …
Extended reasoning...
…Fix: seed survivors only from rows the differ mapped (the new root's resolution is not invalid_package_id), or from the new root's resolutions slice, so the walk covers only what the new root still resolves to; the moved-workspace case still needs the re-enqueued root workspace row, which resolves synchronously before this call.
Shape: workspaces packages/host (private host@ 1.5.0, unpublished) and packages/app; root has "x": "1.0.0" and x@ 1.0.0 depends on registry plugin whose dependencies (or peerDependencies) has host: ^1. First install links plugin's row to the workspace. The user edits package.json once: removes packages/host from workspaces and bumps x to 2.0.0 (x@ 2.0.0 no longer depends on plugin). Second install: Diff::generate finds the x row by name and behavior but Dependency::eql fails (Package.rs:1431), so mapping stays invalid_package_id and the new root's resolution for x is invalid until install_with_manager.rs:572-591 enqueues it. In reresolve_ranges_linked_to_dropped_workspaces the previous x row is not a workspace row, its name is…
Verification: normal — regression vs base (exit 1 where base and a fresh install both exit 0). Trigger: one package.json edit both drops a workspace's root edge (e.g. removes it from workspaces) and changes a root dependency's range so the differ leaves that row unmapped, while the OLD resolution of that row reached a registry package holding a range on the dropped workspace's unpublished name. Mechanism…
There was a problem hiding this comment.
Confirmed. I reproduced it on 1bdca05: one edit removes host from workspaces and moves root x from 1.0.0 to 2.0.0, where only x@1.0.0 reached the package with a range on host. main exits 0. This branch exits 1 with GET <registry>/host - 404, does not update bun.lock, and fails again on the next install. I had seen this case myself and listed it in the PR notes as a limit I accepted. That was the wrong call: the result is an install that stays broken, which is worse than the bug this PR fixes, because that bug repairs itself on the next plain install. I have converted the PR to a draft and put a warning at the top of the description. I am not pushing another seed rule. See my reply on the next thread for why.
| let reached = reachable::packages_from( | ||
| lockfile, | ||
| resolutions, | ||
| &survivors, | ||
| true, | ||
| reachable::Options::all(0), | ||
| ); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Monorepo users who also change a surviving workspace's dependencies in the edit that drops a workspace still get a bun.lock the next install rejects with Duplicate package path. The reach walk at install_with_manager.rs:1610 runs before drain_dependency_list (:648), and a re-parsed survivor's fresh rows are all invalid_package_id, so nothing below that workspace is reached and its packages keep the link to the dropped workspace. Fix: compute reachability from the dependency slices as they were before the root rows at :572 are re-enqueued, or run the pass once pending resolution has settled. A sibling depending on a moved workspace by workspace:* is re-parsed too, so the moved case always hits this. [also at: src/install/PackageManager/install_with_manager.rs:1601 - Users who drop a workspace and, in the same edit, add a root dependency on a package whose range was linked to it still get a bun.lock that bun install --frozen-lockfile rejects with lockfile had changes, but lockfile is frozen. install_with_manager.rs:1600 seeds survivors only from OLD root…]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Extended reasoning...
Shape: root workspaces: ["packages/*", "libs/*"], workspace packages/no-deps (no-deps@ 1.5.0), workspace packages/app with "no-deps": "workspace:*" and "peer-deps-fixed": "1.0.0" (peer no-deps@^1.0.0). First install links the peer to W1 (packages/no-deps). The user moves the folder to libs/no-deps and installs again. Same failure for any history in the PR's matrix when app's package.json gains or bumps any dependency in the same edit.
Diff::generate re-parses app (Package.rs:1504) because its no-deps row now carries path libs/no-deps; Version::eql (dependency.rs:677) fails, generate_inner reports update, update_mapping is false at Package.rs:1551-1554, so app's mapping stays invalid and Package.rs:1597 bumps summary.update.
install_with_manager.rs:572-591 enqueues the root row for app.…
Verification: pre-existing (the base already writes the same unloadable bun.lock by the same route; this PR adds the function meant to fix that class and the fix is bypassed on this path). Trigger: the edit that drops a workspace's root edge (root dependency takes its name, it leaves workspaces, folder deleted, or folder moved) also adds/bumps/removes a dependency of a surviving workspace (e.g.…
There was a problem hiding this comment.
Confirmed, both parts I ran. (1) Root takes the workspace's name and app gains a dependency in the same edit: this branch behaves exactly like main. packages/host stays under workspaces and --frozen-lockfile fails with Duplicate package path. So the fix is bypassed whenever a surviving workspace is parsed again. I had argued that seeding the old package id of app still reaches its packages. That was wrong: the parse replaces the rows under the same package and the walk stops there. (2) A moved workspace that app depends on by workspace:*: same as main, the old workspaces entry stays and the frozen install fails. My test for the move only covers a workspace that nothing depends on, so my earlier reply that the move is fixed was too strong, and I have reopened that thread. I did not run the third shape (a new root dependency on the linked package). Together with the thread above, the estimate is wrong in both directions, so I agree the pass has to run once resolution has settled, with real reachability, or not re-resolve at all. That is a change to the main install loop, and I want a maintainer to choose before I build it.
|
Heads-up from #43567. The history "a root alias takes the registry package of that name" relies on the alias removing the root's dependency on the workspace. An alias installs in The histories with a root dependency of the workspace's own name are not affected. The policy for that case is a separate decision: #43568. |
This branch has not been deployed
1 participant
Problem
host@1.5.0, and a registry package with a peerhost@^1.bun install, give the root"host": "1.0.0", install again. It exits 0 and saves a bun.lock the next install rejects:error: Duplicate package path.install_with_manager.rs) re-resolves only root rows. The peer stays linked through an edge bun.lock cannot record."workspaces"or moves stays in bun.lock. A deleted one makes each install exit 1.Fix
reresolve_ranges_linked_to_dropped_workspacesfinds workspaces that lost their root edge (name gone, or new path). It resets and enqueues each edge a non-workspace package has to them.Lockfile::untag_workspace_linkrestores the range first.cleandiscards those, and their range can 404.root_workspace_package_idneeds the root edge.test/cli/install/bun-workspaces.test.ts. 10 fail on main. 2 guard the reach rule: they pass on main, fail without it.Background
workspacesentry: the root edge. A same-name root dependency replaces it when the workspace version fails its range.packageskey equal to its name.Notes
Correction to the first push (d4f850d). That commit collected rows of every non-workspace package. A review comment pointed out the case where only the dropped workspace reaches the owner, and I reproduced it: workspace
host(a name the registry does not have) depends onplugin,pluginhas a range onhost, thenhostleaves"workspaces". main exits 0 and removes both packages. An install with no lockfile exits 0. d4f850d exited 1 witherror: GET <registry>/host - 404, did not update bun.lock, and every later install failed the same way. It happened with a peer and with a regular dependency. My first description said the second install writes what a fresh install writes. That was false for this shape. The second commit limits the pass to owners the new root still reaches, and two tests cover it.The reach is an estimate, because the pass runs before resolution. It walks from the previous root rows whose name is still in the new root. If one install both drops a workspace and points another root dependency at a new target, the old target still counts as reached. If that old target has a range on the dropped, unpublished name, the 404 can still happen. I chose this over walking only unchanged rows: that would miss a package reached through a workspace whose package.json changed in the same install, which brings back the original bug in a more common case.
The same review listed four more cases. I ran two of them.
workspacesentry and--frozen-lockfilefailed withlockfile had changes). I did not see theDuplicate workspace nameerror the comment predicted.workspace:spec: not run, and left as it is. I have no fixture for it, so I cannot test a change.One arm has no test: in the path comparison, the case where exactly one of the two rows is not a
workspacetag. The three places that create root workspace rows always use that tag, so I could not build an input for it. The tag check stays becauseworkspace()reads a union field.Repro without the test harness (local registry with
host@1.0.0andplugin@1.0.0,pluginhaspeerDependencies: { "host": "^1" }):Why the second install wrote that file:
cleankeeps the workspace package because the peer ofpluginstill resolves to it. The hoister merges the peer into the root'shost(host@1.0.0satisfies^1), so the workspace has no folder in the tree and nopackageskey. The writer lists each workspace package of the lockfile underworkspaces.Why the alias variant differs: no root dependency is named
host, so the peer puts the workspace at the root keyhost, and the file loads. On load the peer binds by version tohost@1.0.0(resolve_peer_dep_version_based), not to the workspace that the file records athost. The loader also adds a root edge for eachworkspacesentry, which the parse of package.json does not have. That is thelockfile had changesfailure.Each test of the history matrix does this: first install (asserts that the peer links the workspace), the change, second install (asserts the
workspacesandpackagesof bun.lock and theno-depsthatpeer-deps-fixedloads),--frozen-lockfileinstall into an emptynode_modules(sameno-deps), plain install (no save, same bytes), install with no lockfile (same bytes).The five histories: a root dependency takes the name of the workspace, a root alias takes the registry package of that name, the workspace leaves
"workspaces", the folder of the workspace is deleted, the folder moves. Each runs with the hoisted and the isolated linker.Suites on the debug build. On the final commit (1bdca05) I ran
bun-workspaces(94),frozen-lockfile-pruned(101),frozen-lockfile-missing-workspace,bun-lock(40),bun-add(71),bun-remove,bun-update(159),bun-workspaces-self-contained,catalogs(89) andisolated-install(79, the six git and github network tests filtered out). All pass. Onebun-lockrun failed in thebeforeAllthat starts the test registry (a 5000 ms hook timeout, before any test ran). The next two runs passed 40 of 40. My earlier runs had left 277 registry processes behind, which I stopped. I think that load caused the timeout, but I did not prove it. The list below is from the first commit only, and I did not repeat it on the final commit:bun-workspaces(90),isolated-install(79, the six git and github network tests filtered out),bun-lock,lockfile-only,lockfile-version-2,bun-lockb,migrate-bun-lockb-v2,bun-add,bun-remove,bun-update,bun-update-transitive,bun-add-filter,overrides,catalogs,frozen-lockfile-pruned,frozen-lockfile-missing-workspace,bun-workspaces-self-contained,hoist,bun-install-registry(281),migration/migrate,bun-pm,bun-security-scanner-workspaces. All pass.bun-installhas 13 failures, all gitlab, bitbucket or external URL tests that need the public network.bun-prune,bun-dedupe,nested-overridesand one test oftest-dev-peer-dependency-priorityhit the 5000 ms test timeout on this machine. I did not run those files without the change, so I cannot say the timeouts are the same there. The onetest-dev-peer-dependency-prioritytest passes in 1.5 s when run alone.One clause has no test: the filter that skips rows of the root and of workspaces. Those rows link by path (
get_workspace_pkg_if_workspace_depis the same predicate in the resolver), so a fresh install keeps them linked. A test needs a workspace that keeps the replaced workspace through its own range. On main that bun.lock fails to reload withDuplicate package pathfor the loader reason below, so the test cannot pass until #37248 lands.error: Duplicate package pathis also the symptom in #37248, #37245 and #41833. I merged #37248 with main and ran this repro on it. The plain repro then passes, but a replaced workspace that has its own dependencies still fails to load (Failed to resolve prod dependency 'leaf' for package 'host'), and the alias variant passes--frozen-lockfilewhile the frozen install links a differenthostthan the install that wrote the file. #37245 and #41833 are aboutfile:dependencies. I did not run them.Not covered here: a workspace that a root dependency replaced, but that another workspace keeps through
workspace:*. A fresh install of that shape writes"beta/alpha": ["alpha@workspace:packages/alpha"]next to"alpha": ["alpha@1.0.0", ...], and the next install also fails withDuplicate package path. That failure needs no second install, and its cause is in the bun.lock loader. #37248 has that fix. The two changes do not touch the same source files.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-workspaces.test.ts