From 54c20699d2395ba0ac658616b8a227fc52ccc414 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Mon, 29 Jan 2024 18:15:27 +0100 Subject: [PATCH] fix: ignore stale keystore lockfiles (#6363) * fix: ignore stale keystore lockfiles * Update error message if lockfile is already acquired * Update keymanager lockfile e2e tests --- packages/cli/package.json | 4 +- packages/cli/src/util/lockfile.ts | 54 +++++++++++-------- .../test/e2e/importKeystoresFromApi.test.ts | 8 +-- .../decryptKeystoreDefinitions.test.ts | 13 +++-- packages/db/src/controller/level.ts | 2 +- yarn.lock | 19 +++---- 6 files changed, 55 insertions(+), 45 deletions(-) diff --git a/packages/cli/package.json b/packages/cli/package.json index c16baf13c25f..8531db5b633f 100644 --- 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 index 65933e8f2897..f7a6ddf4d57d 100644 --- a/packages/cli/src/util/lockfile.ts +++ b/packages/cli/src/util/lockfile.ts @@ -1,29 +1,21 @@ -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; + } } /** @@ -31,7 +23,23 @@ export function lockFilepath(filepath: string): void { * @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 index bb91d467b86a..1cf5f107e226 100644 --- 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 index f24b83ae43a6..0f4173604405 100644 --- 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("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 index 3eed75958e3e..2cea8681c95b 100644 --- a/packages/db/src/controller/level.ts +++ b/packages/db/src/controller/level.ts @@ -52,7 +52,7 @@ export class LevelDbController implements DatabaseController