From 1bca55ef2cfdca15680e05e383c7b870e4507949 Mon Sep 17 00:00:00 2001 From: Eric Wheeler Date: Tue, 27 May 2025 21:09:55 -0700 Subject: [PATCH] feat: Implement inter-process file locking for safeWriteJson Replaces the previous in-memory lock in `safeWriteJson` with `proper-lockfile` to provide robust, cross-process advisory file locking. This enhances safety when multiple processes might attempt concurrent writes to the same JSON file. Key changes: - Added `proper-lockfile` and `@types/proper-lockfile` dependencies. - `safeWriteJson` now uses `proper-lockfile.lock()` with configured retries, staleness checks (31s), and lock update intervals (10s). - An `onCompromised` handler is included to manage scenarios where the lock state is unexpectedly altered. - Logging and comments within `safeWriteJson` have been refined for clarity, ensuring error logs include backtraces. - The test suite `safeWriteJson.test.ts` has been significantly updated to: - Use real timers (`jest.useRealTimers()`). - Employ a more comprehensive mock for `fs/promises`. - Correctly manage file pre-existence for various scenarios. - Simulate lock contention by mocking `proper-lockfile.lock()` using `jest.doMock` and a dynamic require for the SUT. - Verify lock release by checking for the absence of the `.lock` file. All tests are passing with these changes. Signed-off-by: Eric Wheeler --- src/utils/__tests__/safeWriteJson.test.ts | 94 ++++++++++++++++------- src/utils/safeWriteJson.ts | 65 ++++++++++------ 2 files changed, 106 insertions(+), 53 deletions(-) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 494458370c..60b050f786 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -6,28 +6,35 @@ const _originalFsPromisesAccess = actualFsPromises.access jest.mock("fs/promises", () => { const actual = jest.requireActual("fs/promises") - return { - // Explicitly mock functions used by the SUT and tests, defaulting to actual implementations - writeFile: jest.fn(actual.writeFile), - readFile: jest.fn(actual.readFile), - rename: jest.fn(actual.rename), - unlink: jest.fn(actual.unlink), - access: jest.fn(actual.access), - mkdtemp: jest.fn(actual.mkdtemp), - rm: jest.fn(actual.rm), - readdir: jest.fn(actual.readdir), - // Ensure all functions from 'fs/promises' that might be called are explicitly mocked - // or ensure that the SUT and tests only call functions defined here. - // For any function not listed, calls like fs.someOtherFunc would be undefined. - } + // Start with all actual implementations. + const mockedFs = { ...actual } + + // Selectively wrap functions with jest.fn() if they are spied on + // or have their implementations changed in tests. + // This ensures that other fs.promises functions used by the SUT + // (like proper-lockfile's internals) will use their actual implementations. + mockedFs.writeFile = jest.fn(actual.writeFile) + mockedFs.readFile = jest.fn(actual.readFile) + mockedFs.rename = jest.fn(actual.rename) + mockedFs.unlink = jest.fn(actual.unlink) + mockedFs.access = jest.fn(actual.access) + mockedFs.mkdtemp = jest.fn(actual.mkdtemp) + mockedFs.rm = jest.fn(actual.rm) + mockedFs.readdir = jest.fn(actual.readdir) + // fs.stat and fs.lstat will be available via { ...actual } + + return mockedFs }) import * as fs from "fs/promises" // This will now be the mocked version import * as path from "path" import * as os from "os" -import { safeWriteJson, activeLocks } from "../safeWriteJson" +// import * as lockfile from 'proper-lockfile' // No longer directly used in tests +import { safeWriteJson } from "../safeWriteJson" describe("safeWriteJson", () => { + jest.useRealTimers() // Use real timers for this test suite + let tempTestDir: string = "" let currentTestFilePath = "" @@ -36,7 +43,7 @@ describe("safeWriteJson", () => { const tempDirPrefix = path.join(os.tmpdir(), "safeWriteJson-test-") tempTestDir = await fs.mkdtemp(tempDirPrefix) currentTestFilePath = path.join(tempTestDir, "test-data.json") - activeLocks.clear() + // Individual tests will now handle creation of currentTestFilePath if needed. }) afterEach(async () => { @@ -44,7 +51,7 @@ describe("safeWriteJson", () => { await fs.rm(tempTestDir, { recursive: true, force: true }) tempTestDir = "" } - activeLocks.clear() + // activeLocks is no longer used // Explicitly reset mock implementations to default (actual) behavior // This helps prevent state leakage between tests if spy.mockRestore() isn't fully effective @@ -102,6 +109,13 @@ describe("safeWriteJson", () => { // Failure Scenarios test("should handle failure when writing to tempNewFilePath", async () => { + // Ensure the target file does not exist for this test. + try { + await fs.unlink(currentTestFilePath) + } catch (e: any) { + if (e.code !== "ENOENT") throw e + } + const data = { message: "This should not be written" } const writeFileSpy = jest.spyOn(fs, "writeFile") // Make the first call to writeFile (for tempNewFilePath) fail @@ -261,6 +275,13 @@ describe("safeWriteJson", () => { }) test("should handle failure when renaming tempNewFilePath to filePath (filePath does not exist)", async () => { + // Ensure the target file does not exist for this test. + try { + await fs.unlink(currentTestFilePath) + } catch (e: any) { + if (e.code !== "ENOENT") throw e + } + const data = { message: "This should not be written" } const renameSpy = jest.spyOn(fs, "rename") // The rename from tempNew to target fails @@ -285,18 +306,30 @@ describe("safeWriteJson", () => { renameSpy.mockRestore() }) - test("should throw an error if a lock is already held for the filePath", async () => { + test("should throw an error if an inter-process lock is already held for the filePath", async () => { + jest.resetModules() // Clear module cache to ensure fresh imports for this test + const data = { message: "test lock" } - // Manually acquire lock for testing purposes - activeLocks.add(path.resolve(currentTestFilePath)) + // Ensure the resource file exists. + await fs.writeFile(currentTestFilePath, "{}", "utf8") - await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow( - `File operation already in progress for this path: ${path.resolve(currentTestFilePath)}`, - ) + // Temporarily mock proper-lockfile for this test only + jest.doMock("proper-lockfile", () => ({ + ...jest.requireActual("proper-lockfile"), + lock: jest.fn().mockRejectedValueOnce(new Error("Failed to get lock.")), + })) - // Ensure lock is still there (safeWriteJson shouldn't release if it didn't acquire) - expect(activeLocks.has(path.resolve(currentTestFilePath))).toBe(true) - activeLocks.delete(path.resolve(currentTestFilePath)) // Manual cleanup for this test + // Re-require safeWriteJson so it picks up the mocked proper-lockfile + const { safeWriteJson: safeWriteJsonWithMockedLock } = + require("../safeWriteJson") as typeof import("../safeWriteJson") + + try { + await expect(safeWriteJsonWithMockedLock(currentTestFilePath, data)).rejects.toThrow( + /Failed to get lock.|Lock file is already being held/i, + ) + } finally { + jest.unmock("proper-lockfile") // Ensure the mock is removed after this test + } }) test("should release lock even if an error occurs mid-operation", async () => { const data = { message: "test lock release on error" } @@ -306,7 +339,9 @@ describe("safeWriteJson", () => { await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow("Simulated FS Error during writeFile") - expect(activeLocks.has(path.resolve(currentTestFilePath))).toBe(false) // Lock should be released + // Lock should be released, meaning the .lock file should not exist + const lockPath = `${path.resolve(currentTestFilePath)}.lock` + await expect(fs.access(lockPath)).rejects.toThrow(expect.objectContaining({ code: "ENOENT" })) writeFileSpy.mockRestore() }) @@ -321,7 +356,10 @@ describe("safeWriteJson", () => { await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow("Simulated EACCES Error") - expect(activeLocks.has(path.resolve(currentTestFilePath))).toBe(false) // Lock should be released + // Lock should be released, meaning the .lock file should not exist + const lockPath = `${path.resolve(currentTestFilePath)}.lock` + await expect(fs.access(lockPath)).rejects.toThrow(expect.objectContaining({ code: "ENOENT" })) + const tempFiles = await listTempFiles(tempTestDir, "test-data.json") // .new file might have been created before access check, should be cleaned up expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 02412f21bb..b724bc5376 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -1,11 +1,10 @@ import * as fs from "fs/promises" import * as path from "path" - -const activeLocks = new Set() +import * as lockfile from "proper-lockfile" /** * Safely writes JSON data to a file. - * - Uses an in-memory advisory lock to prevent concurrent writes to the same path. + * - Uses 'proper-lockfile' for inter-process advisory locking to prevent concurrent writes to the same path. * - Writes to a temporary file first. * - If the target file exists, it's backed up before being replaced. * - Attempts to roll back and clean up in case of errors. @@ -21,13 +20,35 @@ async function safeWriteJson( space: string | number = 2, ): Promise { const absoluteFilePath = path.resolve(filePath) + const lockPath = `${absoluteFilePath}.lock` + let releaseLock = async () => {} // Initialized to a no-op - if (activeLocks.has(absoluteFilePath)) { - throw new Error(`File operation already in progress for this path: ${absoluteFilePath}`) + // Acquire the lock before any file operations + try { + releaseLock = await lockfile.lock(lockPath, { + stale: 31000, // Stale after 31 seconds + update: 10000, // Update mtime every 10 seconds to prevent staleness if operation is long + retries: { + // Configuration for retrying lock acquisition + retries: 5, // Number of retries after the initial attempt + factor: 2, // Exponential backoff factor (e.g., 100ms, 200ms, 400ms, ...) + minTimeout: 100, // Minimum time to wait before the first retry (in ms) + maxTimeout: 1000, // Maximum time to wait for any single retry (in ms) + }, + realpath: false, // Skip realpath check as we've already resolved absoluteFilePath + onCompromised: (err) => { + console.error(`Lock at ${lockPath} was compromised:`, err) + throw err + }, + }) + } catch (lockError) { + // If lock acquisition fails, we throw immediately. + // The releaseLock remains a no-op, so the finally block in the main file operations + // try-catch-finally won't try to release an unacquired lock if this path is taken. + console.error(`Failed to acquire lock for ${lockPath}:`, lockError) + throw lockError // Propagate the lock acquisition error } - activeLocks.add(absoluteFilePath) - // Variables to hold the actual paths of temp files if they are created. let actualTempNewFilePath: string | null = null let actualTempBackupFilePath: string | null = null @@ -65,22 +86,20 @@ async function safeWriteJson( // If we reach here, the new file is successfully in place. // The original actualTempNewFilePath is now the main file, so we shouldn't try to clean it up as "temp". - // const _successfullyMovedNewFile = actualTempNewFilePath; // This variable is unused actualTempNewFilePath = null // Mark as "used" or "committed" // Step 4: If a backup was created, attempt to delete it. if (actualTempBackupFilePath) { try { await fs.unlink(actualTempBackupFilePath) - // console.log(`Successfully deleted backup file: ${actualTempBackupFilePath}`); actualTempBackupFilePath = null // Mark backup as handled } catch (unlinkBackupError) { // Log this error, but do not re-throw. The main operation was successful. + // actualTempBackupFilePath remains set, indicating an orphaned backup. console.error( `Successfully wrote ${absoluteFilePath}, but failed to clean up backup ${actualTempBackupFilePath}:`, unlinkBackupError, ) - // actualTempBackupFilePath remains set, indicating an orphaned backup. } } } catch (originalError) { @@ -92,30 +111,21 @@ async function safeWriteJson( // Attempt rollback if a backup was made if (backupFileToRollbackOrCleanupWithinCatch) { try { - // Inner try for rollback - console.log( - `[Catch] Attempting to restore backup ${backupFileToRollbackOrCleanupWithinCatch} to ${absoluteFilePath}`, - ) await fs.rename(backupFileToRollbackOrCleanupWithinCatch, absoluteFilePath) - console.log( - `[Catch] Successfully restored backup ${backupFileToRollbackOrCleanupWithinCatch} to ${absoluteFilePath}.`, - ) actualTempBackupFilePath = null // Mark as handled, prevent later unlink of this path } catch (rollbackError) { + // actualTempBackupFilePath (outer scope) remains pointing to backupFileToRollbackOrCleanupWithinCatch console.error( `[Catch] Failed to restore backup ${backupFileToRollbackOrCleanupWithinCatch} to ${absoluteFilePath}:`, rollbackError, ) - // actualTempBackupFilePath (outer scope) remains pointing to backupFileToRollbackOrCleanupWithinCatch } } // Cleanup the .new file if it exists if (newFileToCleanupWithinCatch) { try { - // Inner try for new file cleanup await fs.unlink(newFileToCleanupWithinCatch) - console.log(`[Catch] Cleaned up temporary new file: ${newFileToCleanupWithinCatch}`) } catch (cleanupError) { console.error( `[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`, @@ -126,11 +136,8 @@ async function safeWriteJson( // Cleanup the .bak file if it still needs to be (i.e., wasn't successfully restored) if (actualTempBackupFilePath) { - // Checks outer scope var, which is null if rollback succeeded try { - // Inner try for backup file cleanup await fs.unlink(actualTempBackupFilePath) - console.log(`[Catch] Cleaned up temporary backup file: ${actualTempBackupFilePath}`) } catch (cleanupError) { console.error( `[Catch] Failed to clean up temporary backup file ${actualTempBackupFilePath}:`, @@ -140,8 +147,16 @@ async function safeWriteJson( } throw originalError // This MUST be the error that rejects the promise. } finally { - activeLocks.delete(absoluteFilePath) + // Release the lock in the main finally block. + try { + // releaseLock will be the actual unlock function if lock was acquired, + // or the initial no-op if acquisition failed. + await releaseLock() + } catch (unlockError) { + // Do not re-throw here, as the originalError from the try/catch (if any) is more important. + console.error(`Failed to release lock for ${lockPath}:`, unlockError) + } } } -export { safeWriteJson, activeLocks } // Export activeLocks for testing lock contention +export { safeWriteJson }