fix(task-persistence): add cross-instance lock for concurrent task history updates (#920)#926
Open
easonLiangWorldedtech wants to merge 4 commits into
Conversation
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
July 17, 2026 16:54
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA shared singleton lock now serializes task-history mutations across provider instances and processes. Store helpers, provider workflows, migration, delegation transitions, and concurrency tests use the shared locking path. ChangesTask History Locking
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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
ESLint install timed out. The project may have too many dependencies for 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/TaskHistoryLock.ts`:
- Around line 14-28: The TaskHistoryLock implementation currently serializes
only callers within one process. In src/core/task-persistence/TaskHistoryLock.ts
lines 14-28, update TaskHistoryLock.withLock to acquire and release an exclusive
lock on the shared _history.lock file instead of using the module-local Promise
queue, while preserving sequential callback execution and error propagation. In
src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts lines 832-874, add
coverage that uses independent processes sharing the same storage and lock file
to verify cross-process exclusion.
In `@src/core/webview/ClineProvider.ts`:
- Around line 2096-2101: The normal task deletion path in ClineProvider must use
the same cross-instance taskHistoryLock as the fallback helper. Wrap
deleteTaskWithId’s taskHistoryStore.delete operation, including cache
invalidation and state posting, in taskHistoryLock.withLock, and ensure
deleteMany and other direct task-history store mutations use this centralized
locked boundary. Update
src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts lines 882-918 to
invoke deleteTaskWithId or otherwise verify the centralized mutation boundary.
🪄 Autofix (Beta)
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: 3a1bb978-5396-4cdd-9ca6-b32b5e73cb03
📒 Files selected for processing (4)
src/core/task-persistence/TaskHistoryLock.tssrc/core/task-persistence/__tests__/TaskHistoryLock.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.taskHistory.spec.ts
easonLiangWorldedtech
force-pushed
the
fix/920-concurrent-task-history-cross-instance
branch
from
July 17, 2026 17:43
ba61ec4 to
ac5c2e0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/TaskHistoryLock.ts`:
- Around line 75-83: Update the retries configuration in TaskHistoryLock so its
cumulative backoff window exceeds the 31-second stale threshold, allowing
stale-lock recovery before acquisition fails. Add a test covering a crashed lock
owner that exits without releasing the lock and verifies a subsequent writer
eventually acquires it.
In `@src/core/webview/ClineProvider.ts`:
- Around line 389-391: Update the migration flow around
taskHistoryStore.migrateFromGlobalState so the shared-lock callback reads
alreadyMigrated and legacy history, re-checks whether migration is needed,
performs migration only when necessary, and records the completion marker before
releasing the lock. Remove the pre-lock decision and state reads to prevent
stale migrations from being queued.
- Around line 2055-2057: Move the childIds traversal and allIdsToDelete
derivation inside the taskHistoryLock.withLock callback in ClineProvider, using
the locked persisted snapshot before calling deleteMany so concurrent child
additions are included. In src/core/webview/ClineProvider.ts lines 2055-2057,
update the deletion flow accordingly; in
src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts lines 876-908, add
a concurrent update-and-cascade-delete test rather than relying on the existing
normal-path test that passes false.
- Around line 3842-3866: Revalidate the parent-child relationship inside the
parent callback passed to atomicUpdatePair: require parent.awaitingChildId to
equal childTaskId and verify the parent has an allowed status before modifying
it. Apply this guard before clearing delegation fields or setting completion
metadata, preserving the existing child update and valid transition behavior.
🪄 Autofix (Beta)
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: e5236e42-57e1-42eb-8f70-813716b85e20
📒 Files selected for processing (5)
src/core/task-persistence/TaskHistoryLock.tssrc/core/task-persistence/__tests__/TaskHistoryLock.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.taskHistory.spec.tssrc/shared/globalFileNames.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/__tests__/TaskHistoryStore.spec.ts`:
- Around line 250-259: Replace the Promise.race timeout check in the
mutateLocked test with a direct await of the store.mutateLocked operation,
preserving its reconcileLocked, assertion, and deleteManyLocked steps. Remove
the 100 ms “deadlocked” fallback and rely on the test runner’s standard timeout.
In `@src/core/task-persistence/TaskHistoryStore.ts`:
- Line 264: Update the wrapper methods around deleteManyCore and the other
referenced entry points to acquire the shared storage/file lock before invoking
the instance-local withLock queue. Preserve each method’s existing core
operation and ensure all listed mutation paths serialize across different
TaskHistoryStore instances.
🪄 Autofix (Beta)
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: e1e734fc-ba88-4649-9e0c-2e1e8147e72c
📒 Files selected for processing (7)
src/__tests__/history-resume-delegation.spec.tssrc/core/task-persistence/TaskHistoryLock.tssrc/core/task-persistence/TaskHistoryStore.tssrc/core/task-persistence/__tests__/TaskHistoryLock.spec.tssrc/core/task-persistence/__tests__/TaskHistoryStore.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.taskHistory.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/task-persistence/TaskHistoryLock.ts
- src/core/webview/tests/ClineProvider.taskHistory.spec.ts
- src/core/webview/ClineProvider.ts
easonLiangWorldedtech
force-pushed
the
fix/920-concurrent-task-history-cross-instance
branch
from
July 17, 2026 19:22
62b00b2 to
0d004f7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 471-473: The mutateLocked method in
src/core/task-persistence/TaskHistoryStore.ts lines 471-473 must use only the
instance-local queue and avoid reacquiring taskHistoryLock. Update the test at
src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts lines 284-292 to
remove the shared-lock acquisition expectation and verify mutateLocked succeeds
when called beneath an externally held lock.
- Around line 795-800: Update withSharedStorageLock and the read-modify-write
path using atomicReadAndUpdate to synchronize affected task records from disk
after acquiring the shared storage lock and before applying updates. Ensure each
instance refreshes its cached state within the lock scope so it cannot overwrite
newer changes, while preserving the existing queue and lock behavior.
🪄 Autofix (Beta)
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: dbadf991-4006-42e7-9b66-1ce17cb9681e
📒 Files selected for processing (8)
src/__tests__/history-resume-delegation.spec.tssrc/__tests__/provider-delegation.spec.tssrc/core/task-persistence/TaskHistoryLock.tssrc/core/task-persistence/TaskHistoryStore.tssrc/core/task-persistence/__tests__/TaskHistoryLock.spec.tssrc/core/task-persistence/__tests__/TaskHistoryStore.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.taskHistory.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/core/task-persistence/TaskHistoryLock.ts
- src/core/task-persistence/tests/TaskHistoryLock.spec.ts
- src/core/webview/tests/ClineProvider.taskHistory.spec.ts
- src/tests/history-resume-delegation.spec.ts
- src/core/webview/ClineProvider.ts
Comment on lines
+471
to
+473
| async mutateLocked<T>(fn: () => Promise<T>): Promise<T> { | ||
| return this.withSharedStorageLock(fn) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
mutateLocked deadlocks when invoked under the provider’s shared lock. The real lock queue is non-reentrant, while the test mock masks this behavior.
src/core/task-persistence/TaskHistoryStore.ts#L471-L473: enter only the instance-local queue instead of reacquiringtaskHistoryLock.src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts#L284-L292: replace the shared-lock acquisition expectation with coverage for invocation beneath an externally held lock.
📍 Affects 2 files
src/core/task-persistence/TaskHistoryStore.ts#L471-L473(this comment)src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts#L284-L292
🤖 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 471 - 473, The
mutateLocked method in src/core/task-persistence/TaskHistoryStore.ts lines
471-473 must use only the instance-local queue and avoid reacquiring
taskHistoryLock. Update the test at
src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts lines 284-292 to
remove the shared-lock acquisition expectation and verify mutateLocked succeeds
when called beneath an externally held lock.
Comment on lines
+795
to
+800
| /** | ||
| * Acquires the shared task-history storage lock before entering the instance-local queue. | ||
| */ | ||
| private withSharedStorageLock<T>(fn: () => Promise<T>): Promise<T> { | ||
| return taskHistoryLock.withLock(this.globalStoragePath, () => this.withLock(fn)) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Refresh authoritative state after acquiring the shared lock.
The lock serializes writers but leaves each instance’s cache stale. For example, after instance A updates a task, instance B can acquire the lock and run atomicReadAndUpdate against its older cached copy, overwriting A’s update. Reload the affected task records—or otherwise synchronize the cache from disk—inside the shared-lock scope before each read-modify-write operation.
🤖 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 795 - 800, Update
withSharedStorageLock and the read-modify-write path using atomicReadAndUpdate
to synchronize affected task records from disk after acquiring the shared
storage lock and before applying updates. Ensure each instance refreshes its
cached state within the lock scope so it cannot overwrite newer changes, while
preserving the existing queue and lock behavior.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
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
2 participants
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.
[BUG] Concurrent task history updates from parallel tabs may cause lost entries (#920)
Problem
updateTaskHistory()has serialization within a single ClineProvider instance (seeClineProvider.taskHistory.spec.ts), but when two parallel tabs (different ClineProvider instances) callupdateTaskHistory()concurrently, there may be a race condition that causes task history entries to be overwritten or lost.Root Cause
PR #909 created an independent
viewLocalStatebuffer for each ClineProvider instance, but the serialization logic inupdateTaskHistory()only protects concurrent calls within the same instance. If two parallel tabs (different instances) callupdateTaskHistory()simultaneously:updateTaskHistory(taskX)→ reads current stateupdateTaskHistory(taskY)→ reads same stale state (before Tab A's write)Fix
Add a global file-level lock (
_history.lock) viaproper-lockfileinTaskHistoryStore.withLock()so that mutations from different ClineProvider instances (parallel tabs) are serialized, preventing stale-cache overwrites.Changes
proper-lockfileand addgetGlobalLockFilePath()helperwithLock()before in-process serializationTesting Requirements
Related Issues
Summary by CodeRabbit
Bug Fixes
Tests