Repository navigation
[Fix] Completed task fails to resume after restart when a message write is interrupted - #2136
zoomote[bot] wants to merge 1 commit into
Conversation
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.
|
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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. |
Related GitHub Issue
Closes: #2130
Description
safeWriteJsonreplaced 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 permanentENOENT, 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:
EPERMon 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..bak_*.tmpbackup. It then removes leftover.new_*.tmpand.bak_*.tmpfiles from interrupted writes.readFileWithMissingRetrystays 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:
saveTaskMessagesemits trace points with the task ID and process ID for write start, backup rename, replacement rename, and completion.hasTaskUiMessageTexttraces the marker check result.deactivatetraces shutdown. A sink registered on activation mirrors every line into the Zoo Code output channel and the console.runTest.tspreserves a failed restart-persistence scenario instead of deleting it. Phase results, VS Code logs, and global storage task files are copied toapps/vscode-e2e/test-failures/, and the e2e workflow uploads them as therestart-persistence-failureartifact 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.tsspawns a real Node child process that bundles this repository'ssafeWriteJsonand wrapsfs/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 withreadFileWithMissingRetryand 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 withENOENT3/3; on this branch it passes 18/18 across repeated invocations.Commands run:
pnpm --dir src run check-types: pass.pnpm --dir src exec eslint --prune-suppressions --max-warnings=0on 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 testfrom the repository root: 13/13 turbo tasks pass. One pre-existingextension.spec.tsflake appeared in the first full run and the file passes in isolation and on rerun.pnpm lifecycle:model-check: pass.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-typesandlint: pass.Pre-Submission Checklist
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
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
EPERMprobe) 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)