fix(task): recover stale delegated children after restart - #1210
fix(task): recover stale delegated children after restart#1210edelauna wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesDelegated task recovery
Restart persistence E2E workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/core/task-persistence/TaskHistoryStore.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/core/task-persistence/__tests__/TaskHistoryStore.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
edelauna
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task-persistence/TaskHistoryStore.ts`:
- Around line 370-374: Update the persistedActiveIds construction in
TaskHistoryStore to treat an omitted HistoryItem.status as "active" by
normalizing it before the predicate. Add a regression test covering a persisted
active child with no status and verify its delegated parent is repaired.
- Around line 420-429: Make the parent-child repair in the surrounding
reconciliation flow durable by recording a recoverable repair intent before
either upsertCore call, then completing or rolling it back during startup
reconciliation if either write or onWrite fails. Ensure interrupted recovery
clears the parent’s delegated state and restores the intended statuses, and add
a fault-injection test covering failure between the child and parent writes.
In
`@webview-ui/src/components/settings/utils/__tests__/providerModelConfig.spec.ts`:
- Around line 154-155: Remove the unnecessary `as any` cast in the
`getProviderModelConfig` test and pass the `"unknown-provider"` string directly,
preserving the assertion that the function returns undefined.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: baf3bf9e-7eb4-4256-aab0-212622c9496a
📒 Files selected for processing (4)
src/core/task-persistence/TaskHistoryStore.tssrc/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.tswebview-ui/src/components/settings/utils/__tests__/providerModelConfig.spec.tswebview-ui/src/components/settings/utils/providerModelConfig.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
removed
the
awaiting-review
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts (2)
377-399: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDispose locally created stores even when an assertion fails.
replayedStore.dispose()on Line 398 runs only if every preceding assertion passes.TaskHistoryStore.initialize()starts anfs.watchhandle and a 5-minutesetTimeoutchain. If Line 395, 396, or 397 fails, that store is never disposed. Its watcher can then fire areconcile()against the temp directory during a later test and log errors or keep the worker handle open.The same pattern appears at Lines 342-346, 370-374, and 498-502. Register each locally created store for cleanup instead.
♻️ Proposed cleanup pattern
+ // Near the other hooks: + const disposables: TaskHistoryStore[] = [] + afterEach(() => { + while (disposables.length) disposables.pop()?.dispose() + })Then push each store after construction:
const replayedStore = new TaskHistoryStore(tmpDir) + disposables.push(replayedStore) await replayedStore.initialize() expect(replayedStore.get(child.id)?.status).toBe("interrupted") expect(replayedStore.get(parent.id)?.status).toBe("active") await expect(fs.access(intentPath)).rejects.toThrow() - replayedStore.dispose()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts` around lines 377 - 399, Ensure every locally created TaskHistoryStore in the affected tests, including replayedStore and the stores created around the other cited cases, is registered for guaranteed cleanup immediately after construction rather than relying on a final dispose assertion. Use the test suite’s existing cleanup mechanism so dispose runs even when initialization or later assertions fail, while preserving the current assertions and store behavior.
401-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the two remaining intent-rejection branches.
replayDelegationRepairIntenthas two rejection paths that this suite does not exercise:
TaskHistoryStore.tsLine 494: the intent references aparentTaskIdorchildTaskIdthat has no cached record. Expect quarantine and unchanged startup.TaskHistoryStore.tsLine 510: both records exist, but the parent moved to a status that is neitherdelegatednor theactivetarget. Expect quarantine and no write to either task file.The second case matters most. It leaves the child at
interruptedand the parent outside the repair, so it defines the end state after a guard mismatch.As per path instructions, "Add focused tests for UI binding and save behavior, persistence or normalization, and the value returned by
getStateToPostToWebview(), including true and false/unset cases when defaults could hide omissions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts` around lines 401 - 415, Extend the reconciliation tests around replayDelegationRepairIntent with two focused cases: quarantine an intent whose parentTaskId or childTaskId has no cached record while leaving unrelated startup unchanged, and quarantine an intent where both records exist but the parent status is neither delegated nor the active repair target. For the status-mismatch case, assert the child remains interrupted, the parent remains outside repair, and neither task file is written.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/vscode-e2e/src/runTest.ts`:
- Around line 48-49: Update isRestartPersistenceTargetedRun to recognize
TEST_GREP selections for the “Restart persistence” scenario in addition to
testFile names containing “restart-persistence”. Ensure the helper returns true
when TEST_GREP targets that scenario so the runner uses the restart-specific
execution path.
In `@src/core/task-persistence/TaskHistoryStore.ts`:
- Around line 343-355: Scope the disk refresh in reconcile() so the full
task-file reload occurs only for the startup repair path: add the appropriate
force-refresh option and call reconcile({ forceRefresh: true }) from
initialize(), while watcher and periodic calls use change detection such as
mtime. Update changed only when a parsed item differs from the cached entry,
preventing unnecessary index writes.
- Around line 544-593: Contain failures from each repair initiated by
reconcileDelegationState so a rejected writeTaskFile, onWrite, or related
operation does not abort initialize; preserve the durable repair intent for
replay and allow startup to continue through startWatcher and
startPeriodicReconciliation while initialized resolves. Update the affected
reconciliation tests to expect initialization to resolve with the intent
retained for recovery on restart.
In `@webview-ui/src/components/settings/utils/providerModelConfig.ts`:
- Around line 89-90: Update getProviderServiceConfig and the corresponding
provider-default-model lookup to accept registry values only when the requested
key is an own property of PROVIDER_SERVICE_CONFIG or PROVIDER_DEFAULT_MODEL_IDS.
Preserve the existing fallback behavior for unknown strings, including inherited
keys such as "constructor" and "toString", and add regression coverage for those
keys.
---
Nitpick comments:
In `@src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts`:
- Around line 377-399: Ensure every locally created TaskHistoryStore in the
affected tests, including replayedStore and the stores created around the other
cited cases, is registered for guaranteed cleanup immediately after construction
rather than relying on a final dispose assertion. Use the test suite’s existing
cleanup mechanism so dispose runs even when initialization or later assertions
fail, while preserving the current assertions and store behavior.
- Around line 401-415: Extend the reconciliation tests around
replayDelegationRepairIntent with two focused cases: quarantine an intent whose
parentTaskId or childTaskId has no cached record while leaving unrelated startup
unchanged, and quarantine an intent where both records exist but the parent
status is neither delegated nor the active repair target. For the
status-mismatch case, assert the child remains interrupted, the parent remains
outside repair, and neither task file is written.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b1de36ae-c6fb-4047-a17e-d66364faa6fb
📒 Files selected for processing (13)
apps/vscode-e2e/fixtures/restart-persistence.jsonapps/vscode-e2e/src/restart/phaseProtocol.tsapps/vscode-e2e/src/restart/scenarioWorkspace.tsapps/vscode-e2e/src/restart/vscodeCoordinator.tsapps/vscode-e2e/src/runTest.tsapps/vscode-e2e/src/suite/index.tsapps/vscode-e2e/src/suite/restart-persistence.test.tssrc/core/task-persistence/TaskHistoryStore.tssrc/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.tssrc/eslint-suppressions.jsonsrc/shared/globalFileNames.tswebview-ui/src/components/settings/utils/__tests__/providerModelConfig.spec.tswebview-ui/src/components/settings/utils/providerModelConfig.ts
💤 Files with no reviewable changes (1)
- src/eslint-suppressions.json
🚧 Files skipped from review as they are similar to previous changes (1)
- webview-ui/src/components/settings/utils/tests/providerModelConfig.spec.ts
github-actions
Bot
added
awaiting-review
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
removed
the
awaiting-review
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/core/task-persistence/TaskHistoryStore.ts (2)
739-753: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuarantine files accumulate without bound.
Each quarantine renames the intent to a unique
*.quarantine-<ts>-<rand>name in the tasks directory. Nothing removes those files later. Corruption is rare, so growth is slow, but the files stay forever and are also scanned byreconcile().Consider deleting quarantine files older than a fixed age during startup, or overwriting a single fixed quarantine name instead of creating a new one each time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task-persistence/TaskHistoryStore.ts` around lines 739 - 753, Update quarantineDelegationRepairIntent to prevent unbounded quarantine-file accumulation: use a fixed quarantine destination that is overwritten, or add startup cleanup that removes quarantine files older than a defined retention age. Ensure reconcile() does not continue treating retained quarantine files as active delegation repair intents.
356-383: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRecord the mtime even when the task file fails to parse.
If
readTaskFile()returnsnullfor a corrupt file, the code skipsthis.taskFileMtimes.set(). The cache keeps the previous entry, and every later reconcile pass re-reads and re-parses the same corrupt file. The skip-by-mtime optimization does not apply to it.This is a small waste on the periodic path only. Consider recording the mtime before the parse attempt.
♻️ Proposed change
const item = await this.readTaskFile(taskId) + this.taskFileMtimes.set(taskId, mtimeMs) if (item) { const previous = this.cache.get(taskId) - this.taskFileMtimes.set(taskId, mtimeMs) if (!deepEqual(previous, item)) { this.cache.set(taskId, item) changed = true } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task-persistence/TaskHistoryStore.ts` around lines 356 - 383, Update the reconciliation loop around readTaskFile so this.taskFileMtimes records mtimeMs before attempting to parse the task file, including when readTaskFile returns null. Preserve the existing cache update and changed behavior for successfully parsed items, while allowing the existing mtime check to skip repeated parsing of unchanged corrupt files.src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts (2)
40-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the repair-intent fixture instead of returning
object.
objectaccepts any non-primitive value, so a typo in a field name stays undetected. The tests then pass for the wrong reason: an invalid intent is quarantined and the assertions about quarantine still hold.Declare a local shape or export the intent type for tests.
♻️ Proposed change
-function makeRepairIntent(parent: HistoryItem, child: HistoryItem): object { +type RepairIntentFixture = { + version: 1 + operationId: string + parentTaskId: string + childTaskId: string + expected: { + parent: { status: "delegated"; awaitingChildId: string; delegatedToId?: string } + child: { status: "active"; parentTaskId?: string; rootTaskId?: string } + } + target: { childStatus: "interrupted"; parentStatus: "active" } +} + +function makeRepairIntent(parent: HistoryItem, child: HistoryItem): RepairIntentFixture { return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts` around lines 40 - 60, Update makeRepairIntent to return the concrete repair-intent type rather than object, using the existing exported intent type if available or declaring a local equivalent shape. Ensure the fixture’s fields are checked by TypeScript so misspelled or structurally invalid intent properties fail compilation.
743-767: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new mtime skip path in
reconcile().
reconcile()now skips a task file whenforceRefreshis false and the recorded mtime matches. No test in this suite exercises that branch. A regression that inverts the condition, or that never records an mtime, would still pass every test here.Add one test that calls
reconcile()withoutforceRefreshtwice and asserts the second pass does not re-read unchanged files, plus one that changes a file on disk and asserts the cache updates.As per path instructions: "Prefer the narrowest test layer that proves behavior: unit tests for pure logic and state transitions".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts` around lines 743 - 767, Add focused tests for the mtime skip behavior in reconcile(): invoke reconcile() twice without forceRefresh and verify unchanged task files are not re-read, then modify a task file on disk and verify the cached task updates on the subsequent reconcile. Use the existing TaskHistoryStore test helpers and assertions, keeping coverage at the unit/state-transition level.Source: Path instructions
src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts (2)
402-405: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that
concurrentUpsertremains pending.Line 404 checks only the cache. It does not prove that
concurrentUpserthas not already resolved while migration is blocked. Track a settlement flag and assert it isfalsebefore Line 405 releases the migration write.Proposed test adjustment
+ let concurrentUpsertSettled = false - const concurrentUpsert = store.upsert(concurrent) + const concurrentUpsert = store.upsert(concurrent).finally(() => { + concurrentUpsertSettled = true + }) + await Promise.resolve() + expect(concurrentUpsertSettled).toBe(false) expect(store.get(concurrent.id)).toBeUndefined()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts` around lines 402 - 405, Update the concurrent migration test around concurrentUpsert to track whether its promise settles, and assert the settlement flag is false while the migration write remains blocked. Keep the existing cache assertion, then release the migration write so the pending upsert can complete.
392-394: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the unexplained
as unknown ascast.Line 392 bypasses the
TaskHistoryStoretype to spy onwriteIndex. Use bracket notation or a typed test seam when possible. If the double assertion is unavoidable, add a nearby comment that explains why it is required.As per coding guidelines: “Use double assertions only as a last resort and explain them with a comment.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts` around lines 392 - 394, Replace the unexplained double assertion around TaskHistoryStore.writeIndex in the test with bracket notation or an explicitly typed test seam, while preserving the existing spy behavior; if the cast is unavoidable, add a nearby comment explaining why it is required.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts`:
- Around line 40-60: Update makeRepairIntent to return the concrete
repair-intent type rather than object, using the existing exported intent type
if available or declaring a local equivalent shape. Ensure the fixture’s fields
are checked by TypeScript so misspelled or structurally invalid intent
properties fail compilation.
- Around line 743-767: Add focused tests for the mtime skip behavior in
reconcile(): invoke reconcile() twice without forceRefresh and verify unchanged
task files are not re-read, then modify a task file on disk and verify the
cached task updates on the subsequent reconcile. Use the existing
TaskHistoryStore test helpers and assertions, keeping coverage at the
unit/state-transition level.
In `@src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts`:
- Around line 402-405: Update the concurrent migration test around
concurrentUpsert to track whether its promise settles, and assert the settlement
flag is false while the migration write remains blocked. Keep the existing cache
assertion, then release the migration write so the pending upsert can complete.
- Around line 392-394: Replace the unexplained double assertion around
TaskHistoryStore.writeIndex in the test with bracket notation or an explicitly
typed test seam, while preserving the existing spy behavior; if the cast is
unavoidable, add a nearby comment explaining why it is required.
In `@src/core/task-persistence/TaskHistoryStore.ts`:
- Around line 739-753: Update quarantineDelegationRepairIntent to prevent
unbounded quarantine-file accumulation: use a fixed quarantine destination that
is overwritten, or add startup cleanup that removes quarantine files older than
a defined retention age. Ensure reconcile() does not continue treating retained
quarantine files as active delegation repair intents.
- Around line 356-383: Update the reconciliation loop around readTaskFile so
this.taskFileMtimes records mtimeMs before attempting to parse the task file,
including when readTaskFile returns null. Preserve the existing cache update and
changed behavior for successfully parsed items, while allowing the existing
mtime check to skip repeated parsing of unchanged corrupt files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b97160de-8dc5-47fd-9626-05e5f2e5de78
📒 Files selected for processing (3)
src/core/task-persistence/TaskHistoryStore.tssrc/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.tssrc/core/task-persistence/__tests__/TaskHistoryStore.spec.ts
1 participant
Related GitHub Issue
Closes: #1100
Description
After an unclean VS Code or extension shutdown, a delegated child task can remain persisted as
activeeven though no live child task exists. When the parent task is resumed, a subsequentnew_taskdelegation is rejected because the stale child is treated as an active live child, causing the parent to loop on “Continue”.This PR updates startup delegation reconciliation to treat persisted
activeawaited children as orphaned crash state. During recovery, the child is markedinterrupted, the parent is restored toactive, and the live delegation pointers are cleared so the parent can delegate again. Existing parent/child lineage and historical child IDs are retained. The runtime guard remains unchanged, so a genuinely active child in a live session is still protected from silent detachment.Test Procedure
cd src && npx vitest run core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.tscd src && npx vitest run __tests__/provider-delegation.spec.ts __tests__/removeClineFromStack-delegation.spec.tspnpm --dir src exec eslint --prune-suppressions --max-warnings=0 core/task-persistence/TaskHistoryStore.ts core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.tspnpm check-typesgit diff --checkThe reconciliation regression coverage verifies in-memory and persisted recovery state, chained delegation behavior, and idempotence. Live delegation tests continue to verify that genuinely active children are rejected.
Pre-Submission Checklist
Visual Snapshots
Not applicable; this is a task-persistence recovery fix.
Videos (interaction / animation only)
Not applicable.
Documentation Updates
Additional Notes
The recovery is limited to startup reconciliation. The existing live-session re-delegation guard remains in place to avoid detaching a child that may still be running.
Get in Touch
Summary by CodeRabbit
Bug Fixes
Reliability
Tests