install: resolve a range onto an existing version only when every install has it

src/install/PackageManager/PackageManagerEnqueue.rs

@@ -2078,23 +2078,13 @@
 
     // Was this package already allocated? Let's reuse the existing one.
     //
-    // Determinism: passing `version` here unconditionally lets a
-    // peer like `>= 1.0.2` collapse onto whichever sibling-appended entry
-    // (e.g. `1.0.9`) happens to be highest in the index *at this instant* — a
-    // network-order artefact that the `^1.0.2` peer-hoisting test already
-    // todoIf's on macOS. The floor guard in `get_package_id` was
-    // meant to close that, but its exact-pinned/same-major exemptions reopen
-    // it when *every* candidate is an exact-pinned same-major sibling
-    // (`uses-a-dep-1..10`). For deferred peers, suppress the satisfies-
-    // fallback so only an exact `eql(find_result)` can bind here; everything
-    // else falls through to the `is_peer && !install_peer` defer below and is
-    // resolved deterministically by phase 2's descending-index scan in
-    // `get_or_put_resolved_package`. `*` is left alone — it expresses no
-    // version preference, and the "peer *" hoisting test depends on it
-    // deduping to whatever sibling pin exists rather than the manifest floor.
+    // A peer not being installed yet binds now only to its exact best match
+    // (`*` also to a version every install has) and is otherwise deferred to
+    // the peer pass in `get_or_put_resolved_package`.
     let suppress_peer_satisfies = behavior.is_peer()
         && !install_peer
         && !(version.tag == dependency::version::Tag::Npm && version.npm().version.is_star());
