Repository navigation
Conversation
Resolve conflict in delete.ts: keep narrowed lock scope while adopting main's removeWorktreeFromDisk wrapper and hard-fail on teardown errors.
…ations Worktree creation held a per-project mutex for the entire init (~2-3 min of network I/O), blocking concurrent deletes and other creates. Narrow the lock to only wrap git worktree mutations (~5s), allowing network fetches, teardown scripts, and config copies to run without holding the lock.
📝 WalkthroughWalkthroughThe PR restructures workspace deletion and initialization flows by reordering lock acquisition timing and introducing multi-phase existence checks. The delete flow now executes teardown before acquiring the project lock, then re-verifies and conditionally removes the worktree under lock. The init flow refactors locking to wrap git worktree mutations and adds defensive cancellation checks throughout critical sections. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts`:
- Around line 401-423: Before calling runTeardown, add a filesystem guard that
verifies the worktree.path actually exists on disk (e.g., fs.existsSync or
fs.stat) and skip teardown if the directory is missing so we don't fail deletes;
if the path exists, call runTeardown(project.mainRepoPath, worktree.path,
worktree.branch) as before but ensure any failure is logged with a contextual
prefix (e.g., "[workspaces/delete] Teardown failed: ...") before returning the
error response instead of silently swallowing it; update the block referencing
worktreeExists, runTeardown, project.mainRepoPath, worktree.path, and
worktree.branch accordingly.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts (1)
419-432: Prefer object parameters forcreateWorktreeto avoid ordering bugs.
Line 431 passes four positional args; consider switchingcreateWorktreeto accept an options object (or add a small wrapper) so call sites are self-documenting.
As per coding guidelines: Use object parameters for functions with 2 or more parameters instead of positional arguments.apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
405-415: Consider object parameters forworktreeExists/runTeardowncalls.
These calls pass multiple positional args (Line 405, Line 411). Switching these utilities to accept an options object would align with the codebase style and reduce ordering mistakes.
As per coding guidelines: Use object parameters for functions with 2 or more parameters instead of positional arguments.
| // Check worktree existence and run teardown before acquiring lock. | ||
| // Note: teardown runs user-defined shell commands (.superset/config.json) | ||
| // which could include git operations, but not git worktree mutations | ||
| // (which is what the project lock protects). | ||
| const exists = await worktreeExists( | ||
| project.mainRepoPath, | ||
| worktree.path, | ||
| ); | ||
|
|
||
| try { | ||
| const exists = await worktreeExists( | ||
| if (exists) { | ||
| const teardownResult = await runTeardown( | ||
| project.mainRepoPath, | ||
| worktree.path, | ||
| worktree.branch, | ||
| ); | ||
|
|
||
| if (exists) { | ||
| const teardownResult = await runTeardown( | ||
| project.mainRepoPath, | ||
| worktree.path, | ||
| worktree.branch, | ||
| ); | ||
| if (!teardownResult.success) { | ||
| return { | ||
| success: false, | ||
| error: `Teardown failed: ${teardownResult.error}`, | ||
| }; | ||
| } | ||
| if (!teardownResult.success) { | ||
| return { | ||
| success: false, | ||
| error: `Teardown failed: ${teardownResult.error}`, | ||
| }; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard teardown when the worktree path is missing to avoid failed deletes.
worktreeExists can return true even if the directory was manually removed. In that case runTeardown will fail (cwd missing) and the delete aborts. Add a filesystem guard and log teardown failures before returning.
🔧 Suggested fix
const exists = await worktreeExists(
project.mainRepoPath,
worktree.path,
);
+const pathExists = existsSync(worktree.path);
-if (exists) {
+if (exists && pathExists) {
const teardownResult = await runTeardown(
project.mainRepoPath,
worktree.path,
worktree.branch,
);
if (!teardownResult.success) {
+ console.error(
+ `[worktree/delete] Teardown failed for ${worktree.path}:`,
+ teardownResult.error,
+ );
return {
success: false,
error: `Teardown failed: ${teardownResult.error}`,
};
}
+} else if (exists && !pathExists) {
+ console.warn(
+ `[worktree/delete] Skipping teardown; worktree path missing: ${worktree.path}`,
+ );
}As per coding guidelines: Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly; Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions.
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts` around
lines 401 - 423, Before calling runTeardown, add a filesystem guard that
verifies the worktree.path actually exists on disk (e.g., fs.existsSync or
fs.stat) and skip teardown if the directory is missing so we don't fail deletes;
if the path exists, call runTeardown(project.mainRepoPath, worktree.path,
worktree.branch) as before but ensure any failure is logged with a contextual
prefix (e.g., "[workspaces/delete] Teardown failed: ...") before returning the
error response instead of silently swallowing it; update the block referencing
worktreeExists, runTeardown, project.mainRepoPath, worktree.path, and
worktree.branch accordingly.
|
Closing: stale performance PR from Feb 5 with no recent activity. |
|
Hey — just a heads up, this was closed as part of an automated stale PR cleanup. If you think this was done in error, feel free to reopen it! |
|
Hey — this was closed by an automated cleanup of PRs with major merge conflicts that are 3+ weeks old. If you think this was done incorrectly, please feel free to reopen it. Sorry for any inconvenience! |
Summary
git worktree add/remove(~5s), instead of holding it for the entire init flow (~2-3 min of network I/O)existsSyncguards in post-lock phases to handle the case where a concurrent delete removes the worktree between lock release and config copydeleteWorktreeto eliminate TOCTOU window between pre-lock check and locked removalremoveWorktreeFromDiskwrapper (from main) for consistent error handlingWhat changed
workspace-init.ts— TheinitializeWorkspaceWorktreefunction is restructured into three phases:refreshDefaultBranch,branchExistsOnRemote,fetchDefaultBranch)createWorktree/createWorktreeFromExistingBranch+ cancellation cleanupexistsSyncguard for concurrent delete safety)delete.ts— ThedeleteWorktreemutation moves teardown before the lock, then re-checks existence inside the lock before callingremoveWorktreeFromDisk.Test plan
bun test— 684 pass, 0 failbun run lint:fix— no issuesbun run typecheck— passes (only pre-existingai-chatmissing-module errors)Summary by CodeRabbit