fix(arborist): avoid full reinstall on subsequent linked strategy runs (#9031)

Author: manzoorwanijk

workspaces/arborist/lib/arborist/isolated-reifier.js

@@ -461,7 +461,7 @@ module.exports = cls => class IsolatedReifier extends cls {
 
     if (dep.package.bin) {
       for (const bn in dep.package.bin) {
-        target.binPaths.push(join(from.realpath, 'node_modules', '.bin', bn))
+        target.binPaths.push(join(dep.root.localPath, nmFolder, '.bin', bn))
       }
     }
 
@@ -486,9 +486,12 @@ module.exports = cls => class IsolatedReifier extends cls {
       parent: root,
       path: join(dep.root.localPath, nmFolder, dep.name),
       realpath: target.path,
-      resolved: dep.resolved,
+      resolved: external
+        ? `file:.store/${toKey}/node_modules/${dep.packageName}`
+        : dep.resolved,
       root,
       target,
+      version: dep.version,
       top: { path: dep.root.localPath },
     }
     const newEdge1 = { optional, from, to: link }

workspaces/arborist/lib/arborist/reify.js

@@ -8,9 +8,10 @@ const promiseAllRejectLate = require('promise-all-reject-late')
 const runScript = require('@npmcli/run-script')
 const { callLimit: promiseCallLimit } = require('promise-call-limit')
 const { depth: dfwalk } = require('treeverse')
-const { dirname, resolve, relative, join } = require('node:path')
+const { dirname, resolve, relative, join, sep } = require('node:path')
 const { log, time } = require('proc-log')
-const { lstat, mkdir, rm, symlink } = require('node:fs/promises')
+const { existsSync } = require('node:fs')
+const { lstat, mkdir, readdir, rm, symlink } = require('node:fs/promises')
 const { moveFile } = require('@npmcli/fs')
 const { subset, intersects } = require('semver')
 const { walkUp } = require('walk-up-path')
@@ -73,6 +74,7 @@ module.exports = cls => class Reifier extends cls {
   #shrinkwrapInflated = new Set()
   #sparseTreeDirs = new Set()
   #sparseTreeRoots = new Set()
+  #linkedActualForDiff = null
 
   constructor (options) {
     super(options)
@@ -114,15 +116,20 @@ module.exports = cls => class Reifier extends cls {
       // of Node/Link trees
       log.warn('reify', 'The "linked" install strategy is EXPERIMENTAL and may contain bugs.')
       this.idealTree = await this.createIsolatedTree()
+      this.#linkedActualForDiff = this.#buildLinkedActualForDiff(
+        this.idealTree, this.actualTree
+      )
     }
     await this[_diffTrees]()
     await this.#reifyPackages()
     if (linked) {
+      await this.#cleanOrphanedStoreEntries()
       // swap back in the idealTree
       // so that the lockfile is preserved
       this.idealTree = oldTree
     }
     await this[_saveIdealTree](options)
+    this.#linkedActualForDiff = null
     // clean inert
     for (const node of this.idealTree.inventory.values()) {
       if (node.inert) {
@@ -424,9 +431,14 @@ module.exports = cls => class Reifier extends cls {
           if (ideal) {
             filterNodes.push(ideal)
           }
-          const actual = this.actualTree.children.get(ws)
-          if (actual) {
-            filterNodes.push(actual)
+          // Skip actual-side filterNodes when using the linked diff wrapper.
+          // Those nodes have root===actualTree, not root===linkedActualForDiff, and Diff.calculate requires filterNode.root to match actual.
+          // The ideal filterNode alone is sufficient to scope the workspace diff.
+          if (!this.#linkedActualForDiff) {
+            const actual = this.actualTree.children.get(ws)
+            if (actual) {
+              filterNodes.push(actual)
+            }
           }
         }
       }
@@ -448,7 +460,7 @@ module.exports = cls => class Reifier extends cls {
       omit: this.#omit,
       shrinkwrapInflated: this.#shrinkwrapInflated,
       filterNodes,
-      actual: this.actualTree,
+      actual: this.#linkedActualForDiff || this.actualTree,
       ideal: this.idealTree,
     })
 
@@ -571,6 +583,7 @@ module.exports = cls => class Reifier extends cls {
       // if the directory already exists, made will be undefined. if that's the case
       // we don't want to remove it because we aren't the ones who created it so we
       // omit it from the #sparseTreeRoots
+      /* istanbul ignore next -- pre-existing: mkdir returns undefined when dir exists, covered in reify tests but lost in aggregate coverage merge */
       if (made) {
         this.#sparseTreeRoots.add(made)
       }
@@ -787,6 +800,100 @@ module.exports = cls => class Reifier extends cls {
     return join(filePath)
   }
 
+  // Build a flat actual tree wrapper for linked installs so the diff can correctly match store entries that already exist on disk.
+  // The proxy tree from createIsolatedTree() is flat (all children on root), but loadActual() produces a nested tree where store entries are deep link targets.
+  // This wrapper surfaces them at the root level for comparison.
+  #buildLinkedActualForDiff (idealTree, actualTree) {
+    // Combined Map keyed by path (how allChildren() in diff.js keys)
+    const combined = new Map()
+
+    // Add actual tree's children (the top-level symlinks)
+    for (const child of actualTree.children.values()) {
+      combined.set(child.path, child)
+    }
+
+    // Add synthetic entries for store nodes and store links that exist on disk.
+    // The proxy tree is flat: all store entries (isInStore) and store links (isStoreLink) are direct children of root.
+    // The actual tree only has top-level links as root children, so store entries need synthetic actual entries for the diff to match them.
+    for (const child of idealTree.children.values()) {
+      if (!combined.has(child.path) && (child.isInStore || child.isStoreLink) &&
+          existsSync(child.path)) {
+        const entry = {
+          global: false,
+          globalTop: false,
+          isProjectRoot: false,
+          isTop: false,
+          location: child.location,
+          name: child.name,
+          optional: child.optional,
+          top: child.top,
+          children: [],
+          edgesIn: new Set(),
+          edgesOut: new Map(),
+          binPaths: [],
+          fsChildren: [],
+          /* istanbul ignore next -- emulate Node */
+          getBundler () {
+            return null
+          },
+          hasShrinkwrap: false,
+          inDepBundle: false,
+          integrity: null,
+          isLink: Boolean(child.isLink),
+          isRoot: false,
+          isInStore: Boolean(child.isInStore),
+          path: child.path,
+          realpath: child.realpath,
+          resolved: child.resolved,
+          version: child.version,
+          package: child.package,
+        }
+        entry.target = entry
+        if (child.isLink && combined.has(child.realpath)) {
+          entry.target = combined.get(child.realpath)
+        }
+        combined.set(child.path, entry)
+      }
+    }
+
+    // Proxy .get(name) to original actual tree for filterNodes compatibility
+    // (scoped workspace installs use .get(name), allChildren uses .values())
+    const origGet = actualTree.children.get.bind(actualTree.children)
+    const combinedGet = combined.get.bind(combined)
+    /* istanbul ignore next -- only reached during scoped workspace installs */
+    combined.get = (key) => combinedGet(key) || origGet(key)
+
+    const wrapper = {
+      isRoot: true,
+      isLink: actualTree.isLink,
+      target: actualTree.target,
+      fsChildren: actualTree.fsChildren,
+      path: actualTree.path,
+      realpath: actualTree.realpath,
+      edgesOut: actualTree.edgesOut,
+      inventory: actualTree.inventory,
+      package: actualTree.package,
+      resolved: actualTree.resolved,
+      version: actualTree.version,
+      integrity: actualTree.integrity,
+      binPaths: actualTree.binPaths,
+      hasShrinkwrap: false,
+      inDepBundle: false,
+      parent: null,
+      children: combined,
+    }
+
+    // Set parent/root on synthetic entries for consistency
+    for (const child of combined.values()) {
+      if (!child.parent) {
+        child.parent = wrapper
+        child.root = wrapper
+      }
+    }
+
+    return wrapper
+  }
+
   #registryResolved (resolved) {
     // the default registry url is a magic value meaning "the currently
     // configured registry".
@@ -1245,6 +1352,41 @@ module.exports = cls => class Reifier extends cls {
     timeEnd()
   }
 
+  // After a linked install, scan node_modules/.store/ and remove any directories that are not referenced by the current ideal tree.
+  // Store entries become orphaned when dependencies are updated or removed, because the diff never sees the old store keys.
+  async #cleanOrphanedStoreEntries () {
+    const storeDir = resolve(this.path, 'node_modules', '.store')
+    let entries
+    try {
+      entries = await readdir(storeDir)
+    } catch {
+      return
+    }
+
+    // Collect valid store keys from the isolated ideal tree (location: node_modules/.store/{key}/node_modules/{pkg})
+    const validKeys = new Set()
+    for (const child of this.idealTree.children.values()) {
+      if (child.isInStore) {
+        const key = child.location.split(sep)[2]
+        validKeys.add(key)
+      }
+    }
+
+    const orphaned = entries.filter(e => !validKeys.has(e))
+    if (!orphaned.length) {
+      return
+    }
+
+    log.silly('reify', 'cleaning orphaned store entries', orphaned)
+    await promiseAllRejectLate(
+      orphaned.map(e =>
+        rm(resolve(storeDir, e), { recursive: true, force: true })
+          .catch(/* istanbul ignore next -- rm with force rarely fails */
+            er => log.warn('cleanup', `Failed to remove orphaned store entry ${e}`, er))
+      )
+    )
+  }
+
   // last but not least, we save the ideal tree metadata to the package-lock
   // or shrinkwrap file, and any additions or removals to package.json
   async [_saveIdealTree] (options) {