-
Notifications
You must be signed in to change notification settings - Fork 898
fix(desktop): fix workspace deletion bug #2889
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,4 @@ | ||
| import { existsSync, realpathSync } from "node:fs"; | ||
| import { resolve } from "node:path"; | ||
| import { existsSync } from "node:fs"; | ||
| import type { SelectWorktree } from "@superset/local-db"; | ||
| import { track } from "main/lib/analytics"; | ||
| import { workspaceInitManager } from "main/lib/workspace-init-manager"; | ||
|
|
@@ -21,24 +20,10 @@ import { | |
| deleteLocalBranch, | ||
| hasUncommittedChanges, | ||
| hasUnpushedCommits, | ||
| listExternalWorktrees, | ||
| worktreeExists, | ||
| } from "../utils/git"; | ||
| import { removeWorktreeFromDisk, runTeardown } from "../utils/teardown"; | ||
|
|
||
| /** | ||
| * Normalize a filesystem path for comparison. | ||
| * Uses realpathSync to resolve symlinks and get canonical path. | ||
| * Falls back to resolve if realpathSync fails (e.g., path doesn't exist). | ||
| */ | ||
| const normalizePath = (p: string): string => { | ||
| try { | ||
| return realpathSync(p); | ||
| } catch { | ||
| return resolve(p); | ||
| } | ||
| }; | ||
|
|
||
| export const createDeleteProcedures = () => { | ||
| return router({ | ||
| canDelete: publicProcedure | ||
|
|
@@ -50,6 +35,41 @@ export const createDeleteProcedures = () => { | |
| ) | ||
| .query(async ({ input }) => { | ||
| const workspace = getWorkspace(input.id); | ||
| const draftJob = workspace | ||
| ? null | ||
| : workspaceInitManager.getDraftJob(input.id); | ||
|
|
||
| if (!workspace && draftJob) { | ||
| const progress = workspaceInitManager.getProgress(input.id); | ||
| return { | ||
| canDelete: true, | ||
| reason: null, | ||
| workspace: { | ||
| id: draftJob.workspaceId, | ||
| projectId: draftJob.projectId, | ||
| worktreeId: draftJob.worktreeId, | ||
| type: "worktree" as const, | ||
| branch: draftJob.branch, | ||
| name: draftJob.workspaceName, | ||
| tabOrder: Number.MAX_SAFE_INTEGER, | ||
| createdAt: draftJob.startedAt, | ||
| updatedAt: draftJob.startedAt, | ||
| lastOpenedAt: draftJob.startedAt, | ||
| isUnread: false, | ||
| isUnnamed: draftJob.isUnnamed, | ||
| deletingAt: null, | ||
| portBase: null, | ||
| sectionId: null, | ||
| }, | ||
| warning: | ||
| progress?.step === "failed" | ||
| ? "Workspace provisioning failed before it was created" | ||
| : "Workspace is still provisioning", | ||
| activeTerminalCount: 0, | ||
| hasChanges: false, | ||
| hasUnpushedCommits: false, | ||
| }; | ||
| } | ||
|
|
||
| if (!workspace) { | ||
| return { | ||
|
|
@@ -173,6 +193,25 @@ export const createDeleteProcedures = () => { | |
| ) | ||
| .mutation(async ({ input }) => { | ||
| const workspace = getWorkspace(input.id); | ||
| const draftJob = workspace | ||
| ? null | ||
| : workspaceInitManager.getDraftJob(input.id); | ||
|
|
||
| if (!workspace && draftJob) { | ||
| if (workspaceInitManager.isInitializing(input.id)) { | ||
| console.log( | ||
| `[workspace/delete] Cancelling draft init for ${input.id}, waiting for completion...`, | ||
| ); | ||
| workspaceInitManager.cancel(input.id); | ||
| await workspaceInitManager.waitForInit(input.id, 30000); | ||
| } | ||
|
|
||
| workspaceInitManager.clearJob(input.id); | ||
| hideProjectIfNoWorkspaces(draftJob.projectId); | ||
| track("workspace_deleted", { workspace_id: input.id, draft: true }); | ||
|
|
||
| return { success: true }; | ||
|
Comment on lines
+200
to
+213
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't clear the draft job unless the initializer actually stopped.
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| if (!workspace) { | ||
| return { success: false, error: "Workspace not found" }; | ||
|
|
@@ -267,43 +306,13 @@ export const createDeleteProcedures = () => { | |
| await workspaceInitManager.acquireProjectLock(project.id); | ||
|
|
||
| try { | ||
| // Only delete from disk if this worktree was created by Superset | ||
| // External worktrees should only have their DB records removed | ||
| if (worktree.createdBySuperset) { | ||
| // Safety: Double-check it's not actually external (catches race conditions) | ||
| const externalWorktrees = await listExternalWorktrees( | ||
| project.mainRepoPath, | ||
| ); | ||
| const worktreePathNorm = normalizePath(worktree.path); | ||
| const isActuallyExternal = externalWorktrees.some( | ||
| (wt) => normalizePath(wt.path) === worktreePathNorm, | ||
| ); | ||
|
|
||
| if (isActuallyExternal) { | ||
| console.warn( | ||
| `[workspace/delete] Worktree at ${worktree.path} marked as created by Superset but found in external list - preserving as safety measure`, | ||
| ); | ||
| track("worktree_delete_safety_trigger", { | ||
| workspace_id: input.id, | ||
| worktree_id: worktree.id, | ||
| worktree_path: worktree.path, | ||
| reason: "external_detection_mismatch", | ||
| }); | ||
| } else { | ||
| // Confirmed safe to delete | ||
| const removeResult = await removeWorktreeFromDisk({ | ||
| mainRepoPath: project.mainRepoPath, | ||
| worktreePath: worktree.path, | ||
| }); | ||
| if (!removeResult.success) { | ||
| clearWorkspaceDeletingStatus(input.id); | ||
| return removeResult; | ||
| } | ||
| } | ||
| } else { | ||
| console.log( | ||
| `[workspace/delete] Skipping disk deletion for external worktree at ${worktree.path}`, | ||
| ); | ||
| const removeResult = await removeWorktreeFromDisk({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P0: Deletion no longer checks Prompt for AI agents |
||
| mainRepoPath: project.mainRepoPath, | ||
| worktreePath: worktree.path, | ||
| }); | ||
| if (!removeResult.success) { | ||
| clearWorkspaceDeletingStatus(input.id); | ||
| return removeResult; | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } finally { | ||
| workspaceInitManager.releaseProjectLock(project.id); | ||
|
|
@@ -486,67 +495,40 @@ export const createDeleteProcedures = () => { | |
| worktree.path, | ||
| ); | ||
|
|
||
| // Only delete from disk if this worktree was created by Superset | ||
| if (worktree.createdBySuperset) { | ||
| // Safety: Double-check it's not actually external (catches race conditions) | ||
| const externalWorktrees = await listExternalWorktrees( | ||
| project.mainRepoPath, | ||
| ); | ||
| const isActuallyExternal = externalWorktrees.some( | ||
| (wt) => wt.path === worktree.path, | ||
| ); | ||
|
|
||
| if (isActuallyExternal) { | ||
| console.warn( | ||
| `[worktree/delete] Worktree at ${worktree.path} marked as created by Superset but found in external list - preserving as safety measure`, | ||
| ); | ||
| track("worktree_delete_safety_trigger", { | ||
| worktree_id: input.worktreeId, | ||
| worktree_path: worktree.path, | ||
| reason: "external_detection_mismatch", | ||
| }); | ||
| } else { | ||
| // Confirmed safe to delete | ||
| if (exists) { | ||
| const teardownResult = await runTeardown({ | ||
| mainRepoPath: project.mainRepoPath, | ||
| worktreePath: worktree.path, | ||
| workspaceName: worktree.branch, | ||
| projectId: project.id, | ||
| }); | ||
| if (!teardownResult.success) { | ||
| if (input.force) { | ||
| console.warn( | ||
| `[worktree/delete] Teardown failed but force=true, continuing deletion:`, | ||
| teardownResult.error, | ||
| ); | ||
| } else { | ||
| return { | ||
| success: false, | ||
| error: `Teardown failed: ${teardownResult.error}`, | ||
| output: teardownResult.output, | ||
| }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (exists) { | ||
| const removeResult = await removeWorktreeFromDisk({ | ||
| mainRepoPath: project.mainRepoPath, | ||
| worktreePath: worktree.path, | ||
| }); | ||
| if (!removeResult.success) { | ||
| return removeResult; | ||
| } | ||
| } else { | ||
| if (exists) { | ||
| const teardownResult = await runTeardown({ | ||
| mainRepoPath: project.mainRepoPath, | ||
| worktreePath: worktree.path, | ||
| workspaceName: worktree.branch, | ||
| projectId: project.id, | ||
| }); | ||
| if (!teardownResult.success) { | ||
| if (input.force) { | ||
| console.warn( | ||
| `Worktree ${worktree.path} not found in git, skipping removal`, | ||
| `[worktree/delete] Teardown failed but force=true, continuing deletion:`, | ||
| teardownResult.error, | ||
| ); | ||
| } else { | ||
| return { | ||
| success: false, | ||
| error: `Teardown failed: ${teardownResult.error}`, | ||
| output: teardownResult.output, | ||
| }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (exists) { | ||
| const removeResult = await removeWorktreeFromDisk({ | ||
| mainRepoPath: project.mainRepoPath, | ||
| worktreePath: worktree.path, | ||
| }); | ||
| if (!removeResult.success) { | ||
| return removeResult; | ||
| } | ||
| } else { | ||
| console.log( | ||
| `[worktree/delete] Skipping disk deletion for external worktree at ${worktree.path}`, | ||
| console.warn( | ||
| `Worktree ${worktree.path} not found in git, skipping removal`, | ||
| ); | ||
| } | ||
| } finally { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: After cancelling a draft initialization, verify the initializer has actually stopped before clearing the job and returning success; otherwise a still-running init can re-persist the workspace after deletion.
Prompt for AI agents