2 of 3: corruption fix: Implement atomic read-modify-write for JSON files

[KJ7LNW]

Dependencies

When dependent pull requests have been merged, Github will show more correct line number count

• Depends on 1 of 3: memfix: implement and use safeReadJson to reduce memory usage #5332
• Closes:
  • Bug: Race condition when cancelling task can corrupt conversation history #5333
  • User Input Lost When Clicking “Reject” or “Send” in AI Workflow #4926
• Blocks:
  • 3 of 3: scalability fix: refactor task history persistence to use file-based storage #3785
  • feat: Implement task history scanner and recovery tools #5546

Note to Reviewer

This is a PR series, so the line numbers shown by Github are exaggerated. The commit series clearly marks where each PR begins using lines that say NOTICE: PR ____ STARTS HERE. See below for the annotated diffstat.

The commits tell a clean story, it will be easier to understand what is happening here by looking at each commit individually under "Commits" than by looking at all of the files that were changed.

The changes were kept minimal with detailed human inspection focusing on transactional safety without changing the original behavior.

This is probably the best place to start your review:

• modifyConversation( async modifyFn(clineMessages, apiHistory) )

This function and the other two modify* functions perform an atomic mutation to synchronize Task.* structures with the file on disk.

Use "Hide Whitespace"

Due to significant indentation changes from moving logic into atomic read-modify-write transaction callbacks, using the "hide whitespace" option in the GitHub diff viewer will help focus on the logical changes.

Actual diff excluding tests:

]$ git diff -w --stat  github/stream-json-reads..github/json-read-modify-write -- ':(exclude)**/__tests__/**'  ':(exclude)**/i18n/**'  
// actual transactional implementation:
 src/utils/safeWriteJson.ts                |  69 +++++-

// transactional modify* functions replace the save/overwrite* functions,
// plus hooks for existing use into those functions:
 src/core/task/Task.ts                     | 300 ++++++++++++++++++---------

// moved to rules/ because it is needed by Code, architect, issue fixer, and possibly other modes:
 .roo/rules-code/use-safeWriteJson.md      |   6 -
 .roo/rules/use-safeWriteJson.md           |  11 +

// small changes to hook in the changes above 
 src/core/checkpoints/index.ts             |   8 +-
 src/core/sliding-window/index.ts          |   2 +-
 src/core/task-persistence/apiMessages.ts  |  14 --
 src/core/task-persistence/index.ts        |   3 +-
 src/core/task-persistence/taskMessages.ts |  42 ----
 src/core/webview/webviewMessageHandler.ts | 105 +++++-----
 10 files changed, 345 insertions(+), 215 deletions(-)

Key Changes

This PR enhances JSON file handling by introducing atomic read-modify-write transactions, which prevents race conditions and improves data integrity when modifying files concurrently.

• safeWriteJson.ts is enhanced to support atomic read-modify-write transactions by accepting a readModifyFn parameter.
• The Task.ts class is refactored to introduce modifyConversation, modifyApiConversationHistory, and modifyClineMessages methods, which use the new atomic pattern to safely update conversation history files.
• All Task.ts methods that mutate the conversation state, including say, ask, recursivelyMakeClineRequests, updateApiReqMsg, resumeTaskFromHistory, condenseContext, and attemptApiRequest, are refactored to use these atomic transaction methods.
• Redundant, non-atomic persistence functions like saveApiMessages and saveTaskMessages are removed, centralizing all history mutations within the new atomic methods.
• The test suites for ClineProvider and Task are updated to properly mock and test the new transactional behavior.

---

Important

Enhances JSON file handling with atomic read-modify-write transactions, refactors Task.ts for atomic operations, and updates tests for new functionality.

• Behavior:
  • Introduces atomic read-modify-write transactions in safeWriteJson.ts and safeReadJson.ts to prevent race conditions and improve data integrity.
  • Refactors Task.ts to use new atomic methods for conversation history updates, replacing non-atomic persistence functions.
  • Updates modifyConversation, modifyApiConversationHistory, and modifyClineMessages methods in Task.ts.
• Tests:
  • Updates test files to mock and test new transactional behavior, including safeReadJson.spec.ts and safeWriteJson.test.ts.
  • Adds tests for atomic transactions and error handling in safeWriteJson.test.ts.
