From ac619f106b2547784bdbc89ef8cbb7fbc8043659 Mon Sep 17 00:00:00 2001 From: Eric Wheeler Date: Tue, 20 May 2025 17:05:38 -0700 Subject: [PATCH] feat: Add safeWriteJson utility for atomic file operations Implements a robust JSON file writing utility that: - Prevents concurrent writes to the same file using in-memory locks - Ensures atomic operations with temporary file and backup strategies - Handles error cases with proper rollback mechanisms - Cleans up temporary files even when operations fail - Provides comprehensive test coverage for success and failure scenarios Signed-off-by: Eric Wheeler --- src/utils/__tests__/safeWriteJson.test.ts | 415 ++++++++++++++++++++++ src/utils/safeWriteJson.ts | 142 ++++++++ 2 files changed, 557 insertions(+) create mode 100644 src/utils/__tests__/safeWriteJson.test.ts create mode 100644 src/utils/safeWriteJson.ts diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts new file mode 100644 index 0000000000..494458370c --- /dev/null +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -0,0 +1,415 @@ +const actualFsPromises = jest.requireActual("fs/promises") +const originalFsPromisesRename = actualFsPromises.rename +const originalFsPromisesUnlink = actualFsPromises.unlink +const originalFsPromisesWriteFile = actualFsPromises.writeFile +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. + } +}) + +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" + +describe("safeWriteJson", () => { + let tempTestDir: string = "" + let currentTestFilePath = "" + + beforeEach(async () => { + // Create a unique temporary directory for each test + const tempDirPrefix = path.join(os.tmpdir(), "safeWriteJson-test-") + tempTestDir = await fs.mkdtemp(tempDirPrefix) + currentTestFilePath = path.join(tempTestDir, "test-data.json") + activeLocks.clear() + }) + + afterEach(async () => { + if (tempTestDir) { + await fs.rm(tempTestDir, { recursive: true, force: true }) + tempTestDir = "" + } + activeLocks.clear() + + // Explicitly reset mock implementations to default (actual) behavior + // This helps prevent state leakage between tests if spy.mockRestore() isn't fully effective + // for functions on the module mock created by the factory. + ;(fs.writeFile as jest.Mock).mockImplementation(actualFsPromises.writeFile) + ;(fs.rename as jest.Mock).mockImplementation(actualFsPromises.rename) + ;(fs.unlink as jest.Mock).mockImplementation(actualFsPromises.unlink) + ;(fs.access as jest.Mock).mockImplementation(actualFsPromises.access) + ;(fs.readFile as jest.Mock).mockImplementation(actualFsPromises.readFile) + ;(fs.mkdtemp as jest.Mock).mockImplementation(actualFsPromises.mkdtemp) + ;(fs.rm as jest.Mock).mockImplementation(actualFsPromises.rm) + ;(fs.readdir as jest.Mock).mockImplementation(actualFsPromises.readdir) + }) + + const readJsonFile = async (filePath: string): Promise => { + try { + const content = await fs.readFile(filePath, "utf8") // Now uses the mocked fs + return JSON.parse(content) + } catch (error: any) { + if (error && error.code === "ENOENT") { + return null // File not found + } + throw error + } + } + + const listTempFiles = async (dir: string, baseName: string): Promise => { + const files = await fs.readdir(dir) // Now uses the mocked fs + return files.filter((f: string) => f.startsWith(`.${baseName}.new_`) || f.startsWith(`.${baseName}.bak_`)) + } + + // Success Scenarios + test("should successfully write a new file when filePath does not exist", async () => { + const data = { message: "Hello, new world!" } + await safeWriteJson(currentTestFilePath, data) + + const writtenData = await readJsonFile(currentTestFilePath) + expect(writtenData).toEqual(data) + const tempFiles = await listTempFiles(tempTestDir, "test-data.json") + expect(tempFiles.length).toBe(0) + }) + + test("should successfully overwrite an existing file", async () => { + const initialData = { message: "Initial content" } + await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Now uses the mocked fs for setup + + const newData = { message: "Updated content" } + await safeWriteJson(currentTestFilePath, newData) + + const writtenData = await readJsonFile(currentTestFilePath) + expect(writtenData).toEqual(newData) + const tempFiles = await listTempFiles(tempTestDir, "test-data.json") + expect(tempFiles.length).toBe(0) + }) + + // Failure Scenarios + test("should handle failure when writing to tempNewFilePath", async () => { + const data = { message: "This should not be written" } + const writeFileSpy = jest.spyOn(fs, "writeFile") + // Make the first call to writeFile (for tempNewFilePath) fail + writeFileSpy.mockImplementationOnce(async (filePath: any, fileData: any, options?: any) => { + if (typeof filePath === "string" && filePath.includes(".new_")) { + throw new Error("Simulated FS Error: writeFile tempNewFilePath") + } + // For any other writeFile call (e.g. if tests write initial files), use original + return actualFsPromises.writeFile(filePath, fileData, options) // Call actual for passthrough + }) + + await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow( + "Simulated FS Error: writeFile tempNewFilePath", + ) + + const writtenData = await readJsonFile(currentTestFilePath) + expect(writtenData).toBeNull() // File should not exist or be created + const tempFiles = await listTempFiles(tempTestDir, "test-data.json") + expect(tempFiles.length).toBe(0) // All temp files should be cleaned up + + writeFileSpy.mockRestore() + }) + + test("should handle failure when renaming filePath to tempBackupFilePath (filePath exists)", async () => { + const initialData = { message: "Initial content, should remain" } + await originalFsPromisesWriteFile(currentTestFilePath, JSON.stringify(initialData)) // Use original for setup + + const newData = { message: "This should not be written" } + const renameSpy = jest.spyOn(fs, "rename") + // First rename is target to backup + renameSpy.mockImplementationOnce(async (oldPath: any, newPath: any) => { + if (typeof newPath === "string" && newPath.includes(".bak_")) { + throw new Error("Simulated FS Error: rename to tempBackupFilePath") + } + return originalFsPromisesRename(oldPath, newPath) // Use constant + }) + + await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow( + "Simulated FS Error: rename to tempBackupFilePath", + ) + + const writtenData = await readJsonFile(currentTestFilePath) + expect(writtenData).toEqual(initialData) // Original file should be intact + const tempFiles = await listTempFiles(tempTestDir, "test-data.json") + // tempNewFile was created, but should be cleaned up. Backup was not created. + expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0) + expect(tempFiles.filter((f: string) => f.includes(".bak_")).length).toBe(0) + + renameSpy.mockRestore() + }) + + test("should handle failure when renaming tempNewFilePath to filePath (filePath exists, backup succeeded)", async () => { + const initialData = { message: "Initial content, should be restored" } + await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Use mocked fs for setup + + const newData = { message: "This is in tempNewFilePath" } + const renameSpy = jest.spyOn(fs, "rename") + let renameCallCountTest1 = 0 + renameSpy.mockImplementation(async (oldPath: any, newPath: any) => { + const oldPathStr = oldPath.toString() + const newPathStr = newPath.toString() + renameCallCountTest1++ + console.log(`[TEST 1] fs.rename spy call #${renameCallCountTest1}: ${oldPathStr} -> ${newPathStr}`) + + // First rename call by safeWriteJson (if target exists) is target -> .bak + if (renameCallCountTest1 === 1 && !oldPathStr.includes(".new_") && newPathStr.includes(".bak_")) { + console.log("[TEST 1] Spy: Call #1 (target->backup), executing original rename.") + return originalFsPromisesRename(oldPath, newPath) + } + // Second rename call by safeWriteJson is .new -> target + else if ( + renameCallCountTest1 === 2 && + oldPathStr.includes(".new_") && + path.resolve(newPathStr) === path.resolve(currentTestFilePath) + ) { + console.log("[TEST 1] Spy: Call #2 (.new->target), THROWING SIMULATED ERROR.") + throw new Error("Simulated FS Error: rename tempNewFilePath to filePath") + } + // Fallback for unexpected calls or if the target file didn't exist (only one rename: .new -> target) + else if ( + renameCallCountTest1 === 1 && + oldPathStr.includes(".new_") && + path.resolve(newPathStr) === path.resolve(currentTestFilePath) + ) { + // This case handles if the initial file didn't exist, so only one rename happens. + // For this specific test, we expect two renames. + console.warn( + "[TEST 1] Spy: Call #1 was .new->target, (unexpected for this test scenario, but handling)", + ) + throw new Error("Simulated FS Error: rename tempNewFilePath to filePath") + } + console.warn( + `[TEST 1] Spy: Unexpected call #${renameCallCountTest1} or paths. Defaulting to original rename. ${oldPathStr} -> ${newPathStr}`, + ) + return originalFsPromisesRename(oldPath, newPath) + }) + + // This scenario should reject because the new data couldn't be written to the final path, + // even if rollback succeeds. + await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow( + "Simulated FS Error: rename tempNewFilePath to filePath", + ) + + const writtenData = await readJsonFile(currentTestFilePath) + expect(writtenData).toEqual(initialData) // Original file should be restored from backup + + const tempFiles = await listTempFiles(tempTestDir, "test-data.json") + expect(tempFiles.length).toBe(0) // All temp/backup files should be cleaned up + + renameSpy.mockRestore() + }) + + test("should handle failure when deleting tempBackupFilePath (filePath exists, all renames succeed)", async () => { + const initialData = { message: "Initial content" } + await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Use mocked fs for setup + + const newData = { message: "This should be the final content" } + const unlinkSpy = jest.spyOn(fs, "unlink") + // The unlink that targets the backup file fails + unlinkSpy.mockImplementationOnce(async (filePath: any) => { + const filePathStr = filePath.toString() + if (filePathStr.includes(".bak_")) { + console.log("[TEST unlink bak] Mock: Simulating failure for unlink backup.") + throw new Error("Simulated FS Error: delete tempBackupFilePath") + } + console.log("[TEST unlink bak] Mock: Condition NOT MET. Using originalFsPromisesUnlink.") + return originalFsPromisesUnlink(filePath) + }) + + // The function itself should still succeed from the user's perspective, + // as the primary operation (writing the new data) was successful. + // The error during backup cleanup is logged but not re-thrown to the caller. + // However, the current implementation *does* re-throw. Let's test that behavior. + // If the desired behavior is to not re-throw on backup cleanup failure, the main function needs adjustment. + // The current safeWriteJson logic is to log the error and NOT reject. + const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error + + await expect(safeWriteJson(currentTestFilePath, newData)).resolves.toBeUndefined() + + // The main file should be the new data + const writtenData = await readJsonFile(currentTestFilePath) + expect(writtenData).toEqual(newData) + + // Check that the cleanup failure was logged + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining(`Successfully wrote ${currentTestFilePath}, but failed to clean up backup`), + expect.objectContaining({ message: "Simulated FS Error: delete tempBackupFilePath" }), + ) + + const tempFiles = await listTempFiles(tempTestDir, "test-data.json") + // The .new file is gone (renamed to target), the .bak file failed to delete + expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0) + expect(tempFiles.filter((f: string) => f.includes(".bak_")).length).toBe(1) // Backup file remains + + unlinkSpy.mockRestore() + consoleErrorSpy.mockRestore() + }) + + test("should handle failure when renaming tempNewFilePath to filePath (filePath does not exist)", async () => { + const data = { message: "This should not be written" } + const renameSpy = jest.spyOn(fs, "rename") + // The rename from tempNew to target fails + renameSpy.mockImplementationOnce(async (oldPath: any, newPath: any) => { + const oldPathStr = oldPath.toString() + const newPathStr = newPath.toString() + if (oldPathStr.includes(".new_") && path.resolve(newPathStr) === path.resolve(currentTestFilePath)) { + throw new Error("Simulated FS Error: rename tempNewFilePath to filePath (no prior file)") + } + return originalFsPromisesRename(oldPath, newPath) // Use constant + }) + + await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow( + "Simulated FS Error: rename tempNewFilePath to filePath (no prior file)", + ) + + const writtenData = await readJsonFile(currentTestFilePath) + expect(writtenData).toBeNull() // File should not exist + const tempFiles = await listTempFiles(tempTestDir, "test-data.json") + expect(tempFiles.length).toBe(0) // All temp files should be cleaned up + + renameSpy.mockRestore() + }) + + test("should throw an error if a lock is already held for the filePath", async () => { + const data = { message: "test lock" } + // Manually acquire lock for testing purposes + activeLocks.add(path.resolve(currentTestFilePath)) + + await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow( + `File operation already in progress for this path: ${path.resolve(currentTestFilePath)}`, + ) + + // 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 + }) + test("should release lock even if an error occurs mid-operation", async () => { + const data = { message: "test lock release on error" } + const writeFileSpy = jest.spyOn(fs, "writeFile").mockImplementationOnce(async () => { + throw new Error("Simulated FS Error during writeFile") + }) + + await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow("Simulated FS Error during writeFile") + + expect(activeLocks.has(path.resolve(currentTestFilePath))).toBe(false) // Lock should be released + + writeFileSpy.mockRestore() + }) + + test("should handle fs.access error that is not ENOENT", async () => { + const data = { message: "access error test" } + const accessSpy = jest.spyOn(fs, "access").mockImplementationOnce(async () => { + const err = new Error("Simulated EACCES Error") as NodeJS.ErrnoException + err.code = "EACCES" // Simulate a permissions error, for example + throw err + }) + + await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow("Simulated EACCES Error") + + expect(activeLocks.has(path.resolve(currentTestFilePath))).toBe(false) // Lock should be released + 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) + + accessSpy.mockRestore() + }) + + // Test for rollback failure scenario + test("should log error and re-throw original if rollback fails", async () => { + const initialData = { message: "Initial, should be lost if rollback fails" } + await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Use mocked fs for setup + const newData = { message: "New data" } + + const renameSpy = jest.spyOn(fs, "rename") + const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error + let renameCallCountTest2 = 0 + + renameSpy.mockImplementation(async (oldPath: any, newPath: any) => { + const oldPathStr = oldPath.toString() + const newPathStr = newPath.toString() + renameCallCountTest2++ + const resolvedOldPath = path.resolve(oldPathStr) + const resolvedNewPath = path.resolve(newPathStr) + const resolvedCurrentTFP = path.resolve(currentTestFilePath) + console.log( + `[TEST 2] fs.promises.rename call #${renameCallCountTest2}: oldPath=${oldPathStr} (resolved: ${resolvedOldPath}), newPath=${newPathStr} (resolved: ${resolvedNewPath}), currentTFP (resolved: ${resolvedCurrentTFP})`, + ) + + if (renameCallCountTest2 === 1) { + // Call 1: Original -> Backup (Succeeds) + if (resolvedOldPath === resolvedCurrentTFP && newPathStr.includes(".bak_")) { + console.log("[TEST 2] Call #1 (Original->Backup): Condition MET. originalFsPromisesRename.") + return originalFsPromisesRename(oldPath, newPath) + } + console.error("[TEST 2] Call #1: UNEXPECTED args.") + throw new Error("Unexpected args for rename call #1 in test") + } else if (renameCallCountTest2 === 2) { + // Call 2: New -> Original (Fails - this is the "original error") + if (oldPathStr.includes(".new_") && resolvedNewPath === resolvedCurrentTFP) { + console.log( + '[TEST 2] Call #2 (New->Original): Condition MET. Throwing "Simulated FS Error: new to original".', + ) + throw new Error("Simulated FS Error: new to original") + } + console.error("[TEST 2] Call #2: UNEXPECTED args.") + throw new Error("Unexpected args for rename call #2 in test") + } else if (renameCallCountTest2 === 3) { + // Call 3: Backup -> Original (Rollback attempt - Fails) + if (oldPathStr.includes(".bak_") && resolvedNewPath === resolvedCurrentTFP) { + console.log( + '[TEST 2] Call #3 (Backup->Original Rollback): Condition MET. Throwing "Simulated FS Error: backup to original (rollback)".', + ) + throw new Error("Simulated FS Error: backup to original (rollback)") + } + console.error("[TEST 2] Call #3: UNEXPECTED args.") + throw new Error("Unexpected args for rename call #3 in test") + } + console.error(`[TEST 2] Unexpected fs.promises.rename call count: ${renameCallCountTest2}`) + return originalFsPromisesRename(oldPath, newPath) + }) + + await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Simulated FS Error: new to original") + + // Check that the rollback failure was logged + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining( + `Operation failed for ${path.resolve(currentTestFilePath)}: [Original Error Caught]`, + ), + expect.objectContaining({ message: "Simulated FS Error: new to original" }), // The original error + ) + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringMatching(/\[Catch\] Failed to restore backup .*?\.bak_.*?\s+to .*?:/), // Matches the backup filename pattern + expect.objectContaining({ message: "Simulated FS Error: backup to original (rollback)" }), // The rollback error + ) + // The original error is logged first in safeWriteJson's catch block, then the rollback failure. + + // File system state: original file is lost (backup couldn't be restored and was then unlinked), + // new file was cleaned up. The target path `currentTestFilePath` should not exist. + const finalState = await readJsonFile(currentTestFilePath) + expect(finalState).toBeNull() + + const tempFiles = await listTempFiles(tempTestDir, "test-data.json") + // Backup file should also be cleaned up by the final unlink attempt in safeWriteJson's catch block, + // as that unlink is not mocked to fail. + expect(tempFiles.filter((f: string) => f.includes(".bak_")).length).toBe(0) + expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0) + + renameSpy.mockRestore() + consoleErrorSpy.mockRestore() + }) +}) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts new file mode 100644 index 0000000000..2038b2457b --- /dev/null +++ b/src/utils/safeWriteJson.ts @@ -0,0 +1,142 @@ +import * as fs from "fs/promises" +import * as path from "path" + +const activeLocks = new Set() + +/** + * Safely writes JSON data to a file. + * - Uses an in-memory advisory lock 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. + * + * @param {string} filePath - The absolute path to the target file. + * @param {any} data - The data to serialize to JSON and write. + * @returns {Promise} + */ +async function safeWriteJson(filePath: string, data: any): Promise { + const absoluteFilePath = path.resolve(filePath) + + if (activeLocks.has(absoluteFilePath)) { + throw new Error(`File operation already in progress for this path: ${absoluteFilePath}`) + } + + 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 + + try { + // Step 1: Write data to a new temporary file. + actualTempNewFilePath = path.join( + path.dirname(absoluteFilePath), + `.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, + ) + const jsonData = JSON.stringify(data, null, 2) + await fs.writeFile(actualTempNewFilePath, jsonData, "utf8") + + // Step 2: Check if the target file exists. If so, rename it to a backup path. + try { + await fs.access(absoluteFilePath) // Check for target file existence + // Target exists, create a backup path and rename. + actualTempBackupFilePath = path.join( + path.dirname(absoluteFilePath), + `.${path.basename(absoluteFilePath)}.bak_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, + ) + await fs.rename(absoluteFilePath, actualTempBackupFilePath) + } catch (accessError: any) { + // Explicitly type accessError + if (accessError.code !== "ENOENT") { + // An error other than "file not found" occurred during access check. + throw accessError + } + // Target file does not exist, so no backup is made. actualTempBackupFilePath remains null. + } + + // Step 3: Rename the new temporary file to the target file path. + // This is the main "commit" step. + await fs.rename(actualTempNewFilePath, absoluteFilePath) + + // 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. + console.error( + `Successfully wrote ${absoluteFilePath}, but failed to clean up backup ${actualTempBackupFilePath}:`, + unlinkBackupError, + ) + // actualTempBackupFilePath remains set, indicating an orphaned backup. + } + } + } catch (originalError) { + console.error(`Operation failed for ${absoluteFilePath}: [Original Error Caught]`, originalError) + + const newFileToCleanupWithinCatch = actualTempNewFilePath + const backupFileToRollbackOrCleanupWithinCatch = actualTempBackupFilePath + + // 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) { + 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}:`, + cleanupError, + ) + } + } + + // 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}:`, + cleanupError, + ) + } + } + throw originalError // This MUST be the error that rejects the promise. + } finally { + activeLocks.delete(absoluteFilePath) + } +} + +export { safeWriteJson, activeLocks } // Export activeLocks for testing lock contention