Fix stale lane worktree cleanup#642
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughAdds a ChangesResidual Worktree Cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/lanes/laneService.test.ts (1)
3169-3169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the test worktrees directory separate from
repoRoot.Pointing
worktreesDirat the project root makes the sweeper treat arbitrary root children as managed worktrees, which weakens coverage for the direct-child cleanup guard this PR relies on. Prefer creatingchildPathunder a dedicated fixtureworktreesDirand seeding the lane with that path. As per coding guidelines, “When working in ADE lanes, all file reads, edits, and writes MUST target paths under the lane worktree, never under the main project-root checkout.”🤖 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` at line 3169, The test is incorrectly setting worktreesDir to repoRoot, which causes the sweeper to treat arbitrary root children as managed worktrees and weakens test coverage. Instead of pointing worktreesDir at repoRoot, create a dedicated fixture directory separate from repoRoot for worktreesDir, then create childPath as a subdirectory under this dedicated worktreesDir fixture, and pass this childPath to the lane configuration. This ensures the test properly validates the direct-child cleanup guard behavior as intended by the PR.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 `@apps/desktop/src/main/services/lanes/laneService.test.ts`:
- Line 3169: The test is incorrectly setting worktreesDir to repoRoot, which
causes the sweeper to treat arbitrary root children as managed worktrees and
weakens test coverage. Instead of pointing worktreesDir at repoRoot, create a
dedicated fixture directory separate from repoRoot for worktreesDir, then create
childPath as a subdirectory under this dedicated worktreesDir fixture, and pass
this childPath to the lane configuration. This ensures the test properly
validates the direct-child cleanup guard behavior as intended by the PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 074c2749-dd6c-4179-a13c-17181cdbad97
⛔ Files ignored due to path filters (4)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/lanes/worktree-isolation.mdis excluded by!docs/**docs/features/sync-and-multi-device/crdt-model.mdis excluded by!docs/**
📒 Files selected for processing (4)
apps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/lanes/worktreeResidualCleanup.tsapps/desktop/src/main/services/state/kvDb.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch (error) { | ||
| if (row) { | ||
| noteRetryFailure(row, error); | ||
| } | ||
| logger.warn("lane.residual_worktree_cleanup.remove_failed", { | ||
| projectRoot, | ||
| laneId: row?.lane_id ?? null, | ||
| worktreePath: pathKey, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } |
There was a problem hiding this comment.
removeWorktreeDirectory throws for a dir-scan candidate (no row), the only action is a warn log — the directory will be re-added to candidatePaths on every subsequent sweep as long as it stays empty and old enough, generating a fresh warning each time. On a filesystem where removal consistently fails (e.g., read-only mount, Windows handle lock), this can flood the logs on every list() call. A simple attempt counter in the catch block, or just skipping untracked dirs on a repeating error, would keep noise bounded.
| } catch (error) { | |
| if (row) { | |
| noteRetryFailure(row, error); | |
| } | |
| logger.warn("lane.residual_worktree_cleanup.remove_failed", { | |
| projectRoot, | |
| laneId: row?.lane_id ?? null, | |
| worktreePath: pathKey, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| } | |
| } catch (error) { | |
| if (row) { | |
| noteRetryFailure(row, error); | |
| } else { | |
| // Unknown empty dir — no DB record to update. If this keeps failing | |
| // (e.g. read-only filesystem), the warn fires on every sweep. | |
| // Consider recording a row here to gain backoff / attempt counting. | |
| } | |
| logger.warn("lane.residual_worktree_cleanup.remove_failed", { | |
| projectRoot, | |
| laneId: row?.lane_id ?? null, | |
| worktreePath: pathKey, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/lanes/worktreeResidualCleanup.ts
Line: 284-294
Comment:
Unknown-empty-dir cleanup failures have no backoff or retry record. When `removeWorktreeDirectory` throws for a dir-scan candidate (no `row`), the only action is a `warn` log — the directory will be re-added to `candidatePaths` on every subsequent sweep as long as it stays empty and old enough, generating a fresh warning each time. On a filesystem where removal consistently fails (e.g., read-only mount, Windows handle lock), this can flood the logs on every `list()` call. A simple attempt counter in the catch block, or just skipping untracked dirs on a repeating error, would keep noise bounded.
```suggestion
} catch (error) {
if (row) {
noteRetryFailure(row, error);
} else {
// Unknown empty dir — no DB record to update. If this keeps failing
// (e.g. read-only filesystem), the warn fires on every sweep.
// Consider recording a row here to gain backoff / attempt counting.
}
logger.warn("lane.residual_worktree_cleanup.remove_failed", {
projectRoot,
laneId: row?.lane_id ?? null,
worktreePath: pathKey,
error: error instanceof Error ? error.message : String(error),
});
}
```
How can I resolve this? If you propose a fix, please make it concise.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Test plan
/qualitypass completed; Track B extraction applied/testpass completednpx vitest run src/main/services/lanes/laneService.test.ts— 96 passednpx vitest run --shard=1/8— 49 files, 724 passednpm --prefix apps/desktop run typecheck— passednode scripts/validate-docs.mjs— passednpm --prefix apps/ade-cli run typecheckandnpm --prefix apps/ade-cli test— passednpm --prefix apps/ade-cli run typecheckandcd apps/ade-cli && npx vitest run src/tuiClient— passedSummary by CodeRabbit
Greptile Summary
This PR introduces a persistent retry mechanism for managed worktree directories that survive lane deletion. When
git worktree remove+ manual filesystem cleanup both fail, the delete now records the path in a new machine-locallocal_worktree_residual_cleanupstable and defers removal to subsequentlaneService.list()calls via a TTL-gated sweep.worktreeResidualCleanup.tsimplements the sweep with layered guards (registered git worktrees, active/archived lane paths, in-flight creates, symlinks, unknown non-empty dirs) and exposesrecordFailure,deleteRow, andretryto the service.kvDb.tsadds the new table with a(project_id, worktree_path)unique index and correctly excludes it from CRR replication, since absolute paths are machine-local state.laneService.tswiresretry()into thelist()repair sequence and callsrecordFailure/deleteRowat the appropriate success and non-fatal-failure branches of the delete pipeline.Confidence Score: 5/5
Safe to merge; the new retry sweep is fully guarded and the failing-cleanup code path falls back gracefully.
The guard sets (git worktrees, lane paths including archived, pending creations) are queried fresh on each sweep and applied at multiple checkpoints. The integration in laneService wraps the sweep in a try-catch so failures cannot break list(). Tests cover the retry-on-list flow, the archived-lane exemption, and the unknown-empty-dir boundary. The one cosmetic gap (attempts not reset to 0 on upsert conflict) has no behavioral impact since there is no retry limit.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["lane.delete()"] --> B["git worktree remove"] B -->|success| C{"directory\nstill exists?"} C -->|no| D["residualCleanup.deleteRow()"] C -->|yes| E["removeResidualDirectory()"] E -->|success| D E -->|failure, not registered| F["residualCleanup.recordFailure()\n→ inserts/upserts DB row\n→ lastSweepAt = 0"] F --> G["lane row deleted from DB\ndelete completes with warning"] B -->|failure / stale state| E H["laneService.list()"] --> I{"TTL elapsed or\nlastSweepAt = 0?"} I -->|no| J["return / join activeSweep"] I -->|yes| K["residualCleanup.retry()"] K --> L["Build guard sets:\ngitWorktreePaths\nlaneWorktreePaths\npendingCreations"] L --> M["Load DB rows → rowsByPath"] M --> N["Scan worktreesDir for\nunknown empty old dirs"] N --> O["For each candidate path"] O --> P{"guard\nchecks pass?"} P -->|no| Q["skip or deleteRow if\npath no longer exists"] P -->|yes| R["removeWorktreeDirectory()"] R -->|success| S["deleteRow() + log info"] R -->|failure| T["noteRetryFailure()\n→ attempts++, updated_at\nlog warn"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A["lane.delete()"] --> B["git worktree remove"] B -->|success| C{"directory\nstill exists?"} C -->|no| D["residualCleanup.deleteRow()"] C -->|yes| E["removeResidualDirectory()"] E -->|success| D E -->|failure, not registered| F["residualCleanup.recordFailure()\n→ inserts/upserts DB row\n→ lastSweepAt = 0"] F --> G["lane row deleted from DB\ndelete completes with warning"] B -->|failure / stale state| E H["laneService.list()"] --> I{"TTL elapsed or\nlastSweepAt = 0?"} I -->|no| J["return / join activeSweep"] I -->|yes| K["residualCleanup.retry()"] K --> L["Build guard sets:\ngitWorktreePaths\nlaneWorktreePaths\npendingCreations"] L --> M["Load DB rows → rowsByPath"] M --> N["Scan worktreesDir for\nunknown empty old dirs"] N --> O["For each candidate path"] O --> P{"guard\nchecks pass?"} P -->|no| Q["skip or deleteRow if\npath no longer exists"] P -->|yes| R["removeWorktreeDirectory()"] R -->|success| S["deleteRow() + log info"] R -->|failure| T["noteRetryFailure()\n→ attempts++, updated_at\nlog warn"]Comments Outside Diff (1)
apps/desktop/src/main/services/lanes/worktreeResidualCleanup.ts, line 392-397 (link)lastSweepAtis stamped inside the IIFE so it captures the start time, not the finish time. If a sweep takes, say, 45 s (removing a large worktree viaremoveWorktreeDirectory), the next call that arrives 16 s after the sweep started would see61 s − 0 = 61 s > 60 sand launch a second sweep immediately — withactiveSweepalready null because the first one just finished. Effectively the rest interval can be as short asTTL − sweep_duration. This is unlikely to cause correctness problems in practice (the guard sets are re-queried fresh each sweep), but the comment might mislead future readers into thinking there's a 60 s cool-down between sweep completions.Prompt To Fix With AI
Reviews (2): Last reviewed commit: "ship: iteration 1 - address review fixtu..." | Re-trigger Greptile