Skip to content

[Fix] Completed task fails to resume after restart when a message write is interrupted - #2136

Draft
zoomote[bot] wants to merge 1 commit into
mainfrom
fix/ui-messages-write-interruption-2130
Draft

zoomote[bot] wants to merge 1 commit into
mainfrom
fix/ui-messages-write-interruption-2130

Conversation

@zoomote

@zoomote zoomote Bot commented Oct 21, 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: #2130

Description

safeWriteJson replaced a file with two renames: it renamed the target to a backup, then renamed the replacement into place. A process exit between the two renames left the canonical path absent. Readers then saw a permanent ENOENT, and no exception rollback could run after process termination. This reproduced 5/5 with subprocess fault injection in the issue, and it is the storage failure behind the restart-persistence CI failure in PR #2092.

This PR replaces the file with one atomic rename:

  1. The replacement is written to a temp file and flushed to storage.
  2. The temp file is renamed over the target. The committed target stays in place until the rename lands, so an interrupted write can no longer remove the canonical path.
  3. A single rename over a file that a reader holds open fails with EPERM on Windows. The two-step replacement from [Fix] Restart-persistence E2E flakes when the final ui_messages.json write lands after VS Code quits #2080 remains as the fallback for that case, so the documented Windows open-file behavior is preserved.
  4. A recovery pass runs at the start of every write, under the advisory lock. When the canonical path is absent, it restores the newest orphaned .bak_*.tmp backup. It then removes leftover .new_*.tmp and .bak_*.tmp files from interrupted writes. readFileWithMissingRetry stays unchanged, so the transient reader gap and the permanent absence after writer termination stay distinct failure modes.

The PR also implements the follow-ups from the issue:

  • saveTaskMessages emits trace points with the task ID and process ID for write start, backup rename, replacement rename, and completion. hasTaskUiMessageText traces the marker check result. deactivate traces shutdown. A sink registered on activation mirrors every line into the Zoo Code output channel and the console.
  • runTest.ts preserves a failed restart-persistence scenario instead of deleting it. Phase results, VS Code logs, and global storage task files are copied to apps/vscode-e2e/test-failures/, and the e2e workflow uploads them as the restart-persistence-failure artifact on failure.

Shutdown coordination was evaluated and not needed: the single-rename commit removes the crash window on POSIX, so no stop-the-world drain is required. The EPERM fallback keeps the Windows gap, and the recovery pass heals it on the next write.

Reviewers may want to check two things with care: the recovery pass garbage-collects any matching temp files while holding the lock, which assumes the existing advisory-lock model of safeWriteJson; and the EPERM fallback reports the fallback's replacement error when both the commit and the fallback fail.

Test Procedure

Deterministic fault injection, unit level:

  • src/utils/__tests__/safeWriteJson.interruption.test.ts spawns a real Node child process that bundles this repository's safeWriteJson and wraps fs/promises.rename. The child exits with code 0 before the rename whose destination is the canonical path, which is exactly the reported crash window on the old protocol. The test then reads the canonical path with readFileWithMissingRetry and requires the previous committed content, and performs a restart recovery write that must succeed and consume the leftovers. It runs 3 times per invocation. On the old implementation the read fails with ENOENT 3/3; on this branch it passes 18/18 across repeated invocations.
  • Unit tests cover the single-rename commit, the EPERM fallback success and rollback paths, backup restore when the canonical path is missing, garbage collection when it exists, and the trace event sequences.

Commands run:

  • pnpm --dir src run check-types: pass.
  • pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 on the changed files: clean, no suppression count changes.
  • pnpm --dir src exec vitest run core/task-persistence utils/__tests__/safeWriteJson.test.ts utils/__tests__/safeWriteJson.interruption.test.ts extension/__tests__/api-has-task-ui-message-text.spec.ts: 15 files, 230 tests pass.
  • pnpm test from the repository root: 13/13 turbo tasks pass. One pre-existing extension.spec.ts flake appeared in the first full run and the file passes in isolation and on rerun.
  • pnpm lifecycle:model-check: pass.
  • Mocked restart-persistence E2E (TEST_FILE=restart-persistence.test USE_MOCK=true xvfb-run -a node ./out/runTest.js): create and verify phases pass, 3 complete runs, with the new trace lines visible in the coordinator output.
  • pnpm --filter @roo-code/vscode-e2e check-types and lint: pass.

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 the file replacement protocol, trace logging, tests, and test infrastructure.

Videos (interaction / animation only)

Not applicable.

Documentation Updates

  • No documentation updates are required.

The Zoo Code docs repository documents user-facing features only. It has no coverage of persistence internals, restart recovery, or test-failure artifact handling, and this PR changes no user-facing behavior, setting, or command.

Additional Notes

Related context: #2080 (previous persistence fix and the Windows EPERM probe) and #2092 (the CI run that exposed this failure). The restart-persistence failure mode from the issue is covered at three levels: the subprocess fault-injection regression test, the unit recovery tests, and the mocked restart-persistence E2E.

Get in Touch

0x7777777_ (Discord)

safeWriteJson replaced a file with two renames (target to backup, then
replacement to target). A process exit between the renames left the
canonical path absent, so readers saw a permanent ENOENT that no retry
could repair.

The replacement is now committed with a single atomic rename. Only a
Windows-style EPERM (target held open by a reader) falls back to the
two-step sequence, and a recovery pass under the advisory lock restores
the newest orphaned backup and removes leftover temp files before any
write. Task-message saves emit trace points with the task ID and
process ID, the marker check and shutdown are traced, and a failed
restart-persistence scenario now preserves phase results, VS Code logs,
and task files for CI artifact upload.
@coderabbitai

coderabbitai Bot commented Oct 21, 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: bd99bd59-633f-4164-baf3-336f73cbeb14

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.

@codecov

codecov Bot commented Oct 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/utils/safeWriteJson.ts 89.09% 5 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@github-actions

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.

This branch has not been deployed

No deployments
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.

Restart persistence still fails after #2080: ui_messages.json missing after successful create-phase disk check

1 participant