fix: eliminate getStorageBasePath race condition and vscode popup #7174
Conversation
dosubot
bot
added
size:M
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for your contribution! This PR effectively addresses the race condition issue in getStorageBasePath(). The fix is clean, minimal, and well-tested. I've left a few minor suggestions for potential improvements, but the current implementation looks solid and ready to merge.
|
|
||
| // Test if path is writable | ||
| const testFile = path.join(customStoragePath, ".write_test") | ||
| // Test if path is writable (use unique filename to avoid race conditions) |
There was a problem hiding this comment.
Great fix! Consider making the comment slightly more detailed to explain why we need the unique suffix, e.g.:
| // Test if path is writable | ||
| const testFile = path.join(customStoragePath, ".write_test") | ||
| // Test if path is writable (use unique filename to avoid race conditions) | ||
| const uniqueSuffix = `${Date.now()}-${Math.random().toString(36).slice(2, 11)}` |
There was a problem hiding this comment.
The unique suffix generation pattern works well. As a minor enhancement for future consideration, this could be extracted to a small utility function if similar unique ID generation is needed elsewhere in the codebase. But for a single use case, the current implementation is perfectly fine.
| vi.restoreAllMocks() | ||
| }) | ||
|
|
||
| it("should handle concurrent storage validations without race conditions", async () => { |
There was a problem hiding this comment.
Excellent test coverage for the race condition scenario! For future enhancements, consider adding test cases for:
- Custom path becoming inaccessible during runtime
- Invalid/non-writable custom path handling
- Fallback to default path when custom path fails
These would provide even more comprehensive coverage of edge cases.
hannesrudolph
added
the
Issue/PR - Triage
|
Closing in favor of #7164 |
3 participants
Related GitHub Issue
Closes: # 7173
Roo Code Task Context (Optional)
N/A
Description
When using vscode setting
roo-cline.customStoragePaththere is a race condition in getStorageBasePath().Update getStorageBasePath() to append a random suffix to
.write_testfile to prevent a race condition where concurrent operations want to read or remove the file.add a test to verify functionality.
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
n/a
Documentation Updates
Does this PR necessitate updates to user-facing documentation?
Additional Notes
n/a
Get in Touch
jim.weller
Important
Fix race condition in
getStorageBasePathby appending unique suffix to test file, ensuring concurrent operations do not conflict.getStorageBasePathinstorage.tsby appending unique suffix to.write_testfile.storage.test.tsto verify concurrent storage validations handle race conditions correctly.This description was created by
for 6f3f6eb. You can customize this summary. It will automatically update as commits are pushed.