[node-core-library] Fix LockFile API issues that caused Rush autoinstaller failures (#5734)

* - Fix incorrect "dirty" detection caused by transient/racy lockfiles (e.g., files disappearing mid-acquire or partially written files)
- Change approach to defer cleanup: first determine lock ownership, then clean up truly stale files only after successfully acquiring
- Tighten stale vs valid lock distinction (including PID reuse cases) so we don’t misclassify active or benign states as dirty

* rush change

* test(node-core-library): update test case of LockFile#acquireAsync

---------

Co-authored-by: LPegasus <lpegasus@users.noreply.github.com>

Author: octogonz

common/changes/@rushstack/node-core-library/main_2026-03-31-00-35.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@rushstack/node-core-library",
+      "comment": "Fix an issue where the LockFile API sometimes incorrectly reported a dirty acquisition, causing Rush autoinstaller failures (GitHub #5684)",
+      "type": "patch"
+    }
+  ],
+  "packageName": "@rushstack/node-core-library"
+}

common/changes/@rushstack/node-core-library/main_2026-03-31-00-40.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@rushstack/node-core-library",
+      "comment": "Regression risk: This narrows when a lock is considered \"dirty\". Although the previous behavior was incorrect, the fix could break consumers that implicitly relied on those false positives.",
+      "type": "minor"
+    }
+  ],
+  "packageName": "@rushstack/node-core-library"
+}

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

