Hold a transitive update that would re-fork a deduped package

src/install/update_transitive.rs

@@ -259,6 +259,13 @@ struct Pin {
     to: Option<Semver::Version>,
 }
 
+struct Planned {
+    v: Semver::Version,
+    /// `None` re-resolves the edge through its own dist-tag.
+    to: Option<Semver::Version>,
+    later: Box<[u8]>,
+}
+
 /// The transitive half of a bare `bun update`: every edge owned by a non-workspace package the selected workspaces reach (all of them from the root or with -r) moves to the newest release its own range allows, or to wherever its dist-tag points now.
 #[derive(Default)]
 pub struct TransitiveUpdate {
@@ -1126,6 +1133,7 @@ fn plan_edges(
     if instances.is_empty() {
         return Ok((Vec::new(), Report::default()));
     }
+    let edges_on = edges_on_instances(&manager.lockfile, &instances);
 
     let ids: Vec<PackageID> = instances.iter().map(|inst| inst.pkg_id).collect();
     let msgs_before = manager.log_mut().msgs.len();
@@ -1142,7 +1150,7 @@ fn plan_edges(
     let mut unchecked: Vec<(Box<[u8]>, Box<[u8]>)> = Vec::new();
     // Non-inline prerelease strings of planned versions live in the manifest buffer; copied into the lockfile's below.
     let mut pre_strings: Vec<(core::ops::Range<usize>, u64, Box<[u8]>)> = Vec::new();
-    for inst in &instances {
+    for (inst_i, inst) in instances.iter().enumerate() {
         if inst.held {
             continue;
         }
@@ -1166,41 +1174,63 @@ fn plan_edges(
         let manifest: &PackageManifest = manifest;
         let manifest_buf: &[u8] = &manifest.string_buf;
         let rows_before = report.rows.len();
-        for want in &inst.wants {
-            let (v, to, later) = if want.version.tag == DependencyVersionTag::Npm {
-                let range = &want.version.npm().version;
-                let Some(found) = manifest
-                    .find_best_version_with_filter(range, buf, min_age, excludes)
-                    .unwrap()
-                else {
-                    continue;
-                };
-                let v = found.version;
-                if v.order(inst.current, manifest_buf, buf) != Ordering::Greater {
-                    continue;
-                }
-                if !v.tag.pre.value.is_inline() {
-                    let end = pins.len() + want.dep_ids.len();
-                    pre_strings.push((
-                        pins.len()..end,
-                        v.tag.pre.hash,
-                        Box::from(v.tag.pre.slice(manifest_buf)),
-                    ));
-                }
-                (v, Some(v), later_than(manifest, v, min_age, excludes))
-            } else {
-                let tag = want.version.dist_tag().tag.slice(buf);
-                let Some(found) = manifest
-                    .find_by_dist_tag_with_filter(tag, min_age, excludes)
-                    .unwrap()
-                else {
-                    continue;
-                };
-                if found.version.order(inst.current, manifest_buf, buf) == Ordering::Equal {
-                    continue;
+        let planned: Vec<Option<Planned>> = inst
+            .wants
+            .iter()
+            .map(|want| {
+                if want.version.tag == DependencyVersionTag::Npm {
+                    let range = &want.version.npm().version;
+                    manifest
+                        .find_best_version_with_filter(range, buf, min_age, excludes)
+                        .unwrap()
+                        .map(|found| found.version)
+                        .filter(|&v| v.order(inst.current, manifest_buf, buf) == Ordering::Greater)
+                        .map(|v| Planned {
+                            v,
+                            to: Some(v),
+                            later: later_than(manifest, v, min_age, excludes),
+                        })
+                } else {
+                    let tag = want.version.dist_tag().tag.slice(buf);
+                    manifest
+                        .find_by_dist_tag_with_filter(tag, min_age, excludes)
+                        .unwrap()
+                        .map(|found| found.version)
+                        .filter(|&v| v.order(inst.current, manifest_buf, buf) != Ordering::Equal)
+                        .map(|v| Planned {
+                            v,
+                            to: None,
+                            later: Box::default(),
+                        })
                 }
-                (found.version, None, Box::default())
+            })
+            .collect();
+        for (w, want) in inst.wants.iter().enumerate() {
+            let Some(plan) = &planned[w] else {
+                continue;
             };
+            let (v, to) = (plan.v, plan.to);
+            if to.is_some()
+                && forks_surviving_instance(
+                    &manager.lockfile,
+                    inst,
+                    w,
+                    &planned,
+                    &edges_on[inst_i],
+                    v,
+                    manifest_buf,
+                )
+            {
+                continue;
+            }
+            if to.is_some() && !v.tag.pre.value.is_inline() {
+                let end = pins.len() + want.dep_ids.len();
+                pre_strings.push((
+                    pins.len()..end,
+                    v.tag.pre.hash,
+                    Box::from(v.tag.pre.slice(manifest_buf)),
+                ));
+            }
             pins.extend(want.dep_ids.iter().map(|&dep_id| Pin {
                 dep_id,
                 from: inst.pkg_id,
@@ -1210,7 +1240,7 @@ fn plan_edges(
                 name: Box::from(name),
                 from: text(inst.current.fmt(buf)),
                 to: text(v.fmt(manifest_buf)),
-                later,
+                later: plan.later.clone(),
             });
         }
         if report.rows.len() != rows_before {
@@ -1237,6 +1267,65 @@ fn plan_edges(
     Ok((pins, report))
 }
 
+/// For each planned instance, every edge in the lockfile still resolving to it.
+fn edges_on_instances(lockfile: &Lockfile, instances: &[Instance]) -> Vec<Vec<DependencyID>> {
+    let mut slot_of: Vec<u32> = vec![u32::MAX; lockfile.packages.len()];
+    for (i, inst) in instances.iter().enumerate() {
+        slot_of[inst.pkg_id as usize] = i as u32;
+    }
+    let mut edges_on: Vec<Vec<DependencyID>> = vec![Vec::new(); instances.len()];
+    for (j, &target) in lockfile.buffers.resolutions.iter().enumerate() {
+        let Some(&slot) = slot_of.get(target as usize) else {
+            continue;
+        };
+        if slot != u32::MAX {
+            edges_on[slot as usize].push(j as DependencyID);
+        }
+    }
+    edges_on
+}
+
+/// True when some edge on the instance cannot follow the move to `v` and stays at `current`,
+/// which still satisfies the moving range: the fork would re-add the duplicate `bun dedupe` removes.
+fn forks_surviving_instance(
+    lockfile: &Lockfile,
+    inst: &Instance,
+    want_index: usize,
+    planned: &[Option<Planned>],
+    edges: &[DependencyID],
+    v: Semver::Version,
+    manifest_buf: &[u8],
+) -> bool {
+    let buf = lockfile.buffers.string_bytes.as_slice();
+    let want = &inst.wants[want_index];
+    if want.version.tag != DependencyVersionTag::Npm
+        || !want.version.npm().version.satisfies(inst.current, buf, buf)
+    {
+        return false;
+    }
+    let deps = lockfile.buffers.dependencies.as_slice();
+    let stays = |version: &dependency::Version| {
+        version.tag != DependencyVersionTag::Npm
+            || !version.npm().version.satisfies(v, buf, manifest_buf)
+    };
+    edges.iter().any(|&edge| {
+        match inst
+            .wants
+            .iter()
+            .position(|other| other.dep_ids.contains(&edge))
+        {
+            Some(w) if w == want_index => false,
+            Some(w) => planned[w].is_none() && stays(&inst.wants[w].version),
+            None => {
+                let dep = &deps[edge as usize];
+                dep.behavior.is_bundled()
+                    || dedupe::effective_npm_range(lockfile, edge, dep)
+                        .is_none_or(|range| stays(&range))
+            }
+        }
+    })
+}
+
 /// The `latest` dist-tag when it is newer than the release `v` an in-range move stops at, like the `+` rows' `(vX available)`.
 fn later_than(
     manifest: &PackageManifest,

test/cli/install/bun-update-transitive.test.ts

@@ -1,9 +1,11 @@
 import { file, write } from "bun";
-import { afterAll, beforeAll, expect, test } from "bun:test";
+import { afterAll, beforeAll, expect, setDefaultTimeout, test } from "bun:test";
 import { exists } from "fs/promises";
 import { VerdaccioRegistry, bunEnv, bunExe, tempDir } from "harness";
 import { join } from "path";
 
+setDefaultTimeout(1000 * 60 * 5);
+
 // Registry: no-deps 1.0.0/1.0.1/1.1.0/2.0.0, a-dep 1.0.1..1.0.10, @types/no-deps 1.0.0/2.0.0, one-range-dep@1.0.0 -> no-deps ^1.0.0, one-fixed-dep@1.0.0 -> no-deps 1.0.0, dep-with-tags latest=3.0.0, pre-2=2.0.1, 3.0.1 published above latest.
 
 const registry = new VerdaccioRegistry();
@@ -507,27 +509,54 @@ test.concurrent("a transitive dependency pinned exactly by its dependent stays p
   expect(exitCode).toBe(0);
 });
 
-// The root's no-deps@1.0.0 dedupes both dependents onto 1.0.0 before it is dropped; the update forks only the `^1.0.0` edge.
-test.concurrent("dependents with different ranges are resolved independently", async () => {
+// The root's no-deps@1.0.0 dedupes both dependents onto 1.0.0 before it is dropped; the fixed edge keeps 1.0.0 alive, so the `^1.0.0` edge is held instead of forked into the duplicate `bun dedupe` would remove.
+test.concurrent("a range edge is not forked off an instance a fixed sibling keeps alive", async () => {
   const dependents = { "one-fixed-dep": "1.0.0", "one-range-dep": "1.0.0" };
   const dir = await setup({ "package.json": pkgJson({ "no-deps": "1.0.0", ...dependents }) });
   const packageJson = pkgJson(dependents);
   await reinstall(dir, packageJson);
   expect(await lockedVersions(dir, "no-deps")).toStrictEqual(["1.0.0"]);
+  const before = await lockText(dir);
   const { stdout, stderr, exitCode } = await run(dir, "update");
-  expectSummary(stdout, NO_DEPS_ROW_HINTED, "", installed(1));
+  expectSummary(stdout, noChanges(3, 4));
   expectCleanStderr(stderr);
+  expect(stderr).not.toContain("Saved lockfile");
   expect(await packageJsonOf(dir)).toStrictEqual(packageJson);
-  expect(await lockedVersions(dir, "no-deps")).toStrictEqual(["1.0.0", "1.1.0"]);
-  const { packages } = await lock(dir);
-  expect([packages["no-deps"][0], packages["one-range-dep/no-deps"][0]]).toStrictEqual([
-    "no-deps@1.0.0",
-    "no-deps@1.1.0",
-  ]);
+  expect(await lockText(dir)).toBe(before);
+  expect(await lockedVersions(dir, "no-deps")).toStrictEqual(["1.0.0"]);
   await frozen(dir);
   expect(exitCode).toBe(0);
 });
 
+// #38903: `bun update` and `bun dedupe` must reach a fixed point. After dedupe collapses the `^1.0.0`
+// edge onto the exact pin's 1.0.0, a bare update holds that edge instead of re-adding the duplicate.
+test.concurrent("a deduped lockfile is a fixed point of `bun update`", async () => {
+  const dir = await setup({ "package.json": pkgJson({ "one-range-dep": "1.0.0" }) });
+  const packageJson = pkgJson({ "one-range-dep": "1.0.0", "no-deps": "1.0.0" });
+  await reinstall(dir, packageJson);
+  expect(await lockedVersions(dir, "no-deps")).toStrictEqual(["1.0.0", "1.1.0"]);
+
+  const deduped = await run(dir, "dedupe");
+  expect(deduped.stderr).not.toContain("error:");
+  expect(deduped.exitCode).toBe(0);
+  expect(await lockedVersions(dir, "no-deps")).toStrictEqual(["1.0.0"]);
+  const before = await lockText(dir);
+
+  const { stdout, stderr, exitCode } = await run(dir, "update");
+  expectSummary(stdout, noChanges(2, 3));
+  expectCleanStderr(stderr);
+  expect(stderr).not.toContain("Saved lockfile");
+  expect(await packageJsonOf(dir)).toStrictEqual(packageJson);
+  expect(await lockText(dir)).toBe(before);
+  expect(exitCode).toBe(0);
+
+  const check = await run(dir, "dedupe", "--check");
+  expect(check.stderr).not.toContain("error:");
+  expect(check.exitCode).toBe(0);
+  expect(await lockText(dir)).toBe(before);
+  await frozen(dir);
+});
+
 // The root's exact 1.0.0 takes the root slot and pushes one-range-dep's 1.1.0 into a nested folder; widening the root keeps 1.0.0 locked, so the update collapses both rows onto 1.1.0.
 test.concurrent("hoisted: a bare update removes the nested copy whose row it collapsed", async () => {
   const dir = await setup({ "package.json": pkgJson({ "one-range-dep": "1.0.0" }) });