Skip to content

[Fix] Restart-persistence E2E flakes when the final ui_messages.json write lands after VS Code quits - #2080

Merged
edelauna merged 2 commits into
mainfrom
fix/restart-persistence-flake-1eanwy7dl17iy
Oct 17, 2026
Merged

edelauna merged 2 commits into
mainfrom
fix/restart-persistence-flake-1eanwy7dl17iy

Conversation

@zoomote

@zoomote zoomote Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

​Created by Roomote. Follow up by mentioning @roomote, in the web UI, or in Discord.

Related GitHub Issue

Closes: #2078

Description

The mocked restart-persistence E2E test failed about 1 in 100 runs. The create phase quit VS Code right after TaskCompleted. The final saveClineMessages write could still be in flight. The verify phase then read a ui_messages.json that was not on disk yet, and TaskMessagesReadError hid the real cause in originalError.

This PR makes two changes from the issue:

  1. Diagnosable errors. TaskMessagesReadError messages now include the underlying error detail. A new formatErrorDetail helper 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.
  2. Durable create phase. The E2E create phase now waits until ui_messages.json contains 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 with getTaskWithId, as the sibling API methods do. An unknown task ID returns false and 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. readFileWithMissingRetry keeps its single 1-10 ms retry. A new comment states why it exists. safeWriteJson replaces 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.rename over a file that a reader holds open fails with EPERM. 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. Changing safeWriteJson to a platform-specific design is a separate change.

Reviewers should check with care:

  • hasTaskUiMessageText is on the public RooCodeAPI interface, like hasTaskApiConversationHistorySequence. It is marked "Intended for use in tests only".
  • The method does not check partial. The create phase has already seen the completed message in memory before it waits.

Test Procedure

Deterministic fault-injection tests cover the error paths:

  • Retries exhausted: the error message contains (code: ENOENT) and originalError is the original error.
  • Non-ENOENT failure: rejects at once with kind: "io_error" and (code: EACCES), after one read.
  • Torn write: a truncated JSON file rejects with kind: "invalid" and the SyntaxError detail.
  • Forced write failure: saveTaskMessages rejects with the original coded error (for example ENOSPC).
  • 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 no text, only messages with no text, all three TaskMessagesReadError kinds return false, and an unknown task ID returns false without 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=0 on the changed files: clean.
  • Mocked restart-persistence E2E: xvfb-run -a env TEST_FILE=restart-persistence.test USE_MOCK=true pnpm --filter @roo-code/vscode-e2e test:run. The create and verify phases both pass. I ran it before the retry window was reverted. The retry code it uses is the same code as on main.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I reviewed my own code with care.
  • Writing Style: The PR title and description use Simplified Technical English (ASD-STE100): short sentences, plain words, active voice, one idea per sentence, no contractions.
  • Testing: New and/or updated tests have been added to cover my changes (if applicable).
  • Visual Snapshot (UI changes only): Not applicable. This PR changes no rendered UI.
  • Documentation Impact: I checked if my changes need documentation updates (see "Documentation Updates" section below).
  • Contribution Guidelines: I read and agree to the Contributor Guidelines.

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

  • No documentation updates are required.

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.atomicUpdatePair writes) is untouched. It covers a different file.

Get in Touch

0x7777777_ (Discord)

…-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
@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one): 1
  • coderabbit-review-active

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository: Zoo-Code-Org/Zoo-Code/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: baffe546-b02e-4a09-8aa2-9fccb1a891cb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Review status

This 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. community-approved is managed the same way — do not add or remove it manually. It signals a fresh community code approval for the current head as an advisory priority only; maintainer review is still required.

@codecov

codecov Bot commented Oct 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@edelauna
edelauna marked this pull request as ready for review October 17, 2026 22:56
@edelauna
edelauna enabled auto-merge October 17, 2026 22:57
@edelauna
edelauna added this pull request to the merge queue Oct 17, 2026
Merged via the queue into main with commit 289538e Oct 17, 2026
32 checks passed
@edelauna
edelauna deleted the fix/restart-persistence-flake-1eanwy7dl17iy branch October 17, 2026 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Flaky restart-persistence E2E: TaskMessagesReadError on ui_messages.json after restart

2 participants