install: keep optional peer bindings when cleaning the lockfile

src/install/lockfile.rs

@@ -1020,6 +1020,7 @@
             lockfile: &mut *new,
             mapping: &mut package_id_mapping,
             clone_queue: clone_queue_,
+            optional_peers: PendingResolutions::new(),
             log,
             old_preinstall_state,
             manager: &mut *manager,
@@ -1292,6 +1293,9 @@
 
 pub struct Cloner<'a> {
     pub(crate) clone_queue: PendingResolutions,
+    /// Optional-peer slots, bound in `flush` once every package reachable
+    /// through a non-peer edge has been cloned.
+    pub(crate) optional_peers: PendingResolutions,
     pub lockfile: &'a mut Lockfile,
     pub(crate) old: &'a mut Lockfile,
     pub(crate) mapping: &'a mut [PackageID],
@@ -1318,6 +1322,19 @@
             self.lockfile.buffers.resolutions[to_clone.resolve_id as usize] = new_id;
         }
 
+        // An optional peer stays bound to its target if the target survived the
+        // clean. Loading a lockfile binds these slots before hoisting, so the
+        // `resolve` below has to see them bound as well, or it builds a
+        // different tree than the one `--frozen-lockfile` compares against and
+        // a re-save moves packages around. A target only reachable through
+        // peer slots was never cloned and the slots stay unresolved.
+        for pending in self.optional_peers.drain(..) {
+            let mapping = self.mapping[pending.old_resolution as usize];
+            if (mapping as usize) < max_package_id {
+                self.lockfile.buffers.resolutions[pending.resolve_id as usize] = mapping;
+            }
+        }
+
         // cloning finished, items in lockfile buffer might have a different order, meaning
         // package ids and dependency ids have changed
         self.manager
@@ -1345,8 +1362,22 @@
 // ────────────────────────────────────────────────────────────────────────────
 
 impl Lockfile {
+    /// Builds the tree that is saved to disk.
+    ///
+    /// The resolver leaves optional peers unresolved; hoisting binds each one
+    /// to the same-named package that ends up next to it. When that package is
+    /// placed by an edge processed after the peer's dependent, its subtree is
+    /// queued from that later edge. A lockfile loaded from disk has the binding
+    /// up front and queues the subtree from the dependent instead, which can
+    /// hoist the subtree's dependencies differently. Hoist again in that case
+    /// so the tree is the one a reload builds; otherwise `--frozen-lockfile`
+    /// rejects the lockfile we are about to save. The second pass starts with
+    /// every binding the first one made and cannot bind anything late.
     pub(crate) fn resolve(&mut self, log: &mut bun_ast::Log) -> Result<(), tree::SubtreeError> {
-        self.hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)
+        if self.hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)? {
+            self.hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)?;
+        }
+        Ok(())
     }
 
     pub(crate) fn filter(
@@ -1357,16 +1388,21 @@
         workspace_filters: &[WorkspaceFilter],
         packages_to_install: Option<&[PackageID]>,
     ) -> Result<(), tree::SubtreeError> {
+        // Runs after `resolve` bound the optional peers, so there is nothing
+        // left to bind late here.
         self.hoist::<{ tree::BuilderMethod::Filter }>(
             log,
             Some(manager),
             install_root_dependencies,
             workspace_filters,
             packages_to_install,
-        )
+        )?;
+        Ok(())
     }
 
-    /// Sets `buffers.trees` and `buffers.hoisted_dependencies`
+    /// Sets `buffers.trees` and `buffers.hoisted_dependencies`. Returns whether
+    /// an optional peer was bound after its dependent had been placed
+    /// (`tree::Builder::late_bound_optional_peer`).
     pub(crate) fn hoist<const METHOD: tree::BuilderMethod>(
         &mut self,
         log: &mut bun_ast::Log,
@@ -1376,7 +1412,7 @@
         install_root_dependencies: bool,
         workspace_filters: &[WorkspaceFilter],
         packages_to_install: Option<&[PackageID]>,
-    ) -> Result<(), tree::SubtreeError> {
+    ) -> Result<bool, tree::SubtreeError> {
         let slice = self.packages.slice();
 
         // `tree::Builder` stores `lockfile: ParentRef<Lockfile>` so
@@ -1398,6 +1434,7 @@
             workspace_filters,
             packages_to_install,
             pending_optional_peers: Default::default(),
+            late_bound_optional_peer: false,
             list: Default::default(),
             sort_buf: Default::default(),
         };
@@ -1416,9 +1453,10 @@
         }
 
         let cleaned = builder.clean()?;
+        let late_bound_optional_peer = builder.late_bound_optional_peer;
         self.buffers.trees = cleaned.trees;
         self.buffers.hoisted_dependencies = cleaned.dep_ids;
-        Ok(())
+        Ok(late_bound_optional_peer)
     }
 }
 

src/install/lockfile/Package.rs

