fix: resolve race condition in getStorageBasePath (#7173) #7175
Conversation
…t 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
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| 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) |
There was a problem hiding this comment.
Could we consider wrapping the cleanup in a try-finally block? If fs.rm(testFile) fails, the test file will remain on disk. While the unique naming prevents conflicts, orphaned test files could accumulate over time:
|
|
||
| // Test if path is writable | ||
| const testFile = path.join(customStoragePath, ".write_test") | ||
| // Use unique suffix to prevent race conditions when multiple instances run simultaneously |
There was a problem hiding this comment.
The comment could be more specific about what "race conditions" means here. Perhaps: "Use unique suffix to prevent file conflicts when multiple VSCode instances or concurrent operations access the same custom storage path simultaneously"?
| expect(mockVscode.window.showErrorMessage).toHaveBeenCalled() | ||
|
|
||
| consoleSpy.mockRestore() | ||
| }) |
There was a problem hiding this comment.
Is it worth adding a test case for when fs.rm() fails after successful write? This would verify the function still returns the custom path despite cleanup failure, which seems to be the intended behavior.
| consoleSpy.mockRestore() | ||
| }) | ||
|
|
||
| it("should handle concurrent calls without race conditions", async () => { |
There was a problem hiding this comment.
Would it make sense to group the concurrent/race condition tests into a nested describe block like describe("concurrent access handling") for better test organization?
hannesrudolph
added
the
Issue/PR - Triage
|
Closing in favor of #7164 |
Summary
This PR fixes the race condition in
getStorageBasePath()that was causing intermittent errors when multiple instances tried to write to the same.write_testfile simultaneously.Problem
When multiple instances of the extension run concurrently, they all attempt to create and delete the same
.write_testfile to verify write permissions on the custom storage path. This causes a race condition where one instance might delete the file while another is trying to write to it, resulting in the error:Solution
The fix adds a unique suffix to the
.write_testfile name using:process.pid)Date.now())This ensures each concurrent call uses a unique test file, preventing conflicts.
Changes
Modified
src/utils/storage.ts:.write_testto.write_test_${uniqueSuffix}Added
src/utils/__tests__/storage.spec.ts:getStorageBasePath()Testing
Fixes #7173
Important
Fixes race condition in
getStorageBasePath()by using unique test file names, with tests added instorage.spec.ts.getStorageBasePath()instorage.tsby appending a unique suffix to.write_testfile names.storage.spec.tswith tests forgetStorageBasePath().storage.tsexplaining the race condition fix.This description was created by
for 0eb5b3b. You can customize this summary. It will automatically update as commits are pushed.