From 838705f9dfb593313d3b5401fc95af50c3b51534 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Fri, 6 Nov 2026 06:13:53 +0100 Subject: [PATCH] fix(server,app): close archive and undo races found in review - An archive request now claims the workspace's archive queue before looking up the workspace, so an undo sent right behind it cancels it instead of running first and losing to the archive. - A repeat archive of an already archived workspace no longer wipes the agents the earlier archive saved for undo. - Undo no longer brings back an agent that was archived before the workspace archive and only loaded to read its history. - Each local archive or undo request owns its own intent token, so a late response to an earlier request cannot release a newer request's hold on the row. An archive canceled by another client's undo puts the row back. - The undo failure toast shows translated copy; the cause goes to the log. --- docs/agent-lifecycle.md | 3 + .../browser/workspace-archive-undo.spec.ts | 115 ++++++++---------- .../support/helpers/workspace-archive-undo.ts | 75 ++++++++++++ .../app/src/components/toast-host.test.tsx | 21 ---- .../session-workspace-upserts.test.ts | 19 +-- .../src/contexts/session-workspace-upserts.ts | 75 ++++++++---- .../directory-sync/workspace-replica.test.ts | 8 +- .../archive-undo/internal/undo-window.test.ts | 13 +- .../archive-undo/internal/undo-window.tsx | 7 +- .../src/workspace/workspace-archive.test.ts | 55 ++++++++- .../app/src/workspace/workspace-archive.ts | 45 ++++--- packages/server/src/server/session.ts | 20 +-- .../workspace-archive-operations.test.ts | 18 +++ .../server/workspace-archive-operations.ts | 11 +- .../server/workspace-archive-service.test.ts | 28 +++++ .../src/server/workspace-archive-service.ts | 6 +- .../server/workspace-archive-undo.e2e.test.ts | 30 +++++ 17 files changed, 383 insertions(+), 166 deletions(-) create mode 100644 packages/app/e2e/support/helpers/workspace-archive-undo.ts diff --git a/docs/agent-lifecycle.md b/docs/agent-lifecycle.md index c0ac0c5586..c0aad974f6 100644 --- a/docs/agent-lifecycle.md +++ b/docs/agent-lifecycle.md @@ -270,6 +270,9 @@ The daemon runs one archive lane per workspace, shared by every connection | an undo runs: archive | waits for the undo, then archives | | an undo runs: undo | joins the running undo | +A request takes its place in the lane before its first `await`. Arrival order decides, so an undo +sent right behind its archive still cancels it. + An aborted archive stops at its next checkpoint and reports `canceled`; the undo unarchives only the agents that archive itself archived. Deleting the worktree directory is the point of no return: an undo after it recreates the worktree through workspace recovery. The client shows or diff --git a/packages/app/e2e/browser/workspace-archive-undo.spec.ts b/packages/app/e2e/browser/workspace-archive-undo.spec.ts index 46c81e504a..4806e42b7c 100644 --- a/packages/app/e2e/browser/workspace-archive-undo.spec.ts +++ b/packages/app/e2e/browser/workspace-archive-undo.spec.ts @@ -1,4 +1,3 @@ -import { existsSync } from "node:fs"; import type { Page } from "@playwright/test"; import { expect, test } from "../support/fixtures"; import { gotoAppShell } from "../support/helpers/app"; @@ -11,29 +10,23 @@ import { import { getServerId } from "../support/helpers/server-id"; import { expectWorkspaceAbsentFromSidebar } from "../support/helpers/sidebar"; import { createTempGitRepo } from "../support/helpers/workspace"; +import { + archiveFromRowButton, + expectDirectoryPresence, + expectHostWorkspaceSettledActive, + outlastUndoWindow, + pressUndoShortcut, + rowArchiveButton, + rowKebab, + undoButton, + workspaceRow, + workspaceTitleWidth, +} from "../support/helpers/workspace-archive-undo"; import { waitForSidebarHydration, waitForWorkspaceInSidebar, } from "../support/helpers/workspace-ui"; -const UNDO_SHORTCUT = process.platform === "darwin" ? "Meta+Z" : "Control+Z"; - -function workspaceRow(page: Page, workspaceId: string) { - return page.getByTestId(`sidebar-workspace-row-${getServerId()}:${workspaceId}`); -} - -function archiveButton(page: Page, workspaceId: string) { - return page.getByTestId(`sidebar-workspace-archive-button-${getServerId()}:${workspaceId}`); -} - -async function titleWidth(page: Page, workspaceId: string): Promise { - const box = await page - .getByTestId(`sidebar-workspace-title-${getServerId()}:${workspaceId}`) - .boundingBox(); - if (!box) throw new Error(`Title of ${workspaceId} is not laid out`); - return box.width; -} - test.describe("Workspace archive undo", () => { let client: Awaited>; let tempRepo: { path: string; cleanup: () => Promise }; @@ -55,11 +48,6 @@ test.describe("Workspace archive undo", () => { await tempRepo?.cleanup().catch(() => undefined); }); - async function activeWorkspaceIds(): Promise { - const { entries } = await client.fetchWorkspaces(); - return entries.map((entry) => entry.id); - } - async function seedWorktree(page: Page, slug: string) { await openProjectViaDaemon(client, tempRepo.path); const worktree = await createWorktreeViaDaemon(client, { @@ -78,23 +66,19 @@ test.describe("Workspace archive undo", () => { test("the hover archive button overlays the title instead of narrowing it", async ({ page }) => { const worktree = await seedWorktree(page, "archive-button-overlay"); - const row = workspaceRow(page, worktree.workspaceId); await page.mouse.move(0, 0); - await expect(archiveButton(page, worktree.workspaceId)).toHaveCount(0); - const restingWidth = await titleWidth(page, worktree.workspaceId); + await expect(rowArchiveButton(page, worktree.workspaceId)).toHaveCount(0); + const restingWidth = await workspaceTitleWidth(page, worktree.workspaceId); - await row.hover(); - await expect(archiveButton(page, worktree.workspaceId)).toBeVisible(); + await workspaceRow(page, worktree.workspaceId).hover(); + await expect(rowArchiveButton(page, worktree.workspaceId)).toBeVisible(); await expect(page.getByTestId("sidebar-workspace-trailing-scrim")).toBeVisible(); - expect(await titleWidth(page, worktree.workspaceId)).toBe(restingWidth); + expect(await workspaceTitleWidth(page, worktree.workspaceId)).toBe(restingWidth); - const kebab = page.getByTestId( - `sidebar-workspace-kebab-${getServerId()}:${worktree.workspaceId}`, - ); const [buttonBox, kebabBox] = await Promise.all([ - archiveButton(page, worktree.workspaceId).boundingBox(), - kebab.boundingBox(), + rowArchiveButton(page, worktree.workspaceId).boundingBox(), + rowKebab(page, worktree.workspaceId).boundingBox(), ]); expect(buttonBox && kebabBox && buttonBox.x + buttonBox.width <= kebabBox.x).toBe(true); }); @@ -102,50 +86,57 @@ test.describe("Workspace archive undo", () => { test("Cmd/Ctrl+Z right after archiving keeps the worktree", async ({ page }) => { const worktree = await seedWorktree(page, "archive-undo-shortcut"); - page.once("dialog", (dialog) => void dialog.accept()); - await workspaceRow(page, worktree.workspaceId).hover(); - await archiveButton(page, worktree.workspaceId).click(); - await expectWorkspaceAbsentFromSidebar(page, worktree.workspaceId); - await expect(page.getByTestId("workspace-archived-toast-undo")).toBeVisible(); - - await page.keyboard.press(UNDO_SHORTCUT); + await archiveFromRowButton(page, worktree.workspaceId); + await pressUndoShortcut(page); await expect(workspaceRow(page, worktree.workspaceId)).toBeVisible(); - await expect(page.getByTestId("workspace-archived-toast-undo")).toHaveCount(0); - await expect.poll(activeWorkspaceIds).toContain(worktree.workspaceId); - expect(existsSync(worktree.workspaceDirectory)).toBe(true); - // A late remove from the canceled archive must not take the row back out. - await page.waitForTimeout(1500); + await expect(undoButton(page)).toHaveCount(0); + await expectHostWorkspaceSettledActive(client, worktree.workspaceId); + // Once the host settled, any remove the canceled archive sent has already arrived. await expect(workspaceRow(page, worktree.workspaceId)).toBeVisible(); + await expectDirectoryPresence(worktree.workspaceDirectory, true); }); test("Undo after the worktree was removed recreates it", async ({ page }) => { const worktree = await seedWorktree(page, "archive-undo-recreate"); - page.once("dialog", (dialog) => void dialog.accept()); - await workspaceRow(page, worktree.workspaceId).hover(); - await archiveButton(page, worktree.workspaceId).click(); - const undo = page.getByTestId("workspace-archived-toast-undo"); + await archiveFromRowButton(page, worktree.workspaceId); // Hovering the toast holds it, and with it the undo, open while the archive finishes. - await undo.hover(); - await expect - .poll(() => existsSync(worktree.workspaceDirectory), { timeout: 30_000 }) - .toBe(false); - - await undo.click(); + await undoButton(page).hover(); + await expectDirectoryPresence(worktree.workspaceDirectory, false); + await undoButton(page).click(); await expect(workspaceRow(page, worktree.workspaceId)).toBeVisible({ timeout: 30_000 }); - await expect - .poll(() => existsSync(worktree.workspaceDirectory), { timeout: 30_000 }) - .toBe(true); + await expectDirectoryPresence(worktree.workspaceDirectory, true); + }); + + test("hovering Undo keeps the toast open past its deadline, and its close ends the undo", async ({ + page, + }) => { + await page.clock.install({ time: Date.now() }); + const worktree = await seedWorktree(page, "archive-undo-hold"); + + await archiveFromRowButton(page, worktree.workspaceId); + await page.getByTestId("workspace-archived-toast").hover({ position: { x: 8, y: 8 } }); + await undoButton(page).hover(); + await outlastUndoWindow(page); + await expect(undoButton(page)).toBeVisible(); + + await page.mouse.move(0, 0); + await outlastUndoWindow(page); + await expect(undoButton(page)).toHaveCount(0); + + // The toast took the undo with it. + await pressUndoShortcut(page); + await expectWorkspaceAbsentFromSidebar(page, worktree.workspaceId); }); test("Cmd/Ctrl+Z without an archive to undo leaves the sidebar alone", async ({ page }) => { const worktree = await seedWorktree(page, "archive-undo-none"); - await page.keyboard.press(UNDO_SHORTCUT); + await pressUndoShortcut(page); - await expect(page.getByTestId("workspace-archived-toast-undo")).toHaveCount(0); + await expect(undoButton(page)).toHaveCount(0); await expect(workspaceRow(page, worktree.workspaceId)).toBeVisible(); }); }); diff --git a/packages/app/e2e/support/helpers/workspace-archive-undo.ts b/packages/app/e2e/support/helpers/workspace-archive-undo.ts new file mode 100644 index 0000000000..febe46d435 --- /dev/null +++ b/packages/app/e2e/support/helpers/workspace-archive-undo.ts @@ -0,0 +1,75 @@ +import { existsSync } from "node:fs"; +import type { Page } from "@playwright/test"; +import { expect } from "../fixtures"; +import type { connectNewWorkspaceDaemonClient } from "./new-workspace"; +import { getServerId } from "./server-id"; +import { expectWorkspaceAbsentFromSidebar } from "./sidebar"; + +type DaemonClient = Awaited>; + +const UNDO_SHORTCUT = process.platform === "darwin" ? "Meta+Z" : "Control+Z"; +/** The archived toast's lifetime, from `WORKSPACE_ARCHIVE_UNDO_WINDOW_MS`. */ +const UNDO_WINDOW_MS = 6000; + +export function workspaceRow(page: Page, workspaceId: string) { + return page.getByTestId(`sidebar-workspace-row-${getServerId()}:${workspaceId}`); +} + +export function rowArchiveButton(page: Page, workspaceId: string) { + return page.getByTestId(`sidebar-workspace-archive-button-${getServerId()}:${workspaceId}`); +} + +export function rowKebab(page: Page, workspaceId: string) { + return page.getByTestId(`sidebar-workspace-kebab-${getServerId()}:${workspaceId}`); +} + +export function undoButton(page: Page) { + return page.getByRole("button", { name: /^Undo/ }); +} + +export async function workspaceTitleWidth(page: Page, workspaceId: string): Promise { + const box = await page + .getByTestId(`sidebar-workspace-title-${getServerId()}:${workspaceId}`) + .boundingBox(); + if (!box) throw new Error(`Title of ${workspaceId} is not laid out`); + return box.width; +} + +/** Archives a workspace with its row's hover button and waits for the undo toast. */ +export async function archiveFromRowButton(page: Page, workspaceId: string): Promise { + page.once("dialog", (dialog) => void dialog.accept()); + await workspaceRow(page, workspaceId).hover(); + await rowArchiveButton(page, workspaceId).click(); + await expectWorkspaceAbsentFromSidebar(page, workspaceId); + await expect(undoButton(page)).toBeVisible(); +} + +export async function pressUndoShortcut(page: Page): Promise { + await page.keyboard.press(UNDO_SHORTCUT); +} + +/** Moves the page clock past the moment an unheld archived toast would have closed. */ +export async function outlastUndoWindow(page: Page): Promise { + await page.clock.fastForward(UNDO_WINDOW_MS + 1000); +} + +/** Waits until the host lists the workspace as active with no archive running on it. */ +export async function expectHostWorkspaceSettledActive( + client: DaemonClient, + workspaceId: string, +): Promise { + await expect + .poll( + async () => { + const { entries } = await client.fetchWorkspaces(); + const entry = entries.find((candidate) => candidate.id === workspaceId); + return entry ? { active: true, archiving: Boolean(entry.archivingAt) } : null; + }, + { timeout: 30_000 }, + ) + .toEqual({ active: true, archiving: false }); +} + +export async function expectDirectoryPresence(directory: string, present: boolean): Promise { + await expect.poll(() => existsSync(directory), { timeout: 30_000 }).toBe(present); +} diff --git a/packages/app/src/components/toast-host.test.tsx b/packages/app/src/components/toast-host.test.tsx index ee8683c5ae..32f1bfb8a7 100644 --- a/packages/app/src/components/toast-host.test.tsx +++ b/packages/app/src/components/toast-host.test.tsx @@ -32,25 +32,4 @@ describe("useToastHost", () => { act(() => dismissUploading()); expect(result.current.toast?.content).toBe("Image uploaded"); }); - - it("tells a toast when it leaves the screen, once, however it leaves", () => { - const { result } = renderHook(() => useToastHost()); - const left: string[] = []; - - let dismissFirst = () => {}; - act(() => { - dismissFirst = result.current.api.show("Archived", { - durationMs: null, - onDismiss: () => left.push("replaced"), - }); - }); - act(() => { - result.current.api.show("Copied", { onDismiss: () => left.push("dismissed") }); - }); - act(() => dismissFirst()); - act(() => result.current.dismiss()); - act(() => result.current.dismiss()); - - expect(left).toEqual(["replaced", "dismissed"]); - }); }); diff --git a/packages/app/src/contexts/session-workspace-upserts.test.ts b/packages/app/src/contexts/session-workspace-upserts.test.ts index cc4991b82a..7e942f234b 100644 --- a/packages/app/src/contexts/session-workspace-upserts.test.ts +++ b/packages/app/src/contexts/session-workspace-upserts.test.ts @@ -2,11 +2,11 @@ import { describe, expect, it } from "vitest"; import type { WorkspaceDescriptor } from "@/stores/session-store"; import { clearWorkspaceArchivePending, - clearWorkspaceArchiveUndoPending, isWorkspaceArchivePending, markWorkspaceArchivePending, markWorkspaceArchiveUndoPending, shouldSuppressWorkspaceForLocalArchive, + settleWorkspaceArchiveIntent, shouldSuppressWorkspaceRemovalForLocalUndo, } from "@/contexts/session-workspace-upserts"; @@ -125,7 +125,7 @@ describe("workspace archive undo intent", () => { it("lets host upserts through and holds host removes while an undo is pending", () => { markWorkspaceArchivePending(target); - markWorkspaceArchiveUndoPending(target); + const undo = markWorkspaceArchiveUndoPending(target); expect({ upsertSuppressed: shouldSuppressWorkspaceForLocalArchive({ @@ -135,20 +135,25 @@ describe("workspace archive undo intent", () => { removeSuppressed: shouldSuppressWorkspaceRemovalForLocalUndo(target), }).toEqual({ upsertSuppressed: false, removeSuppressed: true }); - clearWorkspaceArchiveUndoPending(target); + settleWorkspaceArchiveIntent(undo); expect(shouldSuppressWorkspaceRemovalForLocalUndo(target)).toBe(false); }); it("keeps a newer intent when the request behind an older one settles late", () => { - markWorkspaceArchivePending(target); - markWorkspaceArchiveUndoPending(target); + const firstArchive = markWorkspaceArchivePending(target); + const firstUndo = markWorkspaceArchiveUndoPending(target); + settleWorkspaceArchiveIntent(firstArchive); clearWorkspaceArchivePending(target); expect(shouldSuppressWorkspaceRemovalForLocalUndo(target)).toBe(true); markWorkspaceArchivePending(target); - clearWorkspaceArchiveUndoPending(target); + settleWorkspaceArchiveIntent(firstUndo); expect(isWorkspaceArchivePending(target)).toBe(true); - clearWorkspaceArchivePending(target); + const secondUndo = markWorkspaceArchiveUndoPending(target); + settleWorkspaceArchiveIntent(firstUndo); + expect(shouldSuppressWorkspaceRemovalForLocalUndo(target)).toBe(true); + + settleWorkspaceArchiveIntent(secondUndo); }); }); diff --git a/packages/app/src/contexts/session-workspace-upserts.ts b/packages/app/src/contexts/session-workspace-upserts.ts index b5685fe41b..7c03ecf8e2 100644 --- a/packages/app/src/contexts/session-workspace-upserts.ts +++ b/packages/app/src/contexts/session-workspace-upserts.ts @@ -7,12 +7,18 @@ import { normalizeWorkspaceOpaqueId } from "@/utils/workspace-identity"; * - `archive`: the row is hidden; host upserts for it are dropped until its remove arrives. * - `undo`: the row is back; host removes for it are dropped until the undo response arrives. * - * Only the request that set an intent may clear it, so a late response for an earlier - * request never clears the intent of a newer one. + * Each request gets its own intent. A request settles or checks only the intent it set, so a + * late response for an earlier request never touches the intent of a newer one. */ -type LocalWorkspaceArchiveIntent = "archive" | "undo"; +export interface WorkspaceArchiveIntent { + readonly kind: "archive" | "undo"; +} -const intentsByServer = new Map>(); +const intentsByServer = new Map>(); +const targetByIntent = new WeakMap< + WorkspaceArchiveIntent, + { serverId: string; workspaceId: string } +>(); interface WorkspaceIntentTarget { serverId: string; @@ -28,63 +34,82 @@ function resolveTarget( return { serverId, workspaceId }; } -function readIntent(input: WorkspaceIntentTarget): LocalWorkspaceArchiveIntent | null { +function readIntent(input: WorkspaceIntentTarget): WorkspaceArchiveIntent | null { const target = resolveTarget(input); if (!target) return null; return intentsByServer.get(target.serverId)?.get(target.workspaceId) ?? null; } -function setIntent(input: WorkspaceIntentTarget, intent: LocalWorkspaceArchiveIntent): void { +function setIntent( + input: WorkspaceIntentTarget, + kind: WorkspaceArchiveIntent["kind"], +): WorkspaceArchiveIntent { + const intent: WorkspaceArchiveIntent = { kind }; const target = resolveTarget(input); - if (!target) return; + if (!target) return intent; + targetByIntent.set(intent, target); const intents = intentsByServer.get(target.serverId) ?? new Map(); intents.set(target.workspaceId, intent); intentsByServer.set(target.serverId, intents); + return intent; } -function clearIntent(input: WorkspaceIntentTarget, intent: LocalWorkspaceArchiveIntent): void { - const target = resolveTarget(input); - if (!target) return; +function deleteIntent(target: { serverId: string; workspaceId: string }): void { const intents = intentsByServer.get(target.serverId); - if (intents?.get(target.workspaceId) !== intent) return; + if (!intents) return; intents.delete(target.workspaceId); if (intents.size === 0) intentsByServer.delete(target.serverId); } +function clearIntentOfKind( + input: WorkspaceIntentTarget, + kind: WorkspaceArchiveIntent["kind"], +): void { + const target = resolveTarget(input); + if (!target || readIntent(target)?.kind !== kind) return; + deleteIntent(target); +} + +/** True while `intent` is still the latest thing this client asked for its workspace. */ +export function isWorkspaceArchiveIntentCurrent(intent: WorkspaceArchiveIntent): boolean { + const target = targetByIntent.get(intent); + return target !== undefined && readIntent(target) === intent; +} + +/** The request behind `intent` settled. A newer request's intent stays in place. */ +export function settleWorkspaceArchiveIntent(intent: WorkspaceArchiveIntent): void { + const target = targetByIntent.get(intent); + if (target && readIntent(target) === intent) deleteIntent(target); +} + export function markWorkspaceArchivePending(input: { serverId: string; workspaceId: string; -}): void { - setIntent(input, "archive"); +}): WorkspaceArchiveIntent { + return setIntent(input, "archive"); } +/** The host removed the workspace, which settles whichever archive this client has pending. */ export function clearWorkspaceArchivePending(input: { serverId: string; workspaceId: string; }): void { - clearIntent(input, "archive"); + clearIntentOfKind(input, "archive"); } export function isWorkspaceArchivePending(input: WorkspaceIntentTarget): boolean { - return readIntent(input) === "archive"; + return readIntent(input)?.kind === "archive"; } export function markWorkspaceArchiveUndoPending(input: { serverId: string; workspaceId: string; -}): void { - setIntent(input, "undo"); -} - -export function clearWorkspaceArchiveUndoPending(input: { - serverId: string; - workspaceId: string; -}): void { - clearIntent(input, "undo"); +}): WorkspaceArchiveIntent { + return setIntent(input, "undo"); } export function isWorkspaceArchiveUndoPending(input: WorkspaceIntentTarget): boolean { - return readIntent(input) === "undo"; + return readIntent(input)?.kind === "undo"; } export function shouldSuppressWorkspaceForLocalArchive(input: { diff --git a/packages/app/src/runtime/directory-sync/workspace-replica.test.ts b/packages/app/src/runtime/directory-sync/workspace-replica.test.ts index 874773e5c7..52d3692d59 100644 --- a/packages/app/src/runtime/directory-sync/workspace-replica.test.ts +++ b/packages/app/src/runtime/directory-sync/workspace-replica.test.ts @@ -8,9 +8,9 @@ import { } from "@/stores/session-store"; import { clearWorkspaceArchivePending, - clearWorkspaceArchiveUndoPending, markWorkspaceArchivePending, markWorkspaceArchiveUndoPending, + settleWorkspaceArchiveIntent, } from "@/contexts/session-workspace-upserts"; import { useWorkspaceSetupStore } from "@/stores/workspace-setup-store"; import { WorkspaceDirectoryReplica } from "./workspace-replica"; @@ -388,18 +388,18 @@ it("keeps a workspace whose archive is being undone when the host reports its re const replica = new WorkspaceDirectoryReplica(serverId); const restored = normalizeWorkspaceDescriptor(workspace("restored")); + const undo = markWorkspaceArchiveUndoPending({ serverId, workspaceId: restored.id }); try { replica.acceptWorkspaces([restored]); - markWorkspaceArchiveUndoPending({ serverId, workspaceId: restored.id }); expect(replica.applyDelta({ kind: "remove", id: restored.id })).toEqual([]); expect(useSessionStore.getState().sessions[serverId]?.workspaces.has(restored.id)).toBe(true); - clearWorkspaceArchiveUndoPending({ serverId, workspaceId: restored.id }); + settleWorkspaceArchiveIntent(undo); replica.applyDelta({ kind: "remove", id: restored.id }); expect(useSessionStore.getState().sessions[serverId]?.workspaces.has(restored.id)).toBe(false); } finally { - clearWorkspaceArchiveUndoPending({ serverId, workspaceId: restored.id }); + settleWorkspaceArchiveIntent(undo); store.clearSession(serverId); } }); diff --git a/packages/app/src/workspace/archive-undo/internal/undo-window.test.ts b/packages/app/src/workspace/archive-undo/internal/undo-window.test.ts index 1678ff365d..1e36395f2a 100644 --- a/packages/app/src/workspace/archive-undo/internal/undo-window.test.ts +++ b/packages/app/src/workspace/archive-undo/internal/undo-window.test.ts @@ -8,10 +8,9 @@ import { useSessionStore, type WorkspaceDescriptor } from "@/stores/session-stor import { seedSessionHosts } from "@/test/seed-session"; import { archiveWorkspaceWithUndo, - hasUndoableWorkspaceArchive, undoLatestWorkspaceArchive, type UndoableWorkspaceArchiveClient, -} from "./undo-window"; +} from "@/workspace/archive-undo"; const SERVER_ID = "workspace-archive-undo-window"; @@ -159,9 +158,9 @@ describe("archiveWorkspaceWithUndo", () => { expect(committed).toEqual([]); toast.timeOut(); - expect({ committed, undoable: hasUndoableWorkspaceArchive() }).toEqual({ + expect({ committed, undone: undoLatestWorkspaceArchive() }).toEqual({ committed: ["a"], - undoable: false, + undone: false, }); }); @@ -250,7 +249,7 @@ describe("archiveWorkspaceWithUndo", () => { await archive; undoLatestWorkspaceArchive(); host.failUndo("a", new Error("Branch is gone")); - await expect.poll(() => toast.errors).toEqual(["Branch is gone"]); + await expect.poll(() => toast.errors).toEqual(["Couldn't undo the archive"]); expect(storedWorkspace("a")).toBeUndefined(); }); @@ -277,8 +276,8 @@ describe("archiveWorkspaceWithUndo", () => { await expect(archive).rejects.toThrow("registry unavailable"); expect({ toast: toast.shownTestID, - undoable: hasUndoableWorkspaceArchive(), + undone: undoLatestWorkspaceArchive(), row: storedWorkspace("a"), - }).toEqual({ toast: null, undoable: false, row: workspace("a") }); + }).toEqual({ toast: null, undone: false, row: workspace("a") }); }); }); diff --git a/packages/app/src/workspace/archive-undo/internal/undo-window.tsx b/packages/app/src/workspace/archive-undo/internal/undo-window.tsx index dc6cf992d1..65b5d8419e 100644 --- a/packages/app/src/workspace/archive-undo/internal/undo-window.tsx +++ b/packages/app/src/workspace/archive-undo/internal/undo-window.tsx @@ -72,11 +72,8 @@ class UndoWindow { this.release(); this.dismissToast(); void undoWorkspaceArchiveOptimistically(this.input).catch((error: unknown) => { - this.input.toast.error( - error instanceof Error - ? error.message - : i18n.t("sidebar.workspace.toasts.undoArchiveFailed"), - ); + console.warn("[workspace-archive-undo] undo failed", error); + this.input.toast.error(i18n.t("sidebar.workspace.toasts.undoArchiveFailed")); }); return true; }; diff --git a/packages/app/src/workspace/workspace-archive.test.ts b/packages/app/src/workspace/workspace-archive.test.ts index eb4e8bf5de..e519bcffb3 100644 --- a/packages/app/src/workspace/workspace-archive.test.ts +++ b/packages/app/src/workspace/workspace-archive.test.ts @@ -4,7 +4,6 @@ import type { DaemonClient } from "@getpaseo/client/internal/daemon-client"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { clearWorkspaceArchivePending, - clearWorkspaceArchiveUndoPending, isWorkspaceArchivePending, isWorkspaceArchiveUndoPending, } from "@/contexts/session-workspace-upserts"; @@ -105,7 +104,6 @@ afterEach(() => { clearWorkspaceArchivePending({ serverId: SERVER_ID, workspaceId: "workspace-2" }); clearWorkspaceArchivePending({ serverId: SECOND_SERVER_ID, workspaceId: "workspace-1" }); clearWorkspaceArchivePending({ serverId: SECOND_SERVER_ID, workspaceId: "workspace-2" }); - clearWorkspaceArchiveUndoPending({ serverId: SERVER_ID, workspaceId: "workspace-1" }); useSessionStore.setState((state) => ({ ...state, sessions: {} })); }); @@ -314,4 +312,57 @@ describe("undoWorkspaceArchiveOptimistically", () => { expect(storedWorkspace(archived.id)).toBeUndefined(); expect(isWorkspaceArchivePending(target())).toBe(true); }); + it("puts the row back when another client's undo cancels this archive", async () => { + const archived = workspace(); + getHostRuntimeStore().acceptWorkspaceSnapshots(SERVER_ID, [archived]); + const archive = beginOptimisticWorkspaceArchive({ + client: createClient(async () => + archivePayload({ workspaceId: archived.id, canceled: true, error: "undone" }), + ), + workspace: target(), + }); + + await expect(archive.done).resolves.toBe("canceled"); + + expect({ + row: storedWorkspace(archived.id), + archivePending: isWorkspaceArchivePending(target()), + }).toEqual({ row: archived, archivePending: false }); + }); + + it("an earlier undo settling keeps holding the row for a later undo", async () => { + const archived = workspace(); + getHostRuntimeStore().acceptWorkspaceSnapshots(SERVER_ID, [archived]); + const firstUndoResponse = deferred(); + const secondUndoResponse = deferred(); + const first = beginOptimisticWorkspaceArchive({ + client: createClient(async () => archivePayload({ workspaceId: archived.id })), + workspace: target(), + }); + await first.done; + const firstUndo = undoWorkspaceArchiveOptimistically({ + client: { undoWorkspaceArchive: async () => firstUndoResponse.promise }, + workspace: target(), + hidden: first.hidden, + }); + const second = beginOptimisticWorkspaceArchive({ + client: createClient(async () => new Promise(() => {})), + workspace: target(), + }); + const secondUndo = undoWorkspaceArchiveOptimistically({ + client: { undoWorkspaceArchive: async () => secondUndoResponse.promise }, + workspace: target(), + hidden: second.hidden, + }); + + firstUndoResponse.resolve(); + await firstUndo; + + expect({ + row: storedWorkspace(archived.id), + undoPending: isWorkspaceArchiveUndoPending(target()), + }).toEqual({ row: archived, undoPending: true }); + secondUndoResponse.resolve(); + await secondUndo; + }); }); diff --git a/packages/app/src/workspace/workspace-archive.ts b/packages/app/src/workspace/workspace-archive.ts index eeaf928c81..cc2a24db97 100644 --- a/packages/app/src/workspace/workspace-archive.ts +++ b/packages/app/src/workspace/workspace-archive.ts @@ -1,10 +1,10 @@ import { getHostRuntimeStore } from "@/runtime/host-runtime"; import { - clearWorkspaceArchivePending, - clearWorkspaceArchiveUndoPending, - isWorkspaceArchiveUndoPending, + isWorkspaceArchiveIntentCurrent, markWorkspaceArchivePending, markWorkspaceArchiveUndoPending, + settleWorkspaceArchiveIntent, + type WorkspaceArchiveIntent, } from "@/contexts/session-workspace-upserts"; import { useSessionStore, type WorkspaceDescriptor } from "@/stores/session-store"; import { resolveWorkspaceMapKeyByIdentity } from "@/utils/workspace-identity"; @@ -34,6 +34,7 @@ export interface OptimisticWorkspaceArchive { interface OptimisticWorkspaceArchiveSnapshot { workspace: WorkspaceDescriptor | null; + intent: WorkspaceArchiveIntent; } export interface WorkspaceArchiveFailure { @@ -63,23 +64,24 @@ function hideWorkspaceOptimistically( workspaceId: workspace.workspaceId, }); const snapshot = workspaceKey ? (workspaces?.get(workspaceKey) ?? null) : null; - markWorkspaceArchivePending({ + const intent = markWorkspaceArchivePending({ serverId: workspace.serverId, workspaceId: workspace.workspaceId, }); getHostRuntimeStore().removeWorkspaceSnapshot(workspace.serverId, workspace.workspaceId); - return { workspace: snapshot }; + return { workspace: snapshot, intent }; } +/** + * Puts back a row whose archive did not happen. Skipped when a newer request on this client + * (an undo, or another archive) owns the row by now. + */ function restoreOptimisticallyHiddenWorkspace(input: { serverId: string; - workspaceId: string; snapshot: OptimisticWorkspaceArchiveSnapshot; }): void { - clearWorkspaceArchivePending({ - serverId: input.serverId, - workspaceId: input.workspaceId, - }); + if (!isWorkspaceArchiveIntentCurrent(input.snapshot.intent)) return; + settleWorkspaceArchiveIntent(input.snapshot.intent); if (input.snapshot.workspace) { getHostRuntimeStore().acceptWorkspaceSnapshots(input.serverId, [input.snapshot.workspace]); } @@ -99,7 +101,10 @@ async function archiveWorkspaceOrThrow(input: { return "archived"; } -/** Hides the row now and archives it on the host. A failed archive puts the row back. */ +/** + * Hides the row now and archives it on the host. A failed archive puts the row back, and so + * does a canceled one: an undo from another client stopped it, so the workspace stays. + */ export function beginOptimisticWorkspaceArchive(input: { client: WorkspaceArchiveClient; workspace: WorkspaceArchiveTarget; @@ -107,16 +112,16 @@ export function beginOptimisticWorkspaceArchive(input: { const snapshot = hideWorkspaceOptimistically(input.workspace); const done = (async () => { try { - return await archiveWorkspaceOrThrow({ + const outcome = await archiveWorkspaceOrThrow({ client: input.client, workspaceId: input.workspace.workspaceId, }); + if (outcome === "canceled") { + restoreOptimisticallyHiddenWorkspace({ serverId: input.workspace.serverId, snapshot }); + } + return outcome; } catch (error) { - restoreOptimisticallyHiddenWorkspace({ - serverId: input.workspace.serverId, - workspaceId: input.workspace.workspaceId, - snapshot, - }); + restoreOptimisticallyHiddenWorkspace({ serverId: input.workspace.serverId, snapshot }); throw error; } })(); @@ -141,19 +146,19 @@ export async function undoWorkspaceArchiveOptimistically(input: { hidden: WorkspaceDescriptor | null; }): Promise { const { serverId, workspaceId } = input.workspace; - markWorkspaceArchiveUndoPending({ serverId, workspaceId }); + const intent = markWorkspaceArchiveUndoPending({ serverId, workspaceId }); if (input.hidden) { getHostRuntimeStore().acceptWorkspaceSnapshots(serverId, [input.hidden]); } try { await input.client.undoWorkspaceArchive(workspaceId); } catch (error) { - if (isWorkspaceArchiveUndoPending({ serverId, workspaceId })) { + if (isWorkspaceArchiveIntentCurrent(intent)) { getHostRuntimeStore().removeWorkspaceSnapshot(serverId, workspaceId); } throw error; } finally { - clearWorkspaceArchiveUndoPending({ serverId, workspaceId }); + settleWorkspaceArchiveIntent(intent); } } diff --git a/packages/server/src/server/session.ts b/packages/server/src/server/session.ts index 2f600576d0..c652f70908 100644 --- a/packages/server/src/server/session.ts +++ b/packages/server/src/server/session.ts @@ -7807,15 +7807,16 @@ export class Session { request: Extract, ): Promise { try { - const existing = await this.workspaceRegistry.get(request.workspaceId); - if (!existing) { - throw new Error(`Workspace not found: ${request.workspaceId}`); - } - + // Claim the archive queue before any await, so an undo sent right behind this request + // queues after it and cancels it instead of running first. const outcome = await this.workspaceArchiveOperations.archive( - existing.workspaceId, - (signal) => - archiveByScope( + request.workspaceId, + async (signal) => { + const existing = await this.workspaceRegistry.get(request.workspaceId); + if (!existing) { + throw new Error(`Workspace not found: ${request.workspaceId}`); + } + return archiveByScope( { paseoHome: this.paseoHome, paseoWorktreesBaseRoot: this.worktreesRoot, @@ -7844,7 +7845,8 @@ export class Session { requestId: request.requestId, signal, }, - ), + ); + }, ); if (outcome.kind === "canceled") { diff --git a/packages/server/src/server/workspace-archive-operations.test.ts b/packages/server/src/server/workspace-archive-operations.test.ts index 7f2b9f168e..c262f58fa3 100644 --- a/packages/server/src/server/workspace-archive-operations.test.ts +++ b/packages/server/src/server/workspace-archive-operations.test.ts @@ -205,6 +205,24 @@ describe("WorkspaceArchiveOperations", () => { expect(restored).toEqual([["agent-a"]]); }); + test("an archive of an already archived workspace keeps the earlier archive's agents for undo", async () => { + const operations = new WorkspaceArchiveOperations(); + await operations.archive("ws", async () => archived(["agent-a"])); + await operations.archive("ws", async () => ({ + archivedAgentIds: ["agent-a"], + newlyArchivedAgentIds: [], + archivedWorkspaceIds: [], + removedDirectory: false, + })); + const restored: string[][] = []; + + await operations.undo("ws", async ({ agentIds }) => { + restored.push(agentIds); + }); + + expect(restored).toEqual([["agent-a"]]); + }); + test("an undo restores the archive's agents once; a repeat undo restores none", async () => { const operations = new WorkspaceArchiveOperations(); await operations.archive("ws", async () => archived(["agent-a"])); diff --git a/packages/server/src/server/workspace-archive-operations.ts b/packages/server/src/server/workspace-archive-operations.ts index ed1d66b5cb..c8bb15a9d2 100644 --- a/packages/server/src/server/workspace-archive-operations.ts +++ b/packages/server/src/server/workspace-archive-operations.ts @@ -31,7 +31,8 @@ const DEFAULT_UNDO_RETENTION_MS = 10 * 60_000; * Each call claims the lane synchronously, so whichever request arrives last decides the * workspace's final state no matter how the earlier work interleaves. * - * The agents an archive took down are kept in memory for the undo window. After a daemon + * The agents an archive took down are kept in memory for the undo window. An archive that took + * nothing down, such as a repeat archive from a stale client, leaves them alone. After a daemon * restart, undo still restores the workspace but its agents stay archived. */ export class WorkspaceArchiveOperations { @@ -58,11 +59,15 @@ export class WorkspaceArchiveOperations { await settled(current?.done); try { const result = await run(controller.signal); - this.retainUndoable(workspaceId, result.newlyArchivedAgentIds); + if (result.archivedWorkspaceIds.length > 0 || result.newlyArchivedAgentIds.length > 0) { + this.retainUndoable(workspaceId, result.newlyArchivedAgentIds); + } return { kind: "archived", result }; } catch (error) { if (!(error instanceof WorkspaceArchiveCanceledError)) throw error; - this.retainUndoable(workspaceId, error.newlyArchivedAgentIds); + if (error.recordArchived || error.newlyArchivedAgentIds.length > 0) { + this.retainUndoable(workspaceId, error.newlyArchivedAgentIds); + } return { kind: "canceled" }; } })(); diff --git a/packages/server/src/server/workspace-archive-service.test.ts b/packages/server/src/server/workspace-archive-service.test.ts index ac2bfbd565..ecf4210ece 100644 --- a/packages/server/src/server/workspace-archive-service.test.ts +++ b/packages/server/src/server/workspace-archive-service.test.ts @@ -1191,6 +1191,34 @@ describe("archiveByScope cancellation", () => { expect(result.archivedAgentIds.sort()).toEqual(["agent-active", "agent-already-archived"]); expect(result.newlyArchivedAgentIds).toEqual(["agent-active"]); }); + + test("an archived agent loaded to read its history is not reported as newly archived", async () => { + const { tempDir, repoDir } = createGitRepo(); + const paseoHome = path.join(tempDir, ".paseo"); + const worktree = await createPaseoOwnedWorktree(repoDir, paseoHome, "loaded-archived"); + const workspaceId = "ws-loaded-archived"; + const deps = createArchiveDeps({ + paseoHome, + activeWorkspaces: [{ workspaceId, cwd: worktree.worktreePath, kind: "worktree" }], + }); + deps.agentStorage = storedAgents(workspaceId, [ + { id: "agent-active", archivedAt: null }, + { id: "agent-history", archivedAt: "2026-01-01T00:00:00.000Z" }, + ]); + deps.agentManager.listAgents = () => + [ + { id: "agent-active", workspaceId }, + { id: "agent-history", workspaceId }, + ] as ManagedAgent[]; + + const result = await archiveByScope(deps, { + scope: { kind: "workspace", workspaceId }, + requestId: "req-loaded-archived", + signal: new AbortController().signal, + }); + + expect(result.newlyArchivedAgentIds).toEqual(["agent-active"]); + }); }); describe("resolveWorkspaceIdAtPath", () => { diff --git a/packages/server/src/server/workspace-archive-service.ts b/packages/server/src/server/workspace-archive-service.ts index 79d77eee54..54d772add1 100644 --- a/packages/server/src/server/workspace-archive-service.ts +++ b/packages/server/src/server/workspace-archive-service.ts @@ -612,11 +612,15 @@ async function archiveWorkspaceContentsWithProgress( ...matchingStoredRecords.filter((record) => !record.archivedAt).map((record) => record.id), ]), ]; - // Internal agents are retired rather than archived: nothing on disk can bring them back. + // Undo brings back only what this archive took down. Internal agents are retired rather than + // archived, and an agent loaded only to read its history was archived before this request. const restorableAgentIds = new Set(agentIdsToArchive); for (const agent of liveAgents) { if (agent.internal) restorableAgentIds.delete(agent.id); } + for (const record of matchingStoredRecords) { + if (record.archivedAt) restorableAgentIds.delete(record.id); + } const [agentResults, [terminalResult]] = await Promise.all([ Promise.allSettled( agentIdsToArchive.map((agentId) => diff --git a/packages/server/src/server/workspace-archive-undo.e2e.test.ts b/packages/server/src/server/workspace-archive-undo.e2e.test.ts index a9399af5de..31b23bf9df 100644 --- a/packages/server/src/server/workspace-archive-undo.e2e.test.ts +++ b/packages/server/src/server/workspace-archive-undo.e2e.test.ts @@ -190,6 +190,36 @@ test.skipIf(process.platform === "win32")( 60000, ); +test.skipIf(process.platform === "win32")( + "an undo sent right behind its archive leaves the workspace active", + async () => { + const repo = createRepoWithGatedTeardown(); + repo.releaseTeardown(); + const { workspaceId, directory, agentId } = await createWorktreeWorkspaceWithAgent( + repo.repoDir, + "archive-then-undo-same-tick", + ); + + const [archiveResponse] = await Promise.all([ + ctx.client.archiveWorkspace(workspaceId), + ctx.client.undoWorkspaceArchive(workspaceId), + ]); + + expect({ + archiveCanceled: archiveResponse.canceled ?? false, + workspaceActive: await isWorkspaceActive(workspaceId), + directoryExists: existsSync(directory), + agentArchived: await isAgentArchived(agentId), + }).toEqual({ + archiveCanceled: true, + workspaceActive: true, + directoryExists: true, + agentArchived: false, + }); + }, + 60000, +); + test("an undo on a workspace that was never archived leaves it as it was", async () => { const repo = createRepoWithGatedTeardown(); repo.releaseTeardown();