fix: race condition that creates corrupted scripts (#8126)

close #7833

.changeset/moody-cows-invite.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/link-bins": patch
+"pnpm": patch
+---
+
+Deduplicate bin names to prevent race condition and corrupted bin scripts [#7833](https://github.com/pnpm/pnpm/issues/7833).

pkg-manager/link-bins/package.json

@@ -46,6 +46,7 @@
     "normalize-path": "^3.0.0",
     "p-settle": "^4.1.1",
     "ramda": "npm:@pnpm/ramda@0.28.1",
+    "semver": "^7.6.0",
     "symlink-dir": "^6.0.0"
   },
   "devDependencies": {
@@ -55,6 +56,7 @@
     "@types/node": "^18.19.32",
     "@types/normalize-path": "^3.0.2",
     "@types/ramda": "0.29.12",
+    "@types/semver": "7.5.3",
     "cmd-extension": "^1.0.2",
     "path-exists": "^4.0.0",
     "tempy": "^1.0.1"

pkg-manager/link-bins/src/index.ts

@@ -15,10 +15,11 @@ import isSubdir from 'is-subdir'
 import isWindows from 'is-windows'
 import normalizePath from 'normalize-path'
 import pSettle from 'p-settle'
-import { type KeyValuePair } from 'ramda'
 import isEmpty from 'ramda/src/isEmpty'
 import unnest from 'ramda/src/unnest'
+import groupBy from 'ramda/src/groupBy'
 import partition from 'ramda/src/partition'
+import semver from 'semver'
 import symlinkDir from 'symlink-dir'
 import fixBin from 'bin-links/lib/fix-bin'
 
@@ -124,6 +125,7 @@ export async function linkBinsOfPackages (
 interface CommandInfo extends Command {
   ownName: boolean
   pkgName: string
+  pkgVersion: string
   makePowerShellShim: boolean
   nodeExecPath?: string
 }
@@ -135,29 +137,15 @@ async function _linkBins (
 ): Promise<string[]> {
   if (allCmds.length === 0) return [] as string[]
 
+  // deduplicate bin names to prevent race conditions (multiple writers for the same file)
+  allCmds = deduplicateCommands(allCmds, binsDir)
+
   await fs.mkdir(binsDir, { recursive: true })
 
-  const [cmdsWithOwnName, cmdsWithOtherNames] = partition(({ ownName }) => ownName, allCmds)
-
-  const results1 = await pSettle(cmdsWithOwnName.map(async (cmd) => linkBin(cmd, binsDir, opts)))
-
-  const usedNames = Object.fromEntries(cmdsWithOwnName.map((cmd) => [cmd.name, cmd.name] as KeyValuePair<string, string>))
-  const results2 = await pSettle(cmdsWithOtherNames.map(async (cmd) => {
-    if (usedNames[cmd.name]) {
-      binsConflictLogger.debug({
-        binaryName: cmd.name,
-        binsDir,
-        linkedPkgName: usedNames[cmd.name],
-        skippedPkgName: cmd.pkgName,
-      })
-      return Promise.resolve(undefined)
-    }
-    usedNames[cmd.name] = cmd.pkgName
-    return linkBin(cmd, binsDir, opts)
-  }))
+  const results = await pSettle(allCmds.map(async cmd => linkBin(cmd, binsDir, opts)))
 
   // We want to create all commands that we can create before throwing an exception
-  for (const result of [...results1, ...results2]) {
+  for (const result of results) {
     if (result.isRejected) {
       throw result.reason
     }
@@ -166,6 +154,39 @@ async function _linkBins (
   return allCmds.map(cmd => cmd.pkgName)
 }
 
+function deduplicateCommands (commands: CommandInfo[], binsDir: string): CommandInfo[] {
+  const cmdGroups = groupBy(cmd => cmd.name, commands)
+  return Object.values(cmdGroups)
+    .filter((group): group is CommandInfo[] => group !== undefined && group.length !== 0)
+    .map(group => resolveCommandConflicts(group, binsDir))
+}
+
+function resolveCommandConflicts (group: CommandInfo[], binsDir: string): CommandInfo {
+  return group.reduce((a, b) => {
+    const [chosen, skipped] = compareCommandsInConflict(a, b) >= 0 ? [a, b] : [b, a]
+    logCommandConflict(chosen, skipped, binsDir)
+    return chosen
+  })
+}
+
+function compareCommandsInConflict (a: CommandInfo, b: CommandInfo): number {
+  if (a.ownName && !b.ownName) return 1
+  if (!a.ownName && b.ownName) return -1
+  if (a.pkgName !== b.pkgName) return a.pkgName.localeCompare(b.pkgName) // it's pointless to compare versions of 2 different package
+  return semver.compare(a.pkgVersion, b.pkgVersion)
+}
+
+function logCommandConflict (chosen: CommandInfo, skipped: CommandInfo, binsDir: string): void {
+  binsConflictLogger.debug({
+    binaryName: skipped.name,
+    binsDir,
+    linkedPkgName: chosen.pkgName,
+    linkedPkgVersion: chosen.pkgVersion,
+    skippedPkgName: skipped.pkgName,
+    skippedPkgVersion: skipped.pkgVersion,
+  })
+}
+
 async function isFromModules (filename: string): Promise<boolean> {
   const real = await fs.realpath(filename)
   return normalizePath(real).includes('/node_modules/')
@@ -206,6 +227,7 @@ async function getPackageBinsFromManifest (manifest: DependencyManifest, pkgDir:
     ...cmd,
     ownName: cmd.name === manifest.name,
     pkgName: manifest.name,
+    pkgVersion: manifest.version,
     makePowerShellShim: POWER_SHELL_IS_SUPPORTED && manifest.name !== 'pnpm',
     nodeExecPath,
   }))

pkg-manager/link-bins/test/fixtures/bin-name-conflicts-no-own-name/.gitignore

@@ -0,0 +1,2 @@
+!**/node_modules/**/*
+!/node_modules/

pkg-manager/link-bins/test/fixtures/bin-name-conflicts-no-own-name/node_modules/bar/index.js

@@ -0,0 +1 @@
+#!/usr/bin/env node

pkg-manager/link-bins/test/fixtures/bin-name-conflicts-no-own-name/node_modules/bar/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "bar",
+  "version": "1.0.0",
+  "bin": {
+    "my-command": "index.js"
+  }
+}

pkg-manager/link-bins/test/fixtures/bin-name-conflicts-no-own-name/node_modules/foo/index.js

@@ -0,0 +1 @@
+#!/usr/bin/env node

pkg-manager/link-bins/test/fixtures/bin-name-conflicts-no-own-name/node_modules/foo/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "foo",
+  "version": "1.0.0",
+  "bin": {
+    "my-command": "index.js"
+  }
+}

pkg-manager/link-bins/test/fixtures/different-versions/.gitignore

@@ -0,0 +1,3 @@
+!**/node_modules/**/*
+!/node_modules/
+

pkg-manager/link-bins/test/fixtures/different-versions/node_modules/my-command-greater/index.js

@@ -0,0 +1 @@
+#!/usr/bin/env node

pkg-manager/link-bins/test/fixtures/different-versions/node_modules/my-command-greater/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "my-command",
+  "version": "1.2.0",
+  "bin": {
+    "my-command": "index.js"
+  }
+}

pkg-manager/link-bins/test/fixtures/different-versions/node_modules/my-command-lesser/index.js

@@ -0,0 +1 @@
+#!/usr/bin/env node

pkg-manager/link-bins/test/fixtures/different-versions/node_modules/my-command-lesser/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "my-command",
+  "version": "1.0.0",
+  "bin": {
+    "my-command": "index.js"
+  }
+}

pkg-manager/link-bins/test/fixtures/different-versions/node_modules/my-command-middle/index.js

@@ -0,0 +1 @@
+#!/usr/bin/env node

pkg-manager/link-bins/test/fixtures/different-versions/node_modules/my-command-middle/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "my-command",
+  "version": "1.1.0",
+  "bin": {
+    "my-command": "index.js"
+  }
+}

