From 7bb0bc39a28f144e6b71b34435700ded6f3cd566 Mon Sep 17 00:00:00 2001 From: rabbitblood Date: Sun, 26 Apr 2026 13:48:15 -0700 Subject: [PATCH 1/2] fix(canvas): tombstone deleted ids so in-flight hydrate can't resurrect them (#2069) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2069. removeSubtree dropped a parent + descendants locally after DELETE returned 200, but a GET /workspaces request that was IN-FLIGHT before the DELETE completed could land AFTER and hydrate the store with a stale snapshot — re-introducing the deleted nodes on the canvas until the next 10s fallback poll corrected it. New module canvas/src/store/deleteTombstones.ts holds a transient process-lifetime Map. removeSubtree calls markDeleted(removedIds); hydrate calls wasRecentlyDeleted(id) to filter the incoming workspaces. TTL is 10s — matches the WS-fallback poll cadence so a single round-trip is covered, after which a legitimately re-imported id flows through normally. GC happens lazily at every read AND at write time so the map stays bounded — no separate timer / interval / unmount plumbing. Tests: - canvas/src/store/__tests__/deleteTombstones.test.ts: 7 cases covering immediate flag, never-marked, TTL boundary (9999ms vs 10001ms), GC-on-read, GC-on-write, re-mark resets timestamp, iterable input. - canvas/src/store/__tests__/canvas.test.ts: end-to-end "hydrate cannot resurrect ids that removeSubtree just dropped (#2069)" exercises the full chain at the store level. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/store/__tests__/canvas.test.ts | 31 +++++++ .../store/__tests__/deleteTombstones.test.ts | 82 +++++++++++++++++++ canvas/src/store/canvas.ts | 9 ++ canvas/src/store/deleteTombstones.ts | 70 ++++++++++++++++ 4 files changed, 192 insertions(+) create mode 100644 canvas/src/store/__tests__/deleteTombstones.test.ts create mode 100644 canvas/src/store/deleteTombstones.ts diff --git a/canvas/src/store/__tests__/canvas.test.ts b/canvas/src/store/__tests__/canvas.test.ts index 790833f0..a8ee85ff 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,32 @@ describe("removeSubtree", () => { expect(["root", "mid", "leaf", "sibling"]).not.toContain(e.target); } }); + + // #2069: a `GET /workspaces` that was IN-FLIGHT before the DELETE + // completed can land AFTER removeSubtree, hydrate the store with a + // stale snapshot, and resurrect deleted nodes. The tombstone path + // (deleteTombstones.ts) makes hydrate skip ids deleted within the + // last 10s. Lock the contract end-to-end. + 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..0e331150 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,11 @@ export const useCanvasStore = create((set, get) => ({ } } } + // Mark every removed id with a tombstone so an in-flight GET + // /workspaces response that lands AFTER this can't resurrect them + // via hydrate (#2069). Tombstones expire after the WS-fallback + // poll cadence so legitimate re-imports flow normally. + markDeleted(removed); set({ nodes: nodes.filter((n) => !removed.has(n.id)), edges: edges.filter((e) => !removed.has(e.source) && !removed.has(e.target)), @@ -843,6 +849,9 @@ export const useCanvasStore = create((set, get) => ({ }, hydrate: (workspaces: WorkspaceData[]) => { + // Filter out workspaces whose ids match a fresh tombstone — they + // were just deleted locally and this snapshot is stale (#2069). + workspaces = workspaces.filter((w) => !wasRecentlyDeleted(w.id)); const layoutOverrides = computeAutoLayout(workspaces); // Carry the live measured/grown parent sizes from the existing // store into the rebuild. buildNodesAndEdges runs an auto-rescue diff --git a/canvas/src/store/deleteTombstones.ts b/canvas/src/store/deleteTombstones.ts new file mode 100644 index 00000000..4ae4238e --- /dev/null +++ b/canvas/src/store/deleteTombstones.ts @@ -0,0 +1,70 @@ +/** + * Transient "recently deleted" map used by canvas.ts to short-circuit + * the hydrate-races-cascade-delete window described in #2069. + * + * Problem + * ------- + * `removeSubtree(rootId)` drops a parent + descendants locally after + * `DELETE /workspaces/:id?confirm=true` returns 200. `hydrate()` is + * server-authoritative and rebuilds the entire node array from whatever + * `/workspaces` returns. If a `GET /workspaces` was IN-FLIGHT before the + * DELETE completed, its response (still containing the deleted subtree) + * can land AFTER our local `removeSubtree`, hydrate the store with the + * stale snapshot, and re-introduce the deleted nodes on the canvas. + * + * Fix + * --- + * `removeSubtree` calls `markDeleted(ids)` to record a tombstone for every + * removed id. `hydrate` calls `wasRecentlyDeleted(id)` to filter out any + * incoming workspace whose id matches a fresh tombstone. After + * `TOMBSTONE_TTL_MS`, the entry expires and a legitimately-recreated id + * (template re-import, undo, manual recreate) flows through normally. + * + * GC happens lazily at every read AND at write time so the map stays + * bounded — no separate timer / interval / unmount plumbing. + * + * Module-level (not store state) so it doesn't trigger React Flow + * re-renders and isn't part of the public store surface. The store is a + * singleton, so module identity ≡ store identity for this purpose. + */ + +const TOMBSTONE_TTL_MS = 10_000; // matches the 10s WS-fallback poll cadence + +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; +} From 8c69a98da2d654be0d5d00b81076daa2a0cbee2a Mon Sep 17 00:00:00 2001 From: rabbitblood Date: Sun, 26 Apr 2026 13:52:49 -0700 Subject: [PATCH 2/2] chore(simplify): share FALLBACK_POLL_MS as the tombstone TTL + trim verbose comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify pass on top of #2069 fix: - Export FALLBACK_POLL_MS from canvas/src/store/socket.ts and import it as TOMBSTONE_TTL_MS in deleteTombstones.ts. Single source of truth — tuning one without the other would silently re-open the hydrate-races-delete window. Required-fix per simplify reviewer. - Compress deleteTombstones.ts docstring from 30 lines to 10 — keep the "what + why module-level"; drop the long-form problem description (issue #2069 carries it). - Compress canvas.ts call-site comments at removeSubtree (4 lines → 2) and hydrate (2 lines → 2 but tighter). - Don't reassign the workspaces parameter inside hydrate — use a const `live` and thread it through the two downstream calls (computeAutoLayout, buildNodesAndEdges). Same effect, no lint smell. - Trim the canvas.test.ts integration-test preamble. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/store/__tests__/canvas.test.ts | 6 +--- canvas/src/store/canvas.ts | 16 +++++----- canvas/src/store/deleteTombstones.ts | 37 +++++++---------------- canvas/src/store/socket.ts | 2 +- 4 files changed, 20 insertions(+), 41 deletions(-) diff --git a/canvas/src/store/__tests__/canvas.test.ts b/canvas/src/store/__tests__/canvas.test.ts index a8ee85ff..81f13d81 100644 --- a/canvas/src/store/__tests__/canvas.test.ts +++ b/canvas/src/store/__tests__/canvas.test.ts @@ -552,11 +552,7 @@ describe("removeSubtree", () => { } }); - // #2069: a `GET /workspaces` that was IN-FLIGHT before the DELETE - // completed can land AFTER removeSubtree, hydrate the store with a - // stale snapshot, and resurrect deleted nodes. The tombstone path - // (deleteTombstones.ts) makes hydrate skip ids deleted within the - // last 10s. Lock the contract end-to-end. + // #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()) diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index 0e331150..6f09907d 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -833,10 +833,8 @@ export const useCanvasStore = create((set, get) => ({ } } } - // Mark every removed id with a tombstone so an in-flight GET - // /workspaces response that lands AFTER this can't resurrect them - // via hydrate (#2069). Tombstones expire after the WS-fallback - // poll cadence so legitimate re-imports flow normally. + // 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)), @@ -849,10 +847,10 @@ export const useCanvasStore = create((set, get) => ({ }, hydrate: (workspaces: WorkspaceData[]) => { - // Filter out workspaces whose ids match a fresh tombstone — they - // were just deleted locally and this snapshot is stale (#2069). - workspaces = workspaces.filter((w) => !wasRecentlyDeleted(w.id)); - 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 @@ -873,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 index 4ae4238e..2fb57749 100644 --- a/canvas/src/store/deleteTombstones.ts +++ b/canvas/src/store/deleteTombstones.ts @@ -1,34 +1,19 @@ /** - * Transient "recently deleted" map used by canvas.ts to short-circuit - * the hydrate-races-cascade-delete window described in #2069. + * Transient "recently deleted" map keyed by workspace id. * - * Problem - * ------- - * `removeSubtree(rootId)` drops a parent + descendants locally after - * `DELETE /workspaces/:id?confirm=true` returns 200. `hydrate()` is - * server-authoritative and rebuilds the entire node array from whatever - * `/workspaces` returns. If a `GET /workspaces` was IN-FLIGHT before the - * DELETE completed, its response (still containing the deleted subtree) - * can land AFTER our local `removeSubtree`, hydrate the store with the - * stale snapshot, and re-introduce the deleted nodes on the canvas. + * `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. * - * Fix - * --- - * `removeSubtree` calls `markDeleted(ids)` to record a tombstone for every - * removed id. `hydrate` calls `wasRecentlyDeleted(id)` to filter out any - * incoming workspace whose id matches a fresh tombstone. After - * `TOMBSTONE_TTL_MS`, the entry expires and a legitimately-recreated id - * (template re-import, undo, manual recreate) flows through normally. - * - * GC happens lazily at every read AND at write time so the map stays - * bounded — no separate timer / interval / unmount plumbing. - * - * Module-level (not store state) so it doesn't trigger React Flow - * re-renders and isn't part of the public store surface. The store is a - * singleton, so module identity ≡ store identity for this purpose. + * 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) */ -const TOMBSTONE_TTL_MS = 10_000; // matches the 10s WS-fallback poll cadence +import { FALLBACK_POLL_MS } from "./socket"; + +const TOMBSTONE_TTL_MS = FALLBACK_POLL_MS; const tombstones = new Map(); 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;