Skip to content

Conversation

@KJ7LNW
Copy link
Contributor

@KJ7LNW KJ7LNW commented May 21, 2025

Context

This PR introduces safeWriteJson utility and refactors all direct JSON file writes across the codebase to use it, preventing potential data corruption during write operations.

Implementation

The new safeWriteJson utility provides:

  • Atomic write operations using a temporary file approach
  • Backup creation before overwriting existing files
  • Proper error handling and cleanup of temporary files
  • Protection against concurrent writes to the same file
  • Support for custom JSON formatting with optional replacer and space arguments

All direct JSON file writes have been refactored to use this utility, including:

  • Configuration files
  • Settings export/import
  • Task persistence
  • File context tracking

This change ensures all JSON writes throughout the application are safe from corruption due to:

  • Application crashes during writes
  • Power failures
  • Concurrent modification attempts

Fixes: #722

How to Test

  • Run the test suites to verify all tests pass
  • Verify settings can be exported and imported correctly
  • Check that configuration files are properly saved

Get in Touch

Discord: KJ7LNW


Important

Introduces safeWriteJson utility for atomic JSON writes, replacing direct writes in key modules to enhance data integrity and error handling.

  • Behavior:
    • Introduces safeWriteJson utility for atomic JSON file writes with backup and error handling.
    • Replaces direct JSON writes with safeWriteJson in modelCache.ts, modelEndpointCache.ts, and importExport.ts.
    • Handles application crashes, power failures, and concurrent writes.
  • Testing:
    • Adds tests for safeWriteJson in safeWriteJson.test.ts to cover success and failure scenarios.
    • Mocks safeWriteJson in cache-manager.test.ts and McpHub.test.ts to verify integration.
  • Dependencies:
    • Adds proper-lockfile and stream-json to package.json for file locking and streaming.

This description was created by Ellipsis for 4c88c21. You can customize this summary. It will automatically update as commits are pushed.

@KJ7LNW KJ7LNW requested review from cte and mrubens as code owners May 21, 2025 03:27
@hannesrudolph hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap May 22, 2025
@hannesrudolph hannesrudolph moved this from PR [Needs Review] to TEMP in Roo Code Roadmap May 26, 2025
@daniel-lxs daniel-lxs moved this from TEMP to PR [Needs Review] in Roo Code Roadmap May 27, 2025
@KJ7LNW KJ7LNW self-assigned this May 27, 2025
@KJ7LNW KJ7LNW added the bug Something isn't working label May 27, 2025
@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented May 27, 2025

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 ..., null, 2 is the default so it is not passed, however those options are still available for pass-through.

Copy link
Member

@daniel-lxs daniel-lxs left a 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?

@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented May 28, 2025

Do you think we should remove the console logs? Could this implementation have high impact in performance?

  • logs: yes
  • errors: no - should errors throw, or just log? The problem with throwing is I do not know if these are handled further up the stack and I want this to be as resilient as possible (especially in the possible case that blank screen crashes are caused by on handled throws, but that is speculation).

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

Eric Wheeler added 8 commits June 2, 2025 20:21
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]>
@KJ7LNW KJ7LNW force-pushed the use-safe-write-json-for-all-files branch from 9d5c168 to 4c88c21 Compare June 3, 2025 03:29
@KJ7LNW KJ7LNW moved this from PR [Draft / In Progress] to PR [Needs Prelim Review] in Roo Code Roadmap Jun 3, 2025
@KJ7LNW KJ7LNW marked this pull request as ready for review June 3, 2025 03:30
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. enhancement New feature or request labels Jun 3, 2025
@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented Jun 3, 2025

@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) {
Copy link
Contributor

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.

Copy link
Member

@daniel-lxs daniel-lxs left a 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

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 4, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jun 4, 2025
Copy link
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@mrubens mrubens merged commit 1be30fc into RooCodeInc:main Jun 8, 2025
13 checks passed
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Jun 8, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 8, 2025
mrubens added a commit that referenced this pull request Jun 9, 2025
mrubens added a commit that referenced this pull request Jun 9, 2025
Revert "fix: use safeWriteJson for all JSON file writes (#3772)"

This reverts commit 1be30fc.
@KJ7LNW KJ7LNW mentioned this pull request Jun 15, 2025
@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented Jun 15, 2025

See also #4468

daniel-lxs pushed a commit that referenced this pull request Jun 21, 2025
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request lgtm This PR has been approved by a maintainer PR - Needs Review size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Task can't be opened

3 participants