Repository navigation
[Fix] Restart-persistence E2E flakes when the final ui_messages.json write lands after VS Code quits - #2080
Conversation
…-host restart - TaskMessagesReadError now includes the underlying error detail (fs code or error name/message) so flakes are diagnosable as missing vs unreadable vs half-written. - readFileWithMissingRetry retries ENOENT up to 4 times with a jittered 10-100ms delay instead of a single 1-10ms retry, covering late writes. - The restart-persistence E2E create phase now waits until the completion marker is durable in ui_messages.json (via the new test-only hasTaskUiMessageText API) before quitting VS Code. - Add deterministic fault-injection tests for late-appearing files, exhausted retries, torn/truncated JSON, and forced write failures. Fixes #2078
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one): 1
Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review statusThis PR was opened by an automated account. A human maintainer must verify the change intent, provenance, and validation before merging. Current step: Mark the PR ready. Required CI must pass before CodeRabbit starts. Review-state labels are managed by this workflow; do not edit them manually. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Related GitHub Issue
Closes: #2078
Description
The mocked restart-persistence E2E test failed about 1 in 100 runs. The
createphase quit VS Code right afterTaskCompleted. The finalsaveClineMessageswrite could still be in flight. Theverifyphase then read aui_messages.jsonthat was not on disk yet, andTaskMessagesReadErrorhid the real cause inoriginalError.This PR makes two changes from the issue:
TaskMessagesReadErrormessages now include the underlying error detail. A newformatErrorDetailhelper appends the filesystem error code (for example(code: ENOENT)) or the error name and message (for example(SyntaxError: ...)). Future flakes show missing, unreadable, or half-written files in the logs.createphase now waits untilui_messages.jsoncontains the completion marker before it quits VS Code. It uses a new test-only API method,hasTaskUiMessageText(taskId, text). The method first checks that the task is in history withgetTaskWithId, as the sibling API methods do. An unknown task ID returnsfalseand does not touch the disk. The previous wait only checked the API conversation history, which lands on disk earlier.Retry window: not changed. Issue #2078 item 3 proposed a wider ENOENT retry. This PR does not widen it. The E2E wait removes the cause of the flake, and the issue says to re-check the window only if the flake remains.
readFileWithMissingRetrykeeps its single 1-10 ms retry. A new comment states why it exists.safeWriteJsonreplaces a file in two renames: target to backup, then new file to target. A reader can see ENOENT between the two renames.A single rename is not a safe fix for that gap. A probe on Windows 11 (Node 24, Defender on) showed that a single
fs.renameover a file that a reader holds open fails withEPERM. This happened in the temp folder, the Documents folder, and OneDrive. The two-step rename always succeeded on Windows. On Linux, the single rename gave no reader errors. ChangingsafeWriteJsonto a platform-specific design is a separate change.Reviewers should check with care:
hasTaskUiMessageTextis on the publicRooCodeAPIinterface, likehasTaskApiConversationHistorySequence. It is marked "Intended for use in tests only".partial. Thecreatephase has already seen the completed message in memory before it waits.Test Procedure
Deterministic fault-injection tests cover the error paths:
(code: ENOENT)andoriginalErroris the original error.kind: "io_error"and(code: EACCES), after one read.kind: "invalid"and theSyntaxErrordetail.saveTaskMessagesrejects with the original coded error (for exampleENOSPC).formatErrorDetail: tests for the code,Error, and non-error branches.hasTaskUiMessageText(api-has-task-ui-message-text.spec.ts): match found, no match, match in a later message after a message with notext, only messages with notext, all threeTaskMessagesReadErrorkinds returnfalse, and an unknown task ID returnsfalsewithout a disk read.Commands run:
pnpm --dir src exec vitest run core/task-persistence/__tests__ extension/__tests__/api-has-task-ui-message-text.spec.ts: 12 files, 197 tests pass.pnpm --dir src run check-types: pass.pnpm --dir src exec eslint --max-warnings=0on the changed files: clean.xvfb-run -a env TEST_FILE=restart-persistence.test USE_MOCK=true pnpm --filter @roo-code/vscode-e2e test:run. Thecreateandverifyphases both pass. I ran it before the retry window was reverted. The retry code it uses is the same code as onmain.Pre-Submission Checklist
Visual Snapshots
Not applicable. This PR changes no rendered UI. It touches task persistence error messages, a test-only API method, and tests.
Videos (interaction / animation only)
Not applicable.
Documentation Updates
This is an internal persistence and test fix. It changes no user-facing behavior, setting, or command. The only new API surface is
hasTaskUiMessageText, which is marked "Intended for use in tests only".Additional Notes
Related issue #1540 (partial
TaskHistoryStore.atomicUpdatePairwrites) is untouched. It covers a different file.Get in Touch
0x7777777_ (Discord)