fix(fs.hard-link-dir): fill the destination in place (#14242)

Copying a built package to its other hoisted locations staged a fresh copy
of the source in a temp directory and renamed it over the destination. The
destination directory that went away took its own node_modules with it —
under the hoisted node linker that is where the dependencies that could not
be hoisted any higher live, so those were silently deleted from every
location the copy landed on.

The copies of one build chunk run concurrently, so a sibling package could
also be staging its own copy inside that node_modules at the time. Its
staging directory was carried off with the replaced directory and its
commit then failed with ERR_PNPM_ENOENT, aborting the install:

  ENOENT: no such file or directory,
    rename '<dest>/node_modules/_tmp_<pid>_<hash>' -> '<dest>/node_modules/<pkg>'

Link the source into the destination entry by entry instead. A file the
destination already has is replaced through a temp sibling and a rename, so
a reader sees either the whole old file or the whole new one; a destination
entry whose kind does not match the source's is removed first, which also
keeps a symlinked directory from redirecting the writes out of the
destination; and a file that is already the source file is skipped. Nothing
removes the destination directory any more, so its node_modules and everyone
working inside it are left alone.

The rename retry that leaves the destination in place was private to
`@pnpm/fs.indexed-pkg-importer`, which needed it for the same reason. It now
lives in `@pnpm/fs.graceful-fs` and both call sites use it.

Closes pnpm/pnpm#12880

.changeset/hoisted-hard-link-keeps-node-modules.md

@@ -0,0 +1,8 @@
+---
+"@pnpm/fs.graceful-fs": minor
+"@pnpm/fs.indexed-pkg-importer": patch
+"@pnpm/fs.hard-link-dir": patch
+"pnpm": patch
+---
+
+Copying a built package to its other hoisted locations no longer replaces the destination directory. With `nodeLinker: hoisted`, that replacement deleted the dependencies nested inside the destination's `node_modules`, and made concurrent copies of the same build chunk fail with `ERR_PNPM_ENOENT: no such file or directory, rename '.../node_modules/_tmp_...'` [#12880](https://github.com/pnpm/pnpm/issues/12880).

pnpm-lock.yaml

@@ -4831,9 +4831,6 @@ importers:
       path-temp:
         specifier: 'catalog:'
         version: 3.0.0
-      rename-overwrite:
-        specifier: 'catalog:'
-        version: 7.0.1
     devDependencies:
       '@jest/globals':
         specifier: 'catalog:'
@@ -21375,6 +21372,7 @@ snapshots:
       es-object-atoms: 1.1.2
       es-set-tostringtag: 2.1.0
       es-to-primitive: 1.3.4
+      for-each: 0.3.5
       function.prototype.name: 1.2.0
       get-intrinsic: 1.3.0
       get-proto: 1.0.1

pnpm/crates/cli/tests/suite/exec_recursive.rs

@@ -332,7 +332,10 @@ fn recursive_exec_bail_summary_records_every_completed_batch_result() {
             "-r",
             "exec",
             "-c",
-            r#"if [ "$(basename "$PWD")" = fails ]; then exit 1; fi"#,
+            // `passes` has to reach the summary, and bail stops dispatching the
+            // moment `fails` returns — so `fails` waits for the marker that
+            // proves `passes` was handed to a worker before it fails.
+            r#"if [ "$(basename "$PWD")" = fails ]; then i=0; while [ ! -f ../passes/ran.txt ] && [ $i -lt 1000 ]; do i=$((i + 1)); sleep 0.01; done; exit 1; fi; touch ran.txt"#,
         ])
         .assert()
         .failure();

pnpm/crates/workspace-task-scheduler/src/tests.rs

@@ -496,7 +496,16 @@ fn graph_scheduler_does_not_wait_for_an_unrelated_slow_branch() {
         },
     )
     .unwrap();
-    assert_eq!(ran.into_inner().unwrap(), vec!["slow", "fast", "dependent"]);
+    // `slow` blocks until `dependent` starts, so a scheduler that waited for
+    // the unrelated branch would never return here. Which of the two branches
+    // records itself first is up to the workers.
+    let ran = ran.into_inner().unwrap();
+    let mut started = ran.clone();
+    started.sort_unstable();
+    assert_eq!(started, vec!["dependent", "fast", "slow"]);
+    let fast = ran.iter().position(|node| *node == "fast").unwrap();
+    let dependent = ran.iter().position(|node| *node == "dependent").unwrap();
+    assert!(fast < dependent, "dependent starts after the dependency it waits on: {ran:?}");
 }
 
 #[tokio::test]

