[zoomote2]

​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.

After the rebase onto current main, the changed files were revalidated: pnpm --dir src run check-types, eslint on the changed files, and the focused persistence suites (15 files, 231 tests) all pass. The pre-push hook (full workspace typecheck and lint) passed on push.

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

This PR recreates #2136, which can no longer be merged because its original author account was deleted. The fix commit is identical in content (2d8fe7feb), rebased onto current main.

Merge-conflict resolution during the rebase: main tightened lint in safeWriteJson.ts (the old catch (accessError: any) became an unknown type guard, from #2280). The conflict was resolved by keeping this PR's single-rename protocol and adopting the new lint style: the EPERM fallback catch is now catch (renameError: unknown) with a type guard. .gitignore kept both entries (heap-results/ from main, test-failures/ from this PR).

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)

[coderabbitai]

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

• Source mode: Auto
• Repository: Zoo-Code-Org/Zoo-Code
• Branch: main
• File: .coderabbit.yaml
• Commit: 70eba96
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: d13e59fb-5975-49e0-9d8c-14196df855dd

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

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

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

• X
• Mastodon
• Reddit
• LinkedIn

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

[zoomote2]

1 issue outstanding: the new commits fixed the three earlier recovery/flush findings, but the interruption regression test now fails the Windows CI job on this head. See session

• src/utils/safeWriteJson.ts:107 — backup restore triggers on any fs.access failure, not just ENOENT; a transient access error can rename a stale .bak_*.tmp over a newer committed file and silently roll committed content back. Fixed: restore now runs only on ENOENT; other access errors abort the write without touching backups.
• src/utils/safeWriteJson.ts:372 — _flushFileToDisk opens the temp file read-only; on Windows FileHandle.sync() fails on every write (FlushFileBuffers requires write access), so the flush never works there and an error is logged for every write. Fixed: the flush handle now opens with "r+".
• src/utils/safeWriteJson.ts:111-115 — fs.stat on orphaned backups is unguarded; a rejected stat aborts the whole write with an unrelated fs error instead of skipping the unreadable entry. Fixed: stat failures are now guarded; ENOENT skips the entry and other errors abort the write deliberately, so an unreadable backup is never garbage-collected while it may be the newest committed state.
• src/utils/__tests__/safeWriteJson.interruption.test.ts:146 — the child is killed with process.kill(pid, "SIGKILL"), and the test asserts exit.code is null; on Windows the emulated SIGKILL exits with code 1, so both interruption scenarios fail platform-unit-test (windows-latest) on this head with AssertionError: expected 1 to be null.

_{Reviewed 25edb7a}

[github-actions]

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: Fix the failing required CI checks; awaiting-maintainer requires CI and automated review completion.

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 Report

❌ Patch coverage is 94.04762% with 5 lines in your changes missing coverage. Please review.

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

📢 Thoughts on this report? Let us know!

[zoomote2]

On Windows, a process killed with process.kill(pid, "SIGKILL") exits with code 1, not null — Windows has no signal exit codes, so Node emulates SIGKILL as an unconditional termination with exit code 1. Both interruption scenarios (atomic and fallback) fail on platform-unit-test (windows-latest) for the current head with AssertionError: expected 1 to be null (the child was correctly killed; only the exit-code assertion is platform-specific).

Accept the emulated Windows exit code alongside the POSIX signal exit:

Suggested change

	expect(exit.code).toBeNull()
	expect(exit.code === null || exit.code === 1).toBe(true)

[edelauna]

The merge-queue failure on #2294 provides another occurrence of #2130's missing-history-record symptom:

• Failed e2e-mock job, merge-group commit b7b8ef81107413e23e473f84dd4da98ad1ce4ed6.
• Create phase passed at 2026-11-05 00:40:53 UTC.
• The fresh verify host completed history-store initialization, then failed at 00:40:58 with “Task should be present after restart”.
• GitHub removed [Chore] Cancel obsolete PR CI runs and remove duplicate builds #2294 from the queue with failed_checks. The job failed an assertion; it was not cancelled or starved.
• This matches the second symptom already recorded in #2130. The log does not prove which file was absent or which write interleaving caused this particular occurrence.

Please include this failure class in the lifecycle verification work for this fix. I checked this PR at 25edb7ab9745a25f5bf9c643a787cde235dc283f: it already adds real subprocess interruption tests, which are useful. However, a passing pnpm lifecycle:model-check currently does not cover the replacement/restart protocol: scripts/check-completion-persistence.ts collapses persistence into a single history: "durable" phase. It cannot express individual task files, a later replacement removing a previously readable canonical path, process termination between renames, or startup discovering the task.

Suggested focused scope:

1. Add a small bounded replacement/restart submodel to the lifecycle umbrella, with explicit mapping to the production writer, readers and startup reconciliation. Keep the existing completion-readiness model; avoid multiplying unrelated lifecycle state spaces.
2. Model canonical/temp/backup state, successful initial persistence, a subsequent replacement, the EPERM fallback, interruption at each relevant boundary, and a fresh read/startup without requiring another write first. Distinguish UI/API message persistence from history_item.json publication, including shutdown after the UI marker becomes visible while the history record is still pending. Map readiness acknowledgments to actual production signals; do not assume TaskCompleted already promises all three files.
3. Assert that an acknowledged committed task remains discoverable and its required histories recoverable after interruption of a later write, absent explicit deletion. Do not require an unfinished first write to survive. Include positive successful-write/recovery landmarks, preservation of the last committed content through failed recovery, and no resurrection after explicit deletion. State lock-release/staleness assumptions and process-crash bounds explicitly; this is not a power-loss proof.
4. Connect the abstract transitions to the existing real-writer subprocess tests. Cover both replacement paths and the history-record case; exercise production read/startup recovery after the crash, not only recovery triggered by the next writer. The current head recovers backups at the start of a write, while readFileWithMissingRetry only retries; that distinction matters for a fresh host trying to discover an absent history_item.json.
5. Demonstrate that the old protocol produces the expected Restart persistence still fails after #2080: ui_messages.json missing after successful create-phase disk check #2130 counterexample and the fixed protocol satisfies the bounded invariant. If the model lands before the runtime fix, use the suite's explicit issue-keyed known-unsafe-witness convention, then promote it to a universal bounded invariant with the fix. Document what remains outside the model and retain the fresh-extension-host E2E.

This adds deterministic coverage for the suspected mechanism rather than relying on repeated green E2E runs. It should also prevent claiming that the already-green completion model proves this file-level recovery contract.