mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
fix: race condition in safeWriteJson directory handling
Fix race condition where safeWriteJson would fail with ENOENT errors
during lock acquisition when the parent directory was just created.
The issue occurred when the directory creation hadn't fully synchronized
with the filesystem before attempting to acquire a lock. This happened
primarily when Task.saveApiConversationHistory() called the function
immediately after creating the task directory.
The fix ensures directories exist and are fully synchronized before
lock acquisition by:
- Creating directories with fs.mkdir({ recursive: true })
- Verifying access to created directories
- Setting realpath: false in lock options to allow locking non-existent files
Added comprehensive tests for directory creation capabilities.
Fixes: #4468
See-also: #4471, #3772, #722
Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>
This commit is contained in:
Eric Wheeler
Daniel Riccio
parent
4d5668962b
commit
7d23a8ddf4
2 changed files with 156 additions and 25 deletions
|
|
@ -3,6 +3,7 @@ const originalFsPromisesRename = actualFsPromises.rename
|
|||
const originalFsPromisesUnlink = actualFsPromises.unlink
|
||||
const originalFsPromisesWriteFile = actualFsPromises.writeFile
|
||||
const _originalFsPromisesAccess = actualFsPromises.access
|
||||
const originalFsPromisesMkdir = actualFsPromises.mkdir
|
||||
|
||||
jest.mock("fs/promises", () => {
|
||||
const actual = jest.requireActual("fs/promises")
|
||||
|
|
@ -21,6 +22,7 @@ jest.mock("fs/promises", () => {
|
|||
mockedFs.mkdtemp = jest.fn(actual.mkdtemp)
|
||||
mockedFs.rm = jest.fn(actual.rm)
|
||||
mockedFs.readdir = jest.fn(actual.readdir)
|
||||
mockedFs.mkdir = jest.fn(actual.mkdir)
|
||||
// fs.stat and fs.lstat will be available via { ...actual }
|
||||
|
||||
return mockedFs
|
||||
|
|
@ -44,6 +46,30 @@ import { safeWriteJson } from "../safeWriteJson"
|
|||
import { Writable } from "stream" // For typing mock stream
|
||||
|
||||
describe("safeWriteJson", () => {
|
||||
let originalConsoleError: typeof console.error
|
||||
|
||||
beforeAll(() => {
|
||||
// Store original console.error
|
||||
originalConsoleError = console.error
|
||||
|
||||
// Replace with filtered version that suppresses output from the module
|
||||
console.error = function (...args) {
|
||||
// Check if call originated from safeWriteJson.ts
|
||||
if (new Error().stack?.includes("safeWriteJson.ts")) {
|
||||
// Suppress output but allow spy recording
|
||||
return
|
||||
}
|
||||
|
||||
// Pass through all other calls (from tests)
|
||||
return originalConsoleError.apply(console, args)
|
||||
}
|
||||
})
|
||||
|
||||
afterAll(() => {
|
||||
// Restore original behavior
|
||||
console.error = originalConsoleError
|
||||
})
|
||||
|
||||
jest.useRealTimers() // Use real timers for this test suite
|
||||
|
||||
let tempTestDir: string = ""
|
||||
|
|
@ -77,6 +103,7 @@ describe("safeWriteJson", () => {
|
|||
;(fs.mkdtemp as jest.Mock).mockImplementation(actualFsPromises.mkdtemp)
|
||||
;(fs.rm as jest.Mock).mockImplementation(actualFsPromises.rm)
|
||||
;(fs.readdir as jest.Mock).mockImplementation(actualFsPromises.readdir)
|
||||
;(fs.mkdir as jest.Mock).mockImplementation(actualFsPromises.mkdir)
|
||||
// Ensure all mocks are reset after each test
|
||||
jest.restoreAllMocks()
|
||||
})
|
||||
|
|
@ -199,11 +226,9 @@ describe("safeWriteJson", () => {
|
|||
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
|
||||
|
|
@ -212,7 +237,6 @@ describe("safeWriteJson", () => {
|
|||
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)
|
||||
|
|
@ -223,14 +247,8 @@ describe("safeWriteJson", () => {
|
|||
) {
|
||||
// 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)
|
||||
})
|
||||
|
||||
|
|
@ -249,6 +267,118 @@ describe("safeWriteJson", () => {
|
|||
renameSpy.mockRestore()
|
||||
})
|
||||
|
||||
// Tests for directory creation functionality
|
||||
test("should create parent directory if it doesn't exist", async () => {
|
||||
// Create a path in a non-existent subdirectory of the temp dir
|
||||
const nonExistentDir = path.join(tempTestDir, "non-existent-dir")
|
||||
const filePath = path.join(nonExistentDir, "test-data.json")
|
||||
const data = { message: "Hello from new directory" }
|
||||
|
||||
// Verify the directory doesn't exist yet
|
||||
const dirAccessError = await fs.access(nonExistentDir).catch((e) => e)
|
||||
expect(dirAccessError).toBeDefined()
|
||||
expect(dirAccessError.code).toBe("ENOENT")
|
||||
|
||||
// safeWriteJson should now create directories and initialize an empty file automatically
|
||||
|
||||
// safeWriteJson should write the file
|
||||
await safeWriteJson(filePath, data)
|
||||
|
||||
// Verify file was written correctly
|
||||
const writtenData = await readJsonFile(filePath)
|
||||
expect(writtenData).toEqual(data)
|
||||
|
||||
// Verify no temp files remain
|
||||
const tempFiles = await listTempFiles(nonExistentDir, "test-data.json")
|
||||
expect(tempFiles.length).toBe(0)
|
||||
})
|
||||
|
||||
test("should handle multi-level directory creation", async () => {
|
||||
// Create a new non-existent subdirectory path with multiple levels
|
||||
const newDir = path.join(tempTestDir, "new-test-dir", "subdir", "deeper")
|
||||
const filePath = path.join(newDir, "new-file.json")
|
||||
const data = { message: "New directory test" }
|
||||
|
||||
// Verify directories don't exist initially
|
||||
const dirAccessError = await fs.access(newDir).catch((e) => e)
|
||||
expect(dirAccessError).toBeDefined()
|
||||
expect(dirAccessError.code).toBe("ENOENT")
|
||||
|
||||
// Don't create any directories - safeWriteJson should handle it all
|
||||
|
||||
// Call safeWriteJson - it should create all missing directories and the file
|
||||
await safeWriteJson(filePath, data)
|
||||
|
||||
// Verify all directory levels now exist
|
||||
const dirExists = await fs
|
||||
.access(newDir)
|
||||
.then(() => true)
|
||||
.catch(() => false)
|
||||
expect(dirExists).toBe(true)
|
||||
|
||||
// Verify file was written correctly
|
||||
const writtenData = await readJsonFile(filePath)
|
||||
expect(writtenData).toEqual(data)
|
||||
|
||||
// Check that no temp files remain
|
||||
const tempFiles = await listTempFiles(newDir, "new-file.json")
|
||||
expect(tempFiles.length).toBe(0)
|
||||
})
|
||||
|
||||
test("should handle directory creation permission errors", async () => {
|
||||
// Mock mkdir to simulate a permission error
|
||||
const mkdirSpy = jest.spyOn(fs, "mkdir")
|
||||
mkdirSpy.mockImplementationOnce(async () => {
|
||||
const permError = new Error("EACCES: permission denied") as NodeJS.ErrnoException
|
||||
permError.code = "EACCES"
|
||||
throw permError
|
||||
})
|
||||
|
||||
// Create test file path in a directory that will fail with permission error
|
||||
const nonExistentDir = path.join(tempTestDir, "permission-denied-dir")
|
||||
const filePath = path.join(nonExistentDir, "test-data.json")
|
||||
const testData = { message: "Should not be written due to permission error" }
|
||||
|
||||
// Expect the function to fail with the permission error
|
||||
await expect(safeWriteJson(filePath, testData)).rejects.toThrow(/EACCES/)
|
||||
|
||||
// Verify the file was not created
|
||||
const fileExists = await fs
|
||||
.access(filePath)
|
||||
.then(() => true)
|
||||
.catch(() => false)
|
||||
expect(fileExists).toBe(false)
|
||||
|
||||
mkdirSpy.mockRestore()
|
||||
})
|
||||
|
||||
test("should successfully write to a non-existent file in an existing directory", async () => {
|
||||
// Create directory but not the file
|
||||
const existingDir = path.join(tempTestDir, "existing-dir")
|
||||
await fs.mkdir(existingDir, { recursive: true })
|
||||
|
||||
const filePath = path.join(existingDir, "non-existent-file.json")
|
||||
const data = { message: "Creating new file" }
|
||||
|
||||
// Verify file doesn't exist before the operation
|
||||
const accessError = await fs.access(filePath).catch((e) => e)
|
||||
expect(accessError).toBeDefined()
|
||||
expect(accessError.code).toBe("ENOENT")
|
||||
|
||||
// safeWriteJson should automatically create the empty file for lock acquisition
|
||||
|
||||
// Write to the file
|
||||
await safeWriteJson(filePath, data)
|
||||
|
||||
// Verify file was created with correct content
|
||||
const writtenData = await readJsonFile(filePath)
|
||||
expect(writtenData).toEqual(data)
|
||||
|
||||
// Verify no temp files remain
|
||||
const tempFiles = await listTempFiles(existingDir, "non-existent-file.json")
|
||||
expect(tempFiles.length).toBe(0)
|
||||
})
|
||||
|
||||
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
|
||||
|
|
@ -259,10 +389,8 @@ describe("safeWriteJson", () => {
|
|||
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)
|
||||
})
|
||||
|
||||
|
|
@ -438,40 +566,26 @@ describe("safeWriteJson", () => {
|
|||
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)
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ import Stringer from "stream-json/Stringer"
|
|||
|
||||
/**
|
||||
* Safely writes JSON data to a file.
|
||||
* - Creates parent directories if they don't exist
|
||||
* - 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.
|
||||
|
|
@ -21,11 +22,27 @@ async function safeWriteJson(filePath: string, data: any): Promise<void> {
|
|||
const absoluteFilePath = path.resolve(filePath)
|
||||
let releaseLock = async () => {} // Initialized to a no-op
|
||||
|
||||
// For directory creation
|
||||
const dirPath = path.dirname(absoluteFilePath)
|
||||
|
||||
// Ensure directory structure exists with improved reliability
|
||||
try {
|
||||
// Create directory with recursive option
|
||||
await fs.mkdir(dirPath, { recursive: true })
|
||||
|
||||
// Verify directory exists after creation attempt
|
||||
await fs.access(dirPath)
|
||||
} catch (dirError: any) {
|
||||
console.error(`Failed to create or access directory for ${absoluteFilePath}:`, dirError)
|
||||
throw dirError
|
||||
}
|
||||
|
||||
// Acquire the lock before any file operations
|
||||
try {
|
||||
releaseLock = await lockfile.lock(absoluteFilePath, {
|
||||
stale: 31000, // Stale after 31 seconds
|
||||
update: 10000, // Update mtime every 10 seconds to prevent staleness if operation is long
|
||||
realpath: false, // the file may not exist yet, which is acceptable
|
||||
retries: {
|
||||
// Configuration for retrying lock acquisition
|
||||
retries: 5, // Number of retries after the initial attempt
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue