From e6c92917fcb106e79ece3c77a423dcc6ecbca430 Mon Sep 17 00:00:00 2001 From: David Sanders Date: Sun, 12 Apr 2026 15:34:46 -0700 Subject: [PATCH 1/7] refactor: move copyDirectory to own file for testability Signed-off-by: David Sanders --- lib/copy-directory.js | 43 +++++++++++++++++++++++++++++++++++++++++++ lib/install.js | 36 +----------------------------------- 2 files changed, 44 insertions(+), 35 deletions(-) create mode 100644 lib/copy-directory.js diff --git a/lib/copy-directory.js b/lib/copy-directory.js new file mode 100644 index 0000000000..203231799a --- /dev/null +++ b/lib/copy-directory.js @@ -0,0 +1,43 @@ +'use strict' + +const { promises: fs } = require('graceful-fs') +const crypto = require('crypto') +const path = require('path') + +const { backOff } = require('exponential-backoff') + +async function copyDirectory (src, dest, ensure = false) { + try { + await fs.stat(src) + } catch { + throw new Error(`Missing source directory for copy: ${src}`) + } + await fs.mkdir(dest, { recursive: true }) + const entries = await fs.readdir(src, { withFileTypes: true }) + for (const entry of entries) { + if (entry.isDirectory()) { + await copyDirectory(path.join(src, entry.name), path.join(dest, entry.name)) + } else if (entry.isFile()) { + // with parallel installs, copying files may cause file errors on + // Windows so use an exponential backoff to resolve collisions + await backOff(async () => { + try { + await fs.copyFile(path.join(src, entry.name), path.join(dest, entry.name)) + } catch (err) { + // if ensure, check if file already exists and that's good enough + if (ensure && err.code === 'EBUSY') { + try { + await fs.stat(path.join(dest, entry.name)) + return + } catch {} + } + throw err + } + }) + } else { + throw new Error('Unexpected file directory entry type') + } + } +} + +module.exports = copyDirectory diff --git a/lib/install.js b/lib/install.js index 3580cdd003..9bedbb3876 100644 --- a/lib/install.js +++ b/lib/install.js @@ -2,13 +2,13 @@ const { createWriteStream, promises: fs } = require('graceful-fs') const os = require('os') -const { backOff } = require('exponential-backoff') const tar = require('tar') const path = require('path') const { Transform, promises: { pipeline } } = require('stream') const crypto = require('crypto') const log = require('./log') const semver = require('semver') +const copyDirectory = require('./copy-directory') const { download } = require('./download') const processRelease = require('./process-release') @@ -119,40 +119,6 @@ async function install (gyp, argv) { } } - async function copyDirectory (src, dest) { - try { - await fs.stat(src) - } catch { - throw new Error(`Missing source directory for copy: ${src}`) - } - await fs.mkdir(dest, { recursive: true }) - const entries = await fs.readdir(src, { withFileTypes: true }) - for (const entry of entries) { - if (entry.isDirectory()) { - await copyDirectory(path.join(src, entry.name), path.join(dest, entry.name)) - } else if (entry.isFile()) { - // with parallel installs, copying files may cause file errors on - // Windows so use an exponential backoff to resolve collisions - await backOff(async () => { - try { - await fs.copyFile(path.join(src, entry.name), path.join(dest, entry.name)) - } catch (err) { - // if ensure, check if file already exists and that's good enough - if (gyp.opts.ensure && err.code === 'EBUSY') { - try { - await fs.stat(path.join(dest, entry.name)) - return - } catch {} - } - throw err - } - }) - } else { - throw new Error('Unexpected file directory entry type') - } - } - } - async function go () { log.verbose('ensuring devDir is created', devDir) From 3109bbc6134e29fb22ff4c8bac94069e77349269 Mon Sep 17 00:00:00 2001 From: David Sanders Date: Sun, 12 Apr 2026 15:35:30 -0700 Subject: [PATCH 2/7] test: add test to ensure copyDirectory copies files atomically Assisted-by: Claude Opus 4.6 Signed-off-by: David Sanders --- test/test-copy-directory.js | 80 +++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 test/test-copy-directory.js diff --git a/test/test-copy-directory.js b/test/test-copy-directory.js new file mode 100644 index 0000000000..f6349153a0 --- /dev/null +++ b/test/test-copy-directory.js @@ -0,0 +1,80 @@ +'use strict' + +const { describe, it, afterEach } = require('mocha') +const assert = require('assert') +const path = require('path') +const fs = require('fs') +const { promises: fsp } = fs +const os = require('os') +const { FULL_TEST, platformTimeout } = require('./common') +const copyDirectory = require('../lib/copy-directory') + +describe('copyDirectory', function () { + let timer + let tmpDir + + afterEach(async () => { + if (tmpDir) { + await fsp.rm(tmpDir, { recursive: true, force: true }) + tmpDir = null + } + clearInterval(timer) + }) + + it('large file appears atomically (no partial writes visible)', async function () { + if (!FULL_TEST) { + return this.skip('Skipping due to test environment configuration') + } + + this.timeout(platformTimeout(5, { win32: 10 })) + + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'node-gyp-copy-test-')) + const srcDir = path.join(tmpDir, 'src') + const destDir = path.join(tmpDir, 'dest') + await fsp.mkdir(srcDir) + + const fileName = 'large.bin' + const srcFile = path.join(srcDir, fileName) + const destFile = path.join(destDir, fileName) + + // Create a 5 GB sparse file — instant to create, consumes no real + // disk, but fs.copyFile still has to process the full extent map so + // the destination file is visible at size 0 and grows over time. + // fs.rename() is atomic at the VFS level: the file either does not + // exist at the destination or appears at its full size in one step. + const fileSize = 5 * 1024 * 1024 * 1024 + const handle = await fsp.open(srcFile, 'w') + await handle.truncate(fileSize) + await handle.close() + + // Tight synchronous poll: stat the destination on every event-loop + // turn while copyDirectory runs concurrently. + let polls = 0 + const violations = [] + + timer = setInterval(() => { + try { + const stat = fs.statSync(destFile) + polls++ + if (stat.size !== fileSize) { + violations.push({ poll: polls, size: stat.size }) + } + } catch (err) { + if (err.code !== 'ENOENT') throw err + } + }, 0) + + await copyDirectory(srcDir, destDir) + + clearInterval(timer) + timer = undefined + + console.log(` ${polls} stats observed the file during the operation`) + + assert.strictEqual(violations.length, 0, 'file must never be observed at a partial size') + + const finalStat = await fsp.stat(destFile) + assert.strictEqual(finalStat.size, fileSize, + 'destination file should have the correct final size') + }) +}) From 36366eb8d3c6b4323daf3b8cb00f801c4b884de1 Mon Sep 17 00:00:00 2001 From: David Sanders Date: Sun, 12 Apr 2026 15:56:19 -0700 Subject: [PATCH 3/7] fix: ensure copyDirectory atomically copies files Assisted-by: Claude Opus 4.6 Signed-off-by: David Sanders --- lib/copy-directory.js | 55 +++++++++++++++++++++++++++---------------- package.json | 1 - 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/lib/copy-directory.js b/lib/copy-directory.js index 203231799a..ab25300bed 100644 --- a/lib/copy-directory.js +++ b/lib/copy-directory.js @@ -4,9 +4,9 @@ const { promises: fs } = require('graceful-fs') const crypto = require('crypto') const path = require('path') -const { backOff } = require('exponential-backoff') +const RACE_ERRORS = ['ENOTEMPTY', 'EEXIST', 'EBUSY', 'EPERM'] -async function copyDirectory (src, dest, ensure = false) { +async function copyDirectory (src, dest) { try { await fs.stat(src) } catch { @@ -15,27 +15,42 @@ async function copyDirectory (src, dest, ensure = false) { await fs.mkdir(dest, { recursive: true }) const entries = await fs.readdir(src, { withFileTypes: true }) for (const entry of entries) { - if (entry.isDirectory()) { - await copyDirectory(path.join(src, entry.name), path.join(dest, entry.name)) - } else if (entry.isFile()) { - // with parallel installs, copying files may cause file errors on - // Windows so use an exponential backoff to resolve collisions - await backOff(async () => { + if (!entry.isDirectory() && !entry.isFile()) { + throw new Error('Unexpected file directory entry type') + } + + // With parallel installs, multiple processes race to place the same + // entry. Use fs.rename for an atomic move so no process ever sees a + // partially written file. For cross-filesystem (EXDEV), copy to a + // temp path in the dest directory first, then rename within the + // same filesystem to keep it atomic. + // + // When another process wins the race, rename may fail with one of + // these codes — all mean the destination was already placed and + // are safe to ignore since every process extracts identical content. + const srcPath = path.join(src, entry.name) + const destPath = path.join(dest, entry.name) + try { + await fs.rename(srcPath, destPath) + } catch (err) { + if (RACE_ERRORS.includes(err.code)) { + // Another parallel process already placed this entry — ignore + } else if (err.code === 'EXDEV') { + // Cross-filesystem: copy to a uniquely named temp path in the + // dest directory, then rename into place atomically + const tmpPath = `${destPath}.tmp.${crypto.randomBytes(6).toString('hex')}` try { - await fs.copyFile(path.join(src, entry.name), path.join(dest, entry.name)) - } catch (err) { - // if ensure, check if file already exists and that's good enough - if (ensure && err.code === 'EBUSY') { - try { - await fs.stat(path.join(dest, entry.name)) - return - } catch {} + await fs.cp(srcPath, tmpPath, { recursive: true }) + await fs.rename(tmpPath, destPath) + } catch (e) { + await fs.rm(tmpPath, { recursive: true, force: true }).catch(() => {}) + if (!RACE_ERRORS.includes(e.code)) { + throw e } - throw err } - }) - } else { - throw new Error('Unexpected file directory entry type') + } else { + throw err + } } } } diff --git a/package.json b/package.json index 29d95ad41b..c7aa917424 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,6 @@ "main": "./lib/node-gyp.js", "dependencies": { "env-paths": "^2.2.0", - "exponential-backoff": "^3.1.1", "graceful-fs": "^4.2.6", "nopt": "^9.0.0", "proc-log": "^6.0.0", From aba84f140d55bbbc3203ff5fa3786fb30dde9d92 Mon Sep 17 00:00:00 2001 From: David Sanders Date: Sat, 11 Apr 2026 22:12:36 -0700 Subject: [PATCH 4/7] fix: don't fail if Python3 symlink already exists Signed-off-by: David Sanders --- lib/build.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/build.js b/lib/build.js index 00a0abe691..5070124930 100644 --- a/lib/build.js +++ b/lib/build.js @@ -204,7 +204,11 @@ async function build (gyp, argv) { } catch (err) { if (err.code !== 'ENOENT') throw err } - await fs.symlink(python, symlinkDestination) + try { + await fs.symlink(python, symlinkDestination) + } catch (err) { + if (err.code !== 'EEXIST') throw err + } log.verbose('bin symlinks', `created symlink to "${python}" in "${buildBinsDir}" and added to PATH`) } From d7a1e4af8702a8f762e1e534f8fbb6529b419ae6 Mon Sep 17 00:00:00 2001 From: David Sanders Date: Sun, 12 Apr 2026 15:59:54 -0700 Subject: [PATCH 5/7] test: add a test for parallel rebuilds Assisted-by: Claude Opus 4.6 Signed-off-by: David Sanders --- test/test-addon.js | 62 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/test/test-addon.js b/test/test-addon.js index 1f4d95ed7e..d5868a099f 100644 --- a/test/test-addon.js +++ b/test/test-addon.js @@ -1,13 +1,14 @@ 'use strict' -const { describe, it } = require('mocha') +const { describe, it, beforeEach, afterEach } = require('mocha') const assert = require('assert') const path = require('path') const fs = require('graceful-fs') +const { rm, mkdtemp } = require('fs/promises') const os = require('os') const cp = require('child_process') const util = require('../lib/util') -const { platformTimeout } = require('./common') +const { FULL_TEST, platformTimeout } = require('./common') const addonPath = path.resolve(__dirname, 'node_modules', 'hello_world') const nodeGyp = path.resolve(__dirname, '..', 'bin', 'node-gyp.js') @@ -129,4 +130,61 @@ describe('addon', function () { assert.strictEqual(runHello(notNodePath), 'world') fs.unlinkSync(notNodePath) }) + + describe('parallel', function () { + let devDir + let addonCopiesDir + + beforeEach(async () => { + devDir = await mkdtemp(path.join(os.tmpdir(), 'node-gyp-test-')) + addonCopiesDir = await mkdtemp(path.join(os.tmpdir(), 'node-gyp-test-addons-')) + }) + + afterEach(async () => { + await Promise.all([ + rm(devDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 1000 }), + rm(addonCopiesDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 1000 }) + ]) + devDir = null + addonCopiesDir = null + }) + + const runIt = (name, fn) => { + if (!FULL_TEST) { + return it.skip('Skipping parallel rebuild test due to test environment configuration') + } + + if (process.platform === 'darwin' && process.arch === 'x64') { + return it.skip('Skipping parallel rebuild test on x64 macOS') + } + + return it(name, async function () { + this.timeout(platformTimeout(4, { win32: 20 })) + await fn.call(this) + }) + } + + runIt('parallel rebuild', async function () { + // Install dependencies (nan) so copies in temp directories can resolve them + const [npmErr] = await util.execFile('npm', ['install', '--ignore-scripts'], { cwd: addonPath, shell: process.platform === 'win32' }) + assert.strictEqual(npmErr, null) + + const copies = await Promise.all(new Array(5).fill(0).map(async (_, i) => { + const copyDir = path.join(addonCopiesDir, `hello_world_${i}`) + await fs.promises.cp(addonPath, copyDir, { recursive: true }) + return copyDir + })) + await Promise.all(copies.map(async (copyDir, i) => { + const cmd = [nodeGyp, 'rebuild', '-C', copyDir, '--loglevel=verbose', `--devdir=${devDir}`] + const title = `${' '.repeat(8)}parallel rebuild ${(i + 1).toString().padEnd(2, ' ')}` + console.log(`${title} : Start`) + console.time(title) + const [err, logLines] = await execFile(cmd) + console.timeEnd(title) + const lastLine = logLines[logLines.length - 1] + assert.strictEqual(err, null) + assert.strictEqual(lastLine, 'gyp info ok', 'should end in ok') + })) + }) + }) }) From 950ae32bf00dcbea91340315228a881f228f557a Mon Sep 17 00:00:00 2001 From: David Sanders Date: Fri, 24 Apr 2026 20:34:18 -0700 Subject: [PATCH 6/7] chore: rename copyDirectory to moveDirectory Reflects the change in functionality Assisted-by: Claude Opus 4.6 --- lib/copy-directory.js | 4 +- lib/install.js | 4 +- lib/move-directory.js | 58 +++++++++++++++++++++++++++ test/test-copy-directory.js | 8 ++-- test/test-move-directory.js | 80 +++++++++++++++++++++++++++++++++++++ 5 files changed, 146 insertions(+), 8 deletions(-) create mode 100644 lib/move-directory.js create mode 100644 test/test-move-directory.js diff --git a/lib/copy-directory.js b/lib/copy-directory.js index ab25300bed..2e4437b69c 100644 --- a/lib/copy-directory.js +++ b/lib/copy-directory.js @@ -6,7 +6,7 @@ const path = require('path') const RACE_ERRORS = ['ENOTEMPTY', 'EEXIST', 'EBUSY', 'EPERM'] -async function copyDirectory (src, dest) { +async function moveDirectory (src, dest) { try { await fs.stat(src) } catch { @@ -55,4 +55,4 @@ async function copyDirectory (src, dest) { } } -module.exports = copyDirectory +module.exports = moveDirectory diff --git a/lib/install.js b/lib/install.js index 9bedbb3876..747ae0e3bf 100644 --- a/lib/install.js +++ b/lib/install.js @@ -8,7 +8,7 @@ const { Transform, promises: { pipeline } } = require('stream') const crypto = require('crypto') const log = require('./log') const semver = require('semver') -const copyDirectory = require('./copy-directory') +const moveDirectory = require('./move-directory') const { download } = require('./download') const processRelease = require('./process-release') @@ -243,7 +243,7 @@ async function install (gyp, argv) { } // copy over the files from the temp tarball extract directory to devDir - await copyDirectory(tarExtractDir, devDir) + await moveDirectory(tarExtractDir, devDir) } finally { try { // try to cleanup temp dir diff --git a/lib/move-directory.js b/lib/move-directory.js new file mode 100644 index 0000000000..9c470dbb7d --- /dev/null +++ b/lib/move-directory.js @@ -0,0 +1,58 @@ +'use strict' + +const { promises: fs } = require('graceful-fs') +const crypto = require('crypto') +const path = require('path') + +const RACE_ERRORS = ['ENOTEMPTY', 'EEXIST', 'EBUSY', 'EPERM'] + +async function moveDirectory (src, dest) { + try { + await fs.stat(src) + } catch { + throw new Error(`Missing source directory for move: ${src}`) + } + await fs.mkdir(dest, { recursive: true }) + const entries = await fs.readdir(src, { withFileTypes: true }) + for (const entry of entries) { + if (!entry.isDirectory() && !entry.isFile()) { + throw new Error('Unexpected file directory entry type') + } + + // With parallel installs, multiple processes race to place the same + // entry. Use fs.rename for an atomic move so no process ever sees a + // partially written file. For cross-filesystem (EXDEV), copy to a + // temp path in the dest directory first, then rename within the + // same filesystem to keep it atomic. + // + // When another process wins the race, rename may fail with one of + // these codes — all mean the destination was already placed and + // are safe to ignore since every process extracts identical content. + const srcPath = path.join(src, entry.name) + const destPath = path.join(dest, entry.name) + try { + await fs.rename(srcPath, destPath) + } catch (err) { + if (RACE_ERRORS.includes(err.code)) { + // Another parallel process already placed this entry — ignore + } else if (err.code === 'EXDEV') { + // Cross-filesystem: copy to a uniquely named temp path in the + // dest directory, then rename into place atomically + const tmpPath = `${destPath}.tmp.${crypto.randomBytes(6).toString('hex')}` + try { + await fs.cp(srcPath, tmpPath, { recursive: true }) + await fs.rename(tmpPath, destPath) + } catch (e) { + await fs.rm(tmpPath, { recursive: true, force: true }).catch(() => {}) + if (!RACE_ERRORS.includes(e.code)) { + throw e + } + } + } else { + throw err + } + } + } +} + +module.exports = moveDirectory diff --git a/test/test-copy-directory.js b/test/test-copy-directory.js index f6349153a0..33b394cbb0 100644 --- a/test/test-copy-directory.js +++ b/test/test-copy-directory.js @@ -7,9 +7,9 @@ const fs = require('fs') const { promises: fsp } = fs const os = require('os') const { FULL_TEST, platformTimeout } = require('./common') -const copyDirectory = require('../lib/copy-directory') +const moveDirectory = require('../lib/move-directory') -describe('copyDirectory', function () { +describe('moveDirectory', function () { let timer let tmpDir @@ -48,7 +48,7 @@ describe('copyDirectory', function () { await handle.close() // Tight synchronous poll: stat the destination on every event-loop - // turn while copyDirectory runs concurrently. + // turn while moveDirectory runs concurrently. let polls = 0 const violations = [] @@ -64,7 +64,7 @@ describe('copyDirectory', function () { } }, 0) - await copyDirectory(srcDir, destDir) + await moveDirectory(srcDir, destDir) clearInterval(timer) timer = undefined diff --git a/test/test-move-directory.js b/test/test-move-directory.js new file mode 100644 index 0000000000..5df369d0ff --- /dev/null +++ b/test/test-move-directory.js @@ -0,0 +1,80 @@ +'use strict' + +const { describe, it, afterEach } = require('mocha') +const assert = require('assert') +const path = require('path') +const fs = require('fs') +const { promises: fsp } = fs +const os = require('os') +const { FULL_TEST, platformTimeout } = require('./common') +const moveDirectory = require('../lib/move-directory') + +describe('moveDirectory', function () { + let timer + let tmpDir + + afterEach(async () => { + if (tmpDir) { + await fsp.rm(tmpDir, { recursive: true, force: true }) + tmpDir = null + } + clearInterval(timer) + }) + + it('large file appears atomically (no partial writes visible)', async function () { + if (!FULL_TEST) { + return this.skip('Skipping due to test environment configuration') + } + + this.timeout(platformTimeout(5, { win32: 10 })) + + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'node-gyp-move-test-')) + const srcDir = path.join(tmpDir, 'src') + const destDir = path.join(tmpDir, 'dest') + await fsp.mkdir(srcDir) + + const fileName = 'large.bin' + const srcFile = path.join(srcDir, fileName) + const destFile = path.join(destDir, fileName) + + // Create a 5 GB sparse file — instant to create, consumes no real + // disk, but fs.copyFile still has to process the full extent map so + // the destination file is visible at size 0 and grows over time. + // fs.rename() is atomic at the VFS level: the file either does not + // exist at the destination or appears at its full size in one step. + const fileSize = 5 * 1024 * 1024 * 1024 + const handle = await fsp.open(srcFile, 'w') + await handle.truncate(fileSize) + await handle.close() + + // Tight synchronous poll: stat the destination on every event-loop + // turn while moveDirectory runs concurrently. + let polls = 0 + const violations = [] + + timer = setInterval(() => { + try { + const stat = fs.statSync(destFile) + polls++ + if (stat.size !== fileSize) { + violations.push({ poll: polls, size: stat.size }) + } + } catch (err) { + if (err.code !== 'ENOENT') throw err + } + }, 0) + + await moveDirectory(srcDir, destDir) + + clearInterval(timer) + timer = undefined + + console.log(` ${polls} stats observed the file during the operation`) + + assert.strictEqual(violations.length, 0, 'file must never be observed at a partial size') + + const finalStat = await fsp.stat(destFile) + assert.strictEqual(finalStat.size, fileSize, + 'destination file should have the correct final size') + }) +}) From b5992fb567382205621b6aee85ebd40dcc8befae Mon Sep 17 00:00:00 2001 From: David Sanders Date: Fri, 24 Apr 2026 20:40:02 -0700 Subject: [PATCH 7/7] test: add test that moveDirectory moves contents correctly Assisted-by: Claude Opus 4.6 --- test/test-move-directory.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/test-move-directory.js b/test/test-move-directory.js index 5df369d0ff..6f1c305f0a 100644 --- a/test/test-move-directory.js +++ b/test/test-move-directory.js @@ -21,6 +21,22 @@ describe('moveDirectory', function () { clearInterval(timer) }) + it('moves contents to dest with matching structure', async function () { + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'node-gyp-move-test-')) + const srcDir = path.join(tmpDir, 'src') + const destDir = path.join(tmpDir, 'dest') + await fsp.mkdir(path.join(srcDir, 'subdir'), { recursive: true }) + await fsp.writeFile(path.join(srcDir, 'a.txt'), 'hello') + await fsp.writeFile(path.join(srcDir, 'subdir', 'b.txt'), 'world') + + await moveDirectory(srcDir, destDir) + + const destA = await fsp.readFile(path.join(destDir, 'a.txt'), 'utf8') + assert.strictEqual(destA, 'hello') + const destB = await fsp.readFile(path.join(destDir, 'subdir', 'b.txt'), 'utf8') + assert.strictEqual(destB, 'world') + }) + it('large file appears atomically (no partial writes visible)', async function () { if (!FULL_TEST) { return this.skip('Skipping due to test environment configuration')