fix: allow concurrent non-local npx calls

DEPENDENCIES.md

@@ -349,9 +349,11 @@ graph LR;
   libnpmexec-->npmcli-template-oss["@npmcli/template-oss"];
   libnpmexec-->pacote;
   libnpmexec-->proc-log;
+  libnpmexec-->promise-retry;
   libnpmexec-->read-package-json-fast;
   libnpmexec-->read;
   libnpmexec-->semver;
+  libnpmexec-->signal-exit;
   libnpmexec-->tap;
   libnpmexec-->walk-up-path;
   libnpmfund-->npmcli-arborist["@npmcli/arborist"];

package-lock.json

@@ -18950,9 +18950,11 @@
         "npm-package-arg": "^12.0.0",
         "pacote": "^21.0.0",
         "proc-log": "^5.0.0",
+        "promise-retry": "^2.0.1",
         "read": "^4.0.0",
         "read-package-json-fast": "^4.0.0",
         "semver": "^7.3.7",
+        "signal-exit": "^4.1.0",
         "walk-up-path": "^4.0.0"
       },
       "devDependencies": {

workspaces/libnpmexec/lib/index.js

@@ -1,6 +1,6 @@
 'use strict'
 
-const { dirname, resolve } = require('node:path')
+const { dirname, join, resolve } = require('node:path')
 const crypto = require('node:crypto')
 const { mkdir } = require('node:fs/promises')
 const Arborist = require('@npmcli/arborist')
@@ -16,6 +16,7 @@ const getBinFromManifest = require('./get-bin-from-manifest.js')
 const noTTY = require('./no-tty.js')
 const runScript = require('./run-script.js')
 const isWindows = require('./is-windows.js')
+const withLock = require('./with-lock.js')
 
 const binPaths = []
 
@@ -247,7 +248,8 @@ const exec = async (opts) => {
       ...flatOptions,
       path: installDir,
     })