pnpm11/fs/graceful-fs/src/index.ts

@@ -1,7 +1,12 @@
+import fs from 'node:fs'
 import util, { promisify } from 'node:util'
 
 import gfs from 'graceful-fs'
 
+const RENAME_RETRY_BUDGET_MS = 60_000
+const RENAME_RETRY_BACKOFF_CAP_MS = 100
+const renameRetrySleepBuffer = new Int32Array(new SharedArrayBuffer(4))
+
 export default { // eslint-disable-line
   chmod: promisify(gfs.chmod),
   copyFile: promisify(gfs.copyFile),
@@ -46,3 +51,34 @@ function withEagainRetry<T extends unknown[], R> (
     throw new Error('Unreachable')
   }
 }
+
+/**
+ * Renames `src` over `dest`, waiting out a Windows sharing violation — an
+ * EPERM, EACCES or EBUSY from whoever else holds the file open — for up to a
+ * minute before rethrowing it. Every other error is thrown right away.
+ *
+ * `dest` is never removed to make room for the rename: a concurrent install may
+ * still be reading that dirent, and a reader has to see either the whole file
+ * that was there or the whole file replacing it.
+ */
+export function renameFileWithRetry (src: string, dest: string): void {
+  const startedAt = Date.now()
+  let backoffMs = 0
+  for (;;) {
+    try {
+      fs.renameSync(src, dest)
+      return
+    } catch (err) {
+      if (!isTransientRenameError(err) || Date.now() - startedAt >= RENAME_RETRY_BUDGET_MS) throw err
+      if (backoffMs > 0) Atomics.wait(renameRetrySleepBuffer, 0, 0, backoffMs)
+      backoffMs = Math.min(backoffMs + 10, RENAME_RETRY_BACKOFF_CAP_MS)
+    }
+  }
+}
+
+function isTransientRenameError (err: unknown): boolean {
+  return process.platform === 'win32' &&
+    util.types.isNativeError(err) &&
+    'code' in err &&
+    (err.code === 'EPERM' || err.code === 'EACCES' || err.code === 'EBUSY')
+}

pnpm11/fs/hard-link-dir/package.json

