-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: use safeWriteJson for all JSON file writes #3772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: use safeWriteJson for all JSON file writes #3772
Conversation
|
Note to reviewer: src/utils/safeWriteJson.ts is the primary implementation. everything else is just testing and simple 1-line replacements like this to hook it into place: if (!exists) {
- await fs.writeFile(mcpPath, JSON.stringify({ mcpServers: {} }, null, 2))
+ await safeWriteJson(mcpPath, { mcpServers: {} })
}Note that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, @KJ7LNW, thank you for the contribution!. Do you think we should remove the console logs? Could this implementation have high impact in performance?
My preference is always to throw errors, but not at the cost of crashing the application especially if the error is transient because the next write will successfully complete because this implementation is idempotent . In any event the errors should also include back traces, so we can figure out the cause if it happens I am switching to draft mode until logs are cleaned up |
75fbed2 to
168672a
Compare
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 <[email protected]>
This change refactors all direct JSON file writes to use the safeWriteJson utility, which implements atomic file writes to prevent data corruption during write operations. - Modified safeWriteJson to accept optional replacer and space arguments - Updated tests to verify correct behavior with the new implementation Fixes: #722 Signed-off-by: Eric Wheeler <[email protected]>
Replaces the previous in-memory lock in `safeWriteJson` with
`proper-lockfile` to provide robust, cross-process advisory file
locking. This enhances safety when multiple processes might attempt
concurrent writes to the same JSON file.
Key changes:
- Added `proper-lockfile` and `@types/proper-lockfile` dependencies.
- `safeWriteJson` now uses `proper-lockfile.lock()` with configured
retries, staleness checks (31s), and lock update intervals (10s).
- An `onCompromised` handler is included to manage scenarios where
the lock state is unexpectedly altered.
- Logging and comments within `safeWriteJson` have been refined for
clarity, ensuring error logs include backtraces.
- The test suite `safeWriteJson.test.ts` has been significantly
updated to:
- Use real timers (`jest.useRealTimers()`).
- Employ a more comprehensive mock for `fs/promises`.
- Correctly manage file pre-existence for various scenarios.
- Simulate lock contention by mocking `proper-lockfile.lock()`
using `jest.doMock` and a dynamic require for the SUT.
- Verify lock release by checking for the absence of the `.lock`
file.
All tests are passing with these changes.
Signed-off-by: Eric Wheeler <[email protected]>
Refactor safeWriteJson to use stream-json for memory-efficient JSON serialization: - Replace in-memory string creation with streaming pipeline - Add Disassembler and Stringer from stream-json library - Extract streaming logic to a dedicated helper function - Add proper-lockfile and stream-json dependencies This implementation reduces memory usage when writing large JSON objects. Signed-off-by: Eric Wheeler <[email protected]>
- Use file path itself for locking instead of separate lock file - Improve error handling and clarity of code - Enhance cleanup of temporary files Signed-off-by: Eric Wheeler <[email protected]>
- Ensure test file exists before locking - Add proper mocking for fs.createWriteStream - Fix test assertions to match expected behavior - Improve test comments to follow project guidelines Signed-off-by: Eric Wheeler <[email protected]>
Updated tests to work with safeWriteJson instead of direct fs.writeFile calls: - Updated importExport.test.ts to expect safeWriteJson calls instead of fs.writeFile - Fixed McpHub.test.ts by properly mocking fs/promises module: - Moved jest.mock() to the top of the file before any imports - Added mock implementations for all fs functions used by safeWriteJson - Updated the test setup to work with the mocked fs module All tests now pass successfully. Signed-off-by: Eric Wheeler <[email protected]>
Replace all non-test instances of JSON.stringify used for writing to JSON files with safeWriteJson to ensure safer file operations with proper locking, error handling, and atomic writes. - Updated src/services/mcp/McpHub.ts - Updated src/services/code-index/cache-manager.ts - Updated src/api/providers/fetchers/modelEndpointCache.ts - Updated src/api/providers/fetchers/modelCache.ts - Updated tests to match the new implementation Signed-off-by: Eric Wheeler <[email protected]>
Add concise rules for using safeWriteJson instead of JSON.stringify with file operations to ensure atomic writes and prevent data corruption. Signed-off-by: Eric Wheeler <[email protected]>
9d5c168 to
4c88c21
Compare
|
@daniel-lxs I have applied your suggestions, rebased, and all tests are passing. |
| const backupFileToRollbackOrCleanupWithinCatch = actualTempBackupFilePath | ||
|
|
||
| // Attempt rollback if a backup was made | ||
| if (backupFileToRollbackOrCleanupWithinCatch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rollback error handling block is comprehensive but complex; splitting into helper functions may improve readability and testability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the suggestions were addressed, it might be worth considering if the new rule should be added or not.
LGTM
mrubens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
See also #4468 |
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 <[email protected]>
Context
This PR introduces
safeWriteJsonutility and refactors all direct JSON file writes across the codebase to use it, preventing potential data corruption during write operations.Implementation
The new
safeWriteJsonutility provides:replacerandspaceargumentsAll direct JSON file writes have been refactored to use this utility, including:
This change ensures all JSON writes throughout the application are safe from corruption due to:
Fixes: #722
How to Test
Get in Touch
Discord: KJ7LNW
Important
Introduces
safeWriteJsonutility for atomic JSON writes, replacing direct writes in key modules to enhance data integrity and error handling.safeWriteJsonutility for atomic JSON file writes with backup and error handling.safeWriteJsoninmodelCache.ts,modelEndpointCache.ts, andimportExport.ts.safeWriteJsoninsafeWriteJson.test.tsto cover success and failure scenarios.safeWriteJsonincache-manager.test.tsandMcpHub.test.tsto verify integration.proper-lockfileandstream-jsontopackage.jsonfor file locking and streaming.This description was created by
for 4c88c21. You can customize this summary. It will automatically update as commits are pushed.