diff --git a/src/install/PackageManager/PackageManagerEnqueue.rs b/src/install/PackageManager/PackageManagerEnqueue.rs index 5c120f26c777..83a682bc0f70 100644 --- a/src/install/PackageManager/PackageManagerEnqueue.rs +++ b/src/install/PackageManager/PackageManagerEnqueue.rs @@ -2134,14 +2134,13 @@ fn get_or_put_resolved_package_with_find_result( )?)?; 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 and + // `package` was returned by value above, so nothing aliases the lockfile. + let lockfile = unsafe { &mut *(*this_ptr).lockfile }; + if (version.tag == dependency::version::Tag::Npm && version.npm().version.is_exact()) + || lockfile.is_workspace_dependency(dependency_id) + { + lockfile.mark_local_pin(package.meta.id); } // Use scopeguard so success_fn runs on every // return below (including the `?` paths). The guard owns the raw pointer so the diff --git a/src/install/lockfile.rs b/src/install/lockfile.rs index 73d55df73e17..bac5b70cfdfe 100644 --- a/src/install/lockfile.rs +++ b/src/install/lockfile.rs @@ -203,14 +203,11 @@ pub struct Lockfile { /// 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 appended for a root/workspace package.json dependency or an + /// exact `=X.Y.Z` dependency; exempt from the guard in `get_package_id`. + /// + /// Runtime-only — never serialised. + pub(crate) local_pinned: DynamicBitSet, } pub(crate) type PackageList = self::package::List; @@ -1971,7 +1968,7 @@ impl Lockfile { // session-appended, so the order-independence guard in // `get_package_id` applies from id 0. loaded_package_count: 0, - exact_pinned: DynamicBitSet::default(), + local_pinned: DynamicBitSet::default(), } } @@ -1983,15 +1980,14 @@ impl Lockfile { self.loaded_package_count = self.packages.len() as PackageID; } - /// Record that package `id` was appended via an exact-version dependency - /// (`=X.Y.Z`). See the `exact_pinned` field doc. + /// See `local_pinned`. #[inline] - pub(crate) fn mark_exact_pin(&mut self, id: PackageID) { + pub(crate) fn mark_local_pin(&mut self, id: PackageID) { 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.local_pinned.bit_length() <= i { + bun_core::handle_oom(self.local_pinned.resize(i + 1, false)); } - self.exact_pinned.set(i); + self.local_pinned.set(i); } pub(crate) fn get_package_id( @@ -2030,7 +2026,7 @@ impl Lockfile { let buf = self.buffers.string_bytes.as_slice(); let loaded_watermark = self.loaded_package_count; - let exact_pinned = &self.exact_pinned; + let local_pinned = &self.local_pinned; let try_satisfies_dedupe = |id: PackageID| -> bool { let existing = &resolutions[id as usize]; if existing.tag != ResolutionTag::Npm { @@ -2043,22 +2039,10 @@ impl Lockfile { 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) + // A wide range must not collapse onto a lower major that a sibling's + // manifest merely happened to append first. Lockfile-loaded and + // `local_pinned` entries do not depend on manifest order. + if id >= loaded_watermark && !local_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 diff --git a/test/cli/install/bun-install-registry.test.ts b/test/cli/install/bun-install-registry.test.ts index cdc8fbc50790..d7258b94cad4 100644 --- a/test/cli/install/bun-install-registry.test.ts +++ b/test/cli/install/bun-install-registry.test.ts @@ -4568,6 +4568,113 @@ describe("hoisting", async () => { lockfile, ); }); + + test("transitive wide range dedupes onto root range across majors", async () => { + // Root declares `hoist-lockfile-shared: ^1.0.1` (resolves to 1.0.2). + // `hoist-lockfile-1` depends on `hoist-lockfile-shared: *` (best-match 2.0.2). + // The `*` satisfies 1.0.2, so it must dedupe onto the root's copy instead of + // nesting a second install at 2.0.2 (npm also dedupes here). + await write( + packageJson, + JSON.stringify({ + name: "foo", + dependencies: { + "hoist-lockfile-1": "1.0.0", + "hoist-lockfile-shared": "^1.0.1", + }, + }), + ); + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + stderr: "pipe", + stdout: "pipe", + env, + }); + + const [out, err, exitCode] = await Promise.all([stdout.text(), stderr.text(), exited]); + expect(err).toContain("Saved lockfile"); + expect(err).not.toContain("error:"); + expect(out.replace(/\s*\[[0-9\.]+m?s\]\s*$/, "").split(/\r?\n/)).toEqual([ + expect.stringContaining("bun install v1."), + "", + "+ hoist-lockfile-1@1.0.0", + expect.stringContaining("+ hoist-lockfile-shared@1.0.2"), + "", + "2 packages installed", + ]); + expect(await file(join(packageDir, "node_modules", "hoist-lockfile-shared", "package.json")).json()).toMatchObject({ + name: "hoist-lockfile-shared", + version: "1.0.2", + }); + expect(await exists(join(packageDir, "node_modules", "hoist-lockfile-1", "node_modules"))).toBeFalse(); + expect(exitCode).toBe(0); + assertManifestsPopulated(join(packageDir, ".bun-cache"), registryUrl()); + + // Second install from the saved lockfile must not rewrite it. + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + const second = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + stderr: "pipe", + stdout: "pipe", + env, + }); + const [out2, err2, exitCode2] = await Promise.all([second.stdout.text(), second.stderr.text(), second.exited]); + expect(err2).not.toContain("Saved lockfile"); + expect(err2).not.toContain("error:"); + expect(out2).toContain("2 packages installed"); + expect(await exists(join(packageDir, "node_modules", "hoist-lockfile-1", "node_modules"))).toBeFalse(); + expect(exitCode2).toBe(0); + }); + + test("transitive wide range dedupes onto workspace range across majors", async () => { + // Same as above but the narrow range lives in a workspace package.json. + await write( + packageJson, + JSON.stringify({ + name: "foo", + workspaces: ["pkg-a"], + }), + ); + await mkdir(join(packageDir, "pkg-a")); + await write( + join(packageDir, "pkg-a", "package.json"), + JSON.stringify({ + name: "pkg-a", + version: "1.0.0", + dependencies: { + "hoist-lockfile-1": "1.0.0", + "hoist-lockfile-shared": "^1.0.1", + }, + }), + ); + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + stderr: "pipe", + stdout: "pipe", + env, + }); + + const [out, err, exitCode] = await Promise.all([stdout.text(), stderr.text(), exited]); + expect(err).toContain("Saved lockfile"); + expect(err).not.toContain("error:"); + expect(out.replace(/\s*\[[0-9\.]+m?s\]\s*$/, "").split(/\r?\n/)).toEqual([ + expect.stringContaining("bun install v1."), + "", + "3 packages installed", + ]); + expect(await file(join(packageDir, "node_modules", "hoist-lockfile-shared", "package.json")).json()).toMatchObject({ + name: "hoist-lockfile-shared", + version: "1.0.2", + }); + expect(await exists(join(packageDir, "node_modules", "hoist-lockfile-1", "node_modules"))).toBeFalse(); + expect(exitCode).toBe(0); + assertManifestsPopulated(join(packageDir, ".bun-cache"), registryUrl()); + }); }); describe("transitive file dependencies", () => {