Merge pull request #4497 from andrew--r/feature/lockfile-handle-failure-gracefully

[node-core-library] Gracefully handle irregular LockFile.tryAcquire fail on macOS/Linux

Author: iclanton

common/changes/@rushstack/node-core-library/feature-lockfile-handle-failure-gracefully_2024-01-29-14-13.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@rushstack/node-core-library",
+      "comment": "LockFile: prevent accidentaly deleting freshly created lockfile when multiple processes try to acquire the same lock on macOS/Linux",
+      "type": "patch"
+    }
+  ],
+  "packageName": "@rushstack/node-core-library"
+}

common/changes/@rushstack/node-core-library/feature-lockfile-handle-failure-gracefully_2024-01-29-21-11.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@rushstack/node-core-library",
+      "comment": "Add getStatistics() method to FileWriter instances",
+      "type": "minor"
+    }
+  ],
+  "packageName": "@rushstack/node-core-library"
+}

common/reviews/api/node-core-library.api.md

@@ -304,6 +304,7 @@ export type FileSystemStats = fs.Stats;
 export class FileWriter {
     close(): void;
     readonly filePath: string;
+    getStatistics(): FileSystemStats;
     static open(filePath: string, flags?: IFileWriterFlags): FileWriter;
     write(text: string): void;
 }

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

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
 // See LICENSE in the project root for license information.
 
+import type { FileSystemStats } from './FileSystem';
 import { Import } from './Import';
 
 const fsx: typeof import('fs-extra') = Import.lazy('fs-extra', require);
@@ -102,4 +103,16 @@ export class FileWriter {
       fsx.closeSync(fd);
     }
   }
+
+  /**
+   * Gets the statistics for the given file handle. Throws if the file handle has been closed.
+   * Behind the scenes it uses `fs.statSync()`.
+   */
+  public getStatistics(): FileSystemStats {
+    if (!this._fileDescriptor) {
+      throw new Error(`Cannot get file statistics, file descriptor has already been released.`);
+    }
+
+    return fsx.fstatSync(this._fileDescriptor);
+  }
 }

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

@@ -263,8 +263,7 @@ export class LockFile {
       // We should ideally maintain a dictionary of normalized acquired filenames
       lockFileHandle = FileWriter.open(pidLockFilePath);
       lockFileHandle.write(startTime);
-
-      const currentBirthTimeMs: number = FileSystem.getStatistics(pidLockFilePath).birthtime.getTime();
+      const currentBirthTimeMs: number = lockFileHandle.getStatistics().birthtime.getTime();
 
       let smallestBirthTimeMs: number = currentBirthTimeMs;
       let smallestBirthTimePid: string = pid.toString();
@@ -297,8 +296,11 @@ export class LockFile {
             otherPidOldStartTime = FileSystem.readFile(fileInFolderPath);
             // check the timestamp of the file
             otherBirthtimeMs = FileSystem.getStatistics(fileInFolderPath).birthtime.getTime();
-          } catch (err) {
-            // this means the file is probably deleted already
+          } catch (error) {
+            if (FileSystem.isNotExistError(error)) {
+              // the file is already deleted by other process, skip it
+              continue;
+            }
           }
 
           // if the otherPidOldStartTime is invalid, then we should look at the timestamp,

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

@@ -16,6 +16,7 @@ const libTestFolder: string = path.resolve(__dirname, '../../lib/test');
 
 describe(LockFile.name, () => {
   afterEach(() => {
+    jest.restoreAllMocks();
     setLockFileGetProcessStartTime(getProcessStartTime);
   });
 
@@ -201,6 +202,86 @@ describe(LockFile.name, () => {
         // this lock should be undefined since there is an existing lock
         expect(lock).toBeUndefined();
       });
+
+      test('deletes other hanging lockfiles if corresponding processes are not running anymore', () => {
+        // ensure test folder is clean
+        const testFolder: string = path.join(libTestFolder, '4');
+        FileSystem.ensureEmptyFolder(testFolder);
+
+        const resourceName: string = 'test';
+
+        const otherPid: number = 999999999;
+        const otherPidInitialStartTime: string = '2012-01-02 12:53:12';
+
+        // simulate a hanging lockfile that was not cleaned by other process
+        const otherPidLockFileName: string = LockFile.getLockFilePath(testFolder, resourceName, otherPid);
+        const lockFileHandle: FileWriter = FileWriter.open(otherPidLockFileName);
+        lockFileHandle.write(otherPidInitialStartTime);
+        lockFileHandle.close();
+        FileSystem.updateTimes(otherPidLockFileName, {
+          accessedTime: 10000,
+          modifiedTime: 10000
+        });
+
+        // return undefined as if the process was not running anymore
+        setLockFileGetProcessStartTime((pid: number) => {
+          return pid === otherPid ? undefined : getProcessStartTime(pid);
+        });
+
+        const deleteFileSpy = jest.spyOn(FileSystem, 'deleteFile');
+        LockFile.tryAcquire(testFolder, resourceName);
+
+        expect(deleteFileSpy).toHaveBeenCalledTimes(1);
+        expect(deleteFileSpy).toHaveBeenNthCalledWith(1, otherPidLockFileName);
+      });
+
+      test('doesn’t attempt deleting other process lockfile if it is released in the middle of acquiring process', () => {
+        // ensure test folder is clean
+        const testFolder: string = path.join(libTestFolder, '5');
+        FileSystem.ensureEmptyFolder(testFolder);
+
+        const resourceName: string = 'test';
+
+        const otherPid: number = 999999999;
+        const otherPidStartTime: string = '2012-01-02 12:53:12';
+
+        const otherPidLockFileName: string = LockFile.getLockFilePath(testFolder, resourceName, otherPid);
+
+        // create an open lockfile for other process
+        const lockFileHandle: FileWriter = FileWriter.open(otherPidLockFileName);
+        lockFileHandle.write(otherPidStartTime);
+        lockFileHandle.close();
+        FileSystem.updateTimes(otherPidLockFileName, {
+          accessedTime: 10000,
+          modifiedTime: 10000
+        });
+
+        // return other process start time as if it was still running
+        setLockFileGetProcessStartTime((pid: number) => {
+          return pid === otherPid ? otherPidStartTime : getProcessStartTime(pid);
+        });
+
+        const originalReadFile = FileSystem.readFile;
+        jest.spyOn(FileSystem, 'readFile').mockImplementation((filePath: string) => {
+          if (filePath === otherPidLockFileName) {
+            // simulate other process lock release right before the current process reads
+            // other process lockfile to decide on next steps for acquiring the lock
+            FileSystem.deleteFile(filePath);
+          }
+
+          return originalReadFile(filePath);
+        });
+
+        const deleteFileSpy = jest.spyOn(FileSystem, 'deleteFile');
+
+        LockFile.tryAcquire(testFolder, resourceName);
+
+        // 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);
+      });
     });
   }