Merge pull request #2117 from Molecule-AI/fix/canvas-hydrate-delete-tombstones-2069

fix(canvas): tombstone deleted ids so in-flight hydrate can't resurrect them (#2069)

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 ----------

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);
+  });
+});

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<CanvasState>((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<CanvasState>((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<CanvasState>((set, get) => ({
       }
     }
     const { nodes, edges } = buildNodesAndEdges(
-      workspaces,
+      live,
       layoutOverrides,
       currentParentSizes,
     );

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<string, number>();
+
+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<string>): 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;
+}

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;