@@ -34,8 +34,7 @@
   },
   "dependencies": {
     "@pnpm/fs.graceful-fs": "workspace:*",
-    "path-temp": "catalog:",
-    "rename-overwrite": "catalog:"
+    "path-temp": "catalog:"
   },
   "peerDependencies": {
     "@pnpm/logger": "catalog:"

pnpm11/fs/hard-link-dir/src/index.ts

@@ -3,47 +3,53 @@ import fs from 'node:fs'
 import path from 'node:path'
 import util from 'node:util'
 
-import gfs from '@pnpm/fs.graceful-fs'
+import gfs, { renameFileWithRetry } from '@pnpm/fs.graceful-fs'
 import { globalWarn } from '@pnpm/logger'
-import { pathTemp } from 'path-temp'
-import { renameOverwriteSync } from 'rename-overwrite'
+import { fastPathTemp } from 'path-temp'
 
+/**
+ * Hard links the contents of `src` into every directory of `destDirs`, leaving
+ * out `node_modules`. A destination that is `src` itself is skipped; the rest
+ * are created if they are missing, and a missing `src` leaves them empty and
+ * warns.
+ *
+ * A destination is filled in place, entry by entry. Its own `node_modules` —
+ * under the hoisted node linker, the dependencies that could not be hoisted any
+ * higher — stays where it is, and so does anyone working inside it: the copies
+ * of one build chunk run concurrently, so a sibling package may be staging its
+ * own copy in there while this one is written. Entries the destination has that
+ * `src` does not are left alone for the same reason.
+ *
+ * A destination entry of the kind `src` does not have is removed to make room,
+ * a file already there is replaced through a rename, and a filesystem error
+ * that neither of those resolves is thrown.
+ */
 export function hardLinkDir (src: string, destDirs: string[]): void {
-  if (destDirs.length === 0) return
-  const filteredDestDirs: string[] = []
-  const tempDestDirs: string[] = []
-  for (const destDir of destDirs) {
-    if (path.relative(destDir, src) === '') {
-      // Don't try to hard link the source directory to itself
-      continue
-    }
-    filteredDestDirs.push(destDir)
-    tempDestDirs.push(pathTemp(path.dirname(destDir)))
-  }
-  _hardLinkDir(src, tempDestDirs, true)
-  for (let i = 0; i < filteredDestDirs.length; i++) {
-    renameOverwriteSync(tempDestDirs[i], filteredDestDirs[i])
+  const targetDirs = destDirs.filter((destDir) => path.relative(destDir, src) !== '')
+  if (targetDirs.length === 0) return
+  for (const targetDir of targetDirs) {
+    gfs.mkdirSync(targetDir, { recursive: true })
   }
+  _hardLinkDir(src, targetDirs, true)
 }
 
-function _hardLinkDir (src: string, destDirs: string[], isRoot?: boolean) {
+function _hardLinkDir (src: string, destDirs: string[], isRoot?: boolean): void {
   let files: string[] = []
   try {
     files = fs.readdirSync(src)
   } catch (err: unknown) {
     if (!isRoot || !((util.types.isNativeError(err) && 'code' in err && err.code === 'ENOENT'))) throw err
     globalWarn(`Source directory not found when creating hardLinks for: ${src}. Creating destinations as empty: ${destDirs.join(', ')}`)
-    for (const dir of destDirs) {
-      gfs.mkdirSync(dir, { recursive: true })
-    }
     return
   }
   for (const file of files) {
     if (file === 'node_modules') continue
     const srcFile = path.join(src, file)
-    if (fs.lstatSync(srcFile).isDirectory()) {
+    const srcStats = fs.lstatSync(srcFile, { bigint: true })
+    if (srcStats.isDirectory()) {
       const destSubdirs = destDirs.map((destDir) => {
         const destSubdir = path.join(destDir, file)
+        clearMismatchedDirent(destSubdir, true)
         try {
           gfs.mkdirSync(destSubdir, { recursive: true })
         } catch (err: unknown) {
@@ -57,7 +63,7 @@ function _hardLinkDir (src: string, destDirs: string[], isRoot?: boolean) {
     for (const destDir of destDirs) {
       const destFile = path.join(destDir, file)
       try {
-        linkOrCopyFile(srcFile, destFile)
+        linkOrCopyFile(srcFile, destFile, srcStats)
       } catch (err: unknown) {
         if (util.types.isNativeError(err) && 'code' in err && err.code === 'ENOENT') {
           // Ignore broken symlinks
@@ -69,9 +75,10 @@ function _hardLinkDir (src: string, destDirs: string[], isRoot?: boolean) {
   }
 }
 
-function linkOrCopyFile (srcFile: string, destFile: string): void {
+function linkOrCopyFile (srcFile: string, destFile: string, srcStats: fs.BigIntStats): void {
   try {
     linkOrCopy(srcFile, destFile)
+    return
   } catch (err: unknown) {
     assert(util.types.isNativeError(err))
     if ('code' in err && err.code === 'ENOENT') {
@@ -83,6 +90,67 @@ function linkOrCopyFile (srcFile: string, destFile: string): void {
       throw err
     }
   }
+  // Most of what a destination holds is the very file the source holds: both
+  // were linked from the same store entry, and a build touches only a few of
+  // them. Replacing those again would cost a link and a rename apiece.
+  if (isSameFile(destFile, srcStats)) return
+  replaceFile(srcFile, destFile)
+}
+
+// Read as bigints: a 64-bit inode does not survive a JavaScript number, so two
+// unrelated files can round-trip to the same one.
+function isSameFile (destFile: string, srcStats: fs.BigIntStats): boolean {
+  let destStats
+  try {
+    destStats = fs.lstatSync(destFile, { bigint: true })
+  } catch (err: unknown) {
+    // The caller got EEXIST for this path, so only a concurrent removal
+    // explains it being gone; anything else is a real failure to report.
+    if (util.types.isNativeError(err) && 'code' in err && err.code === 'ENOENT') return false
+    throw err
+  }
+  // Filesystems that report neither an inode nor a device (some on Windows)
+  // make every file look like every other one.
+  return destStats.ino !== 0n &&
+    destStats.dev !== 0n &&
+    destStats.ino === srcStats.ino &&
+    destStats.dev === srcStats.dev
+}
+
+// Swap the new file in through a temp sibling, so that whoever reads the
+// destination sees either the whole old file or the whole new one.
+function replaceFile (srcFile: string, destFile: string): void {
+  const tempFile = fastPathTemp(destFile)
+  // A temp file is named after the thread that writes it, so only an
+  // interrupted run can leave one behind for this one to trip over.
+  fs.rmSync(tempFile, { recursive: true, force: true })
+  try {
+    linkOrCopy(srcFile, tempFile)
+    clearMismatchedDirent(destFile, false)
+    renameFileWithRetry(tempFile, destFile)
+  } catch (err: unknown) {
+    try {
+      fs.unlinkSync(tempFile)
+    } catch {} // eslint-disable-line:no-empty
+    throw err
+  }
+}
+
+// A directory cannot be created where a file or a symlink is, and a rename
+// cannot put a file where a directory is. The destination holds an older copy
+// of the same package, so only a build that turned one into the other leaves it
+// with a dirent of the wrong kind. Removing it also keeps a symlinked directory
+// from redirecting the writes below out of the destination.
+function clearMismatchedDirent (destPath: string, wantDirectory: boolean): void {
+  let stats
+  try {
+    stats = fs.lstatSync(destPath)
+  } catch (err: unknown) {
+    if (util.types.isNativeError(err) && 'code' in err && err.code === 'ENOENT') return
+    throw err
+  }
+  if (stats.isDirectory() === wantDirectory) return
+  fs.rmSync(destPath, { recursive: true, force: true })
 }
 
 /*

pnpm11/fs/hard-link-dir/test/index.ts

@@ -46,3 +46,92 @@ test("don't fail on missing source and dest directories", () => {
   expect(fs.existsSync(missingDirSrc)).toBe(false)
   expect(fs.existsSync(missingDirDest)).toBe(true)
 })
+
+// A hoisted destination owns a node_modules that the source does not have: the
+// dependencies that could not be hoisted any higher. The copies of one build
+// chunk run concurrently, so a sibling package may be staging its own copy in
+// there while this one is written, and its rename needs to still find it.
+// See https://github.com/pnpm/pnpm/issues/12880
+test("the destination's own node_modules survives", () => {
+  const tempDir = createTempDir()
+  const srcDir = path.join(tempDir, 'source')
+  const destDir = path.join(tempDir, 'dest')
+
+  fs.mkdirSync(path.join(srcDir, 'subdir'), { recursive: true })
+  fs.writeFileSync(path.join(srcDir, 'file.txt'), 'built')
+  fs.writeFileSync(path.join(srcDir, 'subdir/file.txt'), 'built')
+
+  const nestedDep = path.join(destDir, 'node_modules/dep')
+  const stagedSiblingCopy = path.join(destDir, 'node_modules/_tmp_1_2')
+  fs.mkdirSync(nestedDep, { recursive: true })
+  fs.mkdirSync(stagedSiblingCopy, { recursive: true })
+  fs.writeFileSync(path.join(nestedDep, 'index.js'), 'nested dependency')
+  fs.mkdirSync(path.join(destDir, 'subdir'), { recursive: true })
+  fs.writeFileSync(path.join(destDir, 'file.txt'), 'not built yet')
+  fs.writeFileSync(path.join(destDir, 'subdir/file.txt'), 'not built yet')
+
+  hardLinkDir(srcDir, [destDir])
+
+  expect(fs.readFileSync(path.join(destDir, 'file.txt'), 'utf8')).toBe('built')
+  expect(fs.readFileSync(path.join(destDir, 'subdir/file.txt'), 'utf8')).toBe('built')
+  expect(fs.readFileSync(path.join(nestedDep, 'index.js'), 'utf8')).toBe('nested dependency')
+  expect(fs.existsSync(stagedSiblingCopy)).toBe(true)
+})
+
+test('a file that the destination already has is replaced', () => {
+  const tempDir = createTempDir()
+  const srcDir = path.join(tempDir, 'source')
+  const destDir = path.join(tempDir, 'dest')
+
+  fs.mkdirSync(srcDir, { recursive: true })
+  fs.mkdirSync(destDir, { recursive: true })
+  fs.writeFileSync(path.join(srcDir, 'file.txt'), 'built')
+  fs.writeFileSync(path.join(destDir, 'file.txt'), 'not built yet')
+
+  hardLinkDir(srcDir, [destDir])
+
+  expect(fs.readdirSync(destDir)).toStrictEqual(['file.txt'])
+  expect(fs.readFileSync(path.join(destDir, 'file.txt'), 'utf8')).toBe('built')
+})
+
+// A build can turn one of the package's own entries into the other kind. The
+// destination still holds the pre-build copy, so the entry it has to make way
+// for is of the wrong kind.
+test('an entry that changed between a file and a directory is replaced', () => {
+  const tempDir = createTempDir()
+  const srcDir = path.join(tempDir, 'source')
+  const destDir = path.join(tempDir, 'dest')
+
+  fs.mkdirSync(path.join(srcDir, 'generated'), { recursive: true })
+  fs.writeFileSync(path.join(srcDir, 'generated/index.js'), 'built')
+  fs.writeFileSync(path.join(srcDir, 'placeholder.js'), 'built')
+
+  fs.mkdirSync(path.join(destDir, 'placeholder.js'), { recursive: true })
+  fs.writeFileSync(path.join(destDir, 'placeholder.js/leftover.js'), 'not built yet')
+  fs.writeFileSync(path.join(destDir, 'generated'), 'not built yet')
+
+  hardLinkDir(srcDir, [destDir])
+
+  expect(fs.readFileSync(path.join(destDir, 'generated/index.js'), 'utf8')).toBe('built')
+  expect(fs.readFileSync(path.join(destDir, 'placeholder.js'), 'utf8')).toBe('built')
+})
+
+test('a symlinked directory in the destination does not redirect the writes', () => {
+  const tempDir = createTempDir()
+  const srcDir = path.join(tempDir, 'source')
+  const destDir = path.join(tempDir, 'dest')
+  const outside = path.join(tempDir, 'outside')
+
+  fs.mkdirSync(path.join(srcDir, 'lib'), { recursive: true })
+  fs.writeFileSync(path.join(srcDir, 'lib/index.js'), 'built')
+
+  fs.mkdirSync(outside, { recursive: true })
+  fs.mkdirSync(destDir, { recursive: true })
+  fs.symlinkSync(outside, path.join(destDir, 'lib'), 'junction')
+
+  hardLinkDir(srcDir, [destDir])
+
+  expect(fs.readFileSync(path.join(destDir, 'lib/index.js'), 'utf8')).toBe('built')
+  expect(fs.lstatSync(path.join(destDir, 'lib')).isSymbolicLink()).toBe(false)
+  expect(fs.readdirSync(outside)).toStrictEqual([])
+})

pnpm11/fs/indexed-pkg-importer/src/importIndexedDir.ts

@@ -2,7 +2,7 @@ import fs from 'node:fs'
 import path from 'node:path'
 import util from 'node:util'
 
-import gfs from '@pnpm/fs.graceful-fs'
+import gfs, { renameFileWithRetry } from '@pnpm/fs.graceful-fs'
 import { globalInfo, globalWarn, logger } from '@pnpm/logger'
 import type { ResolvedFrom } from '@pnpm/store.controller-types'
 import { rimrafSync } from '@zkochan/rimraf'
@@ -13,10 +13,7 @@ import { renameOverwriteSync } from 'rename-overwrite'
 import sanitizeFilename from 'sanitize-filename'
 
 const filenameConflictsLogger = logger('_filename-conflicts')
-const RENAME_RETRY_BUDGET_MS = 60_000
-const RENAME_RETRY_BACKOFF_CAP_MS = 100
 const FILE_COMPARE_BUFFER_SIZE = 64 * 1024
-const renameRetrySleepBuffer = new Int32Array(new SharedArrayBuffer(4))
 
 export type ImportFile = (src: string, dest: string) => void
 
@@ -193,30 +190,6 @@ function replaceFileIfDifferent (importFile: ImportFile, src: string, dest: stri
   }
 }
 
-// Retry a Windows sharing violation without rename-overwrite's fallback of
-// deleting the destination. Another install may still be reading that dirent.
-function renameFileWithRetry (src: string, dest: string): void {
-  const startedAt = Date.now()
-  let backoffMs = 0
-  for (;;) {
-    try {
-      fs.renameSync(src, dest)
-      return
-    } catch (err) {
-      if (!isTransientRenameError(err) || Date.now() - startedAt >= RENAME_RETRY_BUDGET_MS) throw err
-      if (backoffMs > 0) Atomics.wait(renameRetrySleepBuffer, 0, 0, backoffMs)
-      backoffMs = Math.min(backoffMs + 10, RENAME_RETRY_BACKOFF_CAP_MS)
-    }
-  }
-}
-
-function isTransientRenameError (err: unknown): boolean {
-  return process.platform === 'win32' &&
-    util.types.isNativeError(err) &&
-    'code' in err &&
-    (err.code === 'EPERM' || err.code === 'EACCES' || err.code === 'EBUSY')
-}
-
 // A rename cannot put a file where a directory is (EISDIR), so one standing in
 // the way has to go first. Only a damaged tree has one.
 function clearDirBlockingFile (dest: string): void {

pnpm11/fs/indexed-pkg-importer/test/createImportPackage.test.ts

@@ -21,6 +21,7 @@ jest.unstable_mockModule('@pnpm/fs.graceful-fs', () => {
     __esModule: true,
     default: fsMock,
     ...fsMock,
+    renameFileWithRetry: fsMock.renameSync,
   }
 })
 jest.unstable_mockModule('path-temp', () => ({ fastPathTemp: (file: string) => `${file}_tmp` }))