@@ -606,26 +606,28 @@ impl Package<u64> {
             .zip(resolutions.iter_mut())
             .enumerate()
         {
-            // Optional-peer slots are re-derived by `hoist` (Cloner::flush), not carried over.
-            if old_dependencies[i].behavior.is_optional_peer() {
+            if *old_resolution >= max_package_id {
                 *resolution = invalid_package_id;
                 continue;
             }
 
-            if *old_resolution >= max_package_id {
-                *resolution = invalid_package_id;
+            let pending = PendingResolution {
+                old_resolution: *old_resolution,
+                resolve_id: new_package.resolutions.off + PackageID::try_from(i).expect("int cast"),
+            };
+
+            // An optional peer does not keep its target alive. `Cloner::flush`
+            // binds the slot again only if a non-peer edge cloned the target.
+            if old_dependencies[i].behavior.is_optional_peer() {
+                cloner.optional_peers.push(pending);
                 continue;
             }
 
             let mapped = package_id_mapping[*old_resolution as usize];
             if mapped < max_package_id {
                 *resolution = mapped;
             } else {
-                cloner.clone_queue.push(PendingResolution {
-                    old_resolution: *old_resolution,
-                    resolve_id: new_package.resolutions.off
-                        + PackageID::try_from(i).expect("int cast"),
-                });
+                cloner.clone_queue.push(pending);
             }
         }
 

src/install/lockfile/Tree.rs

@@ -442,6 +442,11 @@ pub struct Builder<'a, const METHOD: BuilderMethod> {
     // could be visited multiple times before it's resolved.
     pub(crate) pending_optional_peers:
         ArrayHashMap<PackageNameHash, ArrayHashMap<DependencyID, ()>>,
+    /// Set when an unresolved optional peer got bound by a package placed
+    /// after the peer's dependent (`HoistDependencyResult::ResolveReplace`),
+    /// so the target's subtree was queued later than it will be once the
+    /// binding is known up front. See `Lockfile::resolve`.
+    pub(crate) late_bound_optional_peer: bool,
     pub(crate) manager: Option<&'a PackageManager>,
     pub(crate) sort_buf: Vec<DependencyID>,
     pub(crate) workspace_filters: &'a [WorkspaceFilter],
@@ -874,6 +879,7 @@ impl Tree {
                 }
                 HoistDependencyResult::ResolveReplace(replace) => {
                     debug_assert!(pkg_id != invalid_package_id);
+                    builder.late_bound_optional_peer = true;
                     builder.resolutions[replace.dep_id as usize] = pkg_id;
                     if let Some(entry) = builder
                         .pending_optional_peers

test/cli/install/bun-lock.test.ts

@@ -1004,11 +1004,141 @@ it("optional peer with a non-wildcard range is idempotent with two versions of t
   expect(first).toContain('"no-deps": ["no-deps@');
 
   // A second install over the same lockfile must be a byte-for-byte no-op: the
-  // cleared optional-peer slot re-derives to the same value hoist produced on
-  // fresh install.
+  // optional peer stays bound to the no-deps the fresh install bound it to.
   await run(["install"]);
   expect(await file(join(packageDir, "bun.lock")).text()).toBe(first);
 
   await rm(join(packageDir, "node_modules"), { recursive: true, force: true });
   await run(["install", "--frozen-lockfile"]);
 });
+
+// The optional-peer-hoist-* fixtures (registry/packages/create-optional-peer-hoist-packages.ts):
+//
+//   consumer      optional peer on target
+//   deep          -> deep-child -> leaf@1.0.0, target@1.0.0
+//   target@1.0.0  -> leaf@2.0.0
+//   provider      -> target@2.0.0
+//
+// Hoisting is breadth-first, so which leaf ends up in the root node_modules
+// depends on whether consumer's peer is already bound to target when the tree
+// is built: bound, target is placed from consumer and its leaf@2.0.0 reaches
+// the root before deep-child's leaf@1.0.0; unbound, target is only placed once
+// deep-child is reached and leaf@1.0.0 wins. A loaded bun.lock always has the
+// peer bound, so that is the tree every install has to build, otherwise
+// --frozen-lockfile compares two different trees.
+const optionalPeerHoistDeps = {
+  "optional-peer-hoist-consumer": "1.0.0",
+  "optional-peer-hoist-deep": "1.0.0",
+};
+
+it("a fresh install hoists around an optional peer the same way a reinstall does", async () => {
+  const { packageDir, packageJson } = await registry.createTestDir({ bunfigOpts: { saveTextLockfile: true } });
+  const run = makeInstallRunner(packageDir);
+
+  await write(packageJson, JSON.stringify({ name: "foo", dependencies: optionalPeerHoistDeps }));
+  await run(["install"]);
+  const fresh = await file(join(packageDir, "bun.lock")).text();
+  expect(fresh).toContain('"optional-peer-hoist-leaf": ["optional-peer-hoist-leaf@2.0.0"');
+  expect(fresh).toContain(
+    '"optional-peer-hoist-deep-child/optional-peer-hoist-leaf": ["optional-peer-hoist-leaf@1.0.0"',
+  );
+
+  await run(["install", "--frozen-lockfile"]);
+
+  // --lockfile-only always writes, so this checks the tree a reload builds
+  // prints back to the same text.
+  await run(["install", "--lockfile-only"]);
+  expect(await file(join(packageDir, "bun.lock")).text()).toBe(fresh);
+});
+
+it.each([
+  [
+    "leaf@2.0.0 hoisted (target placed from consumer)",
+    {
+      "optional-peer-hoist-leaf": "2.0.0",
+      "optional-peer-hoist-deep-child/optional-peer-hoist-leaf": "1.0.0",
+    },
+  ],
+  [
+    // What a fresh install wrote before the peer binding was carried over.
+    "leaf@1.0.0 hoisted (target placed from deep-child)",
+    {
+      "optional-peer-hoist-leaf": "1.0.0",
+      "optional-peer-hoist-target/optional-peer-hoist-leaf": "2.0.0",
+    },
+  ],
+])("--frozen-lockfile accepts an existing bun.lock with %s", async (_, leafPlacement) => {
+  const { packageDir, packageJson } = await registry.createTestDir({ bunfigOpts: { saveTextLockfile: true } });
+  const run = makeInstallRunner(packageDir);
+
+  const pkg = (name: string, version: string, info: object = {}) => [
+    `${name}@${version}`,
+    `${registry.registryUrl()}${name}/-/${name}-${version}.tgz`,
+    info,
+    "",
+  ];
+  const packages: Record<string, unknown[]> = {
+    "optional-peer-hoist-consumer": pkg("optional-peer-hoist-consumer", "1.0.0", {
+      peerDependencies: { "optional-peer-hoist-target": "*" },
+      optionalPeers: ["optional-peer-hoist-target"],
+    }),
+    "optional-peer-hoist-deep": pkg("optional-peer-hoist-deep", "1.0.0", {
+      dependencies: { "optional-peer-hoist-deep-child": "1.0.0" },
+    }),
+    "optional-peer-hoist-deep-child": pkg("optional-peer-hoist-deep-child", "1.0.0", {
+      dependencies: { "optional-peer-hoist-leaf": "1.0.0", "optional-peer-hoist-target": "1.0.0" },
+    }),
+    "optional-peer-hoist-target": pkg("optional-peer-hoist-target", "1.0.0", {
+      dependencies: { "optional-peer-hoist-leaf": "2.0.0" },
+    }),
+  };
+  for (const [path, version] of Object.entries(leafPlacement)) {
+    packages[path] = pkg("optional-peer-hoist-leaf", version);
+  }
+
+  await write(packageJson, JSON.stringify({ name: "foo", dependencies: optionalPeerHoistDeps }));
+  await write(
+    join(packageDir, "bun.lock"),
+    JSON.stringify({
+      lockfileVersion: 2,
+      configVersion: 1,
+      workspaces: { "": { name: "foo", dependencies: optionalPeerHoistDeps } },
+      packages,
+    }),
+  );
+
+  await run(["install", "--frozen-lockfile"]);
+});
+
+it("adding a dependency keeps an optional peer bound to the package bun.lock bound it to", async () => {
+  const { packageDir, packageJson } = await registry.createTestDir({ bunfigOpts: { saveTextLockfile: true } });
+  const run = makeInstallRunner(packageDir);
+
+  await write(packageJson, JSON.stringify({ name: "foo", dependencies: optionalPeerHoistDeps }));
+  await run(["install"]);
+  expect(await file(join(packageDir, "bun.lock")).text()).toContain(
+    '"optional-peer-hoist-target": ["optional-peer-hoist-target@1.0.0"',
+  );
+
+  // provider brings in target@2.0.0, which consumer's peer range would accept
+  // too. The lockfile already binds consumer to target@1.0.0, so that binding
+  // (and the hoisting that follows from it) is kept and the new version nests.
+  await write(
+    packageJson,
+    JSON.stringify({
+      name: "foo",
+      dependencies: { ...optionalPeerHoistDeps, "optional-peer-hoist-provider": "1.0.0" },
+    }),
+  );
+  await run(["install"]);
+  const lockfile = await file(join(packageDir, "bun.lock")).text();
+  expect(lockfile).toContain('"optional-peer-hoist-target": ["optional-peer-hoist-target@1.0.0"');
+  expect(lockfile).toContain(
+    '"optional-peer-hoist-provider/optional-peer-hoist-target": ["optional-peer-hoist-target@2.0.0"',
+  );
+  expect(lockfile).toContain('"optional-peer-hoist-leaf": ["optional-peer-hoist-leaf@2.0.0"');
+
+  await run(["install", "--frozen-lockfile"]);
+  await run(["install", "--lockfile-only"]);
+  expect(await file(join(packageDir, "bun.lock")).text()).toBe(lockfile);
+});