pkg-manager/link-bins/test/fixtures/matching-alias-vs-matching-name/.gitignore

@@ -0,0 +1,3 @@
+!**/node_modules/**/*
+!/node_modules/
+

pkg-manager/link-bins/test/fixtures/matching-alias-vs-matching-name/node_modules/my-command/index.js

@@ -0,0 +1 @@
+#!/usr/bin/env node

pkg-manager/link-bins/test/fixtures/matching-alias-vs-matching-name/node_modules/my-command/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "aliased-to-my-command",
+  "version": "1.0.0",
+  "bin": {
+    "my-command": "index.js"
+  }
+}

pkg-manager/link-bins/test/fixtures/matching-alias-vs-matching-name/node_modules/original-my-command/index.js

@@ -0,0 +1 @@
+#!/usr/bin/env node

pkg-manager/link-bins/test/fixtures/matching-alias-vs-matching-name/node_modules/original-my-command/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "my-command",
+  "version": "1.0.0",
+  "bin": {
+    "my-command": "index.js",
+    "unaltered": "index.js"
+  }
+}

pkg-manager/link-bins/test/index.ts

@@ -208,7 +208,9 @@ test('linkBins() resolves conflicts. Prefer packages that use their name as bin
     binaryName: 'bar',
     binsDir: binTarget,
     linkedPkgName: 'bar',
+    linkedPkgVersion: expect.any(String),
     skippedPkgName: 'foo',
+    skippedPkgVersion: expect.any(String),
   })
   expect(fs.readdirSync(binTarget)).toEqual(getExpectedBins(['bar', 'foofoo']))
 
