Parallelize workspace deletion for faster teardown#1120
Conversation
📝 WalkthroughWalkthroughRestructures workspace deletion to parallelize terminal kills and teardown, run teardown via spawned shell with output and timeout, perform worktree removal via rename+background rm, use filesystem existence guards, and adjust status/lock handling and logging without changing public APIs. Changes
Sequence DiagramsequenceDiagram
participant User
participant DeleteProc as Delete Procedure
participant Status as Status DB
participant Terminal as Terminal Manager
participant Teardown as Teardown Executor
participant Git as Git Utilities
User->>DeleteProc: initiate workspace delete
DeleteProc->>Status: mark workspace as deleting / set status
par Parallel operations
DeleteProc->>Terminal: killByWorkspaceId(workspaceId)
Terminal->>Terminal: Promise.allSettled(kill panes)
Terminal-->>DeleteProc: { killed, failed }
and
DeleteProc->>Teardown: runTeardown(mainRepoPath, worktreePath, workspaceName) (if exists)
Teardown->>Teardown: spawn shell, capture stdout/stderr, enforce timeout
Teardown-->>DeleteProc: { success|fail, output }
end
DeleteProc->>Git: removeWorktree(worktreePath)
Git->>Git: rename -> tempPath (UUID), git worktree prune
Git->>Git: spawn detached /bin/rm -rf tempPath (background)
Git-->>DeleteProc: removal initiated / results
DeleteProc->>Status: update workspace_deleted tracking
DeleteProc-->>User: deletion complete (status)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
saddlepaddle
force-pushed
the
parallelize-delete
branch
from
a9690e3 to
a67c5a5
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![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/utils/teardown.ts`:
- Around line 116-118: The empty catch after the process.kill(-child.pid,
"SIGKILL") call swallows errors; replace it with a catch that logs the failure
(including child.pid and the caught error) instead of doing nothing—use the
module's existing logger if available (e.g., processLogger or logger) or
console.warn/console.debug as a fallback, and keep behavior non-fatal (do not
rethrow) so benign "no such process" errors won't break teardown.
🧹 Nitpick comments (5)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/init.ts (2)
65-69: Status-based check is correct; consider usingTRPCErrorfor consistency.The transition from
workspace.deletingAttoworkspace.status === "deleting"correctly aligns with the new status-based lifecycle. However, per coding guidelines, tRPC procedures should throwTRPCErrorwith appropriate codes rather than genericError.♻️ Suggested refactor to use TRPCError
+import { TRPCError } from "@trpc/server";if (workspace.status === "deleting") { - throw new Error( - "Cannot retry initialization on a workspace being deleted", - ); + throw new TRPCError({ + code: "BAD_REQUEST", + message: "Cannot retry initialization on a workspace being deleted", + }); }As per coding guidelines: "Use
TRPCErrorwith codeBAD_REQUESTfor invalid input that passed Zod validation" — this applies to state validation failures like attempting operations on a workspace in an incompatible lifecycle state.
59-77: Pre-existing: Error handling doesn't useTRPCErrorthroughout.The other error cases in this function also use generic
Errorinstead ofTRPCError. While not introduced by this PR, consider aligning them with guidelines in a follow-up:
- Lines 60, 72, 76: Resource not found errors should use
TRPCErrorwith codeNOT_FOUND.apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts (1)
96-100: Consider using thesetWorkspaceStatushelper for consistency.The status update works correctly, but
db-helpers.tsexports asetWorkspaceStatushelper for this purpose. Using it would consolidate status-update logic in one place and improve consistency across the codebase.+import { setWorkspaceStatus } from "./db-helpers"; ... - localDb - .update(workspaces) - .set({ status: "ready" }) - .where(eq(workspaces.id, workspaceId)) - .run(); + setWorkspaceStatus(workspaceId, "ready");This applies to all status updates in this file (lines 96-100, 254-258, 280-284, 301-305, 410-414, 444-448).
apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts (2)
128-138: Partial output lost in some error scenarios.When the process emits an error event (line 106-109), the
combinedoutput captured before the error is not included in the returnedoutputfield. Consider preserving partial output for debugging.♻️ Suggested improvement
+ let capturedOutput = ""; try { const shellEnv = await getShellEnvironment(); // ... spawn logic ... + // Store combined in outer scope for error handling const output = await new Promise<string>((resolve, reject) => { // ... + // Update: make combined accessible outside }); return { success: true, output: output || undefined }; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); console.error( `Teardown failed for workspace ${workspaceName}:`, errorMessage, ); return { success: false, error: errorMessage, - output: errorMessage, + output: capturedOutput || errorMessage, }; }
130-133: Inconsistent logging prefix.This log message lacks the
[teardown]prefix used elsewhere in the file. As per coding guidelines, use prefixed console logging with pattern[domain/operation] message.♻️ Proposed fix
console.error( - `Teardown failed for workspace ${workspaceName}:`, + `[teardown] Failed for workspace "${workspaceName}":`, errorMessage, );
| try { | ||
| if (child.pid) process.kill(-child.pid, "SIGKILL"); | ||
| } catch {} |
There was a problem hiding this comment.
Empty catch block silently swallows process.kill errors.
Per coding guidelines, errors should not be silently swallowed. While process.kill failures may be benign (e.g., process already exited), logging helps with debugging.
🔧 Proposed fix
try {
if (child.pid) process.kill(-child.pid, "SIGKILL");
- } catch {}
+ } catch (killError) {
+ console.warn(
+ `[teardown] Failed to kill process group:`,
+ killError instanceof Error ? killError.message : String(killError),
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| if (child.pid) process.kill(-child.pid, "SIGKILL"); | |
| } catch {} | |
| try { | |
| if (child.pid) process.kill(-child.pid, "SIGKILL"); | |
| } catch (killError) { | |
| console.warn( | |
| `[teardown] Failed to kill process group:`, | |
| killError instanceof Error ? killError.message : String(killError), | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts` around lines
116 - 118, The empty catch after the process.kill(-child.pid, "SIGKILL") call
swallows errors; replace it with a catch that logs the failure (including
child.pid and the caught error) instead of doing nothing—use the module's
existing logger if available (e.g., processLogger or logger) or
console.warn/console.debug as a fallback, and keep behavior non-fatal (do not
rethrow) so benign "no such process" errors won't break teardown.
a67c5a5 to
b3f8da2
Compare
saddlepaddle
force-pushed
the
parallelize-delete
branch
from
4114e44 to
cb4301a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts (1)
770-783:⚠️ Potential issue | 🟠 MajorEnsure kill runs even if history cleanup fails.
If
cleanupHistorythrows, the per-pane task rejects beforeclient.kill, which can leave sessions alive and inflatefailedcounts used for the user warning. Catch cleanup errors, still attempt kill, and log kill failures with context.💡 Suggested fix
- await this.historyManager.cleanupHistory(paneId, workspaceId); - await this.client.kill({ sessionId: paneId, deleteHistory: true }); + try { + await this.historyManager.cleanupHistory(paneId, workspaceId); + } catch (error) { + console.warn( + `[DaemonTerminalManager] Failed to cleanup history for ${paneId}:`, + error, + ); + } + try { + await this.client.kill({ sessionId: paneId, deleteHistory: true }); + } catch (error) { + console.warn( + `[DaemonTerminalManager] Failed to kill session ${paneId}:`, + error, + ); + throw error; + }As per coding guidelines: Never swallow errors silently; at minimum log them with context.
apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts (1)
129-137:⚠️ Potential issue | 🟡 MinorPrefix teardown failure logs.
This log is missing the required
[domain/operation]prefix.🔧 Suggested fix
- console.error( - `Teardown failed for workspace ${workspaceName}:`, - errorMessage, - ); + console.error( + `[teardown] Failed for workspace ${workspaceName}:`, + errorMessage, + );As per coding guidelines: Use prefixed console logging with pattern
[domain/operation] messagefor all logging.
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts`:
- Around line 236-252: The console logs in the error handling of the delete
workspace worktree removal should be prefixed with the domain/operation pattern;
update the console.error and console.warn calls in
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (the block
that checks errorMessage and references worktree.path and
clearWorkspaceDeletingStatus) to include a prefix like "[workspaces/delete]" at
the start of each message so both the error logged when removal fails and the
warning when the worktree is not found follow the "[domain/operation] message"
convention.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts (1)
39-43: Use a params object forrunTeardown.The function now has 3 positional args; switch to a named params object to match the project guideline and improve call-site clarity.
♻️ Suggested refactor
-export async function runTeardown( - mainRepoPath: string, - worktreePath: string, - workspaceName: string, -): Promise<TeardownResult> { +export async function runTeardown(params: { + mainRepoPath: string; + worktreePath: string; + workspaceName: string; +}): Promise<TeardownResult> { + const { mainRepoPath, worktreePath, workspaceName } = params;As per coding guidelines: Functions with 2+ parameters should accept a single params object with named properties instead of positional arguments.
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
196-212: PassrunTeardownparameters as an object.Align the call with the params-object guideline (and the suggested
runTeardownrefactor).♻️ Suggested change
- teardownPromise = runTeardown( - project.mainRepoPath, - wt.path, - workspace.name, - ).then((result) => { + teardownPromise = runTeardown({ + mainRepoPath: project.mainRepoPath, + worktreePath: wt.path, + workspaceName: workspace.name, + }).then((result) => {As per coding guidelines: Functions with 2+ parameters should accept a single params object with named properties instead of positional arguments.
- Run terminal kills and teardown scripts concurrently instead of sequentially - Narrow project lock scope to only git worktree removal (was held during teardown) - Replace `git worktree remove --force` with rename + prune + background rm -rf, reducing worktree removal from ~24s to <100ms - Use spawned `rm -rf` for background cleanup (Node's fs.rm hangs on macOS .app bundles) - Rewrite teardown runner with spawn for proper stdout/stderr streaming and process group timeout - Parallelize terminal session kills with Promise.allSettled
saddlepaddle
force-pushed
the
parallelize-delete
branch
from
cb4301a to
c103490
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
426-429:⚠️ Potential issue | 🟡 MinorPrefix logs in
deleteWorktreeprocedure.The
deleteWorktreeprocedure also has logs missing the[domain/operation]prefix.🔧 Suggested fix
if (!teardownResult.success) { console.error( - `Teardown failed for worktree ${worktree.branch}:`, + `[workspace/delete] Teardown failed for worktree ${worktree.branch}:`, teardownResult.error, ); } ... } else { console.warn( - `Worktree ${worktree.path} not found in git, skipping removal`, + `[workspace/delete] Worktree ${worktree.path} not found in git, skipping removal`, ); } ... - console.error("Failed to remove worktree:", errorMessage); + console.error("[workspace/delete] Failed to remove worktree:", errorMessage);As per coding guidelines: Use prefixed console logging with pattern
[domain/operation] messagefor all logging.Also applies to: 437-439, 444-444
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts`:
- Around line 681-688: The empty catch on the fallback git prune call hides
failures; update the try/catch around execFileAsync("git", ["-C", mainRepoPath,
"worktree", "prune"], { env, timeout: 10_000 }) so it logs the caught error
instead of swallowing it—use the same logger used elsewhere (or console.error if
none) and include context like mainRepoPath and the error.message; keep the
original behavior (no rethrow) but ensure failures from getGitEnv/execFileAsync
are recorded for debugging.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts (1)
56-58: Shell selection logic is duplicated fromshell-env.ts.This same pattern exists in
shell-env.ts(lines 40-41). Consider extracting a shared helper to avoid drift between the two implementations.♻️ Suggested approach
Add a shared helper in
shell-env.ts:export function getDefaultShell(): string { return process.env.SHELL || (process.platform === "darwin" ? "/bin/zsh" : "/bin/bash"); }Then import and use it here:
- const shell = - process.env.SHELL || - (process.platform === "darwin" ? "/bin/zsh" : "/bin/bash"); + const shell = getDefaultShell();apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
670-676: Consider including the exit code in the error log for easier debugging.The current log shows
exit ${code}butcodecould benullif the process was killed by a signal.🔧 Suggested enhancement
child.on("exit", (code: number | null) => { - if (code !== 0) { + if (code !== 0 && code !== null) { console.error( `[removeWorktree] Background cleanup of ${tempPath} failed (exit ${code})`, ); } });Or handle the signal case:
- child.on("exit", (code: number | null) => { + child.on("exit", (code: number | null, signal: NodeJS.Signals | null) => { if (code !== 0) { console.error( - `[removeWorktree] Background cleanup of ${tempPath} failed (exit ${code})`, + `[removeWorktree] Background cleanup of ${tempPath} failed (exit ${code}, signal ${signal})`, ); } });
| try { | ||
| const env = await getGitEnv(); | ||
| await execFileAsync("git", ["-C", mainRepoPath, "worktree", "prune"], { | ||
| env, | ||
| timeout: 10_000, | ||
| }); | ||
| } catch {} | ||
| return; |
There was a problem hiding this comment.
Empty catch block silently swallows git worktree prune errors.
When the worktree directory is already gone (ENOENT), the fallback prune operation's errors are silently discarded. Per coding guidelines, at minimum log the error for debugging.
🔧 Proposed fix
if (code === "ENOENT") {
try {
const env = await getGitEnv();
await execFileAsync("git", ["-C", mainRepoPath, "worktree", "prune"], {
env,
timeout: 10_000,
});
- } catch {}
+ } catch (pruneError) {
+ console.warn(
+ `[removeWorktree] Failed to prune after ENOENT:`,
+ pruneError instanceof Error ? pruneError.message : String(pruneError),
+ );
+ }
return;
}Based on learnings: "Never silently swallow errors with catch(() => {}) or catch(e) { return null }"
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts` around lines 681 -
688, The empty catch on the fallback git prune call hides failures; update the
try/catch around execFileAsync("git", ["-C", mainRepoPath, "worktree", "prune"],
{ env, timeout: 10_000 }) so it logs the caught error instead of swallowing
it—use the same logger used elsewhere (or console.error if none) and include
context like mainRepoPath and the error.message; keep the original behavior (no
rethrow) but ensure failures from getGitEnv/execFileAsync are recorded for
debugging.
1 participant
Summary
git worktree remove --forcewith rename + prune + backgroundrm -rf, reducing worktree removal from ~24s to <100msspawnfor proper stdout/stderr streaming and process group timeout handlingPromise.allSettledrm -rffor background cleanup (Node'sfs.rmhangs on macOS.appbundles with extended attributes)Test plan
/tmp/superset-wt-*directories accumulate over timeSummary by CodeRabbit
Bug Fixes
Performance Improvements