diff --git a/canvas/src/store/__tests__/canvas.test.ts b/canvas/src/store/__tests__/canvas.test.ts index 790833f0..81f13d81 100644 --- a/canvas/src/store/__tests__/canvas.test.ts +++ b/canvas/src/store/__tests__/canvas.test.ts @@ -6,6 +6,7 @@ global.fetch = vi.fn(() => ); import { useCanvasStore, summarizeWorkspaceCapabilities } from "../canvas"; +import { __resetTombstonesForTest } from "../deleteTombstones"; import type { WorkspaceData, WSMessage } from "../socket"; // Helper to build a WorkspaceData object with sensible defaults @@ -52,6 +53,10 @@ beforeEach(() => { searchOpen: false, viewport: { x: 0, y: 0, zoom: 1 }, }); + // Tombstones leak across tests because the module-level map is + // process-lifetime by design. Reset between tests so a delete in one + // test doesn't shadow a hydrate in the next. + __resetTombstonesForTest(); vi.clearAllMocks(); }); @@ -546,6 +551,28 @@ describe("removeSubtree", () => { expect(["root", "mid", "leaf", "sibling"]).not.toContain(e.target); } }); + + // #2069: lock the tombstone path end-to-end at the store level. + it("hydrate cannot resurrect ids that removeSubtree just dropped (#2069)", () => { + useCanvasStore.getState().removeSubtree("root"); + expect(useCanvasStore.getState().nodes.map((n) => n.id).sort()) + .toEqual(["unrelated"]); + + // Simulate the in-flight GET response landing AFTER the delete: + // the snapshot still contains every original workspace, including + // the just-removed subtree. + useCanvasStore.getState().hydrate([ + makeWS({ id: "root" }), + makeWS({ id: "mid", parent_id: "root" }), + makeWS({ id: "leaf", parent_id: "mid" }), + makeWS({ id: "sibling", parent_id: "root" }), + makeWS({ id: "unrelated" }), + ]); + + // root/mid/leaf/sibling MUST stay deleted; only `unrelated` survives. + const ids = useCanvasStore.getState().nodes.map((n) => n.id).sort(); + expect(ids).toEqual(["unrelated"]); + }); }); // ---------- isDescendant ---------- diff --git a/canvas/src/store/__tests__/deleteTombstones.test.ts b/canvas/src/store/__tests__/deleteTombstones.test.ts new file mode 100644 index 00000000..71d594f9 --- /dev/null +++ b/canvas/src/store/__tests__/deleteTombstones.test.ts @@ -0,0 +1,82 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + __resetTombstonesForTest, + __tombstoneCountForTest, + markDeleted, + wasRecentlyDeleted, +} from "../deleteTombstones"; + +// Tombstone TTL is hardcoded at 10s in the module — these tests freeze +// time so the GC + read-time expiry can be exercised deterministically +// without sleeping. + +describe("deleteTombstones (#2069)", () => { + beforeEach(() => { + __resetTombstonesForTest(); + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-04-26T20:00:00Z")); + }); + + afterEach(() => { + vi.useRealTimers(); + __resetTombstonesForTest(); + }); + + it("flags ids as recently deleted immediately after markDeleted", () => { + markDeleted(["root-1", "child-a"]); + expect(wasRecentlyDeleted("root-1")).toBe(true); + expect(wasRecentlyDeleted("child-a")).toBe(true); + }); + + it("returns false for ids that were never marked", () => { + markDeleted(["root-1"]); + expect(wasRecentlyDeleted("never-deleted")).toBe(false); + }); + + it("expires tombstones after the 10s TTL", () => { + markDeleted(["root-1"]); + expect(wasRecentlyDeleted("root-1")).toBe(true); + vi.advanceTimersByTime(9_999); + expect(wasRecentlyDeleted("root-1")).toBe(true); + vi.advanceTimersByTime(2); + expect(wasRecentlyDeleted("root-1")).toBe(false); + }); + + it("evicts expired entries on read so the map stays bounded", () => { + markDeleted(["root-1"]); + expect(__tombstoneCountForTest()).toBe(1); + vi.advanceTimersByTime(11_000); + // The read itself triggers eviction — no separate GC pass needed. + wasRecentlyDeleted("root-1"); + expect(__tombstoneCountForTest()).toBe(0); + }); + + it("evicts expired entries on write so the map stays bounded across long sessions", () => { + markDeleted(["root-1"]); + expect(__tombstoneCountForTest()).toBe(1); + vi.advanceTimersByTime(11_000); + // markDeleted GCs before inserting, so the second write should + // evict root-1 (now stale) AND insert root-2 — net size 1, not 2. + markDeleted(["root-2"]); + expect(__tombstoneCountForTest()).toBe(1); + expect(wasRecentlyDeleted("root-1")).toBe(false); + expect(wasRecentlyDeleted("root-2")).toBe(true); + }); + + it("resets the deletedAt timestamp when the same id is marked again", () => { + markDeleted(["root-1"]); + vi.advanceTimersByTime(8_000); + // Same id re-deleted (rare, but legal) — TTL restarts from now. + markDeleted(["root-1"]); + vi.advanceTimersByTime(8_000); + // 16s after the FIRST mark; would have expired without the re-mark. + expect(wasRecentlyDeleted("root-1")).toBe(true); + }); + + it("accepts any iterable, not just arrays", () => { + const ids = new Set(["root-1", "root-2"]); + markDeleted(ids); + expect(wasRecentlyDeleted("root-1")).toBe(true); + expect(wasRecentlyDeleted("root-2")).toBe(true); + }); +}); diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index cbe2f9db..6f09907d 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -9,6 +9,7 @@ import { api } from "@/lib/api"; import { showToast } from "@/components/Toaster"; import type { WorkspaceData, WSMessage } from "./socket"; import { handleCanvasEvent } from "./canvas-events"; +import { markDeleted, wasRecentlyDeleted } from "./deleteTombstones"; import { buildNodesAndEdges, computeAutoLayout, @@ -832,6 +833,9 @@ export const useCanvasStore = create((set, get) => ({ } } } + // Tombstone removed ids so an in-flight GET /workspaces can't + // resurrect them via hydrate (#2069). + markDeleted(removed); set({ nodes: nodes.filter((n) => !removed.has(n.id)), edges: edges.filter((e) => !removed.has(e.source) && !removed.has(e.target)), @@ -843,7 +847,10 @@ export const useCanvasStore = create((set, get) => ({ }, hydrate: (workspaces: WorkspaceData[]) => { - const layoutOverrides = computeAutoLayout(workspaces); + // Drop ids tombstoned by a recent removeSubtree (#2069 — stale + // in-flight GET /workspaces). + const live = workspaces.filter((w) => !wasRecentlyDeleted(w.id)); + const layoutOverrides = computeAutoLayout(live); // Carry the live measured/grown parent sizes from the existing // store into the rebuild. buildNodesAndEdges runs an auto-rescue // pass on each child to detach orphans whose stored relative @@ -864,7 +871,7 @@ export const useCanvasStore = create((set, get) => ({ } } const { nodes, edges } = buildNodesAndEdges( - workspaces, + live, layoutOverrides, currentParentSizes, ); diff --git a/canvas/src/store/deleteTombstones.ts b/canvas/src/store/deleteTombstones.ts new file mode 100644 index 00000000..2fb57749 --- /dev/null +++ b/canvas/src/store/deleteTombstones.ts @@ -0,0 +1,55 @@ +/** + * Transient "recently deleted" map keyed by workspace id. + * + * `removeSubtree` calls `markDeleted(ids)` on every removal; `hydrate` + * calls `wasRecentlyDeleted(id)` to filter out incoming workspaces + * whose ids match a fresh tombstone — prevents an in-flight + * GET /workspaces from resurrecting just-deleted nodes via hydrate. + * + * TTL is shared with the WS-fallback poll cadence so a single + * round-trip is covered. Module-level (not store state) so it doesn't + * trigger React Flow re-renders. (#2069) + */ + +import { FALLBACK_POLL_MS } from "./socket"; + +const TOMBSTONE_TTL_MS = FALLBACK_POLL_MS; + +const tombstones = new Map(); + +function gcExpired(now: number): void { + for (const [id, deletedAt] of tombstones) { + if (now - deletedAt >= TOMBSTONE_TTL_MS) { + tombstones.delete(id); + } + } +} + +export function markDeleted(ids: Iterable): void { + const now = Date.now(); + gcExpired(now); + for (const id of ids) { + tombstones.set(id, now); + } +} + +export function wasRecentlyDeleted(id: string): boolean { + const deletedAt = tombstones.get(id); + if (deletedAt === undefined) return false; + if (Date.now() - deletedAt >= TOMBSTONE_TTL_MS) { + tombstones.delete(id); + return false; + } + return true; +} + +/** Test-only: clear the module-level map between tests. Production code + * must not call this — the map is intentionally process-lifetime. */ +export function __resetTombstonesForTest(): void { + tombstones.clear(); +} + +/** Test-only: inspect the current tombstone count. */ +export function __tombstoneCountForTest(): number { + return tombstones.size; +} diff --git a/canvas/src/store/socket.ts b/canvas/src/store/socket.ts index fca3b8a6..dbc54871 100644 --- a/canvas/src/store/socket.ts +++ b/canvas/src/store/socket.ts @@ -63,7 +63,7 @@ export class RehydrateDedup { * network truly is down. The dedup gate inside rehydrate() collapses * this against the post-onopen rehydrate, so reconnect doesn't pay * for a duplicate fetch. */ -const FALLBACK_POLL_MS = 10_000; +export const FALLBACK_POLL_MS = 10_000; class ReconnectingSocket { private ws: WebSocket | null = null;