Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions src/install/PackageManager/PackageManagerEnqueue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2134,14 +2134,23 @@ 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() {
// Record locally-pinned appends so `Lockfile::get_package_id`'s
// order-independence guard leaves them alone: dependencies declared in a
// local package.json (root or workspace) are enqueued before any
// network-ordered transitive, and an exact `=X.Y.Z` resolves to one
// version regardless of order. Everything else the guard may treat as a
// network-order artefact.
Comment thread
robobun marked this conversation as resolved.
Outdated
let is_local_pin = {
// 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);
// `is_workspace_dependency` reads `lockfile.packages` only (disjoint
// from `package`, which was returned by-value above).
Comment thread
robobun marked this conversation as resolved.
Outdated
unsafe { &*(*this_ptr).lockfile }.is_workspace_dependency(dependency_id)
|| (version.tag == dependency::version::Tag::Npm && version.npm().version.is_exact())
};
if is_local_pin {
// SAFETY: `this_ptr` is the sole live `&mut PackageManager` here;
// `mark_local_pin` touches `lockfile.local_pinned` only.
unsafe { &mut *(*this_ptr).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
Expand Down
50 changes: 27 additions & 23 deletions src/install/lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,14 +203,15 @@ 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,
/// `bit[id] == true` ⇔ package `id` was appended for a dependency that is
/// either (a) declared in a local package.json (root or any workspace), or
/// (b) an exact `=X.Y.Z` anywhere in the tree. `get_package_id`'s
/// order-independence guard never blocks deduping to one of these: a local
/// dependency is enqueued before any network-ordered transitive, and an
/// exact pin resolves to exactly one version regardless of manifest
/// arrival order, so deduping to either is deterministic. Runtime-only —
/// never serialised; sized lazily in `mark_local_pin`.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(crate) local_pinned: DynamicBitSet,
}

pub(crate) type PackageList = self::package::List<u64>;
Expand Down Expand Up @@ -1971,7 +1972,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(),
}
}

Expand All @@ -1983,15 +1984,15 @@ 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.
/// Record that package `id` was appended for a dependency whose resolution
/// order is deterministic (see the `local_pinned` field doc).
Comment thread
robobun marked this conversation as resolved.
Outdated
#[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(
Expand Down Expand Up @@ -2030,7 +2031,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 {
Expand All @@ -2047,18 +2048,21 @@ impl Lockfile {
// *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 entry was NOT appended for a dependency declared in a
// local package.json (root or workspace) nor for an exact
// `=X.Y.Z` — either is processed in a deterministic order
// independent of manifest arrival, so deduping onto it is
// stable and npm-compatible (`dragon test 2` /
// "dependency from root satisfies range from dependency" /
// "transitive wide range dedupes onto root range"),
Comment thread
robobun marked this conversation as resolved.
Outdated
// - 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)
// *transitive range-resolved* lower major depending on whose
// manifest landed first ("text lockfile is hoisted").
Comment thread
robobun marked this conversation as resolved.
Outdated
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
Expand Down
103 changes: 103 additions & 0 deletions test/cli/install/bun-install-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4568,6 +4568,109 @@
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());
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// 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).toContain("packages installed");

Check warning on line 4665 in test/cli/install/bun-install-registry.test.ts

View check run for this annotation

Claude / Claude Code Review

Vacuous stdout assertion in workspace-variant dedupe test

This assertion is vacuous — any successful `bun install` prints "N packages installed", so it passes on both the fixed build (3 packages) and the unfixed build (4 packages, nested duplicate). Tighten to `expect(out).toContain("3 packages installed")` (or match the exact line array like the root-variant test just above) so this line actually asserts the dedupe.
Comment thread
robobun marked this conversation as resolved.
Outdated
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", () => {
Expand Down