@@ -227,6 +229,64 @@ test('linkBins() resolves conflicts. Prefer packages that use their name as bin
   }
 })
 
+test('linkBins() resolves conflicts. Prefer packages whose name is greater in localeCompare', async () => {
+  const binTarget = tempy.directory()
+  const binNameConflictsFixture = f.prepare('bin-name-conflicts-no-own-name')
+  const warn = jest.fn()
+
+  await linkBins(path.join(binNameConflictsFixture, 'node_modules'), binTarget, { warn })
+
+  expect(binsConflictLogger.debug).toHaveBeenCalledWith({
+    binaryName: 'my-command',
+    binsDir: binTarget,
+    linkedPkgName: 'foo',
+    linkedPkgVersion: expect.any(String),
+    skippedPkgName: 'bar',
+    skippedPkgVersion: expect.any(String),
+  })
+  expect(fs.readdirSync(binTarget)).toEqual(getExpectedBins(['my-command']))
+
+  {
+    const binLocation = path.join(binTarget, 'my-command')
+    expect(fs.existsSync(binLocation)).toBe(true)
+    const content = fs.readFileSync(binLocation, 'utf8')
+    expect(content).toMatch('node_modules/foo/index.js')
+  }
+})
+
+test('linkBins() resolves conflicts. Prefer the latest version of the same package', async () => {
+  const binTarget = tempy.directory()
+  const binNameConflictsFixture = f.prepare('different-versions')
+  const warn = jest.fn()
+
+  await linkBins(path.join(binNameConflictsFixture, 'node_modules'), binTarget, { warn })
+
+  expect(binsConflictLogger.debug).toHaveBeenCalledWith({
+    binaryName: 'my-command',
+    binsDir: binTarget,
+    linkedPkgName: 'my-command',
+    linkedPkgVersion: expect.any(String),
+    skippedPkgName: 'my-command',
+    skippedPkgVersion: '1.0.0',
+  })
+  expect(binsConflictLogger.debug).toHaveBeenCalledWith({
+    binaryName: 'my-command',
+    binsDir: binTarget,
+    linkedPkgName: 'my-command',
+    linkedPkgVersion: expect.any(String),
+    skippedPkgName: 'my-command',
+    skippedPkgVersion: '1.1.0',
+  })
+  expect(fs.readdirSync(binTarget)).toEqual(getExpectedBins(['my-command']))
+
+  {
+    const binLocation = path.join(binTarget, 'my-command')
+    expect(fs.existsSync(binLocation)).toBe(true)
+    const content = fs.readFileSync(binLocation, 'utf8')
+    expect(content).toMatch('node_modules/my-command-greater/index.js')
+  }
+})
+
 test('linkBinsOfPackages() resolves conflicts. Prefer packages that use their name as bin name', async () => {
   const binTarget = tempy.directory()
   const binNameConflictsFixture = f.prepare('bin-name-conflicts')
@@ -250,8 +310,12 @@ test('linkBinsOfPackages() resolves conflicts. Prefer packages that use their na
   expect(binsConflictLogger.debug).toHaveBeenCalledWith({
     binaryName: 'bar',
     binsDir: binTarget,
+    linkedPkgAlias: undefined,
     linkedPkgName: 'bar',
+    linkedPkgVersion: expect.any(String),
+    skippedPkgAlias: undefined,
     skippedPkgName: 'foo',
+    skippedPkgVersion: expect.any(String),
   })
   expect(fs.readdirSync(binTarget)).toEqual(getExpectedBins(['bar', 'foofoo']))
 
@@ -270,6 +334,60 @@ test('linkBinsOfPackages() resolves conflicts. Prefer packages that use their na
   }
 })
 
+test('linkBinsOfPackages() resolves conflicts. Prefer the latest version', async () => {
+  const binTarget = tempy.directory()
+  const binNameConflictsFixture = f.prepare('different-versions')
+
+  const modulesPath = path.join(binNameConflictsFixture, 'node_modules')
+
+  await linkBinsOfPackages(
+    [
+      {
+        location: path.join(modulesPath, 'my-command-lesser'),
+        manifest: (await import(path.join(modulesPath, 'my-command-lesser', 'package.json'))).default,
+      },
+      {
+        location: path.join(modulesPath, 'my-command-middle'),
+        manifest: (await import(path.join(modulesPath, 'my-command-middle', 'package.json'))).default,
+      },
+      {
+        location: path.join(modulesPath, 'my-command-greater'),
+        manifest: (await import(path.join(modulesPath, 'my-command-greater', 'package.json'))).default,
+      },
+    ],
+    binTarget
+  )
+
+  expect(binsConflictLogger.debug).toHaveBeenCalledWith({
+    binaryName: 'my-command',
+    binsDir: binTarget,
+    linkedPkgAlias: undefined,
+    linkedPkgName: 'my-command',
+    linkedPkgVersion: expect.any(String),
+    skippedPkgAlias: undefined,
+    skippedPkgName: 'my-command',
+    skippedPkgVersion: '1.0.0',
+  })
+  expect(binsConflictLogger.debug).toHaveBeenCalledWith({
+    binaryName: 'my-command',
+    binsDir: binTarget,
+    linkedPkgAlias: undefined,
+    linkedPkgName: 'my-command',
+    linkedPkgVersion: expect.any(String),
+    skippedPkgAlias: undefined,
+    skippedPkgName: 'my-command',
+    skippedPkgVersion: '1.1.0',
+  })
+  expect(fs.readdirSync(binTarget)).toEqual(getExpectedBins(['my-command']))
+
+  {
+    const binLocation = path.join(binTarget, 'my-command')
+    expect(fs.existsSync(binLocation)).toBe(true)
+    const content = fs.readFileSync(binLocation, 'utf8')
+    expect(content).toMatch('node_modules/my-command-greater/index.js')
+  }
+})
+
 test('linkBins() resolves conflicts. Prefer packages are direct dependencies', async () => {
   const binTarget = tempy.directory()
   const binNameConflictsFixture = f.prepare('bin-name-conflicts')

pnpm-lock.yaml

@@ -3698,6 +3698,9 @@ importers:
       ramda:
         specifier: npm:@pnpm/ramda@0.28.1
         version: '@pnpm/ramda@0.28.1'
+      semver:
+        specifier: ^7.6.0
+        version: 7.6.2
       symlink-dir:
         specifier: ^6.0.0
         version: 6.0.0
@@ -3720,6 +3723,9 @@ importers:
       '@types/ramda':
         specifier: 0.29.12
         version: 0.29.12
+      '@types/semver':
+        specifier: 7.5.3
+        version: 7.5.3
       cmd-extension:
         specifier: ^1.0.2
         version: 1.0.2