+    let exact = version.tag == dependency::version::Tag::Npm && version.npm().version.is_exact();
     if let Some(id) = this.lockfile.get_package_id(
         name_hash,
         if should_update || suppress_peer_satisfies {
@@ -2107,6 +2097,11 @@
             url: find_result.package.tarball_url.value,
         })),
     ) {
+        // A peer binds here or in the peer pass depending on what has arrived,
+        // so only regular rows, which all bind in the first pass, count.
+        if exact && !behavior.is_peer() {
+            this.lockfile.mark_pinned_by_reuse(id);
+        }
         success_fn(this, dependency_id, id);
         return Ok(Some(ResolvedPackageResult {
             package: *this.lockfile.packages.get(id as usize),
@@ -2134,15 +2129,9 @@
     )?)?;
 
     debug_assert!(package.meta.id != invalid_package_id);
-    // Record exact-version pins so `Lockfile::get_package_id`'s
-    // order-independence guard can tell them apart from range-resolved
-    // entries (which it treats as network-order artefacts).
-    if version.tag == dependency::version::Tag::Npm && version.npm().version.is_exact() {
-        // SAFETY: `this_ptr` is the sole live `&mut PackageManager` here;
-        // `lockfile.exact_pinned` is disjoint from `package` (returned
-        // by-value above).
-        unsafe { &mut *(*this_ptr).lockfile }.mark_exact_pin(package.meta.id);
-    }
+    // SAFETY: `this_ptr` is the sole live `&mut PackageManager` here; `package`
+    // was returned by value above.
+    unsafe { &mut *(*this_ptr).lockfile }.mark_appended_for(package.meta.id, dependency_id, exact);
     // Use scopeguard so success_fn runs on every
     // return below (including the `?` paths). The guard owns the raw pointer so the
     // `this` reborrow below doesn't conflict with the closure capture.

src/install/PackageManager/install_with_manager.rs

@@ -2095,6 +2095,7 @@ fn wait_for_resolution(manager: &mut PackageManager) -> crate::Result<()> {
     if manager.pending_task_count() > 0 {
         wait_for_everything_except_peers(manager)?;
     }
+    manager.lockfile.mark_settled_packages();
 
     // Resolving a peer dep can create a NEW package whose own peer deps
     // get re-queued to `peer_dependencies` during `drainDependencyList`.

src/install/lockfile.rs

@@ -190,27 +190,31 @@ pub struct Lockfile {
 
     pub(crate) saved_config_version: Option<ConfigVersion>,
 
-    /// `packages.len()` at the moment lockfile load (including npm/pnpm/yarn
-    /// migration) finished. Packages with `id < this` were carried in from a
-    /// lockfile and represent a user-pinned resolution; packages with
-    /// `id >= this` were appended by manifest fetches in the current
-    /// resolve session. `get_package_id` uses this to keep its
-    /// order-independence guard from overriding lockfile pins. Set by
-    /// `mark_loaded_packages`; defaults to `invalid_package_id` (no lockfile
-    /// loaded → guard applies to nothing, equivalent to "all entries are
-    /// session-appended").
-    ///
-    /// Runtime-only — never serialised.
+    /// `packages.len()` once the lockfile (or a migrated one) was loaded:
+    /// packages below it came from the lockfile, the rest were appended in
+    /// this run (`mark_loaded_packages`). Runtime-only, never serialised.
     pub(crate) loaded_package_count: PackageID,
 
-    /// `bit[id] == true` ⇔ package `id` was appended for a dependency whose
-    /// version range was an exact `=X.Y.Z` (i.e. the user — root or workspace
-    /// — pinned this exact version somewhere in the tree). `get_package_id`'s
-    /// order-independence guard never blocks deduping to one of these: an
-    /// exact pin is a deliberate choice, not an artifact of which manifest
-    /// happened to land first. Runtime-only — never serialised; sized lazily
-    /// in `mark_exact_pin`.
-    pub(crate) exact_pinned: DynamicBitSet,
+    /// Packages below this were loaded or appended before resolution last
+    /// drained (`mark_settled_packages`), so they exist on every install; see
+    /// `is_reusable`. Runtime-only, never serialised.
+    pub(crate) settled_package_count: PackageID,
+
+    /// Indexed by `PackageID`, filled by `mark_appended_for` for the packages
+    /// appended from a manifest in this run. Runtime-only, never serialised.
+    appended_for_by_id: Vec<AppendedFor>,
+}
+
+/// The rows a package appended in this run was resolved for; see
+/// `Lockfile::get_package_id`.
+#[derive(Clone, Copy, Debug, Default)]
+pub(crate) struct AppendedFor {
+    /// The row it was appended for is declared by the root or a workspace.
+    pub direct: bool,
+    /// A row's range (after overrides and catalogs) was this exact version:
+    /// the appending row's, or a later one's while the package was not yet
+    /// reusable (`mark_pinned_by_reuse`).
+    pub pinned: bool,
 }
 
 pub(crate) type PackageList = self::package::List<u64>;
@@ -1967,11 +1971,9 @@ impl Lockfile {
             meta_hash: ZERO_HASH,
             patched_dependencies: PatchedDependenciesMap::default(),
             saved_config_version: None,
-            // Fresh lockfile (no load): every package appended later is
-            // session-appended, so the order-independence guard in
-            // `get_package_id` applies from id 0.
             loaded_package_count: 0,
-            exact_pinned: DynamicBitSet::default(),
+            settled_package_count: 0,
+            appended_for_by_id: Vec::new(),
         }
     }
 
@@ -1981,124 +1983,115 @@ impl Lockfile {
     #[inline]
     pub(crate) fn mark_loaded_packages(&mut self) {
         self.loaded_package_count = self.packages.len() as PackageID;
+        self.settled_package_count = self.loaded_package_count;
     }
 
-    /// Record that package `id` was appended via an exact-version dependency
-    /// (`=X.Y.Z`). See the `exact_pinned` field doc.
+    /// Call only while nothing is being resolved (no row enqueued, no manifest
+    /// in flight); see `get_package_id`.
     #[inline]
-    pub(crate) fn mark_exact_pin(&mut self, id: PackageID) {
+    pub(crate) fn mark_settled_packages(&mut self) {
+        self.settled_package_count = self.packages.len() as PackageID;
+    }
+
+    pub(crate) fn mark_appended_for(
+        &mut self,
+        id: PackageID,
+        dependency_id: DependencyID,
+        pinned: bool,
+    ) {
+        let direct = self.is_workspace_dependency(dependency_id);
+        *self.appended_for_mut(id) = AppendedFor { direct, pinned };
+    }
+
+    /// A regular row whose range is exactly `id`'s version resolved to it.
+    /// Ignored once the package is reusable: from then on rows read
+    /// `pinned`, and which rows have resolved to it by any given moment
+    /// depends on registry timing.
+    pub(crate) fn mark_pinned_by_reuse(&mut self, id: PackageID) {
+        if !self.is_reusable(id) {
+            self.appended_for_mut(id).pinned = true;
+        }
+    }
+
+    fn appended_for_mut(&mut self, id: PackageID) -> &mut AppendedFor {
         let i = id as usize;
-        if self.exact_pinned.bit_length() <= i {
-            bun_core::handle_oom(self.exact_pinned.resize(i + 1, false));
+        if self.appended_for_by_id.len() <= i {
+            self.appended_for_by_id
+                .resize(i + 1, AppendedFor::default());
         }
-        self.exact_pinned.set(i);
+        &mut self.appended_for_by_id[i]
+    }
+
+    fn appended_for(&self, id: PackageID) -> AppendedFor {
+        self.appended_for_by_id
+            .get(id as usize)
+            .copied()
+            .unwrap_or_default()
     }
 
+    /// Whether package `id` exists on every install of this package.json, so a
+    /// range it merely satisfies may resolve to it: it was loaded or settled,
+    /// or appended for a direct row. Direct rows are enqueued before any
+    /// manifest is processed and a manifest's waiting rows resolve FIFO, so
+    /// for one name they resolve before every transitive row; which packages
+    /// transitive rows have appended so far depends on registry timing.
+    fn is_reusable(&self, id: PackageID) -> bool {
+        id < self.settled_package_count || self.appended_for(id).direct
+    }
+
+    /// The package `resolution` is already stored as, if any.
+    ///
+    /// For an npm range `version`, `resolution` is its best match from the
+    /// manifest, and the highest reusable version the range accepts is
+    /// preferred over it (`is_reusable`; a lockfile version always, an
+    /// appended one when a row pinned it or it is of the best match's major).
+    /// Anything else only answers for a range whose best match it is.
     pub(crate) fn get_package_id(
         &self,
         name_hash: u64,
-        // If non-null, attempt to use an existing package
-        // that satisfies this version range.
         version: Option<&DependencyVersion>,
         resolution: &Resolution,
     ) -> Option<PackageID> {
-        let entry = self.package_index.get(&name_hash)?;
-        let resolutions: &[Resolution] = self.packages.items_resolution();
-        // Borrow the `npm` arm's `Semver::Group` (not `Copy` — owns a linked
-        // list head). `version` is held by-value for the whole fn body so the
-        // borrow is sound.
-        let npm_version = match version {
-            Some(v) if v.tag == dependency::Tag::Npm => Some(&v.npm().version),
-            _ => None,
-        };
-        // Order-independence guard for the `satisfies` fallback below: when the
-        // caller already knows the manifest's best-match version (the npm
-        // `resolution` it passes), only dedupe to an existing entry whose
-        // version is at least that. Without this, the result depends on which
-        // sibling's manifest happened to land first — `*` deduping to a
-        // previously-appended `1.0.2` instead of resolving to `2.0.2` is the
-        // long-standing "text lockfile is hoisted" flake. Lockfile-pinned deps
-        // are kept out of this codepath by `Diff::generate`'s
-        // satisfies-preserves-mapping rule (which keeps the resolution slot
-        // populated so the early return in `get_or_put_resolved_package` fires
-        // before we get here).
-        let resolved_npm_floor = if resolution.tag == ResolutionTag::Npm {
-            Some(resolution.npm().version)
-        } else {
-            None
+        // Highest version first (`get_or_put_id`).
+        let ids: &[PackageID] = match self.package_index.get(&name_hash)? {
+            PackageIndexEntry::Id(id) => core::slice::from_ref(id),
+            PackageIndexEntry::Ids(ids) => ids.as_slice(),
         };
+        let resolutions: &[Resolution] = self.packages.items_resolution();
+        debug_assert!(ids.iter().all(|&id| (id as usize) < resolutions.len()));
         let buf = self.buffers.string_bytes.as_slice();
 
-        let loaded_watermark = self.loaded_package_count;
-        let exact_pinned = &self.exact_pinned;
-        let try_satisfies_dedupe = |id: PackageID| -> bool {
-            let existing = &resolutions[id as usize];
-            if existing.tag != ResolutionTag::Npm {
-                return false;
-            }
-            let Some(npm_v) = npm_version else {
-                return false;
-            };
-            let existing_ver = existing.npm().version;
-            if !npm_v.satisfies(existing_ver, buf, buf) {
-                return false;
-            }
-            // Order-independence guard. We refuse to dedupe a wide range to a
-            // *lower* existing entry only when ALL of the following hold:
-            //   - the entry was appended in this resolve session
-            //     (lockfile-loaded entries are the user's existing pin),
-            //   - the entry was NOT appended for an exact-`=X.Y.Z` dependency
-            //     (an exact pin anywhere in the tree is a deliberate choice,
-            //     not a network-order artefact — `dragon test 2` /
-            //     "dependency from root satisfies range from dependency"),
-            //   - the manifest's best-match is a *different major* (within a
-            //     major, deduping to an older patch is the long-standing
-            //     behaviour and the worst case is still ^-compatible).
-            // What this leaves is exactly the cross-parent network-order
-            // flake: a wide range (`*`, `>=X`) collapsing onto a sibling's
-            // *range-resolved* lower major depending on whose manifest landed
-            // first ("text lockfile is hoisted").
-            if id >= loaded_watermark && !exact_pinned.is_set_allow_out_of_bound(id as usize, false)
-            {
-                if let Some(floor) = resolved_npm_floor {
-                    if existing_ver.order(floor, buf, buf) == Ordering::Less
-                        && existing_ver.major != floor.major
-                    {
-                        return false;
-                    }
-                }
-            }
-            true
-        };
-
-        match entry {
-            PackageIndexEntry::Id(id) => {
-                debug_assert!((*id as usize) < resolutions.len());
-
-                if resolutions[*id as usize].eql(resolution, buf, buf) {
-                    return Some(*id);
+        if let Some(range) = version
+            .filter(|v| v.tag == dependency::Tag::Npm)
+            .map(|v| &v.npm().version)
+        {
+            let best_match =
+                (resolution.tag == ResolutionTag::Npm).then(|| resolution.npm().version);
+            let reusable = ids.iter().copied().find(|&id| {
+                let existing = &resolutions[id as usize];
+                if existing.tag != ResolutionTag::Npm {
+                    return false;
                 }
-
-                if try_satisfies_dedupe(*id) {
-                    return Some(*id);
+                let existing_version = existing.npm().version;
+                if !range.satisfies(existing_version, buf, buf) {
+                    return false;
                 }
-            }
-            PackageIndexEntry::Ids(ids) => {
-                for &id in ids.iter() {
-                    debug_assert!((id as usize) < resolutions.len());
-
-                    if resolutions[id as usize].eql(resolution, buf, buf) {
-                        return Some(id);
-                    }
-
-                    if try_satisfies_dedupe(id) {
-                        return Some(id);
-                    }
+                if id < self.loaded_package_count {
+                    return true;
                 }
+                self.is_reusable(id)
+                    && (self.appended_for(id).pinned
+                        || best_match
+                            .is_none_or(|best_match| existing_version.major == best_match.major))
+            });
+            if reusable.is_some() {
+                return reusable;
             }
         }
 
-        None
+        ids.iter()
+            .copied()
+            .find(|&id| resolutions[id as usize].eql(resolution, buf, buf))
     }
 
     /// Appends `pkg` to `this.packages`, and adds to `this.package_index`.