fix: ignore stale keystore lockfiles (#6363)

* fix: ignore stale keystore lockfiles * Update error message if lockfile is already acquired * Update keymanager lockfile e2e tests
ChainSafe · Jan 29, 2024 · 54c2069 · 54c2069
1 parent d6a7a39
commit 54c2069
Show file tree

Hide file tree

Showing 6 changed files with 55 additions and 45 deletions.
diff --git a/packages/cli/package.json b/packages/cli/package.json
@@ -77,7 +77,6 @@
     "@lodestar/utils": "^1.15.0",
     "@lodestar/validator": "^1.15.0",
     "@multiformats/multiaddr": "^12.1.3",
-    "@types/lockfile": "^1.0.2",
     "bip39": "^3.1.0",
     "deepmerge": "^4.3.1",
     "ethers": "^6.7.0",
@@ -86,9 +85,9 @@
     "got": "^11.8.6",
     "inquirer": "^9.1.5",
     "js-yaml": "^4.1.0",
-    "lockfile": "^1.0.4",
     "lodash": "^4.17.21",
     "prom-client": "^15.1.0",
+    "proper-lockfile": "^4.1.2",
     "rimraf": "^4.4.1",
     "source-map-support": "^0.5.21",
     "uint8arrays": "^4.0.9",
@@ -102,6 +101,7 @@
     "@types/got": "^9.6.12",
     "@types/inquirer": "^9.0.3",
     "@types/lodash": "^4.14.192",
+    "@types/proper-lockfile": "^4.1.4",
     "@types/yargs": "^17.0.24"
   }
 }
diff --git a/packages/cli/src/util/lockfile.ts b/packages/cli/src/util/lockfile.ts
@@ -1,37 +1,45 @@
-export type Lockfile = {
-  lockSync(path: string): void;
-  unlockSync(path: string): void;
-};
-
-const lockFile: Lockfile = (await import("lockfile")) as Lockfile;
-
-function getLockFilepath(filepath: string): string {
-  return `${filepath}.lock`;
-}
-
-/**
- * When lockfile is imported, it registers listeners to process
- * Since it's only used by the validator client, require lazily to not pollute
- * beacon_node client context
- */
-function getLockFile(): Lockfile {
-  return lockFile;
-}
+import {lockSync, unlockSync} from "proper-lockfile";
 
 /**
  * Creates a .lock file for `filepath`, argument passed must not be the lock path
  * @param filepath File to lock, i.e. `keystore_0001.json`
  */
 export function lockFilepath(filepath: string): void {
-  getLockFile().lockSync(getLockFilepath(filepath));
+  try {
+    lockSync(filepath, {
+      // Allows to lock files that do not exist
+      realpath: false,
+    });
+  } catch (e) {
+    if (isLockfileError(e) && e.code === "ELOCKED") {
+      e.message = `${filepath} is already in use by another process`;
+    }
+    throw e;
+  }
 }
 
 /**
  * Deletes a .lock file for `filepath`, argument passed must not be the lock path
  * @param filepath File to unlock, i.e. `keystore_0001.json`
  */
 export function unlockFilepath(filepath: string): void {
-  // Does not throw if the lock file is already deleted
-  // https://github.com/npm/lockfile/blob/6590779867ee9bdc5dbebddc962640759892bb91/lockfile.js#L68
-  getLockFile().unlockSync(getLockFilepath(filepath));
+  try {
+    unlockSync(filepath, {
+      // Allows to unlock files that do not exist
+      realpath: false,
+    });
+  } catch (e) {
+    if (isLockfileError(e) && e.code === "ENOTACQUIRED") {
+      // Do not throw if the lock file is already deleted
+      return;
+    }
+    throw e;
+  }
+}
+
+// https://github.com/moxystudio/node-proper-lockfile/blob/9f8c303c91998e8404a911dc11c54029812bca69/lib/lockfile.js#L53
+export type LockfileError = Error & {code: "ELOCKED" | "ENOTACQUIRED"};
+
+function isLockfileError(e: unknown): e is LockfileError {
+  return e instanceof Error && (e as LockfileError).code !== undefined;
 }
