Fix concurrent lane deletes#384
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
📝 WalkthroughWalkthroughThis PR implements concurrent batch deletion of lanes across desktop and iOS platforms. It introduces concurrency helpers with configurable parallelism limits, restructures the main service delete flow to allow concurrent teardowns, and wires the new execution model into both UI layers while preserving per-lane error tracking. ChangesConcurrent Lane Deletion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@copilot review but do not make fixes |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review
Scope: 9 file(s), +391 / −89
Verdict: Looks good
This PR lets lane creation and most delete teardown steps proceed while another lane is deleting, by removing the outer runGitWorktreeMutation wrapper around the full delete pipeline and keeping that guard only around git worktree remove. Desktop batch deletes and iOS batch manage now run up to two independent lane deletes per dependency-safe batch, with tests and docs updated to match.
Notes
- The backend change is narrowly scoped:
laneService.deletestill rejects duplicate in-flight deletes for the same lane, still blocks deleting a lane that has active child rows in the DB, and still serializes actual worktree registry mutations viarunGitWorktreeMutationatgit_worktree_remove(laneService.ts~4222–4260). New tests cover concurrent deletes and create-during-teardown ordering. - Desktop
runLaneDeleteBatchWithConcurrencyuses a small worker pool that can start the next lane as soon as a slot frees; iOSrunLaneDeleteBatchWithConcurrencyuses fixed pairs of chunks, so a third lane in the same batch may wait for the whole first pair to finish. That is a minor scheduling difference, not a correctness gap. - iOS batch delete still does not mirror desktop's "skip parent when a selected child delete failed" handling in
LanesPage.tsx; that parity gap predates this PR and is unchanged by the batching refactor.
Sent by Cursor Automation: BUGBOT in Versic
Reviewed the latest commit and didn’t find any blocking issues to fix. I have not made any code changes. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if chunk.count == 1 { | ||
| results.append(await runLaneDeleteOperation(laneId: chunk[0], operation: operation)) | ||
| } else { | ||
| async let first = runLaneDeleteOperation(laneId: chunk[0], operation: operation) | ||
| async let second = runLaneDeleteOperation(laneId: chunk[1], operation: operation) | ||
| results.append(contentsOf: await [first, second]) | ||
| } |
There was a problem hiding this comment.
else branch hard-codes exactly two concurrent items
The else branch explicitly references only chunk[0] and chunk[1]. Because normalizedConcurrency is clamped to laneDeleteBatchConcurrency (currently 2), chunks can never exceed two items today, but if the constant is bumped to 3 or higher the function will silently stop processing chunk[2] (and beyond) — delivering results for only the first two items in each chunk and leaving the rest untouched. The TypeScript equivalent uses a proper work-stealing pool (Promise.all over N workers) that stays correct at any concurrency level. Consider using withTaskGroup here so the implementation generalises cleanly without an invisible length constraint.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ios/ADE/Views/Lanes/LaneBatchManageSheet.swift
Line: 57-63
Comment:
**`else` branch hard-codes exactly two concurrent items**
The `else` branch explicitly references only `chunk[0]` and `chunk[1]`. Because `normalizedConcurrency` is clamped to `laneDeleteBatchConcurrency` (currently 2), chunks can never exceed two items today, but if the constant is bumped to 3 or higher the function will silently stop processing `chunk[2]` (and beyond) — delivering results for only the first two items in each chunk and leaving the rest untouched. The TypeScript equivalent uses a proper work-stealing pool (`Promise.all` over N workers) that stays correct at any concurrency level. Consider using `withTaskGroup` here so the implementation generalises cleanly without an invisible length constraint.
How can I resolve this? If you propose a fix, please make it concise.
| @MainActor | ||
| func runLaneDeleteBatchWithConcurrency<Value>( | ||
| laneIds: [String], | ||
| concurrency: Int = laneDeleteBatchConcurrency, | ||
| operation: @escaping (String) async throws -> Value | ||
| ) async -> [LaneBatchOperationResult<Value>] { | ||
| guard !laneIds.isEmpty else { return [] } | ||
|
|
||
| let normalizedConcurrency = max(1, min(laneIds.count, min(concurrency, laneDeleteBatchConcurrency))) | ||
| var results: [LaneBatchOperationResult<Value>] = [] | ||
| results.reserveCapacity(laneIds.count) | ||
|
|
||
| var index = 0 | ||
| while index < laneIds.count { | ||
| let nextIndex = min(index + normalizedConcurrency, laneIds.count) | ||
| let chunk = Array(laneIds[index..<nextIndex]) | ||
|
|
||
| if chunk.count == 1 { | ||
| results.append(await runLaneDeleteOperation(laneId: chunk[0], operation: operation)) | ||
| } else { | ||
| async let first = runLaneDeleteOperation(laneId: chunk[0], operation: operation) | ||
| async let second = runLaneDeleteOperation(laneId: chunk[1], operation: operation) | ||
| results.append(contentsOf: await [first, second]) | ||
| } | ||
|
|
||
| index = nextIndex | ||
| } | ||
|
|
||
| return results |
There was a problem hiding this comment.
The Swift implementation divides laneIds into sequential chunks and waits for every item in a chunk to complete before starting the next. The TypeScript runLaneDeleteBatchWithConcurrency uses a work-stealing pool: a worker picks up the next item as soon as it finishes its current one. With concurrency=2 and three lanes, if lane-a finishes much faster than lane-b, TypeScript will start lane-c immediately while lane-b is still running, but the Swift version waits for lane-b before lane-c starts. This is unlikely to cause user-visible issues at concurrency=2, but documents a behavioural divergence worth tracking if either constant changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ios/ADE/Views/Lanes/LaneBatchManageSheet.swift
Line: 40-68
Comment:
**Chunked fixed-size batching vs. TypeScript work-stealing pool**
The Swift implementation divides `laneIds` into sequential chunks and waits for every item in a chunk to complete before starting the next. The TypeScript `runLaneDeleteBatchWithConcurrency` uses a work-stealing pool: a worker picks up the next item as soon as it finishes its current one. With concurrency=2 and three lanes, if lane-a finishes much faster than lane-b, TypeScript will start lane-c immediately while lane-b is still running, but the Swift version waits for lane-b before lane-c starts. This is unlikely to cause user-visible issues at concurrency=2, but documents a behavioural divergence worth tracking if either constant changes.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/main/services/lanes/laneService.test.ts (1)
3003-3059: 💤 Low valueConsider polling instead of a fixed 40ms timeout to reduce CI flakiness.
Line 3048 uses
setTimeout(resolve, 40)to wait for bothstopAllcalls to start. In slow CI environments, this fixed delay could occasionally be insufficient. A polling approach (similar to lines 3104-3106) would be more deterministic:- await new Promise((resolve) => setTimeout(resolve, 40)); - - expect([...startedStops].sort()).toEqual(["lane-child", "lane-sibling"]); + for (let i = 0; i < 20 && startedStops.size < 2; i += 1) { + await new Promise((r) => setTimeout(r, 5)); + } + expect([...startedStops].sort()).toEqual(["lane-child", "lane-sibling"]);This completes as soon as both stops start rather than always waiting the full duration.
🤖 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 `@apps/desktop/src/main/services/lanes/laneService.test.ts` around lines 3003 - 3059, Replace the fixed sleep after starting deletePromises with a polling loop that waits until fake.processService.stopAll has been invoked for both lanes; specifically, instead of awaiting new Promise(resolve => setTimeout(resolve, 40)), poll the startedStops Set (populated by the mocked stopAll) until it contains "lane-child" and "lane-sibling" (with a short delay per iteration and a reasonable overall timeout to avoid hangs) before releasing stops and awaiting deletePromises; keep references to service.delete, fake.processService.stopAll, startedStops, deletePromises, and releaseStops/stopGate to locate where to change the wait.
🤖 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/ios/ADE/Views/Lanes/LaneBatchManageSheet.swift`:
- Around line 57-63: The current async-let code only handles exactly two items
and will drop extra IDs when laneDeleteBatchConcurrency/normalizedConcurrency >
2; replace the hardcoded async-let block with a dynamic TaskGroup over the
chunk: iterate chunk, add each runLaneDeleteOperation(laneId: id, operation:
operation) as a child task, await group results and append them to results. If
runLaneDeleteOperation is `@MainActor`, either remove/main-actor-isolate that
function or run each child task body as Task { await MainActor.run { await
runLaneDeleteOperation(...) } } so the TaskGroup can schedule children
dynamically without losing actor correctness.
In `@apps/ios/ADETests/ADETests.swift`:
- Around line 5100-5104: The test currently asserts strict ordering with
XCTAssertEqual(firstStartedIds, ["lane-a", "lane-b"]) which can flake due to
scheduler timing; change this to an order-insensitive assertion (e.g., compare
Set(firstStartedIds) to Set(["lane-a", "lane-b"]) or assert both expected IDs
are present regardless of order) while keeping the max active count check
(firstMaxActiveCount) intact; update the assertion that references
recorder.startedIds(), firstStartedIds and the XCTAssertEqual call accordingly.
---
Nitpick comments:
In `@apps/desktop/src/main/services/lanes/laneService.test.ts`:
- Around line 3003-3059: Replace the fixed sleep after starting deletePromises
with a polling loop that waits until fake.processService.stopAll has been
invoked for both lanes; specifically, instead of awaiting new Promise(resolve =>
setTimeout(resolve, 40)), poll the startedStops Set (populated by the mocked
stopAll) until it contains "lane-child" and "lane-sibling" (with a short delay
per iteration and a reasonable overall timeout to avoid hangs) before releasing
stops and awaiting deletePromises; keep references to service.delete,
fake.processService.stopAll, startedStops, deletePromises, and
releaseStops/stopGate to locate where to change the wait.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71ecbab2-b78f-4e2f-9d41-bd477a3bbb52
⛔ Files ignored due to path filters (2)
docs/features/lanes/README.mdis excluded by!docs/**docs/features/lanes/worktree-isolation.mdis excluded by!docs/**
📒 Files selected for processing (7)
apps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/lanePageModel.tsapps/ios/ADE/Views/Lanes/LaneBatchManageSheet.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/services/lanes/laneService.ts
| if chunk.count == 1 { | ||
| results.append(await runLaneDeleteOperation(laneId: chunk[0], operation: operation)) | ||
| } else { | ||
| async let first = runLaneDeleteOperation(laneId: chunk[0], operation: operation) | ||
| async let second = runLaneDeleteOperation(laneId: chunk[1], operation: operation) | ||
| results.append(contentsOf: await [first, second]) | ||
| } |
There was a problem hiding this comment.
Hardcoded concurrency handling will silently drop items if constant is changed.
The async let pattern here only handles exactly 2 items (chunk[0] and chunk[1]), but the chunk size is determined by normalizedConcurrency. If laneDeleteBatchConcurrency is increased to a value > 2, the chunk will contain more items than the code processes, silently dropping deletions.
Use TaskGroup for dynamic concurrency or remove the configurable parameter and explicitly document the hard limit of 2.
Proposed fix using TaskGroup for flexible concurrency
- if chunk.count == 1 {
- results.append(await runLaneDeleteOperation(laneId: chunk[0], operation: operation))
- } else {
- async let first = runLaneDeleteOperation(laneId: chunk[0], operation: operation)
- async let second = runLaneDeleteOperation(laneId: chunk[1], operation: operation)
- results.append(contentsOf: await [first, second])
- }
+ let chunkResults = await withTaskGroup(of: LaneBatchOperationResult<Value>.self) { group in
+ for laneId in chunk {
+ group.addTask {
+ await runLaneDeleteOperation(laneId: laneId, operation: operation)
+ }
+ }
+ var collected: [LaneBatchOperationResult<Value>] = []
+ for await result in group {
+ collected.append(result)
+ }
+ return collected
+ }
+ results.append(contentsOf: chunkResults)Note: runLaneDeleteOperation would need to drop @MainActor or be called differently since TaskGroup child tasks inherit the actor context but can't use @MainActor functions directly in this pattern. Consider whether main-actor isolation is actually needed for the operation wrapper.
🤖 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 `@apps/ios/ADE/Views/Lanes/LaneBatchManageSheet.swift` around lines 57 - 63,
The current async-let code only handles exactly two items and will drop extra
IDs when laneDeleteBatchConcurrency/normalizedConcurrency > 2; replace the
hardcoded async-let block with a dynamic TaskGroup over the chunk: iterate
chunk, add each runLaneDeleteOperation(laneId: id, operation: operation) as a
child task, await group results and append them to results. If
runLaneDeleteOperation is `@MainActor`, either remove/main-actor-isolate that
function or run each child task body as Task { await MainActor.run { await
runLaneDeleteOperation(...) } } so the TaskGroup can schedule children
dynamically without losing actor correctness.
| await recorder.waitForStartedCount(2) | ||
| let firstStartedIds = await recorder.startedIds() | ||
| let firstMaxActiveCount = await recorder.maxActiveCount() | ||
| XCTAssertEqual(firstStartedIds, ["lane-a", "lane-b"]) | ||
| XCTAssertEqual(firstMaxActiveCount, 2) |
There was a problem hiding this comment.
Avoid strict ordering assertions for concurrently started tasks.
firstStartedIds can be ["lane-b", "lane-a"] depending on scheduler timing, so this assertion may flake while concurrency is still correct.
Suggested test hardening
- XCTAssertEqual(firstStartedIds, ["lane-a", "lane-b"])
+ XCTAssertEqual(Set(firstStartedIds), Set(["lane-a", "lane-b"]))
+ XCTAssertEqual(firstStartedIds.count, 2)🤖 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 `@apps/ios/ADETests/ADETests.swift` around lines 5100 - 5104, The test
currently asserts strict ordering with XCTAssertEqual(firstStartedIds,
["lane-a", "lane-b"]) which can flake due to scheduler timing; change this to an
order-insensitive assertion (e.g., compare Set(firstStartedIds) to
Set(["lane-a", "lane-b"]) or assert both expected IDs are present regardless of
order) while keeping the max active count check (firstMaxActiveCount) intact;
update the assertion that references recorder.startedIds(), firstStartedIds and
the XCTAssertEqual call accordingly.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Greptile Summary
This PR narrows the worktree-mutation guard in the lane delete pipeline so that only
git_worktree_removeserializes through the shared queue, allowing concurrent lane deletes to progress through process/PTY/watcher teardown independently and unblocking lane creation that would previously wait for the entire delete to finish. The batch delete runner on both desktop and iOS is upgraded from sequential (one-at-a-time) to a limited-concurrency model (2 at a time, dependency-order-safe).laneService.ts): Removes the outerrunGitWorktreeMutationwrapper fromdelete; the guard is retained only around thegit_worktree_removestep, so concurrent deletes share only the narrow Git registry mutation window.lanePageModel.ts):runLaneDeleteBatchSequentiallyis replaced byrunLaneDeleteBatchWithConcurrency, a work-stealing pool that dispatches up to 2 deletions at a time and preserves per-lane failure results.LaneBatchManageSheet.swift): AddslaneDeleteDependencyBatches(topological sort) andrunLaneDeleteBatchWithConcurrency(chunkedasync letpairs); replaces the old sequential for-loop delete with the new batched concurrent dispatch.Confidence Score: 4/5
Safe to merge — the core locking invariant is preserved (only
git_worktree_removeenters the shared worktree-mutation queue) and the TypeScript work-stealing pool is correct at any concurrency level. The Swift implementation has a structural assumption baked in that needs attention if the concurrency constant is ever raised.The architectural change on the desktop side is well-reasoned and well-tested. The Swift
runLaneDeleteBatchWithConcurrencyhard-codesasync let first / async let second, which silently breaks iflaneDeleteBatchConcurrencyis increased beyond 2. The 40 ms fixed-sleep assertion in the new concurrency test is another point of potential intermittent failure on CI. Neither is a blocking correctness issue in the current state, but they deserve attention before the constant or test infrastructure changes.apps/ios/ADE/Views/Lanes/LaneBatchManageSheet.swift (chunk handling is hardcoded for 2 items) and apps/desktop/src/main/services/lanes/laneService.test.ts (fixed 40 ms sleep for concurrency assertion).
Important Files Changed
runGitWorktreeMutationguard from the delete pipeline, leaving it only around thegit_worktree_removestep; independent lane deletes can now proceed through stop_processes, PTYs, watchers, and env cleanup concurrently without holding the worktree-mutation slot.runLaneDeleteBatchSequentiallywithrunLaneDeleteBatchWithConcurrencyusing a work-stealing pool pattern; addsLANE_DELETE_BATCH_CONCURRENCY = 2constant; implementation correctly handles any concurrency level.stopAll, adds a new concurrency test, and rewrites the lane-creation-during-delete test to assert the new allowed-concurrent behavior; uses fixed 40 ms timeout for the new concurrency assertion which may be flaky on loaded CI.laneDeleteDependencyBatches,runLaneDeleteBatchWithConcurrency, andrunLaneDeleteOperation; replaces sequential for-loop delete with batched concurrent dispatch; the concurrent runner hard-codes handling for at most 2 items per chunk (safe now, fragile if constant increases).runLaneDeleteBatchSequentiallytorunLaneDeleteBatchWithConcurrencyat both the import site and the single call site; no logic changes.LaneBatchDeleteRecorderactor for thread-safe concurrency tracking,testLaneDeleteDependencyBatchesKeepAncestorsAfterSelectedDescendants, andtestLaneDeleteBatchRunnerStartsTwoDeletesAtOnceAndPreservesOrder; tests look correct for the current fixed-chunk Swift implementation.Sequence Diagram
sequenceDiagram participant UI as LanesPage participant Batch as runLaneDeleteBatchWithConcurrency participant D1 as delete lane-child participant D2 as delete lane-sibling participant Mut as worktree-mutation guard participant C as create new-lane UI->>Batch: batch with lane-child and lane-sibling par Worker 1 Batch->>D1: start teardown D1->>D1: git_status, stop_processes, PTYs, watchers and Worker 2 Batch->>D2: start teardown D2->>D2: git_status, stop_processes, PTYs, watchers end Note over D1,D2: teardown runs concurrently, no guard held UI->>C: create new-lane C->>Mut: acquire slot Mut-->>C: granted C->>C: git worktree add Mut-->>C: released D1->>Mut: acquire slot Mut-->>D1: granted D1->>D1: git worktree remove Mut-->>D1: released D1->>D1: database_cleanup D2->>Mut: acquire slot Mut-->>D2: granted D2->>D2: git worktree remove Mut-->>D2: released D2->>D2: database_cleanupComments Outside Diff (1)
apps/desktop/src/main/services/lanes/laneService.test.ts, line 60-67 (link)setTimeout(resolve, 40)is used to assert that both deletes have enteredstop_processesbefore any worktree removal begins. On a saturated CI machine the two async tasks may not have reached that checkpoint within 40 ms, causing theexpect([...startedStops].sort()).toEqual(...)assertion to fail intermittently. The sibling "allows lane creation…" test already adopts a polling loop (up to 10 × 5 ms) for the same class of timing assertion — applying that pattern here (or using the gate/promise approach used bystopStartedPromiseelsewhere in the suite) would remove the fixed-timeout dependency.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Fix concurrent lane deletes" | Re-trigger Greptile