Repository navigation
[Fix] Completed task fails to resume after restart when a message write is interrupted - #2297
Merged
Merged
Conversation
Contributor
|
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
 |
9 tasks done
Contributor
Author
|
1 issue outstanding: startup recovery now defers behind a fresh crash lock, the removed test case and the architecture doc no longer match that behavior. The earlier recovery/flush/Windows findings stay fixed in their refactored locations.
Reviewed b460112 |
Contributor
Review statusThis PR was opened by an automated account. A human maintainer must verify the change intent, provenance, and validation before merging. Current step: The required review sequence passed. Remaining merge requirements apply. Review-state labels are managed by this workflow; do not edit them manually. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![zoomote2[bot]](https://avatars.githubusercontent.com/u/278826416?s=60&v=4){kind=small}
Contributor
|
The merge-queue failure on #2294 provides another occurrence of #2130's missing-history-record symptom:
Please include this failure class in the lifecycle verification work for this fix. I checked this PR at Suggested focused scope:
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. |
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.
edelauna
force-pushed
the
fix/ui-messages-write-interruption-2130-v2
branch
from
November 6, 2026 00:54
25edb7a to
01f7b1c
Compare
edelauna
marked this pull request as ready for review
November 6, 2026 00:55
edelauna
requested review from
JamesRobert20,
edelauna and
taltas
as code owners
November 6, 2026 00:55
github-actions
Bot
added
the
awaiting-maintainer
Contributor
Changed-code mutation testingReport for PR head
Details: mutants, guidance, and reproductionChanged-code mutation testing
Focused tests
All surviving and uncovered mutantsWarning annotations highlight up to 20 unique locations (maximum 7 per file). This summary lists every advisory mutant. extension
|
Line | Status | Mutator | Replacement |
|---|---|---|---|
46 | Survived | StringLiteral | "" |
48 | Survived | ConditionalExpression | false |
50 | Survived | StringLiteral | "" |
src/core/task-persistence/TaskHistoryStore.ts
Line | Status | Mutator | Replacement |
|---|---|---|---|
287 | Survived | ConditionalExpression | false |
287 | Survived | ConditionalExpression | true |
287 | Survived | EqualityOperator | getErrorCode(error) === "ENOENT" |
287 | Survived | StringLiteral | "" |
288 | Survived | StringLiteral | "" |
317 | Survived | ConditionalExpression | true |
317 | Survived | ConditionalExpression | false |
317 | Survived | EqualityOperator | getErrorCode(error) === "ENOENT" |
317 | Survived | StringLiteral | "" |
318 | Survived | StringLiteral | "" |
361 | Survived | ConditionalExpression | false |
385 | Survived | StringLiteral | "" |
847 | Survived | ConditionalExpression | false |
852 | Survived | ConditionalExpression | true |
854 | NoCoverage | StringLiteral | "" |
854 | Survived | ConditionalExpression | false |
src/core/webview/ClineProvider.ts
Line | Status | Mutator | Replacement |
|---|---|---|---|
2728 | Survived | StringLiteral | `` |
2746 | NoCoverage | StringLiteral | "" |
2746 | Survived | ConditionalExpression | false |
2750 | NoCoverage | StringLiteral | "" |
2750 | Survived | ConditionalExpression | false |
src/extension.ts
Line | Status | Mutator | Replacement |
|---|---|---|---|
132 | Survived | CallExpression | ; |
410 | Survived | StringLiteral | "" |
src/extension/api.ts
Line | Status | Mutator | Replacement |
|---|---|---|---|
367 | Survived | StringLiteral | "" |
369 | Survived | StringLiteral | "" |
372 | NoCoverage | StringLiteral | "" |
373 | Survived | LogicalOperator | getErrorCode(error) && "unknown" |
373 | Survived | StringLiteral | "" |
378 | Survived | StringLiteral | "" |
378 | Survived | ObjectLiteral | {} |
378 | Survived | StringLiteral | "" |
378 | Survived | StringLiteral | "" |
src/utils/errorCode.ts
Line | Status | Mutator | Replacement |
|---|---|---|---|
3 | Survived | ConditionalExpression | true |
3 | Survived | LogicalOperator | typeof error === "object" && error !== null && "code" in error || typeof error.code === "string" |
3 | Survived | ConditionalExpression | true |
3 | Survived | ConditionalExpression | true |
3 | Survived | ConditionalExpression | true |
src/utils/fileLock.ts
Line | Status | Mutator | Replacement |
|---|---|---|---|
31 | Survived | ConditionalExpression | true |
31 | Survived | ObjectLiteral | {} |
src/utils/jsonFileRecovery.ts
Line | Status | Mutator | Replacement |
|---|---|---|---|
36 | Survived | StringLiteral | "" |
43 | Survived | ConditionalExpression | false |
50 | Survived | StringLiteral | "" |
56 | Survived | BooleanLiteral | false |
58 | Survived | ConditionalExpression | true |
62 | Survived | ConditionalExpression | true |
62 | Survived | ConditionalExpression | false |
62 | Survived | EqualityOperator | options.waitForStaleLock !== false |
62 | Survived | BooleanLiteral | true |
65 | Survived | StringLiteral | "" |
84 | NoCoverage | BlockStatement | {} |
85 | NoCoverage | ConditionalExpression | true |
85 | NoCoverage | ConditionalExpression | false |
85 | NoCoverage | EqualityOperator | getErrorCode(error) !== "ENOENT" |
85 | NoCoverage | StringLiteral | "" |
132 | Survived | ObjectLiteral | {} |
132 | Survived | BooleanLiteral | true |
143 | Survived | ObjectLiteral | {} |
183 | Survived | MethodExpression | backupStats |
183 | Survived | ArithmeticOperator | b.mtimeMs + a.mtimeMs |
183 | Survived | ArrowFunction | () => undefined |
190 | Survived | StringLiteral | `` |
199 | Survived | ConditionalExpression | false |
205 | NoCoverage | StringLiteral | `` |
209 | Survived | BooleanLiteral | false |
src/utils/safeWriteJson.ts
Line | Status | Mutator | Replacement |
|---|---|---|---|
54 | Survived | MethodExpression | Math.random().toString(36) |
112 | Survived | OptionalChaining | options.trace |
115 | Survived | ConditionalExpression | false |
116 | NoCoverage | ObjectLiteral | {} |
116 | NoCoverage | StringLiteral | "" |
116 | NoCoverage | LogicalOperator | recovery.restoredFrom && "cleaned-leftovers" |
116 | NoCoverage | StringLiteral | "" |
161 | Survived | OptionalChaining | options?.trace({ stage: "backup-rename", detail: "eperm-fallback" }) |
164 | Survived | OptionalChaining | options?.trace({ stage: "replacement-rename", detail: "eperm-fallback" }) |
164 | Survived | StringLiteral | "" |
281 | Survived | StringLiteral | `` |
Resolve a mutation gap
Add or strengthen a focused test that fails under the mutation. If the mutant is equivalent, request maintainer approval for the narrowest mutator-specific exclusion and explain why it cannot change behavior:
// Stryker disable next-line ConditionalExpression: normalized input cannot reach the alternate branch
const result = condition ? value : fallbackBroad all exclusions and exclusions without a concrete reason are reported as advisories.
Reproduce locally
From a full checkout containing both commits:
pnpm install --frozen-lockfile
node scripts/stryker-diff.mjs ci --base 7ec58d4d4ccc39d16cae2a8c2ce3c072bd7d9915 --head c75610beb81e0a9845d6c8b4b8d611cbd9eb2838Mutation reports
- extension:
reports/mutation/extension/mutation.html
The workflow uploads generated reports in the changed-code-mutation-report artifact. A direct artifact link appears below after upload.
github-actions
Bot
added
awaiting-maintainer
| const { mtimeMs } = await fs.stat(taskFilePath) | ||
| const { mtimeMs } = await fs.stat(taskFilePath).catch(async (error: unknown) => { | ||
| if (getErrorCode(error) !== "ENOENT") throw error | ||
| await recoverMissingJsonFile(taskFilePath, { waitForStaleLock: false }) |
Contributor
Author
There was a problem hiding this comment.
The switch to { waitForStaleLock: false } makes startup recovery fail fast while a crashed writer's lock is still fresh. For an EPERM-fallback crash that leaves history_item.json absent with a committed backup, a fresh host restarting within 31 seconds now cannot discover the record at startup: reconcile sees ELOCKED, keeps the task live, and the record only appears on a later reconcile tick (watcher activity or the 5-minute periodic pass). The immediate-mode default is clearly deliberate (avoid blocking the store mutex for ~31–70s), but this commit also removes the only test that proved the old guarantee — the ageLock: false case in safeWriteJson.interruption.test.ts — while docs/architecture/file-replacement-restart-model.md (lines 50–51) still claims "One case leaves the crash lock's timestamp unchanged, proving the fresh store can wait for staleness", and the PR description repeats that claim.
If the deferral is intended, the doc and PR body should describe the new startup behavior and its deferred-discovery window, and ideally a focused test should pin the new contract (ELOCKED at startup → record recovered by a reconcile after the lock ages). Otherwise, consider waiting (waitForStaleLock: true) only when the hint scan finds leftovers and the cache lacks the task, which bounds the mutex block to genuinely-missing records.
edelauna
approved these changes
Nov 6, 2026
github-actions
Bot
removed
the
awaiting-maintainer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related GitHub Issue
Closes #2130. Recreates #2136 after its original author account was deleted.
Description
An interrupted two-rename replacement can leave a committed task file absent at its canonical path. A fresh host then loses access to message history or cannot discover the task's history record.
This change commits each replacement with one atomic rename after flushing its temp file. It retains the EPERM fallback required for Windows open-file replacement. Writers recover interrupted replacements under the shared advisory lock before merging data.
Readers and startup now recover missing canonical files from committed backups without requiring another write. Recovery waits for a crashed writer's lock to become stale rather than stealing a live lock. Startup recovers
history_item.jsonbefore its stat/read, and UI/API readers use the same recovery path.Recovery rejects uncertain directory scans, canonical access, backup metadata, and restore operations. These failures preserve committed backups and emit recovery error codes. Single and batch history deletion remove recoverable leftovers before deleting the canonical record under the same lock.
Persistence trace events identify writer stages, recovery failures, marker-check error kinds, and shutdown. They use the existing output-channel logger. Failed restart E2E scenarios retain phase results, logs, and task storage as CI artifacts.
Verification
pnpm test --concurrency=1, 14/14 tasks pass. Extension: 567 files pass, 4 skipped; 12,611 tests pass, 39 skipped.pnpm lifecycle:model-check: pass, including the new independent replacement/restart model.The real-writer subprocess suite covers all three task filenames, both replacement paths, and SIGKILL before and after commit. Production readers and fresh-store startup recover before another write. One case waits out an unmodified crash lock. Focused tests cover recovery I/O failure, later retry, flush/close ordering, deletion cleanup, and preventing recovery from resurrecting deleted history.
The bounded model first requires the old protocol's #2130 counterexample, then checks preservation, restart discovery, recovery failure/retry, and explicit deletion for three files, one later replacement, and one process crash. It distinguishes UI/API persistence from history publication. The E2E readiness check explicitly waits for both persisted histories and the store's published history item; neither TaskCompleted nor the UI marker alone promises all three.
Limits and documentation
This is a process-crash guarantee, not a power-loss proof. Parent directories are not fsynced. All participants must honor advisory locks; fresh recovery can wait about 35 seconds for staleness and reports an error if the lock or filesystem remains unavailable. Successful deletion prevents recovery from old leftovers; it does not prohibit deliberate new writes to a deleted path.
See
docs/architecture/file-replacement-restart-model.mdfor the production mapping, bounded invariants, assumptions, and exclusions. The completion-readiness model remains separate.Checklist