From 2ca91ac275c5fc5f9927063cfa9d6f1254a6eb88 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Thu, 30 Apr 2026 11:52:50 -0700 Subject: [PATCH 1/5] feat(desktop): align v2 delete-workspace dialog with host-service saga MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-locate the v2 delete dialog's preflight on the same host-service that runs `destroy`, so they can never disagree about what's blocked vs warned. Fixes: - main-workspace path comparison was raw string equality (would fail open under symlinks / trailing slash / macOS case folding); now realpath-normalized in a shared isMainWorkspace() helper used by both inspect and destroy - hasUnpushedCommits silently missed branches with no upstream; inspect now uses `rev-list HEAD --not --remotes` which catches never-pushed branches - two concurrent destroys could race phase 3b; host-service now guards with a process-local in-flight Set and surfaces a typed DELETE_IN_PROGRESS cause distinct from dirty-worktree CONFLICT - pending-host states (loading, local-starting) used to flicker as a destructive banner; now render as a disabled "Checking…" button Audit + decisions captured in plans/20260430-v2-delete-workspace-audit.md. --- .../host-service/useDestroyWorkspace/index.ts | 1 + .../useDestroyWorkspace.ts | 100 +++-- .../host-service/useWorkspaceHostUrl/index.ts | 6 +- .../useWorkspaceHostUrl.ts | 52 ++- .../DashboardSidebarDeleteDialog.tsx | 3 + .../DestroyConfirmPane/DestroyConfirmPane.tsx | 15 +- .../useDestroyDialogState.ts | 90 +++-- packages/host-service/src/index.ts | 5 +- packages/host-service/src/trpc/error-types.ts | 21 + packages/host-service/src/trpc/index.ts | 7 + .../workspace-cleanup/is-main-workspace.ts | 68 ++++ .../workspace-cleanup/workspace-cleanup.ts | 372 +++++++++++------- plans/20260430-v2-delete-workspace-audit.md | 52 +++ 13 files changed, 589 insertions(+), 203 deletions(-) create mode 100644 packages/host-service/src/trpc/router/workspace-cleanup/is-main-workspace.ts create mode 100644 plans/20260430-v2-delete-workspace-audit.md diff --git a/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/index.ts b/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/index.ts index 18470f70f61..51ef2498031 100644 --- a/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/index.ts +++ b/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/index.ts @@ -1,6 +1,7 @@ export { type DestroyWorkspaceError, type DestroyWorkspaceInput, + type DestroyWorkspacePreview, type DestroyWorkspaceSuccess, type UseDestroyWorkspace, useDestroyWorkspace, diff --git a/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts b/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts index 83e2681d7e8..fb63674a154 100644 --- a/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts +++ b/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts @@ -1,8 +1,14 @@ -import type { TeardownFailureCause } from "@superset/host-service"; +import type { + DeleteInProgressCause, + TeardownFailureCause, +} from "@superset/host-service"; import { TRPCClientError } from "@trpc/client"; import { useCallback } from "react"; import { getHostServiceClientByUrl } from "renderer/lib/host-service-client"; -import { useWorkspaceHostUrl } from "../useWorkspaceHostUrl"; +import { + useWorkspaceHostTarget, + type WorkspaceHostTarget, +} from "../useWorkspaceHostUrl"; export interface DestroyWorkspaceInput { deleteBranch?: boolean; @@ -17,65 +23,96 @@ export interface DestroyWorkspaceSuccess { warnings: string[]; } +export interface DestroyWorkspacePreview { + canDelete: boolean; + reason: string | null; + hasChanges: boolean; + hasUnpushedCommits: boolean; +} + export type DestroyWorkspaceError = | { kind: "conflict"; message: string } + | { kind: "in-progress"; message: string } | { kind: "teardown-failed"; cause: TeardownFailureCause } + | { kind: "host-unavailable"; reason: WorkspaceHostTarget["status"] } | { kind: "unknown"; message: string }; export interface UseDestroyWorkspace { + hostTarget: WorkspaceHostTarget; destroy: (input?: DestroyWorkspaceInput) => Promise; + inspect: () => Promise; } /** - * Calls `workspaceCleanup.destroy` on the workspace's owning host-service. - * Translates TRPC errors into a typed discriminated union so callers can - * prompt for `force: true` on conflict or teardown failure. - * - * Throws a DestroyWorkspaceError (not a TRPCClientError) for easier handling. + * Calls `workspaceCleanup.{inspect,destroy}` on the workspace's owning + * host-service. Translates TRPC errors into a typed discriminated union + * so callers can: + * - silently retry with `force: true` on `conflict` (dirty-worktree race) + * - surface a toast on `in-progress` (concurrent destroy) — must NOT retry + * - prompt force-retry on `teardown-failed` + * - render `host-unavailable` as a checking-status spinner, not an error */ export function useDestroyWorkspace(workspaceId: string): UseDestroyWorkspace { - const hostUrl = useWorkspaceHostUrl(workspaceId); + const hostTarget = useWorkspaceHostTarget(workspaceId); const destroy = useCallback( async ( input: DestroyWorkspaceInput = {}, ): Promise => { - if (!hostUrl) { - throw { - kind: "unknown", - message: "Host unavailable", - } satisfies DestroyWorkspaceError; - } - - const client = getHostServiceClientByUrl(hostUrl); + const client = getReadyClient(hostTarget); try { - const result = await client.workspaceCleanup.destroy.mutate({ + return await client.workspaceCleanup.destroy.mutate({ workspaceId, deleteBranch: input.deleteBranch ?? false, force: input.force ?? false, }); - return result; } catch (err) { throw normalizeError(err); } }, - [hostUrl, workspaceId], + [hostTarget, workspaceId], ); - return { destroy }; + const inspect = useCallback(async (): Promise => { + const client = getReadyClient(hostTarget); + try { + return await client.workspaceCleanup.inspect.query({ workspaceId }); + } catch (err) { + throw normalizeError(err); + } + }, [hostTarget, workspaceId]); + + return { hostTarget, destroy, inspect }; +} + +function getReadyClient(hostTarget: WorkspaceHostTarget) { + if (hostTarget.status !== "ready") { + throw { + kind: "host-unavailable", + reason: hostTarget.status, + } satisfies DestroyWorkspaceError; + } + return getHostServiceClientByUrl(hostTarget.url); } function normalizeError(err: unknown): DestroyWorkspaceError { + if (isDestroyWorkspaceError(err)) return err; if (err instanceof TRPCClientError) { - const code = err.data?.code as string | undefined; - const teardownFailure = ( - err.data as { teardownFailure?: TeardownFailureCause } - )?.teardownFailure; + const data = err.data as + | { + code?: string; + teardownFailure?: TeardownFailureCause; + deleteInProgress?: DeleteInProgressCause; + } + | undefined; - if (teardownFailure) { - return { kind: "teardown-failed", cause: teardownFailure }; + if (data?.teardownFailure) { + return { kind: "teardown-failed", cause: data.teardownFailure }; + } + if (data?.deleteInProgress) { + return { kind: "in-progress", message: err.message }; } - if (code === "CONFLICT") { + if (data?.code === "CONFLICT") { return { kind: "conflict", message: err.message }; } return { kind: "unknown", message: err.message }; @@ -85,3 +122,12 @@ function normalizeError(err: unknown): DestroyWorkspaceError { message: err instanceof Error ? err.message : String(err), }; } + +function isDestroyWorkspaceError(err: unknown): err is DestroyWorkspaceError { + return ( + !!err && + typeof err === "object" && + "kind" in err && + typeof (err as { kind: unknown }).kind === "string" + ); +} diff --git a/apps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/index.ts b/apps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/index.ts index 4b07976d824..99fe2f43eb0 100644 --- a/apps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/index.ts +++ b/apps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/index.ts @@ -1 +1,5 @@ -export { useWorkspaceHostUrl } from "./useWorkspaceHostUrl"; +export { + useWorkspaceHostTarget, + useWorkspaceHostUrl, + type WorkspaceHostTarget, +} from "./useWorkspaceHostUrl"; diff --git a/apps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/useWorkspaceHostUrl.ts b/apps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/useWorkspaceHostUrl.ts index b6d435675f9..6c681954046 100644 --- a/apps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/useWorkspaceHostUrl.ts +++ b/apps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/useWorkspaceHostUrl.ts @@ -6,15 +6,26 @@ import { env } from "renderer/env.renderer"; import { useCollections } from "renderer/routes/_authenticated/providers/CollectionsProvider"; import { useLocalHostService } from "renderer/routes/_authenticated/providers/LocalHostServiceProvider"; +export type WorkspaceHostTarget = + | { status: "loading" } + | { status: "not-found" } + | { status: "local-starting"; hostId: string } + | { status: "ready"; kind: "local" | "remote"; hostId: string; url: string }; + /** - * Resolves a workspace ID to its host-service URL. - * Local host → localhost port. Remote host → relay proxy URL. + * Resolves a workspace ID to its owning host-service target. + * + * The status union lets callers distinguish "still loading the collection" + * from "local host hasn't booted yet" from "workspace doesn't exist on this + * client" — three states the previous `string | null` API collapsed into one. */ -export function useWorkspaceHostUrl(workspaceId: string | null): string | null { +export function useWorkspaceHostTarget( + workspaceId: string | null, +): WorkspaceHostTarget { const collections = useCollections(); const { machineId, activeHostUrl } = useLocalHostService(); - const { data: workspaceRows = [] } = useLiveQuery( + const { data: workspaceRows = [], isReady } = useLiveQuery( (q) => q .from({ workspaces: collections.v2Workspaces }) @@ -29,9 +40,34 @@ export function useWorkspaceHostUrl(workspaceId: string | null): string | null { const match = workspaceId ? (workspaceRows[0] ?? null) : null; return useMemo(() => { - if (!match) return null; - if (match.hostId === machineId) return activeHostUrl; + if (!workspaceId || !isReady) return { status: "loading" }; + if (!match) return { status: "not-found" }; + if (machineId && match.hostId === machineId) { + if (activeHostUrl) { + return { + status: "ready", + kind: "local", + hostId: match.hostId, + url: activeHostUrl, + }; + } + return { status: "local-starting", hostId: match.hostId }; + } const routingKey = buildHostRoutingKey(match.organizationId, match.hostId); - return `${env.RELAY_URL}/hosts/${routingKey}`; - }, [match, machineId, activeHostUrl]); + return { + status: "ready", + kind: "remote", + hostId: match.hostId, + url: `${env.RELAY_URL}/hosts/${routingKey}`, + }; + }, [workspaceId, isReady, match, machineId, activeHostUrl]); +} + +/** + * Backwards-compatible URL-only form for existing callers. Returns null + * for any non-`ready` status (loading, local-starting, not-found). + */ +export function useWorkspaceHostUrl(workspaceId: string | null): string | null { + const target = useWorkspaceHostTarget(workspaceId); + return target.status === "ready" ? target.url : null; } diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsx b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsx index 8f10b423181..909f66464c9 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsx @@ -29,6 +29,7 @@ export function DashboardSidebarDeleteDialog({ setDeleteBranch, hasChanges, hasUnpushedCommits, + blockingReason, isCheckingStatus, error, handleOpenChange, @@ -63,8 +64,10 @@ export function DashboardSidebarDeleteDialog({ onDeleteBranchChange={setDeleteBranch} hasChanges={hasChanges} hasUnpushedCommits={hasUnpushedCommits} + blockingReason={blockingReason} isCheckingStatus={isCheckingStatus} onConfirm={() => run(hasWarnings)} + confirmLabel={hasWarnings ? "Delete anyway" : "Delete"} /> ); } diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx index b54ede75067..c6b34ba8473 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx @@ -19,8 +19,10 @@ interface DestroyConfirmPaneProps { onDeleteBranchChange: (next: boolean) => void; hasChanges: boolean; hasUnpushedCommits: boolean; + blockingReason: string | null; isCheckingStatus: boolean; onConfirm: () => void; + confirmLabel: string; } export function DestroyConfirmPane({ @@ -31,8 +33,10 @@ export function DestroyConfirmPane({ onDeleteBranchChange, hasChanges, hasUnpushedCommits, + blockingReason, isCheckingStatus, onConfirm, + confirmLabel, }: DestroyConfirmPaneProps) { const checkboxId = useId(); const hasWarnings = hasChanges || hasUnpushedCommits; @@ -59,6 +63,13 @@ export function DestroyConfirmPane({ )} + {blockingReason && ( +
+
+ {blockingReason} +
+
+ )}
- Delete + {confirmLabel} diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts index 4f916cc1922..a2b4494cf5e 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts @@ -1,17 +1,17 @@ import { toast } from "@superset/ui/sonner"; -import { useCallback, useRef, useState } from "react"; -import type { DestroyWorkspaceSuccess } from "renderer/hooks/host-service/useDestroyWorkspace"; +import { useCallback, useEffect, useRef, useState } from "react"; +import type { + DestroyWorkspacePreview, + DestroyWorkspaceSuccess, +} from "renderer/hooks/host-service/useDestroyWorkspace"; import { type DestroyWorkspaceError, useDestroyWorkspace, } from "renderer/hooks/host-service/useDestroyWorkspace"; import { useV2UserPreferences } from "renderer/hooks/useV2UserPreferences/useV2UserPreferences"; -import { electronTrpc } from "renderer/lib/electron-trpc"; import { useNavigateAwayFromWorkspace } from "renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useNavigateAwayFromWorkspace"; import { useDeletingWorkspaces } from "renderer/routes/_authenticated/providers/DeletingWorkspacesProvider"; -const STATUS_STALE_TIME_MS = 5_000; - interface UseDestroyDialogStateOptions { workspaceId: string; workspaceName: string; @@ -20,6 +20,12 @@ interface UseDestroyDialogStateOptions { onDeleted?: () => void; } +type InspectState = + | { status: "idle" } + | { status: "loading" } + | { status: "ready"; preview: DestroyWorkspacePreview } + | { status: "error" }; + export function useDestroyDialogState({ workspaceId, workspaceName, @@ -27,7 +33,7 @@ export function useDestroyDialogState({ onOpenChange, onDeleted, }: UseDestroyDialogStateOptions) { - const { destroy } = useDestroyWorkspace(workspaceId); + const { destroy, inspect, hostTarget } = useDestroyWorkspace(workspaceId); const { markDeleting, clearDeleting } = useDeletingWorkspaces(); const navigateAway = useNavigateAwayFromWorkspace(); @@ -35,21 +41,47 @@ export function useDestroyDialogState({ useV2UserPreferences(); const deleteBranch = preferences.deleteLocalBranch; - const { data: canDeleteData, isPending: isCheckingStatus } = - electronTrpc.workspaces.canDelete.useQuery( - { id: workspaceId }, - { - enabled: open, - staleTime: STATUS_STALE_TIME_MS, - refetchOnWindowFocus: false, - }, - ); - const hasChanges = canDeleteData?.hasChanges ?? false; - const hasUnpushedCommits = canDeleteData?.hasUnpushedCommits ?? false; - + const [inspectState, setInspectState] = useState({ + status: "idle", + }); const [error, setError] = useState(null); const inFlight = useRef(false); + // Run inspect when the dialog opens AND the host is ready. While the host + // is loading/local-starting, sit in a checking state — no destructive + // banner for transient pending-host UX. + useEffect(() => { + if (!open) { + setInspectState({ status: "idle" }); + return; + } + if (hostTarget.status !== "ready") { + setInspectState({ status: "loading" }); + return; + } + + let cancelled = false; + setInspectState({ status: "loading" }); + inspect() + .then((preview) => { + if (cancelled) return; + setInspectState({ status: "ready", preview }); + }) + .catch(() => { + if (cancelled) return; + // Inspect-failure is non-fatal — let the user attempt destroy and + // surface real errors there. Treat as "no warnings, no block". + setInspectState({ status: "error" }); + }); + + return () => { + cancelled = true; + }; + }, [open, hostTarget.status, inspect]); + + const preview = inspectState.status === "ready" ? inspectState.preview : null; + const conflictWasRaced = error?.kind === "conflict"; + const handleOpenChange = useCallback( (next: boolean) => { if (!next) setError(null); @@ -78,9 +110,12 @@ export function useDestroyDialogState({ result = await destroy({ deleteBranch, force }); } catch (firstErr) { const e = firstErr as DestroyWorkspaceError; - // Race: preflight said clean but worktree was dirty by the time - // destroy ran. The user already confirmed once — don't make them - // confirm a second "uncommitted changes" warning, just force. + // Silent force-retry on the dirty-worktree race: preflight said + // clean but the worktree was dirty by destroy time. The user + // already confirmed once — don't bounce them back through a + // second warning. Do NOT extend this to `in-progress` (that's + // a different CONFLICT cause; retrying just races the same + // guard). if (e.kind === "conflict" && !force) { result = await destroy({ deleteBranch, force: true }); } else { @@ -94,8 +129,12 @@ export function useDestroyDialogState({ if (e.kind === "teardown-failed") { setError(e); onOpenChange(true); + } else if (e.kind === "in-progress") { + toast.error(`A delete is already in progress for ${workspaceName}.`); } else { - toast.error(`Failed to delete ${workspaceName}: ${e.message}`); + toast.error( + `Failed to delete ${workspaceName}: ${"message" in e ? e.message : String(e.kind)}`, + ); } } finally { clearDeleting(workspaceId); @@ -118,9 +157,10 @@ export function useDestroyDialogState({ return { deleteBranch, setDeleteBranch, - hasChanges, - hasUnpushedCommits, - isCheckingStatus, + hasChanges: (preview?.hasChanges ?? false) || conflictWasRaced, + hasUnpushedCommits: preview?.hasUnpushedCommits ?? false, + blockingReason: preview && !preview.canDelete ? preview.reason : null, + isCheckingStatus: open && inspectState.status === "loading", error, handleOpenChange, run, diff --git a/packages/host-service/src/index.ts b/packages/host-service/src/index.ts index d6fd19781b2..a3927e9ca29 100644 --- a/packages/host-service/src/index.ts +++ b/packages/host-service/src/index.ts @@ -20,6 +20,9 @@ export { } from "./providers/model-providers"; export type { GitCredentialProvider, GitFactory } from "./runtime/git"; export { installProcessSafetyNet } from "./safety"; -export type { TeardownFailureCause } from "./trpc/error-types"; +export type { + DeleteInProgressCause, + TeardownFailureCause, +} from "./trpc/error-types"; export type { AppRouter } from "./trpc/router"; export type { ApiClient, HostServiceContext } from "./types"; diff --git a/packages/host-service/src/trpc/error-types.ts b/packages/host-service/src/trpc/error-types.ts index 8fa6e3b0564..40ba5d0768b 100644 --- a/packages/host-service/src/trpc/error-types.ts +++ b/packages/host-service/src/trpc/error-types.ts @@ -42,3 +42,24 @@ export function isProjectNotSetupCause( (value as { kind: unknown }).kind === "PROJECT_NOT_SETUP" ); } + +/** + * Thrown by `workspaceCleanup.destroy` when another destroy for the same + * workspace is already in flight. Distinct from a dirty-worktree CONFLICT + * because the renderer must NOT silently retry with `force: true` — the + * second caller should surface as a toast and let the first run finish. + */ +export interface DeleteInProgressCause { + kind: "DELETE_IN_PROGRESS"; +} + +export function isDeleteInProgressCause( + value: unknown, +): value is DeleteInProgressCause { + return ( + !!value && + typeof value === "object" && + "kind" in value && + (value as { kind: unknown }).kind === "DELETE_IN_PROGRESS" + ); +} diff --git a/packages/host-service/src/trpc/index.ts b/packages/host-service/src/trpc/index.ts index dd09c811239..c469ac1cffa 100644 --- a/packages/host-service/src/trpc/index.ts +++ b/packages/host-service/src/trpc/index.ts @@ -2,6 +2,8 @@ import { initTRPC, TRPCError } from "@trpc/server"; import superjson from "superjson"; import type { HostServiceContext } from "../types"; import { + type DeleteInProgressCause, + isDeleteInProgressCause, isProjectNotSetupCause, isTeardownFailureCause, type ProjectNotSetupCause, @@ -46,12 +48,17 @@ const t = initTRPC projectId: error.cause.projectId, } : undefined; + const deleteInProgress: DeleteInProgressCause | undefined = + isDeleteInProgressCause(error.cause) + ? { kind: "DELETE_IN_PROGRESS" } + : undefined; return { ...shape, data: { ...shape.data, teardownFailure, projectNotSetup, + deleteInProgress, }, }; }, diff --git a/packages/host-service/src/trpc/router/workspace-cleanup/is-main-workspace.ts b/packages/host-service/src/trpc/router/workspace-cleanup/is-main-workspace.ts new file mode 100644 index 00000000000..d3b22a7f1ee --- /dev/null +++ b/packages/host-service/src/trpc/router/workspace-cleanup/is-main-workspace.ts @@ -0,0 +1,68 @@ +import { realpathSync } from "node:fs"; +import { resolve } from "node:path"; +import { eq } from "drizzle-orm"; +import { projects, workspaces } from "../../../db/schema"; +import type { HostServiceContext } from "../../../types"; + +export type IsMainWorkspaceResult = + | { isMain: true; reason: string } + | { isMain: false; reason: null }; + +export const MAIN_WORKSPACE_REASON = + "Main workspaces cannot be deleted. Remove them from the sidebar or remove the project from this host instead."; + +/** + * Authoritative "is this a main workspace?" check for the cleanup router. + * + * Two signals, either is sufficient: + * - local: worktreePath equals the project's repoPath, after realpath + * normalization (without it, symlinks / trailing slash / macOS case + * differences silently fail open). + * - cloud: v2Workspaces.type === "main" (only checked if `ctx.api` is wired + * and the local check didn't already fire). + * + * Both signals exist because either side can lag the other: a workspace + * classified as main in cloud may not yet have its local worktreePath + * rewritten, and vice versa. + */ +export async function isMainWorkspace( + ctx: HostServiceContext, + workspaceId: string, +): Promise { + const local = ctx.db.query.workspaces + .findFirst({ where: eq(workspaces.id, workspaceId) }) + .sync(); + const project = local + ? ctx.db.query.projects + .findFirst({ where: eq(projects.id, local.projectId) }) + .sync() + : undefined; + + if ( + local && + project && + normalizePath(local.worktreePath) === normalizePath(project.repoPath) + ) { + return { isMain: true, reason: MAIN_WORKSPACE_REASON }; + } + + if (ctx.api) { + const cloudWorkspace = await ctx.api.v2Workspace.getFromHost.query({ + organizationId: ctx.organizationId, + id: workspaceId, + }); + if (cloudWorkspace?.type === "main") { + return { isMain: true, reason: MAIN_WORKSPACE_REASON }; + } + } + + return { isMain: false, reason: null }; +} + +function normalizePath(p: string): string { + try { + return realpathSync(p); + } catch { + return resolve(p); + } +} diff --git a/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts b/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts index ff7609dbc30..714387e8fad 100644 --- a/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts +++ b/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts @@ -5,10 +5,104 @@ import { projects, workspaces } from "../../../db/schema"; import { invalidateLabelCache } from "../../../ports/static-ports"; import { runTeardown, type TeardownResult } from "../../../runtime/teardown"; import { disposeSessionsByWorkspaceId } from "../../../terminal/terminal"; -import type { TeardownFailureCause } from "../../error-types"; +import type { HostServiceContext } from "../../../types"; +import type { + DeleteInProgressCause, + TeardownFailureCause, +} from "../../error-types"; import { protectedProcedure, router } from "../../index"; +import { isMainWorkspace } from "./is-main-workspace"; + +/** + * Process-local guard against concurrent destroys of the same workspace. + * A second caller observes the live entry and gets a typed CONFLICT (with + * `DELETE_IN_PROGRESS` cause) so the renderer can render a toast instead + * of mistaking it for a dirty-worktree race and silently force-retrying. + * + * Doesn't survive a host-service crash mid-delete — but neither does the + * destroy itself, and the saga is idempotent enough that a second attempt + * after restart is safe. + */ +const destroysInFlight = new Set(); + +interface DestroyInput { + workspaceId: string; + deleteBranch: boolean; + force: boolean; +} export const workspaceCleanupRouter = router({ + /** + * Status preview for the v2 delete dialog. Co-located with `destroy` so + * the two can never disagree about what's blocked vs warned. + * + * Contract: + * - canDelete: false → render `reason` as a blocking banner. + * - hasChanges/Unpushed → render as warnings; user can still confirm. + * - git failures (missing worktree, broken repo) → return as canDelete + * with no warnings; the destroy saga handles those states best-effort. + * + * Unpushed-commit detection uses `rev-list --not --remotes` so brand-new + * branches with no upstream still report unpushed commits correctly. + */ + inspect: protectedProcedure + .input(z.object({ workspaceId: z.string() })) + .query(async ({ ctx, input }) => { + const main = await isMainWorkspace(ctx, input.workspaceId); + if (main.isMain) { + return { + canDelete: false, + reason: main.reason, + hasChanges: false, + hasUnpushedCommits: false, + }; + } + + const local = ctx.db.query.workspaces + .findFirst({ where: eq(workspaces.id, input.workspaceId) }) + .sync(); + if (!local) { + return { + canDelete: true, + reason: null, + hasChanges: false, + hasUnpushedCommits: false, + }; + } + + try { + const git = await ctx.git(local.worktreePath); + const status = await git.status(); + let hasUnpushedCommits = false; + try { + const result = await git.raw([ + "rev-list", + "--count", + "HEAD", + "--not", + "--remotes", + ]); + const count = Number.parseInt(result.trim(), 10); + hasUnpushedCommits = Number.isFinite(count) && count > 0; + } catch { + // Leave false — `rev-list` failure isn't a signal we can act on. + } + return { + canDelete: true, + reason: null, + hasChanges: !status.isClean(), + hasUnpushedCommits, + }; + } catch { + return { + canDelete: true, + reason: null, + hasChanges: false, + hasUnpushedCommits: false, + }; + } + }), + /** * Destroy a workspace in three phases: * @@ -30,9 +124,14 @@ export const workspaceCleanupRouter = router({ * would just silently drop the opt-in. * * Typed errors for the renderer: - * - CONFLICT → dirty worktree; prompt force-retry + * - CONFLICT → dirty worktree; prompt force-retry. + * CONFLICT with `data.deleteInProgress` is a + * different beast — another destroy is in + * flight for the same workspace; surface as + * a toast and do NOT force-retry. * - INTERNAL_SERVER_ERROR with `data.teardownFailure` → teardown * script failed; prompt force-retry + * - BAD_REQUEST → main workspace; cannot be deleted * - PRECONDITION_FAILED → no cloud API configured * - pass-through → cloud auth / network failure */ @@ -45,158 +144,153 @@ export const workspaceCleanupRouter = router({ }), ) .mutation(async ({ ctx, input }) => { - const warnings: string[] = []; - - const local = ctx.db.query.workspaces - .findFirst({ where: eq(workspaces.id, input.workspaceId) }) - .sync(); - const project = local - ? ctx.db.query.projects - .findFirst({ where: eq(projects.id, local.projectId) }) - .sync() - : undefined; - - if (local && project && local.worktreePath === project.repoPath) { + if (destroysInFlight.has(input.workspaceId)) { throw new TRPCError({ - code: "BAD_REQUEST", - message: - "Main workspaces cannot be deleted. Remove them from the sidebar or remove the project from this host instead.", + code: "CONFLICT", + message: "Deletion already in progress for this workspace", + cause: { kind: "DELETE_IN_PROGRESS" } satisfies DeleteInProgressCause, }); } - if (ctx.api) { - const cloudWorkspace = await ctx.api.v2Workspace.getFromHost.query({ - organizationId: ctx.organizationId, - id: input.workspaceId, - }); - if (cloudWorkspace?.type === "main") { - throw new TRPCError({ - code: "BAD_REQUEST", - message: - "Main workspaces cannot be deleted. Remove them from the sidebar or remove the project from this host instead.", - }); - } + destroysInFlight.add(input.workspaceId); + try { + return await runDestroy(ctx, input); + } finally { + destroysInFlight.delete(input.workspaceId); } + }), +}); - // ─── Step 0: Preflight ───────────────────────────────────────── - // Block only on dirty worktree (the common "I forgot to commit" - // case). Anything else the local-cleanup phase handles as warning. - if (!input.force && local && project) { - try { - const git = await ctx.git(local.worktreePath); - const status = await git.status(); - if (!status.isClean()) { - throw new TRPCError({ - code: "CONFLICT", - message: "Worktree has uncommitted changes", - }); - } - } catch (err) { - if (err instanceof TRPCError) throw err; - // Can't read status (missing worktree dir, etc.) — not a - // conflict. Continue; step 3b will skip idempotently. - } - } +async function runDestroy(ctx: HostServiceContext, input: DestroyInput) { + const warnings: string[] = []; - // ─── Step 1: Teardown ────────────────────────────────────────── - // Script is the user's last chance to stop services / flush state - // before the workspace goes away. Failure here is recoverable - // via force-retry, which skips this step. - if (!input.force && local && project) { - const teardown: TeardownResult = await runTeardown({ - db: ctx.db, - workspaceId: input.workspaceId, - worktreePath: local.worktreePath, - }); - if (teardown.status === "failed") { - const cause: TeardownFailureCause = { - kind: "TEARDOWN_FAILED", - exitCode: teardown.exitCode, - signal: teardown.signal, - timedOut: teardown.timedOut, - outputTail: teardown.outputTail, - }; - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "Teardown script failed", - cause, - }); - } - } + const local = ctx.db.query.workspaces + .findFirst({ where: eq(workspaces.id, input.workspaceId) }) + .sync(); + const project = local + ? ctx.db.query.projects + .findFirst({ where: eq(projects.id, local.projectId) }) + .sync() + : undefined; - // ─── Step 2: Cloud delete (commit point) ─────────────────────── - // Past this line, the workspace is gone from the user's perspective - // (sidebar will reflect the cloud state). Local artifacts become - // cleanup debris — never a source of truth. - if (!ctx.api) { + const main = await isMainWorkspace(ctx, input.workspaceId); + if (main.isMain) { + throw new TRPCError({ code: "BAD_REQUEST", message: main.reason }); + } + + // ─── Step 0: Preflight ───────────────────────────────────────── + // Block only on dirty worktree (the common "I forgot to commit" + // case). Anything else the local-cleanup phase handles as warning. + if (!input.force && local && project) { + try { + const git = await ctx.git(local.worktreePath); + const status = await git.status(); + if (!status.isClean()) { throw new TRPCError({ - code: "PRECONDITION_FAILED", - message: "Cloud API not configured", + code: "CONFLICT", + message: "Worktree has uncommitted changes", }); } - await ctx.api.v2Workspace.delete.mutate({ id: input.workspaceId }); + } catch (err) { + if (err instanceof TRPCError) throw err; + // Can't read status (missing worktree dir, etc.) — not a + // conflict. Continue; step 3b will skip idempotently. + } + } - // ─── Step 3: Local cleanup (best-effort) ─────────────────────── - // Every failure in this phase is captured as a warning; the - // caller always sees success. + // ─── Step 1: Teardown ────────────────────────────────────────── + // Script is the user's last chance to stop services / flush state + // before the workspace goes away. Failure here is recoverable + // via force-retry, which skips this step. + if (!input.force && local && project) { + const teardown: TeardownResult = await runTeardown({ + db: ctx.db, + workspaceId: input.workspaceId, + worktreePath: local.worktreePath, + }); + if (teardown.status === "failed") { + const cause: TeardownFailureCause = { + kind: "TEARDOWN_FAILED", + exitCode: teardown.exitCode, + signal: teardown.signal, + timedOut: teardown.timedOut, + outputTail: teardown.outputTail, + }; + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Teardown script failed", + cause, + }); + } + } - // 3a. PTYs - const killed = disposeSessionsByWorkspaceId(input.workspaceId, ctx.db); - if (killed.failed > 0) { - warnings.push(`${killed.failed} terminal(s) may still be running`); - } + // ─── Step 2: Cloud delete (commit point) ─────────────────────── + // Past this line, the workspace is gone from the user's perspective + // (sidebar will reflect the cloud state). Local artifacts become + // cleanup debris — never a source of truth. + if (!ctx.api) { + throw new TRPCError({ + code: "PRECONDITION_FAILED", + message: "Cloud API not configured", + }); + } + await ctx.api.v2Workspace.delete.mutate({ id: input.workspaceId }); - // 3b. Worktree (always --force: we're past the commit point) - // 3c. Optional branch delete - let worktreeRemoved = false; - let branchDeleted = false; - if (local && project) { - const git = await ctx.git(project.repoPath); - try { - await git.raw(["worktree", "remove", "--force", local.worktreePath]); - worktreeRemoved = true; - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - if ( - message.includes("is not a working tree") || - message.includes("No such file or directory") || - message.includes("ENOENT") - ) { - worktreeRemoved = true; - } else { - warnings.push( - `Failed to remove worktree at ${local.worktreePath}: ${message}`, - ); - } - } + // ─── Step 3: Local cleanup (best-effort) ─────────────────────── + // Every failure in this phase is captured as a warning; the + // caller always sees success. - if (input.deleteBranch && local.branch) { - try { - await git.raw(["branch", "-D", local.branch]); - branchDeleted = true; - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - warnings.push( - `Failed to delete branch ${local.branch}: ${message}`, - ); - } - } + // 3a. PTYs + const killed = disposeSessionsByWorkspaceId(input.workspaceId, ctx.db); + if (killed.failed > 0) { + warnings.push(`${killed.failed} terminal(s) may still be running`); + } + + // 3b. Worktree (always --force: we're past the commit point) + // 3c. Optional branch delete + let worktreeRemoved = false; + let branchDeleted = false; + if (local && project) { + const git = await ctx.git(project.repoPath); + try { + await git.raw(["worktree", "remove", "--force", local.worktreePath]); + worktreeRemoved = true; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + if ( + message.includes("is not a working tree") || + message.includes("No such file or directory") || + message.includes("ENOENT") + ) { + worktreeRemoved = true; + } else { + warnings.push( + `Failed to remove worktree at ${local.worktreePath}: ${message}`, + ); } + } - // 3d. Host sqlite row - if (local) { - ctx.db - .delete(workspaces) - .where(eq(workspaces.id, input.workspaceId)) - .run(); - invalidateLabelCache(input.workspaceId); + if (input.deleteBranch && local.branch) { + try { + await git.raw(["branch", "-D", local.branch]); + branchDeleted = true; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + warnings.push(`Failed to delete branch ${local.branch}: ${message}`); } + } + } - return { - success: true, - cloudDeleted: true, - worktreeRemoved, - branchDeleted, - warnings, - }; - }), -}); + // 3d. Host sqlite row + if (local) { + ctx.db.delete(workspaces).where(eq(workspaces.id, input.workspaceId)).run(); + invalidateLabelCache(input.workspaceId); + } + + return { + success: true, + cloudDeleted: true, + worktreeRemoved, + branchDeleted, + warnings, + }; +} diff --git a/plans/20260430-v2-delete-workspace-audit.md b/plans/20260430-v2-delete-workspace-audit.md new file mode 100644 index 00000000000..5a7a6721be5 --- /dev/null +++ b/plans/20260430-v2-delete-workspace-audit.md @@ -0,0 +1,52 @@ +# V2 Delete-Workspace Pattern Audit & Decisions + +Date: 2026-04-30 +Branch: `map-delete-pattern-v2` +Scope: align v2 delete dialog with the host-service destroy saga; surface bugs/inconsistencies in the current pattern, then lock in the implementation decisions. + +## Today's pattern (origin/main) + +- **Preflight** — v2 dialog calls `electronTrpc.workspaces.canDelete` (v1 main-process IPC). Returns `hasChanges`, `hasUnpushedCommits`, `activeTerminalCount`, plus `canDelete/reason` for not-found / already-deleting / unreadable-status. +- **Destroy** — v2 calls host-service `workspaceCleanup.destroy` (3-phase: preflight → teardown → cloud-delete commit point → best-effort local cleanup). Has its own dirty-worktree check at phase 0 and its own main-workspace block (local equality + cloud `getFromHost.type === "main"`). +- **Race handling** — dialog auto-retries with `force: true` on a `conflict` returned by destroy, on the rationale that the user already confirmed once. + +## Issues found + +1. **`hasUnpushedCommits` regression on no-upstream branches.** `status.ahead > 0` returns `0` when there is no upstream. Branches that have never been pushed show no warning, and `deleteBranch=true` then silently drops commits. +2. **Path equality for "main workspace" is unnormalized.** `local.worktreePath === project.repoPath` is raw string equality — no `realpath`, no trailing-slash normalization, no macOS case folding. If columns ever differ in form, the main-workspace block fails open and the saga removes the main worktree. +3. **Inspect ↔ destroy disagree on "is main".** Destroy has *two* checks (local equality + cloud `getFromHost.type === "main"`). A status preview that only does the local check passes workspaces destroy then rejects with `BAD_REQUEST` — surfacing as a generic toast instead of an in-dialog blocking banner. +4. **Concurrent-delete protection is gone in v2.** v1 had a `deletingAt` column. v2 host-service saga has no equivalent; renderer-side `useDeletingWorkspaces` is in-memory only. Two clicks racing the same workspace will both pass preflight and both run the saga. +5. **Loading and `local-starting` host states leak into destructive UI.** Any preview that throws when host isn't ready will render normal pending states as a destructive blocking banner. +6. **`ctx.git()` failures during preview are silently treated as "clean".** Diverges from v1 contract (which returned `canDelete: false` with the error message). +7. **Auto-force-retry on `conflict` was deliberate UX.** Removing it costs one extra click on the known race (preflight clean → destroy dirty). +8. **`activeTerminalCount` is no longer surfaced.** v1 told the user how many PTYs would be killed before they clicked Delete. + +## Decisions + +| # | Decision | Choice | +|---|---|---| +| 1 | Preflight location | Host-service `inspect` query, called from the v2 dialog | +| 2 | "Is main workspace" check | Shared `isMainWorkspace()` helper, `realpath`-normalized paths, both local equality and cloud `getFromHost.type === "main"` | +| 3 | Unpushed commits | No upstream = treat as has-unpushed (use `git for-each-ref` to detect missing upstream, fall back to `git rev-list HEAD`) | +| 4 | Concurrent-delete guard | Process-local `Map` on the host-service; second caller awaits the first or gets a typed `"already in progress"` error | +| 5 | Pending-host UX | `loading` and `local-starting` map to `isCheckingStatus` — disabled Delete button + spinner, no banner | +| 6 | `CONFLICT` after clean preflight | Silent force-retry, with a code comment explaining the race so it's not removed again | +| 7 | Git status failure during inspect | `canDelete: true`, no warnings — matches v2 saga's "best-effort cleanup" contract | +| 8 | Active terminal count in dialog | Skip; rely on saga's post-hoc warning toast | + +## Implementation sequence + +Order chosen so each step is shippable on its own: + +1. **Add `isMainWorkspace()` helper** in host-service (or shared lib if both packages need it). Includes `realpath` normalization. Replace the inline check inside `destroy`. No behavior change yet. +2. **Add `workspaceCleanup.inspect`** on host-service. Uses `isMainWorkspace`, the upstream-aware unpushed check, and the git-failure-is-fine contract. Returns `{ canDelete, reason, hasChanges, hasUnpushedCommits }`. +3. **Add concurrent-delete guard** to `workspaceCleanup.destroy` — `Map` at saga entry. Throws typed `"already in progress"` for the second caller. +4. **Promote `useWorkspaceHostUrl` → `useWorkspaceHostTarget`** with the status union (`loading | not-found | local-starting | ready`). Keep `useWorkspaceHostUrl` as a back-compat thin wrapper. +5. **Migrate `useDestroyDialogState`** off `electronTrpc.workspaces.canDelete` and onto the new `inspect`. Treat `loading`/`local-starting` as `isCheckingStatus`. Keep the silent force-retry on `CONFLICT` (with the comment). +6. **Wire `blockingReason` + `confirmLabel` through the dialog UI** (`DashboardSidebarDeleteDialog` → `DestroyConfirmPane`). + +Each step should be its own commit; steps 1–3 are server-side and ship before steps 4–6. + +## Out of scope + +- v1 `canDelete` / `delete` procedures and their git utilities can be deleted once the v2 path is the only caller (per v1 sunset). Track separately. From 95400dd7b0b1a355deb9971f4216270752cd55ff Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Thu, 30 Apr 2026 12:25:19 -0700 Subject: [PATCH 2/5] test(host-service): cover isMainWorkspace, inspect, destroy in-flight guard 15 tests across three areas: - isMainWorkspace: realpath normalization (incl. symlink case), cloud-type fallback, no-row safe path - workspaceCleanup.inspect: blocked-main, no-row, hasChanges, rev-list-aware unpushed detection, git-failure swallow - workspaceCleanup.destroy in-flight guard: cleanup on success, cleanup on throw, CONFLICT+DELETE_IN_PROGRESS rejection, retry-after-failure (regression test for in-flight leak) Exposes destroysInFlight as __testDestroysInFlight (internal-only, explicit @internal annotation) so tests can assert cleanup state without spinning up a full tRPC client. --- .../workspace-cleanup/workspace-cleanup.ts | 3 + .../test/workspace-cleanup.test.ts | 359 ++++++++++++++++++ 2 files changed, 362 insertions(+) create mode 100644 packages/host-service/test/workspace-cleanup.test.ts diff --git a/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts b/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts index 714387e8fad..ae4fc886af9 100644 --- a/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts +++ b/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts @@ -25,6 +25,9 @@ import { isMainWorkspace } from "./is-main-workspace"; */ const destroysInFlight = new Set(); +/** @internal — exposed for tests to introspect / clear the guard. */ +export const __testDestroysInFlight = destroysInFlight; + interface DestroyInput { workspaceId: string; deleteBranch: boolean; diff --git a/packages/host-service/test/workspace-cleanup.test.ts b/packages/host-service/test/workspace-cleanup.test.ts new file mode 100644 index 00000000000..a1d0b657cb0 --- /dev/null +++ b/packages/host-service/test/workspace-cleanup.test.ts @@ -0,0 +1,359 @@ +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { + mkdirSync, + mkdtempSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { TRPCError } from "@trpc/server"; +import { isMainWorkspace } from "../src/trpc/router/workspace-cleanup/is-main-workspace"; +import { + __testDestroysInFlight, + workspaceCleanupRouter, +} from "../src/trpc/router/workspace-cleanup/workspace-cleanup"; +import type { HostServiceContext } from "../src/types"; + +type WorkspaceRow = { + id: string; + projectId: string; + worktreePath: string; + branch: string; +}; +type ProjectRow = { id: string; repoPath: string }; + +interface ContextSpec { + workspace?: WorkspaceRow; + project?: ProjectRow; + cloudType?: "main" | "worktree"; + cloudDelete?: () => Promise; + gitStatus?: { isClean: () => boolean; ahead?: number }; + revListCount?: string | (() => Promise); + gitFactoryThrows?: boolean; +} + +function makeCtx(spec: ContextSpec): HostServiceContext { + const workspaceFindFirst = mock(() => ({ + sync: () => spec.workspace, + })); + const projectFindFirst = mock(() => ({ + sync: () => spec.project, + })); + + const cloudGetFromHost = mock(async () => + spec.cloudType ? { type: spec.cloudType } : null, + ); + const cloudDelete = mock(spec.cloudDelete ?? (async () => undefined)); + + const status = mock(async () => spec.gitStatus ?? { isClean: () => true }); + const revList = mock(async () => + typeof spec.revListCount === "function" + ? await spec.revListCount() + : (spec.revListCount ?? "0\n"), + ); + const worktreeRemove = mock(async () => undefined); + const branchDelete = mock(async () => undefined); + + const git = mock(async () => { + if (spec.gitFactoryThrows) throw new Error("git factory boom"); + return { + status, + raw: mock(async (args: string[]) => { + if (args[0] === "rev-list") return await revList(); + if (args[0] === "worktree") return await worktreeRemove(); + if (args[0] === "branch") return await branchDelete(); + throw new Error(`unexpected git raw: ${args.join(" ")}`); + }), + }; + }); + + const dbDeleteRun = mock(() => undefined); + const dbDeleteWhere = mock(() => ({ run: dbDeleteRun })); + const terminalSelectAll = mock(() => []); + + return { + isAuthenticated: true, + organizationId: "org-1", + git: git as never, + github: (async () => ({})) as never, + api: { + v2Workspace: { + getFromHost: { query: cloudGetFromHost }, + delete: { mutate: cloudDelete }, + }, + } as never, + db: { + query: { + workspaces: { findFirst: workspaceFindFirst }, + projects: { findFirst: projectFindFirst }, + }, + select: () => ({ + from: () => ({ + where: () => ({ all: terminalSelectAll }), + }), + }), + delete: () => ({ where: dbDeleteWhere }), + } as never, + runtime: {} as never, + eventBus: {} as never, + }; +} + +describe("isMainWorkspace", () => { + test("returns isMain: false when no local workspace row", async () => { + const ctx = makeCtx({}); + const result = await isMainWorkspace(ctx, "ws-1"); + expect(result.isMain).toBe(false); + expect(result.reason).toBe(null); + }); + + test("returns isMain: true when worktreePath equals project repoPath", async () => { + const tmp = mkdtempSync(join(tmpdir(), "is-main-")); + try { + const ctx = makeCtx({ + workspace: { + id: "ws-1", + projectId: "p-1", + worktreePath: tmp, + branch: "main", + }, + project: { id: "p-1", repoPath: tmp }, + }); + const result = await isMainWorkspace(ctx, "ws-1"); + expect(result.isMain).toBe(true); + expect(result.reason).toContain("Main workspaces cannot be deleted"); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } + }); + + test("normalizes paths via realpath (symlinked worktree path equals repoPath)", async () => { + const tmp = mkdtempSync(join(tmpdir(), "is-main-")); + const realRepo = join(tmp, "real-repo"); + const symRepo = join(tmp, "sym-repo"); + mkdirSync(realRepo); + writeFileSync(join(realRepo, ".keep"), ""); + symlinkSync(realRepo, symRepo); + try { + const ctx = makeCtx({ + workspace: { + id: "ws-1", + projectId: "p-1", + worktreePath: symRepo, + branch: "main", + }, + project: { id: "p-1", repoPath: realRepo }, + }); + const result = await isMainWorkspace(ctx, "ws-1"); + expect(result.isMain).toBe(true); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } + }); + + test("returns isMain: true via cloud type even when paths differ", async () => { + const ctx = makeCtx({ + workspace: { + id: "ws-1", + projectId: "p-1", + worktreePath: "/some/branch/wt", + branch: "feature", + }, + project: { id: "p-1", repoPath: "/some/repo" }, + cloudType: "main", + }); + const result = await isMainWorkspace(ctx, "ws-1"); + expect(result.isMain).toBe(true); + }); + + test("returns isMain: false when neither local equality nor cloud type fires", async () => { + const ctx = makeCtx({ + workspace: { + id: "ws-1", + projectId: "p-1", + worktreePath: "/branch/wt", + branch: "feature", + }, + project: { id: "p-1", repoPath: "/repo" }, + cloudType: "worktree", + }); + const result = await isMainWorkspace(ctx, "ws-1"); + expect(result.isMain).toBe(false); + }); +}); + +describe("workspaceCleanup.inspect", () => { + const wsAndProject = { + workspace: { + id: "ws-1", + projectId: "p-1", + worktreePath: "/branch/wt", + branch: "feature", + }, + project: { id: "p-1", repoPath: "/repo" }, + }; + + test("blocks main workspaces with a destructive reason", async () => { + const ctx = makeCtx({ ...wsAndProject, cloudType: "main" }); + const caller = workspaceCleanupRouter.createCaller(ctx); + const result = await caller.inspect({ workspaceId: "ws-1" }); + expect(result.canDelete).toBe(false); + expect(result.reason).toContain("Main workspaces cannot be deleted"); + expect(result.hasChanges).toBe(false); + expect(result.hasUnpushedCommits).toBe(false); + }); + + test("returns canDelete: true with no warnings when no local row", async () => { + const ctx = makeCtx({}); + const caller = workspaceCleanupRouter.createCaller(ctx); + const result = await caller.inspect({ workspaceId: "ws-1" }); + expect(result).toEqual({ + canDelete: true, + reason: null, + hasChanges: false, + hasUnpushedCommits: false, + }); + }); + + test("flags hasChanges from git status", async () => { + const ctx = makeCtx({ + ...wsAndProject, + cloudType: "worktree", + gitStatus: { isClean: () => false }, + revListCount: "0\n", + }); + const caller = workspaceCleanupRouter.createCaller(ctx); + const result = await caller.inspect({ workspaceId: "ws-1" }); + expect(result.hasChanges).toBe(true); + expect(result.hasUnpushedCommits).toBe(false); + }); + + test("flags hasUnpushedCommits from rev-list count > 0", async () => { + const ctx = makeCtx({ + ...wsAndProject, + cloudType: "worktree", + gitStatus: { isClean: () => true }, + revListCount: "3\n", + }); + const caller = workspaceCleanupRouter.createCaller(ctx); + const result = await caller.inspect({ workspaceId: "ws-1" }); + expect(result.hasChanges).toBe(false); + expect(result.hasUnpushedCommits).toBe(true); + }); + + test("treats rev-list failure as no-unpushed-signal (doesn't block)", async () => { + const ctx = makeCtx({ + ...wsAndProject, + cloudType: "worktree", + gitStatus: { isClean: () => true }, + revListCount: () => Promise.reject(new Error("rev-list boom")), + }); + const caller = workspaceCleanupRouter.createCaller(ctx); + const result = await caller.inspect({ workspaceId: "ws-1" }); + expect(result.hasUnpushedCommits).toBe(false); + expect(result.canDelete).toBe(true); + }); + + test("swallows git factory failures and returns canDelete: true with no warnings", async () => { + const ctx = makeCtx({ + ...wsAndProject, + cloudType: "worktree", + gitFactoryThrows: true, + }); + const caller = workspaceCleanupRouter.createCaller(ctx); + const result = await caller.inspect({ workspaceId: "ws-1" }); + expect(result).toEqual({ + canDelete: true, + reason: null, + hasChanges: false, + hasUnpushedCommits: false, + }); + }); +}); + +describe("workspaceCleanup.destroy in-flight guard", () => { + beforeEach(() => { + __testDestroysInFlight.clear(); + }); + afterEach(() => { + __testDestroysInFlight.clear(); + }); + + test("clears the Set on success", async () => { + const ctx = makeCtx({}); + const caller = workspaceCleanupRouter.createCaller(ctx); + await caller.destroy({ + workspaceId: "ws-1", + deleteBranch: false, + force: false, + }); + expect(__testDestroysInFlight.has("ws-1")).toBe(false); + }); + + test("clears the Set when phase 2 (cloud delete) throws", async () => { + const ctx = makeCtx({ + cloudDelete: async () => { + throw new Error("cloud is down"); + }, + }); + const caller = workspaceCleanupRouter.createCaller(ctx); + await expect( + caller.destroy({ + workspaceId: "ws-1", + deleteBranch: false, + force: false, + }), + ).rejects.toThrow(); + expect(__testDestroysInFlight.has("ws-1")).toBe(false); + }); + + test("rejects a concurrent call with CONFLICT + DELETE_IN_PROGRESS cause", async () => { + __testDestroysInFlight.add("ws-1"); + const ctx = makeCtx({}); + const caller = workspaceCleanupRouter.createCaller(ctx); + try { + await caller.destroy({ + workspaceId: "ws-1", + deleteBranch: false, + force: false, + }); + throw new Error("expected destroy to throw"); + } catch (err) { + expect(err).toBeInstanceOf(TRPCError); + const trpcErr = err as TRPCError; + expect(trpcErr.code).toBe("CONFLICT"); + expect(trpcErr.cause).toMatchObject({ kind: "DELETE_IN_PROGRESS" }); + } + }); + + test("retry after a failed destroy succeeds (no in-flight leak)", async () => { + let cloudCallCount = 0; + const ctx = makeCtx({ + cloudDelete: async () => { + cloudCallCount += 1; + if (cloudCallCount === 1) throw new Error("transient cloud failure"); + }, + }); + const caller = workspaceCleanupRouter.createCaller(ctx); + + await expect( + caller.destroy({ + workspaceId: "ws-1", + deleteBranch: false, + force: false, + }), + ).rejects.toThrow(); + expect(__testDestroysInFlight.has("ws-1")).toBe(false); + + // Second attempt must NOT see DELETE_IN_PROGRESS — the Set was cleaned. + await caller.destroy({ + workspaceId: "ws-1", + deleteBranch: false, + force: false, + }); + expect(cloudCallCount).toBe(2); + expect(__testDestroysInFlight.has("ws-1")).toBe(false); + }); +}); From 9411a550965dd65127431cdde603415be18f524d Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Thu, 30 Apr 2026 12:34:35 -0700 Subject: [PATCH 3/5] fix(desktop): address PR review for v2 delete-workspace dialog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight findings from greptile + coderabbit: - phase 3 ctx.git(project.repoPath) was outside the per-step try blocks; a failure there past the cloud-delete commit point would surface as a hard error for an already-deleted workspace. Now captured as a warning and the rest of phase 3 skips cleanly. - DestroyConfirmPane disabled on !!blockingReason; if server ever returned { canDelete: false, reason: null } the button would stay enabled. Renderer now derives canConfirm from preview.canDelete directly. - host-target's `not-found` was collapsed into "loading"; dialog would sit in perpetual "Checking…". Now renders a destructive blocking banner ("Workspace is no longer available on this host"). - useDestroyWorkspace's useCallback closed over the full hostTarget object; useLiveQuery returns a new array per tick, so identity churn was rebuilding inspect/destroy and re-firing the dialog effect (visible flicker). Stabilized on hostUrl + hostStatus scalars. - confirm button label said "Delete" during preflight; now flips to "Checking…" while inspect is in flight. - removed dead `conflictWasRaced` (error is only ever set to `teardown-failed`; conflict path either silently retries or toasts). - de-duped sqlite reads: isMainWorkspace already loads workspace + project rows; thread them through to runDestroy and inspect instead of re-querying. - audit doc said `for-each-ref` and Map; reflect what shipped (`rev-list --not --remotes` and Set). Adds a test for the phase-3 git-factory warning behavior. Test count now 16 (was 15), all green. --- .../useDestroyWorkspace.ts | 26 ++++--- .../DashboardSidebarDeleteDialog.tsx | 9 ++- .../DestroyConfirmPane/DestroyConfirmPane.tsx | 4 +- .../useDestroyDialogState.ts | 28 ++++++-- .../workspace-cleanup/is-main-workspace.ts | 19 ++++-- .../workspace-cleanup/workspace-cleanup.ts | 68 ++++++++++--------- .../test/workspace-cleanup.test.ts | 35 ++++++++++ plans/20260430-v2-delete-workspace-audit.md | 6 +- 8 files changed, 139 insertions(+), 56 deletions(-) diff --git a/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts b/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts index fb63674a154..123220267c9 100644 --- a/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts +++ b/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts @@ -55,11 +55,18 @@ export interface UseDestroyWorkspace { export function useDestroyWorkspace(workspaceId: string): UseDestroyWorkspace { const hostTarget = useWorkspaceHostTarget(workspaceId); + // Reduce the (object-identity-unstable) hostTarget down to two scalars so + // memoized callbacks below don't churn on every collection notification. + // useLiveQuery returns a new array each tick, which would otherwise rebuild + // `inspect`/`destroy` and re-fire effects that depend on them. + const hostUrl = hostTarget.status === "ready" ? hostTarget.url : null; + const hostStatus = hostTarget.status; + const destroy = useCallback( async ( input: DestroyWorkspaceInput = {}, ): Promise => { - const client = getReadyClient(hostTarget); + const client = getReadyClient(hostUrl, hostStatus); try { return await client.workspaceCleanup.destroy.mutate({ workspaceId, @@ -70,29 +77,32 @@ export function useDestroyWorkspace(workspaceId: string): UseDestroyWorkspace { throw normalizeError(err); } }, - [hostTarget, workspaceId], + [hostUrl, hostStatus, workspaceId], ); const inspect = useCallback(async (): Promise => { - const client = getReadyClient(hostTarget); + const client = getReadyClient(hostUrl, hostStatus); try { return await client.workspaceCleanup.inspect.query({ workspaceId }); } catch (err) { throw normalizeError(err); } - }, [hostTarget, workspaceId]); + }, [hostUrl, hostStatus, workspaceId]); return { hostTarget, destroy, inspect }; } -function getReadyClient(hostTarget: WorkspaceHostTarget) { - if (hostTarget.status !== "ready") { +function getReadyClient( + hostUrl: string | null, + hostStatus: WorkspaceHostTarget["status"], +) { + if (hostUrl == null) { throw { kind: "host-unavailable", - reason: hostTarget.status, + reason: hostStatus, } satisfies DestroyWorkspaceError; } - return getHostServiceClientByUrl(hostTarget.url); + return getHostServiceClientByUrl(hostUrl); } function normalizeError(err: unknown): DestroyWorkspaceError { diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsx b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsx index 909f66464c9..35db1ddb85b 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsx @@ -29,6 +29,7 @@ export function DashboardSidebarDeleteDialog({ setDeleteBranch, hasChanges, hasUnpushedCommits, + canConfirm, blockingReason, isCheckingStatus, error, @@ -54,6 +55,11 @@ export function DashboardSidebarDeleteDialog({ } const hasWarnings = hasChanges || hasUnpushedCommits; + const confirmLabel = isCheckingStatus + ? "Checking…" + : hasWarnings + ? "Delete anyway" + : "Delete"; return ( run(hasWarnings)} - confirmLabel={hasWarnings ? "Delete anyway" : "Delete"} + confirmLabel={confirmLabel} /> ); } diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx index c6b34ba8473..ba54b2fa382 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx @@ -19,6 +19,7 @@ interface DestroyConfirmPaneProps { onDeleteBranchChange: (next: boolean) => void; hasChanges: boolean; hasUnpushedCommits: boolean; + canConfirm: boolean; blockingReason: string | null; isCheckingStatus: boolean; onConfirm: () => void; @@ -33,6 +34,7 @@ export function DestroyConfirmPane({ onDeleteBranchChange, hasChanges, hasUnpushedCommits, + canConfirm, blockingReason, isCheckingStatus, onConfirm, @@ -101,7 +103,7 @@ export function DestroyConfirmPane({ size="sm" className="h-7 px-3 text-xs" onClick={onConfirm} - disabled={isCheckingStatus || !!blockingReason} + disabled={isCheckingStatus || !canConfirm} > {confirmLabel} diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts index a2b4494cf5e..12e897011ee 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts @@ -47,18 +47,34 @@ export function useDestroyDialogState({ const [error, setError] = useState(null); const inFlight = useRef(false); - // Run inspect when the dialog opens AND the host is ready. While the host - // is loading/local-starting, sit in a checking state — no destructive - // banner for transient pending-host UX. + // Run inspect when the dialog opens AND the host is ready. Distinguish + // transient pending-host states (loading / local-starting → silent + // "Checking…") from terminal ones (not-found → blocking banner) so the + // user can't sit in a forever-disabled dialog. useEffect(() => { if (!open) { setInspectState({ status: "idle" }); return; } - if (hostTarget.status !== "ready") { + if ( + hostTarget.status === "loading" || + hostTarget.status === "local-starting" + ) { setInspectState({ status: "loading" }); return; } + if (hostTarget.status === "not-found") { + setInspectState({ + status: "ready", + preview: { + canDelete: false, + reason: "Workspace is no longer available on this host.", + hasChanges: false, + hasUnpushedCommits: false, + }, + }); + return; + } let cancelled = false; setInspectState({ status: "loading" }); @@ -80,7 +96,6 @@ export function useDestroyDialogState({ }, [open, hostTarget.status, inspect]); const preview = inspectState.status === "ready" ? inspectState.preview : null; - const conflictWasRaced = error?.kind === "conflict"; const handleOpenChange = useCallback( (next: boolean) => { @@ -157,8 +172,9 @@ export function useDestroyDialogState({ return { deleteBranch, setDeleteBranch, - hasChanges: (preview?.hasChanges ?? false) || conflictWasRaced, + hasChanges: preview?.hasChanges ?? false, hasUnpushedCommits: preview?.hasUnpushedCommits ?? false, + canConfirm: preview ? preview.canDelete : true, blockingReason: preview && !preview.canDelete ? preview.reason : null, isCheckingStatus: open && inspectState.status === "loading", error, diff --git a/packages/host-service/src/trpc/router/workspace-cleanup/is-main-workspace.ts b/packages/host-service/src/trpc/router/workspace-cleanup/is-main-workspace.ts index d3b22a7f1ee..a518adb6dce 100644 --- a/packages/host-service/src/trpc/router/workspace-cleanup/is-main-workspace.ts +++ b/packages/host-service/src/trpc/router/workspace-cleanup/is-main-workspace.ts @@ -4,9 +4,13 @@ import { eq } from "drizzle-orm"; import { projects, workspaces } from "../../../db/schema"; import type { HostServiceContext } from "../../../types"; -export type IsMainWorkspaceResult = - | { isMain: true; reason: string } - | { isMain: false; reason: null }; +type WorkspaceRow = typeof workspaces.$inferSelect; +type ProjectRow = typeof projects.$inferSelect; + +export type IsMainWorkspaceResult = { + local: WorkspaceRow | undefined; + project: ProjectRow | undefined; +} & ({ isMain: true; reason: string } | { isMain: false; reason: null }); export const MAIN_WORKSPACE_REASON = "Main workspaces cannot be deleted. Remove them from the sidebar or remove the project from this host instead."; @@ -24,6 +28,9 @@ export const MAIN_WORKSPACE_REASON = * Both signals exist because either side can lag the other: a workspace * classified as main in cloud may not yet have its local worktreePath * rewritten, and vice versa. + * + * Returns the loaded `local`/`project` rows alongside the verdict so callers + * (notably `runDestroy`) can avoid re-querying SQLite for the same rows. */ export async function isMainWorkspace( ctx: HostServiceContext, @@ -43,7 +50,7 @@ export async function isMainWorkspace( project && normalizePath(local.worktreePath) === normalizePath(project.repoPath) ) { - return { isMain: true, reason: MAIN_WORKSPACE_REASON }; + return { isMain: true, reason: MAIN_WORKSPACE_REASON, local, project }; } if (ctx.api) { @@ -52,11 +59,11 @@ export async function isMainWorkspace( id: workspaceId, }); if (cloudWorkspace?.type === "main") { - return { isMain: true, reason: MAIN_WORKSPACE_REASON }; + return { isMain: true, reason: MAIN_WORKSPACE_REASON, local, project }; } } - return { isMain: false, reason: null }; + return { isMain: false, reason: null, local, project }; } function normalizePath(p: string): string { diff --git a/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts b/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts index ae4fc886af9..873ec7b0748 100644 --- a/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts +++ b/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts @@ -1,7 +1,7 @@ import { TRPCError } from "@trpc/server"; import { eq } from "drizzle-orm"; import { z } from "zod"; -import { projects, workspaces } from "../../../db/schema"; +import { workspaces } from "../../../db/schema"; import { invalidateLabelCache } from "../../../ports/static-ports"; import { runTeardown, type TeardownResult } from "../../../runtime/teardown"; import { disposeSessionsByWorkspaceId } from "../../../terminal/terminal"; @@ -61,9 +61,7 @@ export const workspaceCleanupRouter = router({ }; } - const local = ctx.db.query.workspaces - .findFirst({ where: eq(workspaces.id, input.workspaceId) }) - .sync(); + const { local } = main; if (!local) { return { canDelete: true, @@ -166,19 +164,13 @@ export const workspaceCleanupRouter = router({ async function runDestroy(ctx: HostServiceContext, input: DestroyInput) { const warnings: string[] = []; - const local = ctx.db.query.workspaces - .findFirst({ where: eq(workspaces.id, input.workspaceId) }) - .sync(); - const project = local - ? ctx.db.query.projects - .findFirst({ where: eq(projects.id, local.projectId) }) - .sync() - : undefined; - + // `isMainWorkspace` already loads workspace + project rows from sqlite; + // thread them through to avoid duplicate sync queries downstream. const main = await isMainWorkspace(ctx, input.workspaceId); if (main.isMain) { throw new TRPCError({ code: "BAD_REQUEST", message: main.reason }); } + const { local, project } = main; // ─── Step 0: Preflight ───────────────────────────────────────── // Block only on dirty worktree (the common "I forgot to commit" @@ -253,32 +245,46 @@ async function runDestroy(ctx: HostServiceContext, input: DestroyInput) { let worktreeRemoved = false; let branchDeleted = false; if (local && project) { - const git = await ctx.git(project.repoPath); + // Past the commit point — every failure here is a warning, including + // failure to even open the repo. Letting `ctx.git` escape would surface + // as a hard error for a workspace that's already been deleted in cloud. + let git: Awaited> | null = null; try { - await git.raw(["worktree", "remove", "--force", local.worktreePath]); - worktreeRemoved = true; + git = await ctx.git(project.repoPath); } catch (err) { const message = err instanceof Error ? err.message : String(err); - if ( - message.includes("is not a working tree") || - message.includes("No such file or directory") || - message.includes("ENOENT") - ) { - worktreeRemoved = true; - } else { - warnings.push( - `Failed to remove worktree at ${local.worktreePath}: ${message}`, - ); - } + warnings.push( + `Failed to open project repo at ${project.repoPath}: ${message}`, + ); } - if (input.deleteBranch && local.branch) { + if (git) { try { - await git.raw(["branch", "-D", local.branch]); - branchDeleted = true; + await git.raw(["worktree", "remove", "--force", local.worktreePath]); + worktreeRemoved = true; } catch (err) { const message = err instanceof Error ? err.message : String(err); - warnings.push(`Failed to delete branch ${local.branch}: ${message}`); + if ( + message.includes("is not a working tree") || + message.includes("No such file or directory") || + message.includes("ENOENT") + ) { + worktreeRemoved = true; + } else { + warnings.push( + `Failed to remove worktree at ${local.worktreePath}: ${message}`, + ); + } + } + + if (input.deleteBranch && local.branch) { + try { + await git.raw(["branch", "-D", local.branch]); + branchDeleted = true; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + warnings.push(`Failed to delete branch ${local.branch}: ${message}`); + } } } } diff --git a/packages/host-service/test/workspace-cleanup.test.ts b/packages/host-service/test/workspace-cleanup.test.ts index a1d0b657cb0..b13875fc696 100644 --- a/packages/host-service/test/workspace-cleanup.test.ts +++ b/packages/host-service/test/workspace-cleanup.test.ts @@ -357,3 +357,38 @@ describe("workspaceCleanup.destroy in-flight guard", () => { expect(__testDestroysInFlight.has("ws-1")).toBe(false); }); }); + +describe("workspaceCleanup.destroy phase-3 best-effort cleanup", () => { + beforeEach(() => __testDestroysInFlight.clear()); + afterEach(() => __testDestroysInFlight.clear()); + + test("git-factory failure in phase 3 becomes a warning, not a hard error", async () => { + // Past phase 2 (cloud delete) the workspace is gone in cloud — every + // failure here must surface as a warning so the mutation still + // resolves with `success: true`. Otherwise the user sees a + // "Failed to delete" toast for a workspace that's actually deleted. + const ctx = makeCtx({ + workspace: { + id: "ws-1", + projectId: "p-1", + worktreePath: "/branch/wt", + branch: "feature", + }, + project: { id: "p-1", repoPath: "/repo" }, + cloudType: "worktree", + gitFactoryThrows: true, + }); + const caller = workspaceCleanupRouter.createCaller(ctx); + const result = await caller.destroy({ + workspaceId: "ws-1", + deleteBranch: false, + force: true, // skip phase 0/1 so we go straight to phase 2/3 + }); + expect(result.success).toBe(true); + expect(result.cloudDeleted).toBe(true); + expect(result.worktreeRemoved).toBe(false); + expect( + result.warnings.some((w) => w.includes("Failed to open project repo")), + ).toBe(true); + }); +}); diff --git a/plans/20260430-v2-delete-workspace-audit.md b/plans/20260430-v2-delete-workspace-audit.md index 5a7a6721be5..db8e5dba1ee 100644 --- a/plans/20260430-v2-delete-workspace-audit.md +++ b/plans/20260430-v2-delete-workspace-audit.md @@ -27,8 +27,8 @@ Scope: align v2 delete dialog with the host-service destroy saga; surface bugs/i |---|---|---| | 1 | Preflight location | Host-service `inspect` query, called from the v2 dialog | | 2 | "Is main workspace" check | Shared `isMainWorkspace()` helper, `realpath`-normalized paths, both local equality and cloud `getFromHost.type === "main"` | -| 3 | Unpushed commits | No upstream = treat as has-unpushed (use `git for-each-ref` to detect missing upstream, fall back to `git rev-list HEAD`) | -| 4 | Concurrent-delete guard | Process-local `Map` on the host-service; second caller awaits the first or gets a typed `"already in progress"` error | +| 3 | Unpushed commits | No upstream = treat as has-unpushed (`git rev-list HEAD --not --remotes` — upstream-agnostic, catches commits not reachable from any remote ref) | +| 4 | Concurrent-delete guard | Process-local `Set` on the host-service; second caller throws `CONFLICT` with `DELETE_IN_PROGRESS` cause (distinct from dirty-worktree CONFLICT so the renderer doesn't silent-retry) | | 5 | Pending-host UX | `loading` and `local-starting` map to `isCheckingStatus` — disabled Delete button + spinner, no banner | | 6 | `CONFLICT` after clean preflight | Silent force-retry, with a code comment explaining the race so it's not removed again | | 7 | Git status failure during inspect | `canDelete: true`, no warnings — matches v2 saga's "best-effort cleanup" contract | @@ -40,7 +40,7 @@ Order chosen so each step is shippable on its own: 1. **Add `isMainWorkspace()` helper** in host-service (or shared lib if both packages need it). Includes `realpath` normalization. Replace the inline check inside `destroy`. No behavior change yet. 2. **Add `workspaceCleanup.inspect`** on host-service. Uses `isMainWorkspace`, the upstream-aware unpushed check, and the git-failure-is-fine contract. Returns `{ canDelete, reason, hasChanges, hasUnpushedCommits }`. -3. **Add concurrent-delete guard** to `workspaceCleanup.destroy` — `Map` at saga entry. Throws typed `"already in progress"` for the second caller. +3. **Add concurrent-delete guard** to `workspaceCleanup.destroy` — `Set` at saga entry. Second caller throws `CONFLICT` with a `DELETE_IN_PROGRESS` cause; distinct from dirty-worktree CONFLICT so the renderer surfaces a toast instead of silently force-retrying. 4. **Promote `useWorkspaceHostUrl` → `useWorkspaceHostTarget`** with the status union (`loading | not-found | local-starting | ready`). Keep `useWorkspaceHostUrl` as a back-compat thin wrapper. 5. **Migrate `useDestroyDialogState`** off `electronTrpc.workspaces.canDelete` and onto the new `inspect`. Treat `loading`/`local-starting` as `isCheckingStatus`. Keep the silent force-retry on `CONFLICT` (with the comment). 6. **Wire `blockingReason` + `confirmLabel` through the dialog UI** (`DashboardSidebarDeleteDialog` → `DestroyConfirmPane`). From 7166cdc50ab4624e132aad30f4f6a06ccf6a9868 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Thu, 30 Apr 2026 12:41:50 -0700 Subject: [PATCH 4/5] chore(host-service): tighten workspace-cleanup tests Three small wins, net -11 lines: - drop unused `ahead` field from ContextSpec (leftover from when inspect used status.ahead instead of rev-list) - drop redundant afterEach (beforeEach already protects the next test from any leak) - replace manual try/catch in the concurrent-CONFLICT test with `await expect(...).rejects.toMatchObject(...)` No coverage change. 16 tests, all green. --- .../test/workspace-cleanup.test.ts | 33 +++++++------------ 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/packages/host-service/test/workspace-cleanup.test.ts b/packages/host-service/test/workspace-cleanup.test.ts index b13875fc696..61bcbcf8f6b 100644 --- a/packages/host-service/test/workspace-cleanup.test.ts +++ b/packages/host-service/test/workspace-cleanup.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { beforeEach, describe, expect, mock, test } from "bun:test"; import { mkdirSync, mkdtempSync, @@ -8,7 +8,6 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { TRPCError } from "@trpc/server"; import { isMainWorkspace } from "../src/trpc/router/workspace-cleanup/is-main-workspace"; import { __testDestroysInFlight, @@ -29,7 +28,7 @@ interface ContextSpec { project?: ProjectRow; cloudType?: "main" | "worktree"; cloudDelete?: () => Promise; - gitStatus?: { isClean: () => boolean; ahead?: number }; + gitStatus?: { isClean: () => boolean }; revListCount?: string | (() => Promise); gitFactoryThrows?: boolean; } @@ -274,12 +273,7 @@ describe("workspaceCleanup.inspect", () => { }); describe("workspaceCleanup.destroy in-flight guard", () => { - beforeEach(() => { - __testDestroysInFlight.clear(); - }); - afterEach(() => { - __testDestroysInFlight.clear(); - }); + beforeEach(() => __testDestroysInFlight.clear()); test("clears the Set on success", async () => { const ctx = makeCtx({}); @@ -311,21 +305,17 @@ describe("workspaceCleanup.destroy in-flight guard", () => { test("rejects a concurrent call with CONFLICT + DELETE_IN_PROGRESS cause", async () => { __testDestroysInFlight.add("ws-1"); - const ctx = makeCtx({}); - const caller = workspaceCleanupRouter.createCaller(ctx); - try { - await caller.destroy({ + const caller = workspaceCleanupRouter.createCaller(makeCtx({})); + await expect( + caller.destroy({ workspaceId: "ws-1", deleteBranch: false, force: false, - }); - throw new Error("expected destroy to throw"); - } catch (err) { - expect(err).toBeInstanceOf(TRPCError); - const trpcErr = err as TRPCError; - expect(trpcErr.code).toBe("CONFLICT"); - expect(trpcErr.cause).toMatchObject({ kind: "DELETE_IN_PROGRESS" }); - } + }), + ).rejects.toMatchObject({ + code: "CONFLICT", + cause: { kind: "DELETE_IN_PROGRESS" }, + }); }); test("retry after a failed destroy succeeds (no in-flight leak)", async () => { @@ -360,7 +350,6 @@ describe("workspaceCleanup.destroy in-flight guard", () => { describe("workspaceCleanup.destroy phase-3 best-effort cleanup", () => { beforeEach(() => __testDestroysInFlight.clear()); - afterEach(() => __testDestroysInFlight.clear()); test("git-factory failure in phase 3 becomes a warning, not a hard error", async () => { // Past phase 2 (cloud delete) the workspace is gone in cloud — every From c27e934d1673058e475ea53ec6166bfca81c1ca9 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Thu, 30 Apr 2026 12:51:41 -0700 Subject: [PATCH 5/5] fix(host-service): broaden phase-3 try/catch + tighten preview type Two more PR-review findings: - disposeSessionsByWorkspaceId, the host sqlite delete, and invalidateLabelCache were still outside any try/catch. Anything that throws past phase 2 (cloud-commit point) breaks the "phase 3 = best-effort" contract and surfaces a hard "Failed to delete" toast for an already-deleted workspace. Each is now its own try/catch + warning push. - InspectResult is now a discriminated union on both server and renderer sides. The bad combination { canDelete: false, reason: null } is unrepresentable at the type level, so the previous "button still enabled when reason is null" bug can't be reintroduced even if a future refactor drops the canConfirm consumer-side check. Adds a sqlite-delete-throws regression test. 17 tests, all green. --- .../useDestroyWorkspace.ts | 24 ++++++--- .../workspace-cleanup/workspace-cleanup.ts | 51 ++++++++++++++++--- .../test/workspace-cleanup.test.ts | 34 ++++++++++++- 3 files changed, 96 insertions(+), 13 deletions(-) diff --git a/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts b/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts index 123220267c9..846b1c6236c 100644 --- a/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts +++ b/apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts @@ -23,12 +23,24 @@ export interface DestroyWorkspaceSuccess { warnings: string[]; } -export interface DestroyWorkspacePreview { - canDelete: boolean; - reason: string | null; - hasChanges: boolean; - hasUnpushedCommits: boolean; -} +/** + * Mirrors the server's `InspectResult` discriminated union so the renderer + * can't accidentally treat `{ canDelete: false, reason: null }` as a no-op + * — that combination is unrepresentable. + */ +export type DestroyWorkspacePreview = + | { + canDelete: true; + reason: null; + hasChanges: boolean; + hasUnpushedCommits: boolean; + } + | { + canDelete: false; + reason: string; + hasChanges: false; + hasUnpushedCommits: false; + }; export type DestroyWorkspaceError = | { kind: "conflict"; message: string } diff --git a/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts b/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts index 873ec7b0748..ce0bc2bf575 100644 --- a/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts +++ b/packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts @@ -34,6 +34,25 @@ interface DestroyInput { force: boolean; } +/** + * Discriminated so the renderer can't accidentally treat + * `{ canDelete: false, reason: null }` as a no-op — it's an unrepresentable + * combination at the type level. + */ +type InspectResult = + | { + canDelete: true; + reason: null; + hasChanges: boolean; + hasUnpushedCommits: boolean; + } + | { + canDelete: false; + reason: string; + hasChanges: false; + hasUnpushedCommits: false; + }; + export const workspaceCleanupRouter = router({ /** * Status preview for the v2 delete dialog. Co-located with `destroy` so @@ -50,7 +69,7 @@ export const workspaceCleanupRouter = router({ */ inspect: protectedProcedure .input(z.object({ workspaceId: z.string() })) - .query(async ({ ctx, input }) => { + .query(async ({ ctx, input }): Promise => { const main = await isMainWorkspace(ctx, input.workspaceId); if (main.isMain) { return { @@ -235,9 +254,14 @@ async function runDestroy(ctx: HostServiceContext, input: DestroyInput) { // caller always sees success. // 3a. PTYs - const killed = disposeSessionsByWorkspaceId(input.workspaceId, ctx.db); - if (killed.failed > 0) { - warnings.push(`${killed.failed} terminal(s) may still be running`); + try { + const killed = disposeSessionsByWorkspaceId(input.workspaceId, ctx.db); + if (killed.failed > 0) { + warnings.push(`${killed.failed} terminal(s) may still be running`); + } + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + warnings.push(`Failed to dispose terminal sessions: ${message}`); } // 3b. Worktree (always --force: we're past the commit point) @@ -291,8 +315,23 @@ async function runDestroy(ctx: HostServiceContext, input: DestroyInput) { // 3d. Host sqlite row if (local) { - ctx.db.delete(workspaces).where(eq(workspaces.id, input.workspaceId)).run(); - invalidateLabelCache(input.workspaceId); + try { + ctx.db + .delete(workspaces) + .where(eq(workspaces.id, input.workspaceId)) + .run(); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + warnings.push( + `Failed to remove local workspace row for ${input.workspaceId}: ${message}`, + ); + } + try { + invalidateLabelCache(input.workspaceId); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + warnings.push(`Failed to invalidate label cache: ${message}`); + } } return { diff --git a/packages/host-service/test/workspace-cleanup.test.ts b/packages/host-service/test/workspace-cleanup.test.ts index 61bcbcf8f6b..efa9433cd4e 100644 --- a/packages/host-service/test/workspace-cleanup.test.ts +++ b/packages/host-service/test/workspace-cleanup.test.ts @@ -31,6 +31,7 @@ interface ContextSpec { gitStatus?: { isClean: () => boolean }; revListCount?: string | (() => Promise); gitFactoryThrows?: boolean; + dbDeleteThrows?: boolean; } function makeCtx(spec: ContextSpec): HostServiceContext { @@ -68,7 +69,9 @@ function makeCtx(spec: ContextSpec): HostServiceContext { }; }); - const dbDeleteRun = mock(() => undefined); + const dbDeleteRun = mock(() => { + if (spec.dbDeleteThrows) throw new Error("sqlite delete boom"); + }); const dbDeleteWhere = mock(() => ({ run: dbDeleteRun })); const terminalSelectAll = mock(() => []); @@ -380,4 +383,33 @@ describe("workspaceCleanup.destroy phase-3 best-effort cleanup", () => { result.warnings.some((w) => w.includes("Failed to open project repo")), ).toBe(true); }); + + test("sqlite row-delete failure in phase 3d becomes a warning", async () => { + // Same contract as the git-factory case: any phase-3 op that throws + // past the cloud-commit point must degrade to a warning, not bubble. + const ctx = makeCtx({ + workspace: { + id: "ws-1", + projectId: "p-1", + worktreePath: "/branch/wt", + branch: "feature", + }, + project: { id: "p-1", repoPath: "/repo" }, + cloudType: "worktree", + dbDeleteThrows: true, + }); + const caller = workspaceCleanupRouter.createCaller(ctx); + const result = await caller.destroy({ + workspaceId: "ws-1", + deleteBranch: false, + force: true, + }); + expect(result.success).toBe(true); + expect(result.cloudDeleted).toBe(true); + expect( + result.warnings.some((w) => + w.includes("Failed to remove local workspace row"), + ), + ).toBe(true); + }); });