• Misc:
  • Removes redundant functions like saveApiMessages and saveTaskMessages from task-persistence.
  • Updates MdmService.ts and related tests to use safeReadJson for configuration loading.

^{This description was created by for 38eba3ab658d9df6b0a3b9a5b00ec290e41e87ba. You can customize this summary. It will automatically update as commits are pushed.}

[KJ7LNW]

rebased on main, ready to review.

[KJ7LNW]

rebased on v3.23.14

[daniel-lxs]

Hey @KJ7LNW I took a look at your implementation and it looks solid, I left a couple of minor points that might be worth taking a look at.

Let me know what you think.

[daniel-lxs]

The modifyConversation method attempts to acquire locks on two different files through nested calls to modifyClineMessages and modifyApiConversationHistory. This pattern could lead to:

1. Deadlocks if another process tries to acquire these locks in reverse order
2. Data inconsistency if one modification succeeds but the other fails

Is this intentional? The proper-lockfile documentation recommends against holding multiple locks simultaneously. Consider either:

• Using a single lock file for both operations
• Implementing a two-phase commit pattern
• Documenting why this approach is safe in your specific use case

[KJ7LNW]

Deadlocks if another process tries to acquire these locks in reverse order

I am aware of that, this is why are there is a function to guarantee ordering.

FWIW, I have been running this pull request on my desk in my personal build for a week or two and have never had a deadlock. Additionally, I have not had any task corruption since running this PR series.

Data inconsistency if one modification succeeds but the other fails

I gave a specific issues serious consideration during development (because multi-file would makes the code very complicated and increases the risk of error) and decided on the compromise that multi-file transactions guarantee the following:

1. both files are locked, in order, before proceeding
2. the transform function completes atomically: if it aborts early, then neither file is written and both locks are released in a proper "stack-order"
3. the only possibility for corruption is if the actual file write fails, which is unlikely unless the user runs out of disk space or something but then all bets are off

no matter how you slice it, this is 1000x better than the current implementation.

[daniel-lxs]

The error handling strategy differs between safeReadJson and safeWriteJson:

• safeReadJson: Throws all errors including ENOENT
• safeWriteJson: Returns undefined for ENOENT when using readModifyFn

This inconsistency could confuse API consumers. Consider standardizing the approach:

// Option 1: Always throw
try {
  const data = await safeReadJson(path);
} catch (error) {
  if (error.code === 'ENOENT') {
    // Handle missing file
  }
}

// Option 2: Return undefined for ENOENT
const data = await safeReadJson(path) ?? defaultValue;

Which pattern better serves your use cases?

[KJ7LNW]

probably both should return undefined; it is simpler to implement and more intuitive

[daniel-lxs]

The readModifyFn pattern seems to load the entire JSON into memory before modification. For large conversation histories or task data, this could cause:

1. Out-of-memory errors
2. Performance degradation
3. GC pressure

Have you considered the maximum expected file sizes? For large files, you might want to:

• Implement streaming modifications for specific use cases
• Add file size checks with warnings
• Document memory requirements
• Consider pagination for conversation history

[KJ7LNW]

I considered that too, but the implementation becomes too complicated for a first pass.

[daniel-lxs]

While the test suite is comprehensive for single-process scenarios, it lacks tests for concurrent access - which is the primary purpose of these utilities. Consider adding tests that:

1. Simulate multiple processes trying to write simultaneously
2. Test lock acquisition/release under race conditions
3. Verify data integrity with concurrent read-modify-write operations
4. Test behavior when locks become stale

Example approach:

test('should handle concurrent writes safely', async () => {
  const promises = Array(5).fill(0).map((_, i) => 
    safeWriteJson(filePath, {}, async (data) => {
      data[`process${i}`] = true;
      await delay(Math.random() * 100);
      return data;
    })
  );
  
  await Promise.all(promises);
  const result = await safeReadJson(filePath);
  expect(Object.keys(result)).toHaveLength(5);
});

[KJ7LNW]

that seems reasonable

[KJ7LNW]

@daniel-lxs I do not have the bandwidth to work on this right now so feel free to push anything that you would like to address and I can review the change if you would like.

[daniel-lxs]

Thanks for the contribution. At this stage, the impact of these changes isn’t clear and we’re focusing on higher-priority items. Closing for now, but we can reconsider in the future if priorities shift.