-    const npxTree = await npxArb.loadActual()
+    const lockPath = join(installDir, 'concurrency.lock')
+    const npxTree = await withLock(lockPath, () => npxArb.loadActual())
     await Promise.all(needInstall.map(async ({ spec }) => {
       const { manifest } = await missingFromTree({
         spec,
@@ -290,11 +292,11 @@ const exec = async (opts) => {
           }
         }
       }
-      await npxArb.reify({
+      await withLock(lockPath, () => npxArb.reify({
         ...flatOptions,
         save: true,
         add,
-      })
+      }))
     }
     binPaths.push(resolve(installDir, 'node_modules/.bin'))
     const pkgJson = await PackageJson.load(installDir)

workspaces/libnpmexec/lib/with-lock.js

@@ -0,0 +1,154 @@
+const fs = require('node:fs/promises')
+const { rmdirSync } = require('node:fs')
+const promiseRetry = require('promise-retry')
+const { onExit } = require('signal-exit')
+
+// a lockfile implementation inspired by the unmaintained proper-lockfile library
+//
+// similarities:
+// - based on mkdir's atomicity
+// - works across processes and even machines (via NFS)
+// - cleans up after itself
+// - detects compromised locks
+//
+// differences:
+// - higher-level API (just a withLock function)
+// - written in async/await style
+// - uses mtime + inode for more reliable compromised lock detection
+// - more ergonomic compromised lock handling (i.e. withLock will reject, and callbacks have access to an AbortSignal)
+// - uses a more recent version of signal-exit
+
+const touchInterval = 100
+// mtime precision is platform dependent, so use a reasonably large threshold
+const staleThreshold = 5_000
+
+// track current locks and their cleanup functions
+const currentLocks = new Map()
+
+function cleanupLocks () {
+  for (const [, cleanup] of currentLocks) {
+    try {
+      cleanup()
+    } catch (err) {
+      //
+    }
+  }
+}
+
+// clean up any locks that were not released normally
+onExit(cleanupLocks)
+
+/**
+ * Acquire an advisory lock for the given path and hold it for the duration of the callback.
+ *
+ * The lock will be released automatically when the callback resolves or rejects.
+ * Concurrent calls to withLock() for the same path will wait until the lock is released.
+ */
+async function withLock (lockPath, cb) {
+  try {
+    const signal = await acquireLock(lockPath)
+    return await new Promise((resolve, reject) => {
+      signal.addEventListener('abort', () => {
+        reject(Object.assign(new Error('Lock compromised'), { code: 'ECOMPROMISED' }))
+      });
+
+      (async () => {
+        try {
+          resolve(await cb(signal))
+        } catch (err) {
+          reject(err)
+        }
+      })()
+    })
+  } finally {
+    await releaseLock(lockPath)
+  }
+}
+
+function acquireLock (lockPath) {
+  return promiseRetry({
+    minTimeout: 100,
+    maxTimeout: 5_000,
+    // if another process legitimately holds the lock, wait for it to release; if it dies abnormally and the lock becomes stale, we'll acquire it automatically
+    forever: true,
+  }, async (retry) => {
+    try {
+      await fs.mkdir(lockPath)
+    } catch (err) {
+      if (err.code !== 'EEXIST') {
+        throw err
+      }
+
+      const status = await getLockStatus(lockPath)
+
+      if (status === 'locked') {
+        // let's see if we can acquire it on the next attempt 🤞
+        return retry(err)
+      }
+      if (status === 'stale') {
+        // there is a very tiny window where another process could also release the stale lock and acquire it before we release it here; the lock compromise checker should detect this and throw an error
+        await releaseLock(lockPath)
+      }
+      return await acquireLock(lockPath)
+    }
+    const signal = await maintainLock(lockPath)
+    return signal
+  })
+}
+
+async function releaseLock (lockPath) {
+  try {
+    currentLocks.get(lockPath)?.()
+    currentLocks.delete(lockPath)
+    await fs.rmdir(lockPath)
+  } catch (err) {
+    if (err.code !== 'ENOENT') {
+      throw err
+    }
+  }
+}
+
+async function getLockStatus (lockPath) {
+  try {
+    const stat = await fs.stat(lockPath)
+    return (Date.now() - stat.mtimeMs > staleThreshold) ? 'stale' : 'locked'
+  } catch (err) {
+    if (err.code === 'ENOENT') {
+      return 'unlocked'
+    }
+    throw err
+  }
+}
+
+async function maintainLock (lockPath) {
+  const controller = new AbortController()
+  const stats = await fs.stat(lockPath)
+  let mtimeMs = stats.mtimeMs
+  const signal = controller.signal
+
+  async function touchLock () {
+    try {
+      const currentStats = (await fs.stat(lockPath))
+      if (currentStats.ino !== stats.ino || currentStats.mtimeMs !== mtimeMs) {
+        throw new Error('Lock compromised')
+      }
+      await fs.utimes(lockPath, new Date(), new Date(mtimeMs = Date.now()))
+    } catch (err) {
+      // stats mismatch or other fs error means the lock was compromised, unless we just released the lock during this iteration
+      if (currentLocks.has(lockPath)) {
+        controller.abort()
+      }
+    }
+  }
+
+  const timeout = setInterval(touchLock, touchInterval)
+  timeout.unref()
+  function cleanup () {
+    rmdirSync(lockPath)
+    clearInterval(timeout)
+  }
+  currentLocks.set(lockPath, cleanup)
+  return signal
+}
+
+module.exports = withLock

workspaces/libnpmexec/package.json

@@ -67,9 +67,11 @@
     "npm-package-arg": "^12.0.0",
     "pacote": "^21.0.0",
     "proc-log": "^5.0.0",
+    "promise-retry": "^2.0.1",
     "read": "^4.0.0",
     "read-package-json-fast": "^4.0.0",
     "semver": "^7.3.7",
+    "signal-exit": "^4.1.0",
     "walk-up-path": "^4.0.0"
   },
   "templateOSS": {