fix: better handle concurrent builds

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`)
     }
 

lib/copy-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 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() && !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 = copyDirectory

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)
 

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",
     "make-fetch-happen": "^15.0.0",
     "nopt": "^9.0.0",

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')
+      }))
+    })
+  })
 })

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')
+  })
+})