[rush] Avoid unnecessary package manager install lock by EscapeB · Pull Request #5844 · microsoft/rushstack

EscapeB · 2026-06-23T05:30:41Z

Summary

This PR extracts the package-manager install lock optimization from #5830 into a standalone change.

Rush now skips acquiring the global package manager install lock when the requested package manager is already installed and no matching install lock file exists. If a lock file exists, Rush still acquires the lock to preserve stale/dirty recovery behavior.

Testing

node common/scripts/install-run-rush.js test --to @microsoft/rush-lib --verbose

dmichon-msft · 2026-06-24T18:26:45Z

+        path: `${tempFolderPath}/rush-global`,
+        nodeSpecificPath: `${tempFolderPath}/rush-global/node-${process.version}`
+      } as RushGlobalFolder;
+      const packageManagerToolFolder: string = path.join(rushGlobalFolder.nodeSpecificPath, 'pnpm-10.27.0');


How is this version of pnpm selected, and when was it installed? I don't see anything mocking the file system checks to ensure that the version exists and/or writing files to do so.

Thanks for review !
I updated the setup to explicitly create a fake installed pnpm in the temp Rush global folder. The fixture now writes the root package.json, node_modules/pnpm/package.json, node_modules/.bin/pnpm, and the last-install.flag. The version comes from the test constant, currently matching this repo's rush.json pnpmVersion: 10.27.0.

dmichon-msft · 2026-06-24T18:28:03Z

+        packageManagerToolVersion: '10.27.0'
+      } as RushConfiguration;
+
+      await InstallHelpers.ensureLocalPackageManagerAsync(rushConfiguration, rushGlobalFolder, 1, true);


Ensure that the lockAcquireSpy returns false so that the test doesn't try to actually install the package if something goes wrong.

iclanton · 2026-07-12T20:10:57Z

    });
+  }
+
+  private static _doesPackageManagerInstallLockFileExist(


Do this async and make this a loose function, not a private static

Seems like this should be a utility on LastInstallFlag

iclanton · 2026-07-12T20:13:38Z

+    }
+
+    function getPackageManagerToolFolder(rushGlobalFolder: RushGlobalFolder): string {
+      return path.join(rushGlobalFolder.nodeSpecificPath, `${packageManager}-${packageManagerVersion}`);


Just use a template string

iclanton · 2026-07-12T20:14:28Z

+        { ensureFolderExists: true }
+      );
+
+      JsonFile.save(


Do this async and in parallel with the other one

iclanton · 2026-07-12T20:14:40Z

+          name: packageManager,
+          version: packageManagerVersion
+        },
+        path.join(packageManagerToolFolder, 'node_modules', packageManager, 'package.json'),


Use a template string. Forward slashes work fine on Windows.

iclanton · 2026-07-12T20:17:03Z

+
+      const packageManagerBinFolder: string = path.join(packageManagerToolFolder, 'node_modules', '.bin');
+      FileSystem.ensureFolder(packageManagerBinFolder);
+      FileSystem.writeFile(path.join(packageManagerBinFolder, packageManager), '');


Suggested change

FileSystem.writeFile(path.join(packageManagerBinFolder, packageManager), '');

await FileSystem.writeFileAsync(`${packageManagerBinFolder}/${packageManager}`, '', { ensureFolderExists: true });

iclanton · 2026-07-12T20:17:28Z

+      );
+
+      const packageManagerBinFolder: string = path.join(packageManagerToolFolder, 'node_modules', '.bin');
+      FileSystem.ensureFolder(packageManagerBinFolder);


Suggested change

FileSystem.ensureFolder(packageManagerBinFolder);

iclanton · 2026-07-12T20:18:19Z

+
+      expect(lockAcquireSpy).not.toHaveBeenCalled();
+      expect(installSpy).not.toHaveBeenCalled();
+      expect(FileSystem.exists(`${rushConfiguration.commonTempFolder}/pnpm-local`)).toEqual(true);


Suggested change

expect(FileSystem.exists(`${rushConfiguration.commonTempFolder}/pnpm-local`)).toEqual(true);

await expect(FileSystem.existsAsync(`${rushConfiguration.commonTempFolder}/pnpm-local`)).resolves.toEqual(true);

iclanton · 2026-07-12T20:18:54Z

+      expect(lockAcquireSpy).toHaveBeenCalledTimes(1);
+      expect(installSpy).toHaveBeenCalledTimes(1);
+      expect(releaseLockMock).toHaveBeenCalledTimes(1);
+      expect(FileSystem.exists(`${rushConfiguration.commonTempFolder}/pnpm-local`)).toEqual(true);


Suggested change

expect(FileSystem.exists(`${rushConfiguration.commonTempFolder}/pnpm-local`)).toEqual(true);

