install: keep optional peer bindings when cleaning the lockfile

src/install/lockfile.rs

@@ -1020,6 +1020,7 @@ impl Lockfile {
             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,8 @@ impl Lockfile {
 
 pub struct Cloner<'a> {
     pub(crate) clone_queue: PendingResolutions,
+    /// Bound in `flush`, once `clone_queue` has decided which targets survive.
+    pub(crate) optional_peers: PendingResolutions,
     pub lockfile: &'a mut Lockfile,
     pub(crate) old: &'a mut Lockfile,
     pub(crate) mapping: &'a mut [PackageID],
@@ -1318,6 +1321,16 @@ impl<'a> Cloner<'a> {
             self.lockfile.buffers.resolutions[to_clone.resolve_id as usize] = new_id;
         }
 
+        // Loading a lockfile binds optional peers before hoisting, so the hoist
+        // below has to see them bound too or `--frozen-lockfile` compares two
+        // different trees. A target nothing else cloned leaves its slots unbound.
+        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 +1358,15 @@ impl<'a> Cloner<'a> {
 // ────────────────────────────────────────────────────────────────────────────
 
 impl Lockfile {
+    /// Builds the tree that is saved to disk: hoists until a pass binds no
+    /// optional peer late. A peer bound mid-pass had its target's subtree
+    /// queued from a later edge than a reload (which has the binding up
+    /// front) queues it from, so that pass can hoist differently than the
+    /// reload `--frozen-lockfile` compares against. Repeating only ever fills
+    /// more slots, so this ends.
     pub(crate) fn resolve(&mut self, log: &mut bun_ast::Log) -> Result<(), tree::SubtreeError> {
-        self.hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)
+        while self.hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)? {}
+        Ok(())
     }
 
     pub(crate) fn filter(
@@ -1357,16 +1377,19 @@ impl Lockfile {
         workspace_filters: &[WorkspaceFilter],
         packages_to_install: Option<&[PackageID]>,
     ) -> Result<(), tree::SubtreeError> {
+        // `resolve` already bound every optional peer; nothing binds 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
+    /// `tree::Builder::late_bound_optional_peer`.
     pub(crate) fn hoist<const METHOD: tree::BuilderMethod>(
         &mut self,
         log: &mut bun_ast::Log,
@@ -1376,7 +1399,7 @@ impl Lockfile {
         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 +1421,7 @@ impl Lockfile {
             workspace_filters,
             packages_to_install,
             pending_optional_peers: Default::default(),
+            late_bound_optional_peer: false,
             list: Default::default(),
             sort_buf: Default::default(),
         };
@@ -1416,9 +1440,10 @@ impl Lockfile {
         }
 
         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,27 @@ 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"),
+            };
+
+            // Peer slots must not keep their target alive; bound in `Cloner::flush`.
+            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

@@ -135,6 +135,9 @@ enum HoistDependencyResult {
     Resolve(PackageID),
     ResolveReplace(ResolveReplace),
     ResolveLater,
+    /// `Hoisted`, and the optional peer's slot is repointed at the version it
+    /// deduplicated onto, which is what loading the saved tree binds it to.
+    Rebind(PackageID),
     Placement(Placement),
 }
 
@@ -442,6 +445,9 @@ 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, ()>>,
+    /// A `ResolveReplace` bound an optional peer after its dependent was
+    /// placed. 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 +880,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
@@ -909,6 +916,10 @@ impl Tree {
                         })?;
                     }
                 }
+                HoistDependencyResult::Rebind(res_id) => {
+                    debug_assert!(dependency.behavior.is_optional_peer());
+                    builder.resolutions[dep_id as usize] = res_id;
+                }
                 HoistDependencyResult::ResolveLater => {
                     // `dep_id` is an unresolved optional peer. while hoisting it deduplicated
                     // with another unresolved optional peer. save it so we remember resolve it
@@ -1046,22 +1057,35 @@ impl Tree {
             // or hoist if peer version allows it
 
             if dependency.behavior.is_peer() {
+                // An optional peer follows the version it dedupes onto (its
+                // incoming binding may be carried over from an older tree), and
+                // only the saved tree decides that. Required peers keep the
+                // resolver's pick, which `bun.lock.rs` re-derives by version.
+                let dedupe = || {
+                    if METHOD == BuilderMethod::Resolvable && dependency.behavior.is_optional_peer()
+                    {
+                        HoistDependencyResult::Rebind(res_id)
+                    } else {
+                        HoistDependencyResult::Hoisted
+                    }
+                };
+
                 if dependency.version.tag == crate::dependency::VersionTag::Npm {
                     let resolution: Resolution =
                         builder.lockfile().packages.items_resolution()[res_id as usize];
                     let version = &dependency.version.npm().version;
                     if resolution.tag == crate::resolution::Tag::Npm
                         && version.satisfies(resolution.npm().version, builder.buf(), builder.buf())
                     {
-                        return Ok(HoistDependencyResult::Hoisted); // 1
+                        return Ok(dedupe()); // 1
                     }
                 }
 
                 // Root dependencies are manually chosen by the user. Allow them
                 // to hoist other peers even if they don't satisfy the version
                 if builder.lockfile().is_workspace_root_dependency(dep_id) {
                     // TODO: warning about peer dependency version mismatch
-                    return Ok(HoistDependencyResult::Hoisted); // 1
+                    return Ok(dedupe()); // 1
                 }
             }