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 <roo-code@z.ewheeler.org>

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)

src/utils/safeWriteJson.ts

@@ -1,11 +1,10 @@
 import * as fs from "fs/promises"
 import * as path from "path"
-
-const activeLocks = new Set<string>()
+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<void> {
 	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 }