Commit graph

3 commits

Author SHA1 Message Date
KJ7LNW
8455909809
fix: use safeWriteJson for all JSON file writes with race condition fix (#4733)
Co-authored-by: Eric Wheeler <roo-code@z.ewheeler.org>
Co-authored-by: Daniel Riccio <ricciodaniel98@gmail.com>
2025-06-25 16:05:02 -04:00
Matt Rubens
8d2eeda4ad
Revert "fix: use safeWriteJson for all JSON file writes" (#4471)
Revert "fix: use safeWriteJson for all JSON file writes (#3772)"

This reverts commit 1be30fc8c2.
2025-06-09 13:53:17 -04:00
KJ7LNW
1be30fc8c2
fix: use safeWriteJson for all JSON file writes (#3772)
* feat: Add safeWriteJson utility for atomic file operations

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 <roo-code@z.ewheeler.org>

* fix: use safeWriteJson for all JSON file writes

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 <roo-code@z.ewheeler.org>

* feat: Implement inter-process file locking for safeWriteJson

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 <roo-code@z.ewheeler.org>

* feat: implement streaming JSON write in safeWriteJson

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 <roo-code@z.ewheeler.org>

* fix: improve safeWriteJson locking mechanism

- 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 <roo-code@z.ewheeler.org>

* test: fix safeWriteJson test failures

- 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 <roo-code@z.ewheeler.org>

* test: update tests to work with safeWriteJson

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 <roo-code@z.ewheeler.org>

* refactor: replace JSON.stringify with safeWriteJson for file operations

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 <roo-code@z.ewheeler.org>

* docs: add rules for using safeWriteJson

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 <roo-code@z.ewheeler.org>

---------

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>
Co-authored-by: Eric Wheeler <roo-code@z.ewheeler.org>
Co-authored-by: Daniel <57051444+daniel-lxs@users.noreply.github.com>
2025-06-08 07:17:20 -07:00