mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
fix: resolve race condition in getStorageBasePath by using unique test file names
- Add unique suffix to .write_test file using process ID, timestamp, and random string - Prevents concurrent calls from conflicting on the same test file - Add comprehensive test coverage for storage.ts including concurrent call testing Fixes #7173
This commit is contained in:
parent
a8aea14078
commit
0eb5b3b67a
2 changed files with 180 additions and 1 deletions
177
src/utils/__tests__/storage.spec.ts
Normal file
177
src/utils/__tests__/storage.spec.ts
Normal file
|
|
@ -0,0 +1,177 @@
|
|||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
|
||||
import * as vscode from "vscode"
|
||||
import * as fs from "fs/promises"
|
||||
import * as path from "path"
|
||||
import { getStorageBasePath } from "../storage"
|
||||
|
||||
// Mock vscode module
|
||||
vi.mock("vscode", () => ({
|
||||
workspace: {
|
||||
getConfiguration: vi.fn(),
|
||||
},
|
||||
window: {
|
||||
showErrorMessage: vi.fn(),
|
||||
},
|
||||
}))
|
||||
|
||||
// Mock fs/promises module
|
||||
vi.mock("fs/promises", () => ({
|
||||
mkdir: vi.fn(),
|
||||
writeFile: vi.fn(),
|
||||
rm: vi.fn(),
|
||||
}))
|
||||
|
||||
describe("storage", () => {
|
||||
const mockFs = fs as any
|
||||
const mockVscode = vscode as any
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
describe("getStorageBasePath", () => {
|
||||
const defaultPath = "/default/storage/path"
|
||||
|
||||
it("should return default path when no custom path is configured", async () => {
|
||||
mockVscode.workspace.getConfiguration.mockReturnValue({
|
||||
get: vi.fn().mockReturnValue(""),
|
||||
})
|
||||
|
||||
const result = await getStorageBasePath(defaultPath)
|
||||
expect(result).toBe(defaultPath)
|
||||
})
|
||||
|
||||
it("should return default path when vscode configuration is not accessible", async () => {
|
||||
mockVscode.workspace.getConfiguration.mockImplementation(() => {
|
||||
throw new Error("VSCode not available")
|
||||
})
|
||||
|
||||
const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {})
|
||||
const result = await getStorageBasePath(defaultPath)
|
||||
|
||||
expect(result).toBe(defaultPath)
|
||||
expect(consoleSpy).toHaveBeenCalledWith("Could not access VSCode configuration - using default path")
|
||||
|
||||
consoleSpy.mockRestore()
|
||||
})
|
||||
|
||||
it("should return custom path when it is writable", async () => {
|
||||
const customPath = "/custom/storage/path"
|
||||
|
||||
mockVscode.workspace.getConfiguration.mockReturnValue({
|
||||
get: vi.fn().mockReturnValue(customPath),
|
||||
})
|
||||
|
||||
mockFs.mkdir.mockResolvedValue(undefined)
|
||||
mockFs.writeFile.mockResolvedValue(undefined)
|
||||
mockFs.rm.mockResolvedValue(undefined)
|
||||
|
||||
const result = await getStorageBasePath(defaultPath)
|
||||
|
||||
expect(result).toBe(customPath)
|
||||
expect(mockFs.mkdir).toHaveBeenCalledWith(customPath, { recursive: true })
|
||||
expect(mockFs.writeFile).toHaveBeenCalledWith(
|
||||
expect.stringMatching(/\.write_test_\d+_\d+_[a-z0-9]+$/),
|
||||
"test",
|
||||
)
|
||||
expect(mockFs.rm).toHaveBeenCalledWith(expect.stringMatching(/\.write_test_\d+_\d+_[a-z0-9]+$/))
|
||||
})
|
||||
|
||||
it("should return default path when custom path is not writable", async () => {
|
||||
const customPath = "/custom/storage/path"
|
||||
|
||||
mockVscode.workspace.getConfiguration.mockReturnValue({
|
||||
get: vi.fn().mockReturnValue(customPath),
|
||||
})
|
||||
|
||||
mockFs.mkdir.mockResolvedValue(undefined)
|
||||
mockFs.writeFile.mockRejectedValue(new Error("Permission denied"))
|
||||
|
||||
const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {})
|
||||
const result = await getStorageBasePath(defaultPath)
|
||||
|
||||
expect(result).toBe(defaultPath)
|
||||
expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining("Custom storage path is unusable"))
|
||||
expect(mockVscode.window.showErrorMessage).toHaveBeenCalled()
|
||||
|
||||
consoleSpy.mockRestore()
|
||||
})
|
||||
|
||||
it("should handle concurrent calls without race conditions", async () => {
|
||||
const customPath = "/custom/storage/path"
|
||||
|
||||
mockVscode.workspace.getConfiguration.mockReturnValue({
|
||||
get: vi.fn().mockReturnValue(customPath),
|
||||
})
|
||||
|
||||
mockFs.mkdir.mockResolvedValue(undefined)
|
||||
mockFs.writeFile.mockResolvedValue(undefined)
|
||||
mockFs.rm.mockResolvedValue(undefined)
|
||||
|
||||
// Simulate multiple concurrent calls
|
||||
const promises = Array.from({ length: 10 }, () => getStorageBasePath(defaultPath))
|
||||
const results = await Promise.all(promises)
|
||||
|
||||
// All should return the custom path
|
||||
results.forEach((result) => {
|
||||
expect(result).toBe(customPath)
|
||||
})
|
||||
|
||||
// Check that each call used a unique test file
|
||||
const writeFileCalls = mockFs.writeFile.mock.calls
|
||||
const testFilePaths = writeFileCalls.map((call: any[]) => call[0])
|
||||
const uniquePaths = new Set(testFilePaths)
|
||||
|
||||
// Each concurrent call should have used a unique test file
|
||||
expect(uniquePaths.size).toBe(10)
|
||||
|
||||
// All test files should have unique suffixes
|
||||
testFilePaths.forEach((filePath: string) => {
|
||||
expect(filePath).toMatch(/\.write_test_\d+_\d+_[a-z0-9]+$/)
|
||||
})
|
||||
})
|
||||
|
||||
it("should use unique test file names with process ID, timestamp, and random suffix", async () => {
|
||||
const customPath = "/custom/storage/path"
|
||||
|
||||
mockVscode.workspace.getConfiguration.mockReturnValue({
|
||||
get: vi.fn().mockReturnValue(customPath),
|
||||
})
|
||||
|
||||
mockFs.mkdir.mockResolvedValue(undefined)
|
||||
mockFs.writeFile.mockResolvedValue(undefined)
|
||||
mockFs.rm.mockResolvedValue(undefined)
|
||||
|
||||
const processId = process.pid
|
||||
const beforeTime = Date.now()
|
||||
|
||||
await getStorageBasePath(defaultPath)
|
||||
|
||||
const afterTime = Date.now()
|
||||
|
||||
const writeFileCall = mockFs.writeFile.mock.calls[0]
|
||||
const testFilePath = writeFileCall[0]
|
||||
|
||||
// Extract the suffix from the test file path
|
||||
const match = testFilePath.match(/\.write_test_(\d+)_(\d+)_([a-z0-9]+)$/)
|
||||
expect(match).toBeTruthy()
|
||||
|
||||
const [, pid, timestamp, random] = match
|
||||
|
||||
// Verify process ID
|
||||
expect(parseInt(pid)).toBe(processId)
|
||||
|
||||
// Verify timestamp is within expected range
|
||||
const ts = parseInt(timestamp)
|
||||
expect(ts).toBeGreaterThanOrEqual(beforeTime)
|
||||
expect(ts).toBeLessThanOrEqual(afterTime)
|
||||
|
||||
// Verify random suffix format
|
||||
expect(random).toMatch(/^[a-z0-9]{7}$/)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -33,7 +33,9 @@ export async function getStorageBasePath(defaultPath: string): Promise<string> {
|
|||
await fs.mkdir(customStoragePath, { recursive: true })
|
||||
|
||||
// Test if path is writable
|
||||
const testFile = path.join(customStoragePath, ".write_test")
|
||||
// Use unique suffix to prevent race conditions when multiple instances run simultaneously
|
||||
const uniqueSuffix = `${process.pid}_${Date.now()}_${Math.random().toString(36).substring(2, 9)}`
|
||||
const testFile = path.join(customStoragePath, `.write_test_${uniqueSuffix}`)
|
||||
await fs.writeFile(testFile, "test")
|
||||
await fs.rm(testFile)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue