From 679eb00ec49ad1035802fb44b6a5d21f01f76188 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 1 Jul 2026 15:33:52 +0100 Subject: [PATCH] fix: surface workspace delete failures from AgentsPage archive flow (#26900) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Right-clicking **Archive & delete workspace** on an agent chat could leave the workspace behind without the user noticing. The archive step ran first and removed the chat from the sidebar, so when the delete enqueue failed the user lost the surface to retry and the workspace lingered. ## Fix - Delete the workspace first, archive the chat second. If the delete enqueue fails for anything other than 404/410, the chat stays in the list so the user can retry. 404/410 are still treated as "already gone" so the archive proceeds. - Errors are wrapped in `ArchiveAndDeleteError` tagged with `step: "delete" | "archive"`. The toast branches on the tag: `delete` failures show an actionable "Open workspace" link, `archive` failures explain the delete already ran so no manual deletion is needed. - Both mutation call sites navigate away on `onSuccess` only (previously `onSettled`), so a delete failure keeps the chat's retry surface reachable. The confirm dialog still closes on `onSettled`. - When the archive step fails after a successful delete, workspace-related caches are invalidated to keep the rest of the app in sync with the ongoing deletion. - On successful enqueue, warn when the build response's `matched_provisioners.count` is 0. That field is populated on `POST /workspacebuilds`; `job.queue_position` / `job.queue_size` are not. > 🤖 This PR was generated by Coder Agents on behalf of @johnstcn. --- site/src/pages/AgentsPage/AgentsPage.tsx | 41 ++- .../utils/agentWorkspaceUtils.test.ts | 344 ++++++++++++++++-- .../AgentsPage/utils/agentWorkspaceUtils.ts | 103 +++++- 3 files changed, 429 insertions(+), 59 deletions(-) diff --git a/site/src/pages/AgentsPage/AgentsPage.tsx b/site/src/pages/AgentsPage/AgentsPage.tsx index 03ec1b8403..65b4182d92 100644 --- a/site/src/pages/AgentsPage/AgentsPage.tsx +++ b/site/src/pages/AgentsPage/AgentsPage.tsx @@ -44,6 +44,7 @@ import { import { invalidateWorkspaceMutationQueries, workspaceById, + workspaceByIdKey, } from "#/api/queries/workspaces"; import type * as TypesGen from "#/api/typesGenerated"; import { ConfirmDialog } from "#/components/Dialogs/ConfirmDialog/ConfirmDialog"; @@ -60,7 +61,10 @@ import { useAgentsPageKeybindings } from "./hooks/useAgentsPageKeybindings"; import { useAgentsPWA } from "./hooks/useAgentsPWA"; import { getAgentSidebarFilters } from "./utils/agentSidebarFilters"; import { + ArchiveAndDeleteError, archiveChatAndDeleteWorkspace, + notifyArchiveAndDeleteFailed, + notifyDeleteQueueState, resolveArchiveAndDeleteAction, shouldNavigateAfterArchive, } from "./utils/agentWorkspaceUtils"; @@ -238,7 +242,7 @@ const AgentsPage: FC = () => { (id) => API.experimental.updateChat(id, { archived: true }), (id) => API.deleteWorkspace(id), ), - onSuccess: ({ chatId }) => { + onSuccess: ({ chatId, workspaceId, deleteBuild }) => { applyChatArchiveStateToCaches(queryClient, chatId, true); clearChatErrorReason(chatId); clearPersistedSidebarTabId(chatId); @@ -255,12 +259,30 @@ const AgentsPage: FC = () => { organizationName, username: user.username, }); - }, - onError: (error) => { - toast.error( - getErrorMessage(error, "Failed to archive and delete workspace."), + notifyDeleteQueueState( + queryClient.getQueryData( + workspaceByIdKey(workspaceId), + ), + deleteBuild, ); }, + onError: (error, { workspaceId }) => { + notifyArchiveAndDeleteFailed( + queryClient.getQueryData( + workspaceByIdKey(workspaceId), + ), + error, + (path) => navigate(path), + ); + // Archive failed after the delete already ran; refresh + // workspace state so consumers see the deletion. + if (error instanceof ArchiveAndDeleteError && error.step === "archive") { + void invalidateWorkspaceMutationQueries(queryClient, { + organizationName, + username: user.username, + }); + } + }, }); const [pendingArchiveChatId, setPendingArchiveChatId] = useState< string | null @@ -418,7 +440,7 @@ const AgentsPage: FC = () => { archiveAndDeleteMutation.mutate( { chatId, workspaceId }, { - onSettled: () => { + onSuccess: () => { navigateAfterArchive(chatId); }, }, @@ -429,11 +451,6 @@ const AgentsPage: FC = () => { // about interrupting a live workspace, which is moot // when the workspace no longer exists. archiveAgentMutation.mutate(chatId, { - // Navigate only on success. The proceed/confirm paths - // use onSettled because their pre-existing behavior - // navigates regardless of delete outcome. This path - // has no delete step, so a failed archive should not - // redirect the user. onSuccess: () => { navigateAfterArchive(chatId); }, @@ -453,6 +470,8 @@ const AgentsPage: FC = () => { archiveAndDeleteMutation.mutate(pendingArchiveAndDelete, { onSettled: () => { setPendingArchiveAndDelete(null); + }, + onSuccess: () => { navigateAfterArchive(archivedChatId); }, }); diff --git a/site/src/pages/AgentsPage/utils/agentWorkspaceUtils.test.ts b/site/src/pages/AgentsPage/utils/agentWorkspaceUtils.test.ts index f8820a3294..941a23ee1a 100644 --- a/site/src/pages/AgentsPage/utils/agentWorkspaceUtils.test.ts +++ b/site/src/pages/AgentsPage/utils/agentWorkspaceUtils.test.ts @@ -1,9 +1,27 @@ -import { describe, expect, it, vi } from "vitest"; -import { PrebuildsSystemUserID } from "#/api/typesGenerated"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("sonner", () => ({ + toast: { + error: vi.fn(), + info: vi.fn(), + success: vi.fn(), + warning: vi.fn(), + }, +})); + +import { toast } from "sonner"; import { + PrebuildsSystemUserID, + type Workspace, + type WorkspaceBuild, +} from "#/api/typesGenerated"; +import { + ArchiveAndDeleteError, archiveChatAndDeleteWorkspace, isWorkspaceAutoCreated, isWorkspaceNotFound, + notifyArchiveAndDeleteFailed, + notifyDeleteQueueState, resolveArchiveAndDeleteAction, shouldNavigateAfterArchive, workspaceAcquiredAt, @@ -259,13 +277,18 @@ describe("isWorkspaceNotFound", () => { }); describe("archiveChatAndDeleteWorkspace", () => { - it("archives and deletes when both succeed", async () => { + const BUILD_OK = { + job: { queue_position: 0, queue_size: 1 }, + } as unknown as WorkspaceBuild; + + it("archives and deletes when both succeed, deleting first", async () => { const callOrder: string[] = []; const doArchive = vi.fn(async () => { callOrder.push("archive"); }); const doDelete = vi.fn(async () => { callOrder.push("delete"); + return BUILD_OK; }); await expect( @@ -275,17 +298,25 @@ describe("archiveChatAndDeleteWorkspace", () => { doArchive, doDelete, ), - ).resolves.toEqual({ chatId: "chat-1", workspaceId: "workspace-1" }); + ).resolves.toEqual({ + chatId: "chat-1", + workspaceId: "workspace-1", + deleteBuild: BUILD_OK, + }); expect(doArchive).toHaveBeenCalledTimes(1); expect(doArchive).toHaveBeenCalledWith("chat-1"); expect(doDelete).toHaveBeenCalledTimes(1); expect(doDelete).toHaveBeenCalledWith("workspace-1"); - expect(callOrder).toEqual(["archive", "delete"]); + expect(callOrder).toEqual(["delete", "archive"]); }); - it("succeeds when delete returns 404", async () => { - const doArchive = vi.fn(async () => undefined); + it("archives even when delete returns 404, with null deleteBuild", async () => { + const callOrder: string[] = []; + const doArchive = vi.fn(async () => { + callOrder.push("archive"); + }); const doDelete = vi.fn(async () => { + callOrder.push("delete"); throw { isAxiosError: true, response: { @@ -302,12 +333,15 @@ describe("archiveChatAndDeleteWorkspace", () => { doArchive, doDelete, ), - ).resolves.toEqual({ chatId: "chat-1", workspaceId: "workspace-1" }); - expect(doArchive).toHaveBeenCalledTimes(1); - expect(doDelete).toHaveBeenCalledTimes(1); + ).resolves.toEqual({ + chatId: "chat-1", + workspaceId: "workspace-1", + deleteBuild: null, + }); + expect(callOrder).toEqual(["delete", "archive"]); }); - it("succeeds when delete returns 410", async () => { + it("archives even when delete returns 410, with null deleteBuild", async () => { const doArchive = vi.fn(async () => undefined); const doDelete = vi.fn(async () => { throw { @@ -326,14 +360,18 @@ describe("archiveChatAndDeleteWorkspace", () => { doArchive, doDelete, ), - ).resolves.toEqual({ chatId: "chat-1", workspaceId: "workspace-1" }); + ).resolves.toEqual({ + chatId: "chat-1", + workspaceId: "workspace-1", + deleteBuild: null, + }); expect(doArchive).toHaveBeenCalledTimes(1); expect(doDelete).toHaveBeenCalledTimes(1); }); - it("throws when delete returns non-404-or-410 error", async () => { + it("wraps non-404-or-410 delete failures and skips archive", async () => { const doArchive = vi.fn(async () => undefined); - const error = { + const cause = { isAxiosError: true, response: { status: 500, @@ -341,38 +379,83 @@ describe("archiveChatAndDeleteWorkspace", () => { }, }; const doDelete = vi.fn(async () => { - throw error; + throw cause; }); - await expect( - archiveChatAndDeleteWorkspace( - "chat-1", - "workspace-1", - doArchive, - doDelete, - ), - ).rejects.toBe(error); - expect(doArchive).toHaveBeenCalledTimes(1); + const promise = archiveChatAndDeleteWorkspace( + "chat-1", + "workspace-1", + doArchive, + doDelete, + ); + await expect(promise).rejects.toBeInstanceOf(ArchiveAndDeleteError); + const err = await promise.catch((e: unknown) => e); + expect((err as ArchiveAndDeleteError).step).toBe("delete"); + expect((err as ArchiveAndDeleteError).cause).toBe(cause); expect(doDelete).toHaveBeenCalledTimes(1); + expect(doArchive).not.toHaveBeenCalled(); }); - it("throws when archive fails without attempting delete", async () => { - const error = new Error("archive failed"); + it("wraps archive failures that follow a successful delete", async () => { + const cause = new Error("archive failed"); const doArchive = vi.fn(async () => { - throw error; + throw cause; }); - const doDelete = vi.fn(async () => undefined); + const doDelete = vi.fn(async () => BUILD_OK); - await expect( - archiveChatAndDeleteWorkspace( - "chat-1", - "workspace-1", - doArchive, - doDelete, - ), - ).rejects.toBe(error); + const promise = archiveChatAndDeleteWorkspace( + "chat-1", + "workspace-1", + doArchive, + doDelete, + ); + await expect(promise).rejects.toBeInstanceOf(ArchiveAndDeleteError); + const err = await promise.catch((e: unknown) => e); + expect((err as ArchiveAndDeleteError).step).toBe("archive"); + expect((err as ArchiveAndDeleteError).cause).toBe(cause); + expect((err as ArchiveAndDeleteError).deleteEnqueued).toBe(true); + expect(doDelete).toHaveBeenCalledTimes(1); expect(doArchive).toHaveBeenCalledTimes(1); - expect(doDelete).not.toHaveBeenCalled(); + }); + + it("marks archive failures with deleteEnqueued=false when delete was skipped", async () => { + const doArchive = vi.fn(async () => { + throw new Error("archive failed"); + }); + const doDelete = vi.fn(async () => { + throw { + isAxiosError: true, + response: { status: 410, data: { message: "gone" } }, + }; + }); + + const promise = archiveChatAndDeleteWorkspace( + "chat-1", + "workspace-1", + doArchive, + doDelete, + ); + const err = (await promise.catch( + (e: unknown) => e, + )) as ArchiveAndDeleteError; + expect(err.step).toBe("archive"); + expect(err.deleteEnqueued).toBe(false); + }); + + it("returns the delete build payload on success", async () => { + const build = { + job: { queue_position: 4, queue_size: 7 }, + } as unknown as WorkspaceBuild; + const doArchive = vi.fn(async () => undefined); + const doDelete = vi.fn(async () => build); + + const result = await archiveChatAndDeleteWorkspace( + "chat-1", + "workspace-1", + doArchive, + doDelete, + ); + expect(result.deleteBuild).toBe(build); }); }); @@ -633,3 +716,190 @@ describe("shouldNavigateAfterArchive", () => { ).toBe(expected); }); }); + +const makeWorkspace = (overrides: Partial = {}): Workspace => + ({ + id: "ws-1", + name: "my-workspace", + owner_name: "alice", + ...overrides, + }) as Workspace; + +const makeDeleteBuild = (matched?: { + count: number; + available?: number; +}): WorkspaceBuild => + ({ + job: { queue_position: 0, queue_size: 0 }, + matched_provisioners: matched + ? { count: matched.count, available: matched.available ?? matched.count } + : undefined, + }) as unknown as WorkspaceBuild; + +describe("notifyDeleteQueueState", () => { + const toastWarning = toast.warning as unknown as ReturnType; + const toastInfo = toast.info as unknown as ReturnType; + const toastSuccess = toast.success as unknown as ReturnType; + + beforeEach(() => { + toastWarning.mockClear(); + toastInfo.mockClear(); + toastSuccess.mockClear(); + }); + + it("is silent when no build is returned (workspace already gone)", () => { + notifyDeleteQueueState(makeWorkspace(), null); + expect(toastWarning).not.toHaveBeenCalled(); + expect(toastInfo).not.toHaveBeenCalled(); + expect(toastSuccess).not.toHaveBeenCalled(); + }); + + it("is silent when workspace is not in cache", () => { + notifyDeleteQueueState(undefined, makeDeleteBuild({ count: 0 })); + expect(toastWarning).not.toHaveBeenCalled(); + }); + + it("is silent when matched_provisioners is absent (older servers)", () => { + notifyDeleteQueueState(makeWorkspace(), makeDeleteBuild()); + expect(toastWarning).not.toHaveBeenCalled(); + }); + + it("is silent on the happy path (at least one matching provisioner)", () => { + notifyDeleteQueueState(makeWorkspace(), makeDeleteBuild({ count: 2 })); + expect(toastWarning).not.toHaveBeenCalled(); + }); + + it("warns when no matching provisioners exist (count = 0)", () => { + notifyDeleteQueueState( + makeWorkspace({ name: "stuck-ws" }), + makeDeleteBuild({ count: 0 }), + ); + expect(toastWarning).toHaveBeenCalledTimes(1); + const message = toastWarning.mock.calls[0][0] as string; + expect(message).toContain("stuck-ws"); + expect(message).toContain("no matching provisioners"); + }); +}); + +describe("notifyArchiveAndDeleteFailed", () => { + const toastError = toast.error as unknown as ReturnType; + + beforeEach(() => { + toastError.mockClear(); + }); + + it("shows a generic delete-failed toast when workspace is not in cache", () => { + const onOpen = vi.fn(); + notifyArchiveAndDeleteFailed( + undefined, + new ArchiveAndDeleteError("delete", {}), + onOpen, + ); + expect(toastError).toHaveBeenCalledTimes(1); + expect(toastError.mock.calls[0][0]).toContain( + "Failed to delete workspace.", + ); + expect(toastError.mock.calls[0][1]).toBeUndefined(); + }); + + it("includes workspace name and an Open workspace action when delete fails", () => { + const onOpen = vi.fn(); + notifyArchiveAndDeleteFailed( + makeWorkspace({ name: "left-behind", owner_name: "bob" }), + new ArchiveAndDeleteError("delete", {}), + onOpen, + ); + expect(toastError).toHaveBeenCalledTimes(1); + const [message, options] = toastError.mock.calls[0] as [ + string, + { + description: string; + action: { label: string; onClick: () => void }; + }, + ]; + expect(message).toContain("left-behind"); + expect(options.description).toContain("not archived"); + expect(options.action.label).toBe("Open workspace"); + options.action.onClick(); + expect(onOpen).toHaveBeenCalledWith("/@bob/left-behind"); + }); + + it("announces partial success when only the archive step fails after enqueue", () => { + const onOpen = vi.fn(); + notifyArchiveAndDeleteFailed( + makeWorkspace({ name: "deleting-ws" }), + new ArchiveAndDeleteError("archive", new Error("forbidden"), true), + onOpen, + ); + expect(toastError).toHaveBeenCalledTimes(1); + const [message, options] = toastError.mock.calls[0] as [string, undefined]; + expect(message).toContain("deleting-ws"); + expect(message).toContain("Deleting"); + expect(message).toContain("failed to archive"); + expect(options).toBeUndefined(); + expect(onOpen).not.toHaveBeenCalled(); + }); + + it("omits the 'Deleting' claim when the workspace was already gone (delete swallowed)", () => { + const onOpen = vi.fn(); + notifyArchiveAndDeleteFailed( + makeWorkspace({ name: "already-gone" }), + new ArchiveAndDeleteError("archive", new Error("forbidden"), false), + onOpen, + ); + expect(toastError).toHaveBeenCalledTimes(1); + const message = toastError.mock.calls[0][0] as string; + expect(message).toContain("already-gone"); + expect(message).toContain("Failed to archive"); + expect(message).not.toContain("Deleting"); + }); + + it("handles archive-step failure with no workspace in cache", () => { + const onOpen = vi.fn(); + notifyArchiveAndDeleteFailed( + undefined, + new ArchiveAndDeleteError("archive", new Error("forbidden"), true), + onOpen, + ); + expect(toastError).toHaveBeenCalledTimes(1); + const [message, options] = toastError.mock.calls[0] as [string, undefined]; + expect(message).toContain("the workspace"); + expect(message).toContain("failed to archive"); + expect(options).toBeUndefined(); + }); + + it("surfaces the original error's message when present", () => { + const onOpen = vi.fn(); + notifyArchiveAndDeleteFailed( + makeWorkspace(), + new ArchiveAndDeleteError( + "delete", + new Error("template version archived"), + ), + onOpen, + ); + const message = toastError.mock.calls[0][0] as string; + expect(message).toContain("template version archived"); + }); + + it("falls back to the delete branch (with action) for non-tagged errors", () => { + const onOpen = vi.fn(); + notifyArchiveAndDeleteFailed( + makeWorkspace({ name: "raw-ws", owner_name: "carol" }), + new Error("raw"), + onOpen, + ); + expect(toastError).toHaveBeenCalledTimes(1); + const [message, options] = toastError.mock.calls[0] as [ + string, + { + description: string; + action: { label: string; onClick: () => void }; + }, + ]; + expect(message).toContain("raw"); + expect(options.action.label).toBe("Open workspace"); + options.action.onClick(); + expect(onOpen).toHaveBeenCalledWith("/@carol/raw-ws"); + }); +}); diff --git a/site/src/pages/AgentsPage/utils/agentWorkspaceUtils.ts b/site/src/pages/AgentsPage/utils/agentWorkspaceUtils.ts index c2566e31e5..ae349c49ef 100644 --- a/site/src/pages/AgentsPage/utils/agentWorkspaceUtils.ts +++ b/site/src/pages/AgentsPage/utils/agentWorkspaceUtils.ts @@ -1,6 +1,9 @@ import { isAxiosError } from "axios"; +import { toast } from "sonner"; +import { getErrorMessage } from "#/api/errors"; import { PrebuildsSystemUserID, + type Workspace, type WorkspaceBuild, } from "#/api/typesGenerated"; @@ -86,26 +89,50 @@ export function isWorkspaceNotFound(error: unknown): boolean { return status === 404 || status === 410; } -/** - * Archives a chat and then deletes its associated workspace. - * If the workspace is already gone (404 or 410), the delete step is - * treated as a no-op so the archive still succeeds. - */ +export class ArchiveAndDeleteError extends Error { + readonly step: "delete" | "archive"; + readonly deleteEnqueued: boolean; + declare readonly cause: unknown; + + constructor( + step: "delete" | "archive", + cause: unknown, + deleteEnqueued = false, + ) { + super( + step === "delete" ? "workspace delete failed" : "chat archive failed", + { cause }, + ); + this.step = step; + this.deleteEnqueued = deleteEnqueued; + } +} + +// Delete-first, archive-second. 404/410 on delete falls through to archive. export async function archiveChatAndDeleteWorkspace( chatId: string, workspaceId: string, doArchive: (chatId: string) => Promise, - doDelete: (workspaceId: string) => Promise, -): Promise<{ chatId: string; workspaceId: string }> { - await doArchive(chatId); + doDelete: (workspaceId: string) => Promise, +): Promise<{ + chatId: string; + workspaceId: string; + deleteBuild: WorkspaceBuild | null; +}> { + let deleteBuild: WorkspaceBuild | null = null; try { - await doDelete(workspaceId); + deleteBuild = await doDelete(workspaceId); } catch (error) { if (!isWorkspaceNotFound(error)) { - throw error; + throw new ArchiveAndDeleteError("delete", error); } } - return { chatId, workspaceId }; + try { + await doArchive(chatId); + } catch (error) { + throw new ArchiveAndDeleteError("archive", error, deleteBuild !== null); + } + return { chatId, workspaceId, deleteBuild }; } /** @@ -189,3 +216,57 @@ export async function resolveArchiveAndDeleteAction( } return "confirm"; } + +export function notifyDeleteQueueState( + workspace: Workspace | undefined, + deleteBuild: WorkspaceBuild | null, +): void { + if (!deleteBuild || !workspace) { + return; + } + const matched = deleteBuild.matched_provisioners; + if (matched && matched.count === 0) { + toast.warning( + `Delete enqueued for "${workspace.name}", but no matching provisioners are available. The workspace will be deleted once one comes online.`, + ); + } +} + +export function notifyArchiveAndDeleteFailed( + workspace: Workspace | undefined, + error: unknown, + onOpenWorkspace: (path: string) => void, +): void { + const step = error instanceof ArchiveAndDeleteError ? error.step : undefined; + const cause = error instanceof ArchiveAndDeleteError ? error.cause : error; + + if (step === "archive") { + const label = workspace ? `"${workspace.name}"` : "the workspace"; + const deleteEnqueued = + error instanceof ArchiveAndDeleteError && error.deleteEnqueued; + const prefix = deleteEnqueued + ? `Deleting ${label}, but failed to archive the chat.` + : `Failed to archive the chat for ${label}.`; + const detail = getErrorMessage(cause, ""); + toast.error(detail ? `${prefix} ${detail}` : prefix); + return; + } + + if (!workspace) { + toast.error(getErrorMessage(cause, "Failed to delete workspace.")); + return; + } + + const path = `/@${workspace.owner_name}/${workspace.name}`; + toast.error( + getErrorMessage(cause, `Failed to delete workspace "${workspace.name}".`), + { + description: + "The chat was not archived. Open the workspace to delete it manually.", + action: { + label: "Open workspace", + onClick: () => onOpenWorkspace(path), + }, + }, + ); +}