diff --git a/packages/cli/test/e2e/importKeystoresFromApi.test.ts b/packages/cli/test/e2e/importKeystoresFromApi.test.ts
@@ -95,10 +95,10 @@ describe("import keystores from api", function () {
       validator.on("exit", (code) => {
         if (code !== null && code > 0) {
           // process should exit with code > 0, and an error related to locks. Sample error:
-          // vc 351591:  ✖ Error: EEXIST: file already exists, open '/tmp/tmp-351554-dMctEAj7sJIz/import-keystores-test/keystores/0x8be678633e927aa0435addad5dcd5283fef6110d91362519cd6d43e61f6c017d724fa579cc4b2972134e050b6ba120c0/voting-keystore.json.lock'
-          // at Object.openSync (node:fs:585:3)
-          // at Module.exports.lockSync (/home/lion/Code/eth2.0/lodestar/node_modules/lockfile/lockfile.js:277:17)
-          if (/EEXIST.*voting-keystore\.json\.lock/.test(vcProc2Stderr.read())) {
+          // vc 351591:  ✖ Error: /tmp/tmp-5080-lwNxdM5Ok9ya/import-keystores-test/keystores/0x8be678633e927aa0435addad5dcd5283fef6110d91362519cd6d43e61f6c017d724fa579cc4b2972134e050b6ba120c0/voting-keystore.json is already in use by another process
+          // at /home/runner/actions-runner/_work/lodestar/lodestar/node_modules/proper-lockfile/lib/lockfile.js:68:47
+          // ... more stack trace
+          if (/Error.*voting-keystore\.json is already in use by another process/.test(vcProc2Stderr.read())) {
             resolve();
           } else {
             reject(Error(`Second validator proc exited with unknown error. stderr:\n${vcProc2Stderr.read()}`));

diff --git a/packages/cli/test/unit/validator/decryptKeystoreDefinitions.test.ts b/packages/cli/test/unit/validator/decryptKeystoreDefinitions.test.ts
@@ -7,6 +7,7 @@ import {cachedSeckeysHex} from "../../utils/cachedKeys.js";
 import {testFilesDir} from "../../utils.js";
 import {decryptKeystoreDefinitions} from "../../../src/cmds/validator/keymanager/decryptKeystoreDefinitions.js";
 import {LocalKeystoreDefinition} from "../../../src/cmds/validator/keymanager/interface.js";
+import {LockfileError, unlockFilepath} from "../../../src/util/lockfile.js";
 
 describe("decryptKeystoreDefinitions", () => {
   vi.setConfig({testTimeout: 100_000});
@@ -22,6 +23,10 @@ describe("decryptKeystoreDefinitions", () => {
   let definitions: LocalKeystoreDefinition[] = [];
 
   beforeEach(async () => {
+    // remove lockfiles from proper-lockfile cache
+    for (const {keystorePath} of definitions) {
+      unlockFilepath(keystorePath);
+    }
     rimraf.sync(dataDir);
     rimraf.sync(importFromDir);
 
@@ -46,7 +51,9 @@ describe("decryptKeystoreDefinitions", () => {
       expect(fs.existsSync(cacheFilePath)).toBe(true);
 
       // remove lockfiles created during cache file preparation
-      rimraf.sync(path.join(importFromDir, "*.lock"), {glob: true});
+      for (const {keystorePath} of definitions) {
+        unlockFilepath(keystorePath);
+      }
     });
 
     testDecryptKeystoreDefinitions(cacheFilePath);
@@ -75,14 +82,14 @@ describe("decryptKeystoreDefinitions", () => {
         await decryptKeystoreDefinitions(definitions, {logger: console, signal, cacheFilePath});
         expect.fail("Second decrypt should fail due to failure to get lockfile");
       } catch (e) {
-        expect((e as Error).message.startsWith("EEXIST: file already exists")).toBe(true);
+        expect((e as LockfileError).code).toBe<LockfileError["code"]>("ELOCKED");
       }
     });
 
     it("decrypt keystores if lockfiles already exist if ignoreLockFile=true", async () => {
       await decryptKeystoreDefinitions(definitions, {logger: console, signal, cacheFilePath});
-
       // lockfiles should exist after the first run
+
       await decryptKeystoreDefinitions(definitions, {logger: console, signal, cacheFilePath, ignoreLockFile: true});
     });
   }

diff --git a/packages/db/src/controller/level.ts b/packages/db/src/controller/level.ts
@@ -52,7 +52,7 @@ export class LevelDbController implements DatabaseController<Uint8Array, Uint8Ar
       await db.open();
     } catch (e) {
       if ((e as LevelDbError).cause?.code === "LEVEL_LOCKED") {
-        throw new Error("Database is already in use by another Lodestar instance");
+        throw new Error("Database is already in use by another process");
       }
       throw e;
     }

diff --git a/yarn.lock b/yarn.lock
@@ -2955,11 +2955,6 @@
     "@types/level-errors" "*"
     "@types/node" "*"
 
-"@types/lockfile@^1.0.2":
-  version "1.0.2"
-  resolved "https://registry.yarnpkg.com/@types/lockfile/-/lockfile-1.0.2.tgz#3f77e84171a2b7e3198bd5717c7547a54393baf8"
-  integrity sha512-jD5VbvhfMhaYN4M3qPJuhMVUg3Dfc4tvPvLEAXn6GXbs/ajDFtCQahX37GIE65ipTI3I+hEvNaXS3MYAn9Ce3Q==
-
 "@types/lodash@^4.14.192":
   version "4.14.192"
   resolved "https://registry.yarnpkg.com/@types/lodash/-/lodash-4.14.192.tgz#5790406361a2852d332d41635d927f1600811285"
@@ -3043,6 +3038,13 @@
   resolved "https://registry.yarnpkg.com/@types/normalize-package-data/-/normalize-package-data-2.4.1.tgz#d3357479a0fdfdd5907fe67e17e0a85c906e1301"
   integrity sha512-Gj7cI7z+98M282Tqmp2K5EIsoouUEzbBJhQQzDE3jSIRk6r9gsz0oUokqIUR4u1R3dMHo0pDHM7sNOHyhulypw==
 
+"@types/proper-lockfile@^4.1.4":
+  version "4.1.4"
+  resolved "https://registry.yarnpkg.com/@types/proper-lockfile/-/proper-lockfile-4.1.4.tgz#cd9fab92bdb04730c1ada542c356f03620f84008"
+  integrity sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ==
+  dependencies:
+    "@types/retry" "*"
+
 "@types/qs@^6.9.7":
   version "6.9.7"
   resolved "https://registry.yarnpkg.com/@types/qs/-/qs-6.9.7.tgz#63bb7d067db107cc1e457c303bc25d511febf6cb"
@@ -9045,13 +9047,6 @@ locate-path@^7.1.0:
   dependencies:
     p-locate "^6.0.0"
 
-lockfile@^1.0.4:
-  version "1.0.4"
-  resolved "https://registry.npmjs.org/lockfile/-/lockfile-1.0.4.tgz"
-  integrity sha512-cvbTwETRfsFh4nHsL1eGWapU1XFi5Ot9E85sWAwia7Y7EgB7vfqcZhTKZ+l7hCGxSPoushMv5GKhT5PdLv03WA==
-  dependencies:
-    signal-exit "^3.0.2"
-
 lodash._reinterpolate@^3.0.0:
   version "3.0.0"
   resolved "https://registry.yarnpkg.com/lodash._reinterpolate/-/lodash._reinterpolate-3.0.0.tgz#0ccf2d89166af03b3663c796538b75ac6e114d9d"