fix(task): recover stale delegated children after restart - #1210
fix(task): recover stale delegated children after restart#1210edelauna wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughStartup reconciliation now uses durable repair intents to recover persisted delegated task state across crashes. A two-phase VS Code E2E scenario verifies restart persistence. Provider configuration accepts string identifiers and expands canonical registry and side-effect tests. ChangesDelegated task recovery
Restart persistence E2E workflow
Provider model configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TaskHistoryStore
participant RepairIntent
participant ChildTask
participant ParentTask
TaskHistoryStore->>RepairIntent: write repair intent
TaskHistoryStore->>ChildTask: mark interrupted
TaskHistoryStore->>ParentTask: restore active
TaskHistoryStore->>RepairIntent: remove completed intent
sequenceDiagram
participant TestRunner
participant VSCodeCreate
participant PhaseResults
participant VSCodeVerify
TestRunner->>VSCodeCreate: launch create phase
VSCodeCreate->>PhaseResults: write persistence result
TestRunner->>VSCodeVerify: launch verify phase
VSCodeVerify->>PhaseResults: read create result and write verify result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
apps/vscode-e2e/src/runTest.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/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.
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
| // Task files are authoritative. Always refresh entries from disk so a stale | ||
| // index cannot overwrite a repair or another instance's newer task state. | ||
| for (const taskId of onDiskIds) { | ||
| if (!cacheIds.has(taskId)) { | ||
| try { | ||
| const item = await this.readTaskFile(taskId) | ||
| if (item) { | ||
| this.cache.set(taskId, item) | ||
| changed = true | ||
| } | ||
| } catch { | ||
| // Corrupted or missing file, skip | ||
| try { | ||
| const item = await this.readTaskFile(taskId) | ||
| if (item) { | ||
| this.cache.set(taskId, item) | ||
| changed = true | ||
| } | ||
| } catch { | ||
| // Corrupted or missing file, skip | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Scope the unconditional disk refresh; it now runs on every watcher and periodic reconcile.
reconcile() is called from three places: initialize(), the fs.watch handler (500 ms debounce), and the 5‑minute periodic timer. This loop now reads and parses every task file on each of those calls, instead of only the files absent from the cache. Each single task write in the tasks directory triggers a watcher event and therefore a full O(N) re-read of all task history files.
changed is also set to true whenever any file is read, so scheduleIndexWrite() now fires on every reconcile pass and rewrites the whole index, even when nothing drifted.
The authoritative-refresh guarantee is only needed to protect the startup repair path from a stale index. Consider keeping the full refresh for the startup call and using mtime or a force flag for the watcher and periodic paths, and set changed only when the parsed item differs from the cached entry.
♻️ Sketch: gate the full refresh and the dirty flag
- async reconcile(): Promise<void> {
+ async reconcile(options: { forceRefresh?: boolean } = {}): Promise<void> {
// Run through the write lock to prevent interleaving with upsert/delete
return this.withLock(async () => {
@@
for (const taskId of onDiskIds) {
try {
+ if (!options.forceRefresh && this.cache.has(taskId)) {
+ continue
+ }
const item = await this.readTaskFile(taskId)
- if (item) {
+ if (item && JSON.stringify(item) !== JSON.stringify(this.cache.get(taskId))) {
this.cache.set(taskId, item)
changed = true
}
} catch {Then call this.reconcile({ forceRefresh: true }) from initialize().
🤖 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 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.
github-actions
Bot
added
the
awaiting-review
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
Tests