await expect(FileSystem.existsAsync(`${rushConfiguration.commonTempFolder}/pnpm-local`)).resolves.toEqual(true);

iclanton · 2026-07-12T20:20:02Z

      node: process.versions.node
    });

+    if (


Assign the async result to a variable outside of the if to avoid refactoring mistakes.

iclanton · 2026-07-20T17:07:25Z

+    if (itemName === `${lockFileResourceName}.lock`) {
+      return true;
+    }
+
+    if (itemName.startsWith(`${lockFileResourceName}#`) && itemName.endsWith('.lock')) {
+      return true;
+    }


Where do these names come from? Can you share code with whatever generates them when they're written?

rushstack/libraries/rush-lib/src/logic/installManager/InstallHelpers.ts

Line 422 in 8d872db

const packageManagerAndVersion: string = `${packageManager}-${packageManagerVersion}`;

rushstack/libraries/node-core-library/src/LockFile.ts

Line 228 in 8d872db

const lockFilePath: string = LockFile.getLockFilePath(resourceFolder, resourceName);

Original from here, but I noticed that we should reuse this part of name generating logic instead of re-copy it.

Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>

iclanton · 2026-07-21T08:23:44Z

+      "comment": "Add a LockFile helper for matching lock file names to lock resources.",
+      "type": "patch"


Suggested change

"comment": "Add a LockFile helper for matching lock file names to lock resources.",

"type": "patch"

"comment": "Add a `LockFile.isLockFileNameForResource` helper for matching lock file names to lock resources.",

"type": "minor"

I reverted changes in node-core-library, just do a minimal fix.

Copilot

Pull request overview

This PR refines Rush’s package-manager bootstrapping path by avoiding the global package manager install lock when it’s not needed (package manager already installed and no install lock file is present), while preserving the existing “stale/dirty recovery” behavior when a lock file exists.

Changes:

Skip acquiring the global package-manager install lock when the existing install is valid and no corresponding lock file exists.
Add an internal helper to detect presence of matching lock files for a given lock resource name.
Add unit tests covering the “skip lock” and “lock when lockfile exists” behaviors, plus a change log entry.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
libraries/rush-lib/src/logic/installManager/InstallHelpers.ts	Adds the early-exit path that avoids acquiring the global install lock when safe, and refactors symlink creation into a helper.
libraries/rush-lib/src/api/LastInstallFlag.ts	Adds `doesLastInstallFlagLockFileExistAsync()` to detect existing lock files for the same lock resource, supporting the optimization.
libraries/rush-lib/src/logic/test/InstallHelpers.test.ts	Adds test coverage ensuring the lock is skipped when already installed, and still acquired when a lock file is present.
common/changes/@microsoft/rush/package-manager-install-lock_2026-06-23-13-27.json	Records the behavior change as a patch.

EscapeB requested review from apostolisms, bmiddha, dmichon-msft, iclanton, jxanthony and octogonz as code owners June 23, 2026 05:30

github-project-automation Bot added this to Bug Triage Jun 23, 2026

github-project-automation Bot moved this to Needs triage in Bug Triage Jun 23, 2026

EscapeB mentioned this pull request Jun 23, 2026

[rush] Add PNPM global virtual store support for rush install #5830

Open

dmichon-msft reviewed Jun 24, 2026

View reviewed changes

iclanton reviewed Jul 12, 2026

View reviewed changes

EscapeB force-pushed the codex/package-manager-install-lock branch from fe56e6f to 37fea3f Compare July 20, 2026 11:48

iclanton reviewed Jul 20, 2026

View reviewed changes

fix: avoid unnecessary package manager install lock

f5d1d4b

Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>

EscapeB force-pushed the codex/package-manager-install-lock branch from 0b789cf to f5d1d4b Compare July 21, 2026 08:29

iclanton reviewed Jul 21, 2026

View reviewed changes

iclanton requested a review from Copilot July 21, 2026 08:31

Copilot started reviewing on behalf of iclanton July 21, 2026 08:31 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

	FileSystem.writeFile(path.join(packageManagerBinFolder, packageManager), '');
	await FileSystem.writeFileAsync(`${packageManagerBinFolder}/${packageManager}`, '', { ensureFolderExists: true });

	expect(FileSystem.exists(`${rushConfiguration.commonTempFolder}/pnpm-local`)).toEqual(true);
	await expect(FileSystem.existsAsync(`${rushConfiguration.commonTempFolder}/pnpm-local`)).resolves.toEqual(true);

		"comment": "Add a LockFile helper for matching lock file names to lock resources.",
		"type": "patch"

Uh oh!

Conversation

EscapeB commented Jun 23, 2026

Summary

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants