diff --git a/.changeset/hoisted-hard-link-keeps-node-modules.md b/.changeset/hoisted-hard-link-keeps-node-modules.md new file mode 100644 index 0000000000..ec6485f52a --- /dev/null +++ b/.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). diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b92dfb1c4f..b27765f5f7 100644 --- a/pnpm-lock.yaml +++ b/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 diff --git a/pnpm/crates/cli/tests/suite/exec_recursive.rs b/pnpm/crates/cli/tests/suite/exec_recursive.rs index a669b096f5..ea8250a93f 100644 --- a/pnpm/crates/cli/tests/suite/exec_recursive.rs +++ b/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(); diff --git a/pnpm/crates/workspace-task-scheduler/src/tests.rs b/pnpm/crates/workspace-task-scheduler/src/tests.rs index cea91c7af9..1b9e99a4bc 100644 --- a/pnpm/crates/workspace-task-scheduler/src/tests.rs +++ b/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] diff --git a/pnpm11/fs/graceful-fs/src/index.ts b/pnpm11/fs/graceful-fs/src/index.ts index fc9a9a476f..4e95c43dc6 100644 --- a/pnpm11/fs/graceful-fs/src/index.ts +++ b/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 ( 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') +} diff --git a/pnpm11/fs/hard-link-dir/package.json b/pnpm11/fs/hard-link-dir/package.json index dd19a7637a..edafe092a1 100644 --- a/pnpm11/fs/hard-link-dir/package.json +++ b/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:" diff --git a/pnpm11/fs/hard-link-dir/src/index.ts b/pnpm11/fs/hard-link-dir/src/index.ts index 1cd6917d07..14a541e2ba 100644 --- a/pnpm11/fs/hard-link-dir/src/index.ts +++ b/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 }) } /* diff --git a/pnpm11/fs/hard-link-dir/test/index.ts b/pnpm11/fs/hard-link-dir/test/index.ts index a18432e0fc..bbcc2491b9 100644 --- a/pnpm11/fs/hard-link-dir/test/index.ts +++ b/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([]) +}) diff --git a/pnpm11/fs/indexed-pkg-importer/src/importIndexedDir.ts b/pnpm11/fs/indexed-pkg-importer/src/importIndexedDir.ts index 69c6d0777a..a0d8b69838 100644 --- a/pnpm11/fs/indexed-pkg-importer/src/importIndexedDir.ts +++ b/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 { diff --git a/pnpm11/fs/indexed-pkg-importer/test/createImportPackage.test.ts b/pnpm11/fs/indexed-pkg-importer/test/createImportPackage.test.ts index b6107e2bdf..f780d046f6 100644 --- a/pnpm11/fs/indexed-pkg-importer/test/createImportPackage.test.ts +++ b/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` }))