From a803560128b34774482e6e8eee90013344d28378 Mon Sep 17 00:00:00 2001 From: AviPeltz Date: Wed, 25 Mar 2026 17:08:46 -0700 Subject: [PATCH 1/2] feat(desktop): stage workspace provisioning before persistence --- .../routers/workspaces/procedures/create.ts | 88 +++--- .../routers/workspaces/procedures/delete.ts | 200 ++++++------ .../external-worktree-import.test.ts | 24 +- .../workspaces/procedures/git-status.ts | 6 +- .../routers/workspaces/procedures/init.ts | 176 ++++++++--- .../routers/workspaces/procedures/query.ts | 96 +++++- .../workspaces/utils/draft-workspace.ts | 40 +++ .../lib/trpc/routers/workspaces/utils/git.ts | 12 +- .../workspaces/utils/workspace-creation.ts | 8 +- .../workspaces/utils/workspace-init.ts | 120 ++++++-- .../src/main/lib/workspace-init-manager.ts | 57 +++- .../workspace/$workspaceId/page.tsx | 5 +- .../TabsContent/Terminal/Terminal.tsx | 2 +- deletionResearch.md | 288 ++++++++++++++++++ 14 files changed, 846 insertions(+), 276 deletions(-) create mode 100644 apps/desktop/src/lib/trpc/routers/workspaces/utils/draft-workspace.ts create mode 100644 deletionResearch.md diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts index 1f28701f2bd..947c405af7f 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts @@ -1,8 +1,12 @@ +import { randomUUID } from "node:crypto"; import { projects, workspaces, worktrees } from "@superset/local-db"; import { and, eq, isNull, not } from "drizzle-orm"; import { track } from "main/lib/analytics"; import { localDb } from "main/lib/local-db"; -import { workspaceInitManager } from "main/lib/workspace-init-manager"; +import { + type DraftWorkspaceProvisioningJob, + workspaceInitManager, +} from "main/lib/workspace-init-manager"; import { z } from "zod"; import { publicProcedure, router } from "../../.."; import { attemptWorkspaceAutoRenameFromPrompt } from "../utils/ai-name"; @@ -21,6 +25,7 @@ import { setLastActiveWorkspace, touchWorkspace, } from "../utils/db-helpers"; +import { buildDraftWorkspaceRow } from "../utils/draft-workspace"; import { createWorktreeFromPr, generateBranchName, @@ -29,7 +34,7 @@ import { getPrInfo, getPrLocalBranchName, listBranches, - listExternalWorktrees, + listGitWorktrees, type PullRequestInfo, parsePrUrl, safeCheckoutBranch, @@ -463,65 +468,44 @@ export const createCreateProcedures = () => { defaultBranch: project.defaultBranch, knownBranches: existingBranches, }); - - const worktree = localDb - .insert(worktrees) - .values({ - projectId: input.projectId, - path: worktreePath, - branch, - baseBranch: compareBaseBranch, - gitStatus: null, - createdBySuperset: true, - }) - .returning() - .get(); - - const maxTabOrder = getMaxProjectChildTabOrder(input.projectId); - - const workspace = localDb - .insert(workspaces) - .values({ - projectId: input.projectId, - worktreeId: worktree.id, - type: "worktree", - branch, - name: input.name ?? branch, - isUnnamed: !input.name, - tabOrder: maxTabOrder + 1, - }) - .returning() - .get(); - - setLastActiveWorkspace(workspace.id); activateProject(project); - - track("workspace_created", { - workspace_id: workspace.id, - project_id: project.id, - branch: branch, - base_branch: compareBaseBranch, - use_existing_branch: input.useExistingBranch ?? false, - }); - - await setBranchBaseConfig({ - repoPath: project.mainRepoPath, + const draftJob: DraftWorkspaceProvisioningJob = { + workspaceId: randomUUID(), + worktreeId: randomUUID(), + projectId: input.projectId, branch, + workspaceName: input.name ?? branch, + isUnnamed: !input.name, + worktreePath, + mainRepoPath: project.mainRepoPath, compareBaseBranch, - isExplicit: Boolean(requestedCompareBaseBranch?.trim()), - }); + compareBaseBranchIsExplicit: Boolean( + requestedCompareBaseBranch?.trim(), + ), + startPointBranch: sourceWorkspace?.branch, + namingPrompt: input.prompt, + useExistingBranch: input.useExistingBranch, + startedAt: Date.now(), + }; + + const workspace = buildDraftWorkspaceRow(draftJob); - workspaceInitManager.startJob(workspace.id, input.projectId); + workspaceInitManager.startJob( + draftJob.workspaceId, + input.projectId, + draftJob, + ); initializeWorkspaceWorktree({ - workspaceId: workspace.id, + workspaceId: draftJob.workspaceId, projectId: input.projectId, - worktreeId: worktree.id, + worktreeId: draftJob.worktreeId, worktreePath, branch, mainRepoPath: project.mainRepoPath, startPointBranch: sourceWorkspace?.branch, namingPrompt: input.prompt, useExistingBranch: input.useExistingBranch, + draftJob, }); const setupConfig = loadSetupConfig({ @@ -864,12 +848,10 @@ export const createCreateProcedures = () => { } // 2. Import external worktrees (on disk, not tracked in DB) - const allExternalWorktrees = await listExternalWorktrees( - project.mainRepoPath, - ); + const allGitWorktrees = await listGitWorktrees(project.mainRepoPath); const trackedPaths = new Set(projectWorktrees.map((wt) => wt.path)); - const externalWorktrees = allExternalWorktrees.filter((wt) => { + const externalWorktrees = allGitWorktrees.filter((wt) => { if (wt.path === project.mainRepoPath) return false; if (wt.isBare) return false; if (wt.isDetached) return false; diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts index 100c7028f48..09e4a48461c 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts @@ -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 }; + } 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({ + mainRepoPath: project.mainRepoPath, + worktreePath: worktree.path, + }); + if (!removeResult.success) { + clearWorkspaceDeletingStatus(input.id); + return removeResult; } } 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 { diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/external-worktree-import.test.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/external-worktree-import.test.ts index 8ddf2edae7d..1cca9110209 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/external-worktree-import.test.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/external-worktree-import.test.ts @@ -112,26 +112,28 @@ describe("External worktree detection and import", () => { expect(worktreeList).toContain("feature-external"); }); - test("listExternalWorktrees detects external worktree", async () => { + test("listGitWorktrees returns the main repo and registered worktrees", async () => { // Create external worktree createExternalWorktree(mainRepoPath, "feature-test", externalWorktreePath); - // Import the listExternalWorktrees function - const { listExternalWorktrees } = await import("../utils/git"); + // Import the listGitWorktrees function + const { listGitWorktrees } = await import("../utils/git"); - // List external worktrees - const externalWorktrees = await listExternalWorktrees(mainRepoPath); + // List registered git worktrees + const gitWorktrees = await listGitWorktrees(mainRepoPath); - // Find our external worktree - const found = externalWorktrees.find((wt) => wt.branch === "feature-test"); + // The helper includes the main repo entry and the added worktree + const mainRepoEntry = gitWorktrees.find((wt) => wt.path === mainRepoPath); + const found = gitWorktrees.find((wt) => wt.branch === "feature-test"); + expect(mainRepoEntry).toBeDefined(); expect(found).toBeDefined(); expect(found?.path).toBe(externalWorktreePath); expect(found?.isBare).toBe(false); expect(found?.isDetached).toBe(false); }); - test("external worktree data survives simulated deletion", () => { + test("external worktree can contain user data before import or deletion", () => { // Create external worktree with important data createExternalWorktree( mainRepoPath, @@ -153,12 +155,6 @@ describe("External worktree detection and import", () => { expect(existsSync(join(externalWorktreePath, "important-data.txt"))).toBe( true, ); - - // This test verifies that external worktrees are NOT deleted - // In the actual implementation, the delete procedure will check - // the createdBySuperset flag and skip disk deletion for external worktrees - - // Verify data still exists (would be deleted if we didn't have protection) expect(existsSync(join(externalWorktreePath, "important-data.txt"))).toBe( true, ); diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts index a21e718c4cb..b3e3555b70a 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts @@ -15,7 +15,7 @@ import { fetchDefaultBranch, getAheadBehindCount, getDefaultBranch, - listExternalWorktrees, + listGitWorktrees, refreshDefaultBranch, } from "../utils/git"; import { @@ -266,7 +266,7 @@ export const createGitStatusProcedures = () => { return []; } - const allWorktrees = await listExternalWorktrees(project.mainRepoPath); + const allGitWorktrees = await listGitWorktrees(project.mainRepoPath); const trackedWorktrees = localDb .select({ path: worktrees.path }) @@ -275,7 +275,7 @@ export const createGitStatusProcedures = () => { .all(); const trackedPaths = new Set(trackedWorktrees.map((wt) => wt.path)); - return allWorktrees + return allGitWorktrees .filter((wt) => { if (wt.path === project.mainRepoPath) return false; if (wt.isBare) return false; diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/init.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/init.ts index ffa81cf4886..bd9ce5884db 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/init.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/init.ts @@ -2,7 +2,10 @@ import { workspaces, worktrees } from "@superset/local-db"; import { observable } from "@trpc/server/observable"; import { eq } from "drizzle-orm"; import { localDb } from "main/lib/local-db"; -import { workspaceInitManager } from "main/lib/workspace-init-manager"; +import { + type DraftWorkspaceProvisioningJob, + workspaceInitManager, +} from "main/lib/workspace-init-manager"; import type { WorkspaceInitProgress } from "shared/types/workspace-init"; import { deduplicateBranchName } from "shared/utils/branch"; import { z } from "zod"; @@ -18,28 +21,49 @@ type WorkspaceRelations = NonNullable< ReturnType >; -function getRetryInitRelations(workspaceId: string): { - workspace: WorkspaceRelations["workspace"]; - worktree: NonNullable; - project: NonNullable; -} { +type RetryInitTarget = + | { + kind: "persisted"; + workspace: WorkspaceRelations["workspace"]; + worktree: NonNullable; + project: NonNullable; + } + | { + kind: "draft"; + draftJob: DraftWorkspaceProvisioningJob; + project: NonNullable>; + }; + +function getRetryInitTarget(workspaceId: string): RetryInitTarget { const relations = getWorkspaceWithRelations(workspaceId); - if (!relations) { - throw new Error("Workspace not found"); - } + if (relations) { + const { workspace, worktree, project } = relations; + if (workspace.deletingAt) { + throw new Error( + "Cannot retry initialization on a workspace being deleted", + ); + } + if (!worktree) { + throw new Error("Worktree not found"); + } + if (!project) { + throw new Error("Project not found"); + } - const { workspace, worktree, project } = relations; - if (workspace.deletingAt) { - throw new Error("Cannot retry initialization on a workspace being deleted"); + return { kind: "persisted", workspace, worktree, project }; } - if (!worktree) { - throw new Error("Worktree not found"); + + const draftJob = workspaceInitManager.getDraftJob(workspaceId); + if (!draftJob) { + throw new Error("Workspace not found"); } + + const project = getProject(draftJob.projectId); if (!project) { throw new Error("Project not found"); } - return { workspace, worktree, project }; + return { kind: "draft", draftJob, project }; } function persistRetryBranchUpdate({ @@ -70,38 +94,38 @@ function persistRetryBranchUpdate({ } async function resolveRetryTarget({ - workspace, - worktree, + currentBranch, + currentPath, project, deduplicateBranchName: shouldDeduplicateBranchName, + applyUpdate, }: { - workspace: WorkspaceRelations["workspace"]; - worktree: NonNullable; - project: NonNullable; + currentBranch: string; + currentPath: string; + project: NonNullable>; deduplicateBranchName: boolean; + applyUpdate: (next: { branch: string; worktreePath: string }) => void; }): Promise<{ branch: string; worktreePath: string }> { - const currentBranch = worktree.branch; - const currentPath = worktree.path; + const branch = currentBranch; + const path = currentPath; if (!shouldDeduplicateBranchName) { - return { branch: currentBranch, worktreePath: currentPath }; + return { branch, worktreePath: path }; } const { local, remote } = await listBranches(project.mainRepoPath); - const deduplicatedBranch = deduplicateBranchName(currentBranch, [ + const deduplicatedBranch = deduplicateBranchName(branch, [ ...local, ...remote, ]); - if (deduplicatedBranch === currentBranch) { - return { branch: currentBranch, worktreePath: currentPath }; + if (deduplicatedBranch === branch) { + return { branch, worktreePath: path }; } const deduplicatedPath = resolveWorktreePath(project, deduplicatedBranch); - persistRetryBranchUpdate({ - workspace, - worktreeId: worktree.id, + applyUpdate({ branch: deduplicatedBranch, - path: deduplicatedPath, + worktreePath: deduplicatedPath, }); return { branch: deduplicatedBranch, worktreePath: deduplicatedPath }; @@ -150,27 +174,81 @@ export const createInitProcedures = () => { }), ) .mutation(async ({ input }) => { - const { workspace, worktree, project } = getRetryInitRelations( - input.workspaceId, - ); - const { branch, worktreePath } = await resolveRetryTarget({ - workspace, - worktree, - project, - deduplicateBranchName: input.deduplicateBranchName, - }); + const target = getRetryInitTarget(input.workspaceId); - workspaceInitManager.clearJob(input.workspaceId); - workspaceInitManager.startJob(input.workspaceId, workspace.projectId); + if (target.kind === "persisted") { + const { workspace, worktree, project } = target; + const { branch, worktreePath } = await resolveRetryTarget({ + currentBranch: worktree.branch, + currentPath: worktree.path, + project, + deduplicateBranchName: input.deduplicateBranchName, + applyUpdate: (next) => { + persistRetryBranchUpdate({ + workspace, + worktreeId: worktree.id, + branch: next.branch, + path: next.worktreePath, + }); + }, + }); - initializeWorkspaceWorktree({ - workspaceId: input.workspaceId, - projectId: workspace.projectId, - worktreeId: worktree.id, - worktreePath, - branch, - mainRepoPath: project.mainRepoPath, - }); + workspaceInitManager.clearJob(input.workspaceId); + workspaceInitManager.startJob(input.workspaceId, workspace.projectId); + + initializeWorkspaceWorktree({ + workspaceId: input.workspaceId, + projectId: workspace.projectId, + worktreeId: worktree.id, + worktreePath, + branch, + mainRepoPath: project.mainRepoPath, + }); + } else { + const { draftJob, project } = target; + const { branch, worktreePath } = await resolveRetryTarget({ + currentBranch: draftJob.branch, + currentPath: draftJob.worktreePath, + project, + deduplicateBranchName: input.deduplicateBranchName, + applyUpdate: (next) => { + workspaceInitManager.updateDraftJob(input.workspaceId, { + branch: next.branch, + worktreePath: next.worktreePath, + workspaceName: draftJob.isUnnamed + ? next.branch + : draftJob.workspaceName, + }); + }, + }); + const nextDraftJob = { + ...(workspaceInitManager.getDraftJob(input.workspaceId) ?? + draftJob), + branch, + worktreePath, + workspaceName: draftJob.isUnnamed ? branch : draftJob.workspaceName, + }; + + workspaceInitManager.clearJob(input.workspaceId); + workspaceInitManager.startJob( + input.workspaceId, + nextDraftJob.projectId, + nextDraftJob, + ); + + initializeWorkspaceWorktree({ + workspaceId: input.workspaceId, + projectId: nextDraftJob.projectId, + worktreeId: nextDraftJob.worktreeId, + worktreePath, + branch, + mainRepoPath: project.mainRepoPath, + startPointBranch: nextDraftJob.startPointBranch, + namingPrompt: nextDraftJob.namingPrompt, + useExistingBranch: nextDraftJob.useExistingBranch, + draftJob: nextDraftJob, + }); + } return { success: true }; }), diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts index 4fa87789c46..5cc34f08fa5 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts @@ -7,9 +7,14 @@ import { import { TRPCError } from "@trpc/server"; import { eq, isNotNull, isNull } from "drizzle-orm"; import { localDb } from "main/lib/local-db"; +import { workspaceInitManager } from "main/lib/workspace-init-manager"; import { z } from "zod"; import { publicProcedure, router } from "../../.."; import { getWorkspace } from "../utils/db-helpers"; +import { + buildDraftWorkspaceRow, + buildDraftWorktreeRow, +} from "../utils/draft-workspace"; import { getProjectChildItems } from "../utils/project-children-order"; import { loadSetupConfig } from "../utils/setup"; import { computeVisualOrder } from "../utils/visual-order"; @@ -30,10 +35,21 @@ function getWorkspacesInVisualOrder(): string[] { .from(workspaces) .where(isNull(workspaces.deletingAt)) .all(); + const draftWorkspaces = workspaceInitManager + .getAllDraftJobs() + .filter( + (draft) => + !allWorkspaces.some((workspace) => workspace.id === draft.workspaceId), + ) + .map((draft) => buildDraftWorkspaceRow(draft)); const allSections = localDb.select().from(workspaceSections).all(); - return computeVisualOrder(activeProjects, allWorkspaces, allSections); + return computeVisualOrder( + activeProjects, + [...allWorkspaces, ...draftWorkspaces], + allSections, + ); } export const createQueryProcedures = () => { @@ -42,30 +58,43 @@ export const createQueryProcedures = () => { .input(z.object({ id: z.string() })) .query(async ({ input }) => { const workspace = getWorkspace(input.id); - if (!workspace) { + const draftJob = workspace + ? null + : workspaceInitManager.getDraftJob(input.id); + if (!workspace && !draftJob) { throw new TRPCError({ code: "NOT_FOUND", message: `Workspace ${input.id} not found`, }); } + const resolvedWorkspace = + workspace ?? + buildDraftWorkspaceRow(draftJob as NonNullable); const project = localDb .select() .from(projects) - .where(eq(projects.id, workspace.projectId)) + .where(eq(projects.id, resolvedWorkspace.projectId)) .get(); - const worktree = workspace.worktreeId - ? localDb - .select() - .from(worktrees) - .where(eq(worktrees.id, workspace.worktreeId)) - .get() - : null; + const worktree = workspace + ? workspace.worktreeId + ? localDb + .select() + .from(worktrees) + .where(eq(worktrees.id, workspace.worktreeId)) + .get() + : null + : draftJob + ? buildDraftWorktreeRow(draftJob) + : null; return { - ...workspace, - type: workspace.type as "worktree" | "branch", - worktreePath: getWorkspacePath(workspace) ?? "", + ...resolvedWorkspace, + type: resolvedWorkspace.type as "worktree" | "branch", + worktreePath: workspace + ? (getWorkspacePath(workspace) ?? "") + : (draftJob?.worktreePath ?? ""), + isDraft: draftJob !== null, project: project ? { id: project.id, @@ -87,11 +116,23 @@ export const createQueryProcedures = () => { }), getAll: publicProcedure.query(() => { - return localDb + const persistedWorkspaces = localDb .select() .from(workspaces) .where(isNull(workspaces.deletingAt)) - .all() + .all(); + const draftWorkspaces = workspaceInitManager + .getAllDraftJobs() + .filter( + (draft) => + !persistedWorkspaces.some( + (workspace) => workspace.id === draft.workspaceId, + ), + ) + .map((draft) => buildDraftWorkspaceRow(draft)); + + return persistedWorkspaces + .concat(draftWorkspaces) .sort((a, b) => a.tabOrder - b.tabOrder); }), @@ -112,6 +153,7 @@ export const createQueryProcedures = () => { isUnread: boolean; isUnnamed: boolean; createdBySuperset: boolean | null; + isDraft: boolean; }; type SectionItem = { @@ -224,6 +266,7 @@ export const createQueryProcedures = () => { createdBySuperset: workspace.worktreeId ? (worktreeCreatedBySupersetMap.get(workspace.worktreeId) ?? null) : null, + isDraft: false, }; if (workspace.sectionId) { @@ -242,6 +285,29 @@ export const createQueryProcedures = () => { } } + for (const draft of workspaceInitManager + .getAllDraftJobs() + .filter( + (job) => + getWorkspace(job.workspaceId) === undefined && + groupsMap.has(job.projectId), + ) + .sort((a, b) => a.startedAt - b.startedAt)) { + const group = groupsMap.get(draft.projectId); + if (!group) continue; + + group.workspaces.push({ + ...buildDraftWorkspaceRow(draft), + sectionId: null, + type: "worktree", + worktreePath: draft.worktreePath, + isUnread: false, + isUnnamed: draft.isUnnamed, + createdBySuperset: true, + isDraft: true, + }); + } + return Array.from(groupsMap.values()) .map((group) => { const projectWorkspaces = [ diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/draft-workspace.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/draft-workspace.ts new file mode 100644 index 00000000000..9a0bf9e68b6 --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/draft-workspace.ts @@ -0,0 +1,40 @@ +import type { workspaces, worktrees } from "@superset/local-db"; +import type { DraftWorkspaceProvisioningJob } from "main/lib/workspace-init-manager"; + +export function buildDraftWorkspaceRow( + draft: DraftWorkspaceProvisioningJob, +): typeof workspaces.$inferSelect { + return { + id: draft.workspaceId, + projectId: draft.projectId, + worktreeId: draft.worktreeId, + type: "worktree", + branch: draft.branch, + name: draft.workspaceName, + tabOrder: Number.MAX_SAFE_INTEGER, + createdAt: draft.startedAt, + updatedAt: draft.startedAt, + lastOpenedAt: draft.startedAt, + isUnread: false, + isUnnamed: draft.isUnnamed, + deletingAt: null, + portBase: null, + sectionId: null, + }; +} + +export function buildDraftWorktreeRow( + draft: DraftWorkspaceProvisioningJob, +): typeof worktrees.$inferSelect { + return { + id: draft.worktreeId, + projectId: draft.projectId, + path: draft.worktreePath, + branch: draft.branch, + baseBranch: draft.compareBaseBranch, + createdAt: draft.startedAt, + gitStatus: null, + githubStatus: null, + createdBySuperset: true, + }; +} diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts index 0df864005b5..456198fa716 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts @@ -727,22 +727,22 @@ export async function worktreeExists( } } -export interface ExternalWorktree { +export interface GitWorktreeEntry { path: string; branch: string | null; isDetached: boolean; isBare: boolean; } -export async function listExternalWorktrees( +export async function listGitWorktrees( mainRepoPath: string, -): Promise { +): Promise { try { const git = await getSimpleGitWithShellPath(mainRepoPath); const output = await git.raw(["worktree", "list", "--porcelain"]); - const result: ExternalWorktree[] = []; - let current: Partial = {}; + const result: GitWorktreeEntry[] = []; + let current: Partial = {}; for (const line of output.split("\n")) { if (line.startsWith("worktree ")) { @@ -775,7 +775,7 @@ export async function listExternalWorktrees( return result; } catch (error) { - console.error(`Failed to list external worktrees: ${error}`); + console.error(`Failed to list git worktrees: ${error}`); throw error; } } diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts index 52d3f6b87c0..c6148e5027b 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts @@ -12,7 +12,7 @@ import { touchWorkspace, updateActiveWorkspaceIfRemoved, } from "./db-helpers"; -import { listExternalWorktrees, worktreeExists } from "./git"; +import { listGitWorktrees, worktreeExists } from "./git"; import { resolveWorktreePath } from "./resolve-worktree-path"; import { copySupersetConfigToWorktree, loadSetupConfig } from "./setup"; @@ -109,11 +109,11 @@ export async function createWorkspaceFromExternalWorktree({ throw new Error(`Project ${projectId} not found`); } - // Check for external worktree (exists on disk but not tracked in DB) - const externalWorktrees = await listExternalWorktrees(project.mainRepoPath); + // Check for existing git worktrees, then filter down to importable external ones. + const gitWorktrees = await listGitWorktrees(project.mainRepoPath); // Filter candidates: exclude main repo, bare, and detached - const candidates = externalWorktrees.filter( + const candidates = gitWorktrees.filter( (wt) => wt.branch === branch && !wt.isBare && diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts index 25315115ed4..c7a144fccfd 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts @@ -1,12 +1,20 @@ -import { projects, worktrees } from "@superset/local-db"; +import { projects, workspaces, worktrees } from "@superset/local-db"; import { eq } from "drizzle-orm"; import { track } from "main/lib/analytics"; import { localDb } from "main/lib/local-db"; -import { workspaceInitManager } from "main/lib/workspace-init-manager"; +import { + type DraftWorkspaceProvisioningJob, + workspaceInitManager, +} from "main/lib/workspace-init-manager"; import type { WorkspaceInitStep } from "shared/types/workspace-init"; import { attemptWorkspaceAutoRenameFromPrompt } from "./ai-name"; import { resolveWorkspaceBaseBranch } from "./base-branch"; import { getBranchBaseConfig, setBranchBaseConfig } from "./base-branch-config"; +import { + activateProject, + getMaxProjectChildTabOrder, + setLastActiveWorkspace, +} from "./db-helpers"; import { branchExistsOnRemote, createWorktree, @@ -33,6 +41,8 @@ export interface WorkspaceInitParams { useExistingBranch?: boolean; /** If true, skip worktree creation (worktree already exists on disk) */ skipWorktreeCreation?: boolean; + /** If provided, persist the workspace/worktree rows only after git acquisition succeeds */ + draftJob?: DraftWorkspaceProvisioningJob; } /** @@ -52,8 +62,10 @@ export async function initializeWorkspaceWorktree({ namingPrompt, useExistingBranch, skipWorktreeCreation, + draftJob, }: WorkspaceInitParams): Promise { const manager = workspaceInitManager; + let workspacePersisted = false; const completeReadyState = async (): Promise => { let warning: string | undefined; try { @@ -101,13 +113,79 @@ export async function initializeWorkspaceWorktree({ branch, }); let effectiveCompareBaseBranch = + draftJob?.compareBaseBranch || configuredCompareBaseBranch || resolveWorkspaceBaseBranch({ workspaceBaseBranch: project?.workspaceBaseBranch, defaultBranch: project?.defaultBranch, }); - const requestedStartPoint = startPointBranch?.trim() || null; + const requestedStartPoint = + startPointBranch?.trim() || draftJob?.startPointBranch?.trim() || null; let effectiveStartPoint = requestedStartPoint ?? effectiveCompareBaseBranch; + const compareBaseWasExplicit = + draftJob?.compareBaseBranchIsExplicit ?? compareBaseBranchWasExplicit; + + const persistDraftWorkspaceIfNeeded = async (): Promise => { + if (!draftJob) return; + + const existingWorkspace = localDb + .select() + .from(workspaces) + .where(eq(workspaces.id, workspaceId)) + .get(); + + if (!existingWorkspace) { + localDb + .insert(worktrees) + .values({ + id: draftJob.worktreeId, + projectId, + path: worktreePath, + branch, + baseBranch: effectiveCompareBaseBranch, + gitStatus: null, + createdBySuperset: true, + }) + .run(); + + const maxTabOrder = getMaxProjectChildTabOrder(projectId); + localDb + .insert(workspaces) + .values({ + id: workspaceId, + projectId, + worktreeId: draftJob.worktreeId, + type: "worktree", + branch, + name: draftJob.workspaceName, + isUnnamed: draftJob.isUnnamed, + tabOrder: maxTabOrder + 1, + }) + .run(); + + setLastActiveWorkspace(workspaceId); + if (project) { + activateProject(project); + } + + track("workspace_created", { + workspace_id: workspaceId, + project_id: projectId, + branch, + base_branch: effectiveCompareBaseBranch, + use_existing_branch: useExistingBranch ?? false, + }); + } + + await setBranchBaseConfig({ + repoPath: mainRepoPath, + branch, + compareBaseBranch: effectiveCompareBaseBranch, + isExplicit: draftJob.compareBaseBranchIsExplicit, + }); + + workspacePersisted = true; + }; if (useExistingBranch) { if (skipWorktreeCreation) { @@ -158,6 +236,7 @@ export async function initializeWorkspaceWorktree({ } manager.updateProgress(workspaceId, "finalizing", "Finalizing setup..."); + await persistDraftWorkspaceIfNeeded(); localDb .update(worktrees) .set({ @@ -242,7 +321,7 @@ export async function initializeWorkspaceWorktree({ return null; } - if (compareBaseBranchWasExplicit) { + if (compareBaseWasExplicit) { console.log( `[workspace-init] ${reason}. Compare base "${effectiveCompareBaseBranch}" was explicitly set, not using fallback.`, ); @@ -291,17 +370,19 @@ export async function initializeWorkspaceWorktree({ ); effectiveCompareBaseBranch = result.fallbackBranch; effectiveStartPoint = result.fallbackBranch; - await setBranchBaseConfig({ - repoPath: mainRepoPath, - branch, - compareBaseBranch: result.fallbackBranch, - isExplicit: false, - }); - localDb - .update(worktrees) - .set({ baseBranch: result.fallbackBranch }) - .where(eq(worktrees.id, worktreeId)) - .run(); + if (!draftJob) { + await setBranchBaseConfig({ + repoPath: mainRepoPath, + branch, + compareBaseBranch: result.fallbackBranch, + isExplicit: false, + }); + localDb + .update(worktrees) + .set({ baseBranch: result.fallbackBranch }) + .where(eq(worktrees.id, worktreeId)) + .run(); + } manager.updateProgress( workspaceId, progressStep, @@ -348,7 +429,7 @@ export async function initializeWorkspaceWorktree({ workspaceId, "failed", "No local reference available", - requestedStartPoint || compareBaseBranchWasExplicit + requestedStartPoint || compareBaseWasExplicit ? `Branch "${effectiveStartPoint}" exists on remote but has not been fetched yet, and no local branch exists. Please run "git fetch origin ${effectiveStartPoint}" and try again.` : `Branch "${effectiveStartPoint}" not found locally. Please run "git fetch" and try again.`, ); @@ -387,7 +468,7 @@ export async function initializeWorkspaceWorktree({ workspaceId, "failed", "No local reference available", - requestedStartPoint || compareBaseBranchWasExplicit + requestedStartPoint || compareBaseWasExplicit ? `${failureDetail} and branch "${effectiveStartPoint}" doesn't exist locally.${isNetworkError ? " Please check your network connection and try again." : " Please try again with a different base branch."}` : `${failureDetail} and no local ref for "${effectiveStartPoint}" exists.${isNetworkError ? " Please check your network connection and try again." : ""}`, ); @@ -406,7 +487,7 @@ export async function initializeWorkspaceWorktree({ workspaceId, "failed", "No local reference available", - requestedStartPoint || compareBaseBranchWasExplicit + requestedStartPoint || compareBaseWasExplicit ? `No remote configured and branch "${effectiveStartPoint}" doesn't exist locally.` : `No remote configured and no local ref for "${effectiveStartPoint}" exists.`, ); @@ -503,6 +584,7 @@ export async function initializeWorkspaceWorktree({ } manager.updateProgress(workspaceId, "finalizing", "Finalizing setup..."); + await persistDraftWorkspaceIfNeeded(); localDb .update(worktrees) @@ -533,7 +615,7 @@ export async function initializeWorkspaceWorktree({ errorMessage, ); - if (manager.wasWorktreeCreated(workspaceId)) { + if (manager.wasWorktreeCreated(workspaceId) && !workspacePersisted) { try { await removeWorktree(mainRepoPath, worktreePath); console.log( diff --git a/apps/desktop/src/main/lib/workspace-init-manager.ts b/apps/desktop/src/main/lib/workspace-init-manager.ts index 5602e1732b1..c86ce6e4da2 100644 --- a/apps/desktop/src/main/lib/workspace-init-manager.ts +++ b/apps/desktop/src/main/lib/workspace-init-manager.ts @@ -4,12 +4,30 @@ import type { WorkspaceInitStep, } from "shared/types/workspace-init"; +export interface DraftWorkspaceProvisioningJob { + workspaceId: string; + worktreeId: string; + projectId: string; + branch: string; + workspaceName: string; + isUnnamed: boolean; + worktreePath: string; + mainRepoPath: string; + compareBaseBranch: string; + compareBaseBranchIsExplicit: boolean; + startPointBranch?: string; + namingPrompt?: string; + useExistingBranch?: boolean; + startedAt: number; +} + interface InitJob { workspaceId: string; projectId: string; progress: WorkspaceInitProgress; cancelled: boolean; worktreeCreated: boolean; // Track for cleanup on failure + draft: DraftWorkspaceProvisioningJob | null; } /** @@ -66,10 +84,46 @@ class WorkspaceInitManager extends EventEmitter { return Array.from(this.jobs.values()).map((job) => job.progress); } + hasJob(workspaceId: string): boolean { + return this.jobs.has(workspaceId); + } + + getDraftJob(workspaceId: string): DraftWorkspaceProvisioningJob | null { + return this.jobs.get(workspaceId)?.draft ?? null; + } + + getAllDraftJobs(): DraftWorkspaceProvisioningJob[] { + return Array.from(this.jobs.values()) + .map((job) => job.draft) + .filter( + (draft): draft is DraftWorkspaceProvisioningJob => draft !== null, + ); + } + + updateDraftJob( + workspaceId: string, + patch: Partial, + ): void { + const job = this.jobs.get(workspaceId); + if (!job?.draft) { + console.warn(`[workspace-init] No draft job found for ${workspaceId}`); + return; + } + + job.draft = { + ...job.draft, + ...patch, + }; + } + /** * Start tracking a new initialization job */ - startJob(workspaceId: string, projectId: string): void { + startJob( + workspaceId: string, + projectId: string, + draft?: DraftWorkspaceProvisioningJob, + ): void { if (this.jobs.has(workspaceId)) { console.warn( `[workspace-init] Job already exists for ${workspaceId}, clearing old job`, @@ -102,6 +156,7 @@ class WorkspaceInitManager extends EventEmitter { progress, cancelled: false, worktreeCreated: false, + draft: draft ?? null, }); this.emit("progress", progress); diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx index 3fc0082907b..93f23893cdf 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx @@ -91,15 +91,16 @@ function WorkspacePage() { const { data: workspace } = electronTrpc.workspaces.get.useQuery({ id: workspaceId, }); + const isDraftWorkspace = workspace?.isDraft ?? false; useWorkspaceFileEventBridge( workspaceId, workspace?.worktreePath, - Boolean(workspace?.worktreePath), + Boolean(workspace?.worktreePath) && !isDraftWorkspace, ); useWorkspaceRenameReconciliation({ workspaceId, worktreePath: workspace?.worktreePath, - enabled: Boolean(workspace?.worktreePath), + enabled: Boolean(workspace?.worktreePath) && !isDraftWorkspace, }); const navigate = useNavigate(); const routeNavigate = Route.useNavigate(); diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx index cfcea7d20d2..582b926d64c 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx @@ -60,7 +60,7 @@ export const Terminal = ({ paneId, tabId, workspaceId }: TerminalProps) => { const defaultRestartCommandRef = useRef(undefined); defaultRestartCommandRef.current = (isWorkspaceRunPane - ? (buildTerminalCommand(workspaceRunConfig?.commands) ?? undefined) + ? buildTerminalCommand(workspaceRunConfig?.commands) : undefined) ?? pane?.workspaceRun?.command; const utils = electronTrpc.useUtils(); diff --git a/deletionResearch.md b/deletionResearch.md new file mode 100644 index 00000000000..72cda91920d --- /dev/null +++ b/deletionResearch.md @@ -0,0 +1,288 @@ +# Worktree Deletion Research + +## Scope + +This document reviews: + +- PR [#2573](https://github.com/superset-sh/superset/pull/2573), merged on March 22, 2026 +- the current local code in this worktree +- the original failure mode: an external worktree failed to attach, then deleting the failed workspace deleted the user's real worktree + +I did not use `WORKTREE_DELETION_ANALYSIS.md`. + +## Short version + +The PR solved the right symptom in the wrong place. + +The original bug was: + +1. Superset created a DB-backed workspace for a branch +2. background init later tried `git worktree add` +3. that failed because the worktree already existed outside Superset +4. the failed workspace record still pointed at the real external worktree path +5. deleting the failed workspace removed that path from disk + +The PR tried to prevent this by making delete treat "external worktrees" as undeletable. The problem is that the helper used for that safety check does not actually return external worktrees. It returns every worktree in `git worktree list --porcelain`, including Superset-created ones and the main repo. That means the delete path is using the wrong primitive and broadly preserves worktrees that should be deleted. + +So the current design has two issues: + +- it handles the problem at delete time instead of fixing the failed-import/create path +- the delete-time safety check is logically wrong and causes deletion regressions + +## What PR #2573 changed + +The important parts were: + +1. Added `worktrees.createdBySuperset` in [packages/local-db/src/schema/schema.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/packages/local-db/src/schema/schema.ts) +2. Added auto-import/open flows for existing worktrees in [apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts) +3. Changed deletion to skip disk removal if the worktree looked "external" in [apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts) + +The good part is the import work: + +- `createWorkspaceFromExternalWorktree()` in [workspace-creation.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts#L96) is directionally correct +- imported worktrees get `createdBySuperset: false` +- that is a reasonable ownership model + +The bad part is the delete safety logic: + +- workspace delete uses `listExternalWorktrees()` at [delete.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L266) +- worktree delete uses the same helper at [delete.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L489) + +## The core bug in the current implementation + +`listExternalWorktrees()` is misnamed. + +See [apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts#L737). + +It is just a parser for: + +```bash +git worktree list --porcelain +``` + +That command returns: + +- the main repo +- every registered worktree +- no distinction between "Superset-created" and "external" + +So the delete logic is effectively doing: + +- "if this worktree appears in git's worktree list, treat it as external and preserve it" + +But every valid worktree appears in git's worktree list. + +That makes the "double-check" invalid by definition. + +## Why this breaks deletion + +### Workspace delete + +At [delete.ts:272](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L272), if `createdBySuperset` is true, deletion still asks `listExternalWorktrees()` whether the path is "actually external". + +At [delete.ts:278](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L278), the match is: + +```ts +externalWorktrees.some((wt) => normalizePath(wt.path) === worktreePathNorm) +``` + +Because the current helper includes all registered worktrees, that condition is true for normal Superset worktrees too. The code then skips `removeWorktreeFromDisk()`. + +Result: + +- DB records are deleted +- the worktree remains on disk and stays registered in git +- from a user perspective, deletion looks broken or incomplete + +### Worktree delete + +At [delete.ts:495](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L495), `deleteWorktree` does the same thing, but with an even weaker comparison: + +```ts +externalWorktrees.some((wt) => wt.path === worktree.path) +``` + +That has two problems: + +1. same logical problem: the helper returns all worktrees +2. no normalization, unlike the workspace delete path + +So the two delete flows are inconsistent, and the supposedly safer one is also less robust on macOS path aliases/symlinks. + +## Why the original bug happened + +The underlying bug is earlier in the lifecycle, not in delete. + +Superset creates DB records first, then does real git work in the background in [apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts#L466). + +The critical sequence is: + +1. create mutation inserts a `worktrees` row with `createdBySuperset: true` +2. create mutation inserts a `workspaces` row +3. init later calls `createWorktree(...)` +4. if `git worktree add` fails because the worktree already exists, init marks the workspace failed +5. that failed workspace still points at the external worktree path +6. delete later trusts the DB row and removes the path + +Relevant code: + +- worktree row creation in [create.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts#L460) +- background git creation in [workspace-init.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts#L466) +- init failure handling in [workspace-init.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts#L529) + +The key detail is that init only cleans up the worktree on failure if Superset knows it created it during this process: + +```ts +if (manager.wasWorktreeCreated(workspaceId)) { + await removeWorktree(...) +} +``` + +That is correct. The dangerous deletion happens later, after a failed row is already persisted. + +## What is worth keeping from PR #2573 + +I would keep these parts: + +1. `createdBySuperset` +2. external worktree import/open flows +3. bulk import of existing untracked worktrees + +These parts are useful and align with the real model: + +- Superset-created worktree: Superset can delete from disk +- imported external worktree: Superset should only detach from DB unless the user explicitly asks for destructive deletion + +The part I would remove is the delete-time "double-check" based on `listExternalWorktrees()`. + +## Why a straight revert is not enough + +Reverting the whole PR would restore previous deletion behavior, but it would also remove the good parts: + +- auto-import of existing worktrees +- ownership tracking +- explicit representation of imported worktrees + +That would likely reintroduce the original data-loss path. + +So I do not recommend a full revert of `#2573`. + +I recommend: + +1. revert the broken delete-side safety logic +2. keep the import/ownership model +3. fix the failed-create path so the bad state never survives to deletion + +## Recommended fix + +### Phase 1: Fix deletion regression immediately + +Remove the delete-time `listExternalWorktrees()` safety checks from: + +- [apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L266) +- [apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L489) + +Deletion should go back to this rule: + +- if `createdBySuperset === false`, remove only DB records +- if `createdBySuperset === true`, allow `removeWorktreeFromDisk()` + +That restores expected behavior for normal Superset worktrees. + +### Phase 2: Fix the actual root cause + +When worktree creation fails because the target already exists, Superset should adopt/import that worktree instead of leaving a failed Superset-owned record behind. + +The right place for this is the failure path in [workspace-init.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts#L529), not delete. + +Specifically: + +1. detect git errors that mean "worktree already exists" or "branch already checked out" +2. inspect current `git worktree list --porcelain` +3. if the target branch/path is present, convert the DB record into an imported worktree: + - set `createdBySuperset = false` + - if needed, update `worktrees.path` to the discovered path + - seed `gitStatus` + - copy config + - mark init as ready with a warning like "Opened existing worktree" +4. only leave the workspace in failed state if adoption also fails + +That directly fixes the user story that motivated the PR. + +## Stronger model if we want this to stay correct + +`createdBySuperset` is useful, but it is not enough for the provisioning race/failure window because the row is inserted before git creation succeeds. + +The more correct model is: + +- `createdBySuperset`: ownership intent +- `provisioningState`: `pending | ready | failed | imported` +- maybe `createdOnDiskBySupersetAt` or a boolean set only after `createWorktree()` succeeds + +Then delete can make a safer decision: + +- imported external: DB-only delete +- ready and created-on-disk-by-Superset: remove from disk +- failed before creation completed: never assume disk ownership + +That is the most defensible long-term fix, but it is more invasive than the immediate repair above. + +## Concrete implementation plan + +### Minimal practical fix + +1. Rename `listExternalWorktrees()` to `listGitWorktrees()` because that is what it actually does. +2. Update creation/import helpers to use a new filtered helper, something like `listImportableWorktrees()`: + - exclude `project.mainRepoPath` + - exclude bare and detached entries + - exclude DB-tracked paths + - normalize paths before comparing +3. Remove delete-side use of `listExternalWorktrees()` entirely. +4. In `workspace-init.ts`, adopt existing worktrees on create conflict instead of leaving a failed Superset-owned row. + +### If we want the smallest possible code change first + +1. Remove only the delete-side "double-check" logic +2. keep the `createdBySuperset` guard +3. ship that to restore normal deletion +4. then add init-time adoption in a follow-up + +This is probably the safest rollout path. + +## Testing gaps in the current PR + +The current test file is [external-worktree-import.test.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/external-worktree-import.test.ts). + +It does not actually test the real deletion mutation. + +Current coverage is mostly: + +- can create an external worktree +- can parse `git worktree list` +- "simulated deletion" by checking the file still exists without calling delete code + +That is why the current regression could pass. + +Tests that should exist: + +1. create a normal Superset-owned worktree, call workspace delete, assert it is removed from git and from disk +2. create an imported external worktree, call workspace delete, assert DB records are removed but disk path still exists +3. simulate failed create where a matching worktree already exists, then assert init adopts/imports it instead of leaving a dangerous failed row +4. exercise both `delete` and `deleteWorktree` + +## Recommendation + +Recommended path: + +1. Do not fully revert `#2573` +2. Revert only the broken delete-side "external list" safety logic +3. Keep `createdBySuperset` and the import/open flows +4. Fix `workspace-init.ts` so failed creation adopts existing worktrees instead of leaving a bad record behind +5. Add real delete mutation tests before merging + +If we have to choose between "revert now" and "fix now", the best pragmatic version is: + +- restore deletion behavior immediately by removing the bogus `listExternalWorktrees()` checks +- then land the init-time adoption fix right after + +That gets back correct deletion semantics without giving up the useful external-import behavior. From 77ff7fbd4a603f6430a4248d3aa6c8462410fc1d Mon Sep 17 00:00:00 2001 From: AviPeltz Date: Wed, 25 Mar 2026 17:47:24 -0700 Subject: [PATCH 2/2] chore: remove local deletion notes from PR --- deletionResearch.md | 288 -------------------------------------------- 1 file changed, 288 deletions(-) delete mode 100644 deletionResearch.md diff --git a/deletionResearch.md b/deletionResearch.md deleted file mode 100644 index 72cda91920d..00000000000 --- a/deletionResearch.md +++ /dev/null @@ -1,288 +0,0 @@ -# Worktree Deletion Research - -## Scope - -This document reviews: - -- PR [#2573](https://github.com/superset-sh/superset/pull/2573), merged on March 22, 2026 -- the current local code in this worktree -- the original failure mode: an external worktree failed to attach, then deleting the failed workspace deleted the user's real worktree - -I did not use `WORKTREE_DELETION_ANALYSIS.md`. - -## Short version - -The PR solved the right symptom in the wrong place. - -The original bug was: - -1. Superset created a DB-backed workspace for a branch -2. background init later tried `git worktree add` -3. that failed because the worktree already existed outside Superset -4. the failed workspace record still pointed at the real external worktree path -5. deleting the failed workspace removed that path from disk - -The PR tried to prevent this by making delete treat "external worktrees" as undeletable. The problem is that the helper used for that safety check does not actually return external worktrees. It returns every worktree in `git worktree list --porcelain`, including Superset-created ones and the main repo. That means the delete path is using the wrong primitive and broadly preserves worktrees that should be deleted. - -So the current design has two issues: - -- it handles the problem at delete time instead of fixing the failed-import/create path -- the delete-time safety check is logically wrong and causes deletion regressions - -## What PR #2573 changed - -The important parts were: - -1. Added `worktrees.createdBySuperset` in [packages/local-db/src/schema/schema.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/packages/local-db/src/schema/schema.ts) -2. Added auto-import/open flows for existing worktrees in [apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts) -3. Changed deletion to skip disk removal if the worktree looked "external" in [apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts) - -The good part is the import work: - -- `createWorkspaceFromExternalWorktree()` in [workspace-creation.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts#L96) is directionally correct -- imported worktrees get `createdBySuperset: false` -- that is a reasonable ownership model - -The bad part is the delete safety logic: - -- workspace delete uses `listExternalWorktrees()` at [delete.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L266) -- worktree delete uses the same helper at [delete.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L489) - -## The core bug in the current implementation - -`listExternalWorktrees()` is misnamed. - -See [apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts#L737). - -It is just a parser for: - -```bash -git worktree list --porcelain -``` - -That command returns: - -- the main repo -- every registered worktree -- no distinction between "Superset-created" and "external" - -So the delete logic is effectively doing: - -- "if this worktree appears in git's worktree list, treat it as external and preserve it" - -But every valid worktree appears in git's worktree list. - -That makes the "double-check" invalid by definition. - -## Why this breaks deletion - -### Workspace delete - -At [delete.ts:272](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L272), if `createdBySuperset` is true, deletion still asks `listExternalWorktrees()` whether the path is "actually external". - -At [delete.ts:278](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L278), the match is: - -```ts -externalWorktrees.some((wt) => normalizePath(wt.path) === worktreePathNorm) -``` - -Because the current helper includes all registered worktrees, that condition is true for normal Superset worktrees too. The code then skips `removeWorktreeFromDisk()`. - -Result: - -- DB records are deleted -- the worktree remains on disk and stays registered in git -- from a user perspective, deletion looks broken or incomplete - -### Worktree delete - -At [delete.ts:495](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L495), `deleteWorktree` does the same thing, but with an even weaker comparison: - -```ts -externalWorktrees.some((wt) => wt.path === worktree.path) -``` - -That has two problems: - -1. same logical problem: the helper returns all worktrees -2. no normalization, unlike the workspace delete path - -So the two delete flows are inconsistent, and the supposedly safer one is also less robust on macOS path aliases/symlinks. - -## Why the original bug happened - -The underlying bug is earlier in the lifecycle, not in delete. - -Superset creates DB records first, then does real git work in the background in [apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts#L466). - -The critical sequence is: - -1. create mutation inserts a `worktrees` row with `createdBySuperset: true` -2. create mutation inserts a `workspaces` row -3. init later calls `createWorktree(...)` -4. if `git worktree add` fails because the worktree already exists, init marks the workspace failed -5. that failed workspace still points at the external worktree path -6. delete later trusts the DB row and removes the path - -Relevant code: - -- worktree row creation in [create.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts#L460) -- background git creation in [workspace-init.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts#L466) -- init failure handling in [workspace-init.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts#L529) - -The key detail is that init only cleans up the worktree on failure if Superset knows it created it during this process: - -```ts -if (manager.wasWorktreeCreated(workspaceId)) { - await removeWorktree(...) -} -``` - -That is correct. The dangerous deletion happens later, after a failed row is already persisted. - -## What is worth keeping from PR #2573 - -I would keep these parts: - -1. `createdBySuperset` -2. external worktree import/open flows -3. bulk import of existing untracked worktrees - -These parts are useful and align with the real model: - -- Superset-created worktree: Superset can delete from disk -- imported external worktree: Superset should only detach from DB unless the user explicitly asks for destructive deletion - -The part I would remove is the delete-time "double-check" based on `listExternalWorktrees()`. - -## Why a straight revert is not enough - -Reverting the whole PR would restore previous deletion behavior, but it would also remove the good parts: - -- auto-import of existing worktrees -- ownership tracking -- explicit representation of imported worktrees - -That would likely reintroduce the original data-loss path. - -So I do not recommend a full revert of `#2573`. - -I recommend: - -1. revert the broken delete-side safety logic -2. keep the import/ownership model -3. fix the failed-create path so the bad state never survives to deletion - -## Recommended fix - -### Phase 1: Fix deletion regression immediately - -Remove the delete-time `listExternalWorktrees()` safety checks from: - -- [apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L266) -- [apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts#L489) - -Deletion should go back to this rule: - -- if `createdBySuperset === false`, remove only DB records -- if `createdBySuperset === true`, allow `removeWorktreeFromDisk()` - -That restores expected behavior for normal Superset worktrees. - -### Phase 2: Fix the actual root cause - -When worktree creation fails because the target already exists, Superset should adopt/import that worktree instead of leaving a failed Superset-owned record behind. - -The right place for this is the failure path in [workspace-init.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts#L529), not delete. - -Specifically: - -1. detect git errors that mean "worktree already exists" or "branch already checked out" -2. inspect current `git worktree list --porcelain` -3. if the target branch/path is present, convert the DB record into an imported worktree: - - set `createdBySuperset = false` - - if needed, update `worktrees.path` to the discovered path - - seed `gitStatus` - - copy config - - mark init as ready with a warning like "Opened existing worktree" -4. only leave the workspace in failed state if adoption also fails - -That directly fixes the user story that motivated the PR. - -## Stronger model if we want this to stay correct - -`createdBySuperset` is useful, but it is not enough for the provisioning race/failure window because the row is inserted before git creation succeeds. - -The more correct model is: - -- `createdBySuperset`: ownership intent -- `provisioningState`: `pending | ready | failed | imported` -- maybe `createdOnDiskBySupersetAt` or a boolean set only after `createWorktree()` succeeds - -Then delete can make a safer decision: - -- imported external: DB-only delete -- ready and created-on-disk-by-Superset: remove from disk -- failed before creation completed: never assume disk ownership - -That is the most defensible long-term fix, but it is more invasive than the immediate repair above. - -## Concrete implementation plan - -### Minimal practical fix - -1. Rename `listExternalWorktrees()` to `listGitWorktrees()` because that is what it actually does. -2. Update creation/import helpers to use a new filtered helper, something like `listImportableWorktrees()`: - - exclude `project.mainRepoPath` - - exclude bare and detached entries - - exclude DB-tracked paths - - normalize paths before comparing -3. Remove delete-side use of `listExternalWorktrees()` entirely. -4. In `workspace-init.ts`, adopt existing worktrees on create conflict instead of leaving a failed Superset-owned row. - -### If we want the smallest possible code change first - -1. Remove only the delete-side "double-check" logic -2. keep the `createdBySuperset` guard -3. ship that to restore normal deletion -4. then add init-time adoption in a follow-up - -This is probably the safest rollout path. - -## Testing gaps in the current PR - -The current test file is [external-worktree-import.test.ts](/Users/avipeltz/.superset/worktrees/superset/fix-worktree-deletion/apps/desktop/src/lib/trpc/routers/workspaces/procedures/external-worktree-import.test.ts). - -It does not actually test the real deletion mutation. - -Current coverage is mostly: - -- can create an external worktree -- can parse `git worktree list` -- "simulated deletion" by checking the file still exists without calling delete code - -That is why the current regression could pass. - -Tests that should exist: - -1. create a normal Superset-owned worktree, call workspace delete, assert it is removed from git and from disk -2. create an imported external worktree, call workspace delete, assert DB records are removed but disk path still exists -3. simulate failed create where a matching worktree already exists, then assert init adopts/imports it instead of leaving a dangerous failed row -4. exercise both `delete` and `deleteWorktree` - -## Recommendation - -Recommended path: - -1. Do not fully revert `#2573` -2. Revert only the broken delete-side "external list" safety logic -3. Keep `createdBySuperset` and the import/open flows -4. Fix `workspace-init.ts` so failed creation adopts existing worktrees instead of leaving a bad record behind -5. Add real delete mutation tests before merging - -If we have to choose between "revert now" and "fix now", the best pragmatic version is: - -- restore deletion behavior immediately by removing the bogus `listExternalWorktrees()` checks -- then land the init-time adoption fix right after - -That gets back correct deletion semantics without giving up the useful external-import behavior.