@@ -296,10 +296,13 @@ export class LockFile {
     resourceName: string,
     pidLockFilePath: string
   ): LockFile | undefined {
-    let dirtyWhenAcquired: boolean = false;
-
-    // get the current process' pid
+    // get the current process identifier (PID)
     const pid: number = process.pid;
+
+    // Suppose that a process terminates unexpectedly without deleting its PID-based lockfile,
+    // then we check to see if the process is still alive.  The OS may have given the same PID
+    // to a new process, how to detect that?  We will rely on getProcessStartTime() which
+    // is stored in the file itself for comparison.
     const startTime: string | undefined = LockFile._getStartTime(pid);
 
     if (!startTime) {
@@ -327,6 +330,11 @@ export class LockFile {
       // look for anything ending with # then numbers and ".lock"
       const lockFileRegExp: RegExp = /^(.+)#([0-9]+)\.lock$/;
 
+      // If we are the process to acquire the lock, it becomes our responsibility to clean up these
+      // stale files.  If there is at least 1 stale file, then the resource is assumed to be "dirty"
+      // (for example, the previous process was interrupted before releasing or while acquiring).
+      const staleFilesToDelete: string[] = [];
+
       let match: RegExpMatchArray | null;
       let otherPid: string;
       for (const fileInFolder of files) {
@@ -335,14 +343,16 @@ export class LockFile {
           match[1] === resourceName &&
           (otherPid = match[2]) !== pid.toString()
         ) {
-          // we found at least one lockfile hanging around that isn't ours
+          // We found at least one lockfile hanging around that isn't ours
           const fileInFolderPath: string = `${resourceFolder}/${fileInFolder}`;
-          dirtyWhenAcquired = true;
 
           // console.log(`FOUND OTHER LOCKFILE: ${otherPid}`);
 
+          // Actual start time of the other PID
           const otherPidCurrentStartTime: string | undefined = LockFile._getStartTime(parseInt(otherPid, 10));
 
+          // The start time from the file, which we will compare with otherPidCurrentStartTime
+          // to determine whether the PID got reused by a new process.
           let otherPidOldStartTime: string | undefined;
           let otherBirthtimeMs: number | undefined;
           try {
@@ -351,47 +361,62 @@ export class LockFile {
             otherBirthtimeMs = FileSystem.getStatistics(fileInFolderPath).birthtime.getTime();
           } catch (error) {
             if (FileSystem.isNotExistError(error)) {
-              // the file is already deleted by other process, skip it
+              // ==> Properly closed lockfile, safe to ignore:
+              // The other process deleted the file, which we assume means it completed successfully,
+              // so the state is not dirty.  This is equivalent to if readFolderItemNames() never saw
+              // the file in the firstplace.
               continue;
             }
           }
 
-          // if the otherPidOldStartTime is invalid, then we should look at the timestamp,
-          // if this file was created after us, ignore it
-          // if it was created within 1 second before us, then it could be good, so we
-          //  will conservatively fail
-          // otherwise it is an old lock file and will be deleted
-          if (otherPidOldStartTime === '' && otherBirthtimeMs !== undefined) {
+          // What the other process's file exists, but it is an empty file?
+          // Either they were terminated while acquiring, or else they haven't finished writing it yet.
+          if (otherBirthtimeMs !== undefined && otherPidOldStartTime === '') {
             if (otherBirthtimeMs > currentBirthTimeMs) {
-              // ignore this file, he will be unable to get the lock since this process
-              // will hold it
+              // ==> Safe to ignore
+              // If the other process was terminated, it happened before they finished acquiring.
+              // If the other process is alive, their file is newer, so we will acquire instead of them.
+
               // console.log(`Ignoring lock for pid ${otherPid} because its lockfile is newer than ours.`);
               continue;
             } else if (
-              otherBirthtimeMs - currentBirthTimeMs < 0 && // it was created before us AND
+              otherBirthtimeMs - currentBirthTimeMs < 0 &&
               otherBirthtimeMs - currentBirthTimeMs > -1000
             ) {
-              // it was created less than a second before
-
-              // conservatively be unable to keep the lock
-              return undefined;
+              // ==> Race condition
+              // The other process created their file first, so they will probably acquire the lock
+              // after they finish writing the contents.  But what if their process is actually dead
+              // and replaced by a new process with the same PID?  Normally the otherPidOldStartTime
+              // gives the answer, but in this edge case we are missing that information.
+              // So we conservatively assume that it should not take them more than 1000ms to
+              // open a file, write a PID, and close the file.
+              return undefined; // fail to acquire and retry later
             }
           }
 
           // console.log(`Other pid ${otherPid} lockfile has start time: "${otherPidOldStartTime}"`);
           // console.log(`Other pid ${otherPid} actually has start time: "${otherPidCurrentStartTime}"`);
 
-          // this means the process is no longer executing, delete the file
+          // Time to compare
           if (!otherPidCurrentStartTime || otherPidOldStartTime !== otherPidCurrentStartTime) {
+            // ==> Stale lockfile
+            // This file doesn't prevent us from acquiring the lock, but it does indicate that
+            // the resource was left in a dirty state.  (If we delete the file right now, that
+            // information would be lost, so we clean up later when we acquire successfully.)
+
             // console.log(`Other pid ${otherPid} is no longer executing!`);
-            FileSystem.deleteFile(fileInFolderPath);
+            staleFilesToDelete.push(fileInFolderPath);
             continue;
           }
 
           // console.log(`Pid ${otherPid} lockfile has birth time: ${otherBirthtimeMs}`);
           // console.log(`Pid ${pid} lockfile has birth time: ${currentBirthTimeMs}`);
-          // this is a lockfile pointing at something valid
+
           if (otherBirthtimeMs !== undefined) {
+            // ==> We found a valid file belonging to another process.
+            // With multiple parties trying to acquire, the winner is the smallestBirthTime,
+            // so we need to sort.
+
             // the other lock file was created before the current earliest lock file
             // or the other lock file was created at the same exact time, but has earlier pid
 
@@ -418,9 +443,15 @@ export class LockFile {
         return undefined;
       }
 
-      // we have the lock!
+      let dirtyWhenAcquired: boolean = false;
+      for (const staleFileToDelete of staleFilesToDelete) {
+        FileSystem.deleteFile(staleFileToDelete, { throwIfNotExists: false });
+        dirtyWhenAcquired = true;
+      }
+
+      // We have the lock!
       lockFile = new LockFile(lockFileHandle, pidLockFilePath, dirtyWhenAcquired);
-      lockFileHandle = undefined; // we have handed the descriptor off to the instance
+      lockFileHandle = undefined; // The LockFile object has taken ownership of our handle
     } finally {
       if (lockFileHandle) {
         // ensure our lock is closed

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

@@ -130,6 +130,8 @@ describe(LockFile.name, () => {
 
     const lock2Exists: boolean = await FileSystem.existsAsync(lock2.filePath);
     expect(lock2Exists).toEqual(true);
+    // The second lock should not be dirty since it is acquired after the first lock is released
+    expect(lock2.dirtyWhenAcquired).toEqual(false);
     expect(lock2.isReleased).toEqual(false);
 
     expect(lock2Acquired).toEqual(true);
@@ -244,7 +246,7 @@ describe(LockFile.name, () => {
         expect(lock).toBeUndefined();
       });
 
-      test('deletes other hanging lockfiles if corresponding processes are not running anymore', () => {
+      test('deletes other hanging lockfiles if corresponding processes are not running anymore and marks dirtyWhenAcquired', () => {
         // ensure test folder is clean
         const testFolder: string = path.join(libTestFolder, '4');
         FileSystem.ensureEmptyFolder(testFolder);
@@ -270,10 +272,19 @@ describe(LockFile.name, () => {
         });
 
         const deleteFileSpy = jest.spyOn(FileSystem, 'deleteFile');
-        LockFile.tryAcquire(testFolder, resourceName);
+
+        const lock: LockFile | undefined = LockFile.tryAcquire(testFolder, resourceName);
+
+        expect(lock).toBeDefined();
+        expect(lock!.dirtyWhenAcquired).toEqual(true);
+        expect(lock!.isReleased).toEqual(false);
 
         expect(deleteFileSpy).toHaveBeenCalledTimes(1);
-        expect(deleteFileSpy).toHaveBeenNthCalledWith(1, otherPidLockFileName);
+        expect(deleteFileSpy).toHaveBeenNthCalledWith(1, otherPidLockFileName, {
+          throwIfNotExists: false
+        });
+
+        lock!.release();
       });
 
       test("doesn't attempt deleting other process lockfile if it is released in the middle of acquiring process", () => {
@@ -302,7 +313,7 @@ describe(LockFile.name, () => {
           return pid === otherPid ? otherPidStartTime : getProcessStartTime(pid);
         });
 
-        const originalReadFile = FileSystem.readFile;
+        const originalReadFile: typeof FileSystem.readFile = FileSystem.readFile;
         jest.spyOn(FileSystem, 'readFile').mockImplementation((filePath: string) => {
           if (filePath === otherPidLockFileName) {
             // simulate other process lock release right before the current process reads
@@ -315,13 +326,19 @@ describe(LockFile.name, () => {
 
         const deleteFileSpy = jest.spyOn(FileSystem, 'deleteFile');
 
-        LockFile.tryAcquire(testFolder, resourceName);
+        const lock: LockFile | undefined = LockFile.tryAcquire(testFolder, resourceName);
+
+        expect(lock).toBeDefined();
+        expect(lock!.dirtyWhenAcquired).toEqual(false);
+        expect(lock!.isReleased).toEqual(false);
 
         // Ensure there were no other FileSystem.deleteFile calls after our lock release simulation.
         // An extra attempt to delete the lockfile might lead to unexpectedly deleting a new lockfile
         // created by another process right after releasing/deleting the previous lockfile
         expect(deleteFileSpy).toHaveBeenCalledTimes(1);
         expect(deleteFileSpy).toHaveBeenNthCalledWith(1, otherPidLockFileName);
+
+        lock!.release();
       });
     });
   }