fix(canvas): client races surfaced by review — star order, eviction, build status, home seed (#3931)

Author: k11kirky

packages/core/src/canvas/canvasBuildSchemas.test.ts

@@ -2,6 +2,7 @@ import { describe, expect, it, vi } from "vitest";
 import {
   type CanvasBuildLifecycle,
   type CanvasBuildRecord,
+  currentHeadBuildFailure,
   hasActiveCanvasBuild,
   latestFinishedCanvasBuild,
   publishedCanvasBuild,
@@ -73,6 +74,32 @@ describe("canvas build lifecycle", () => {
     expect(publishedCanvasBuild(value)).toBe(ready);
   });
 
+  it("surfaces a failed build of the current head even when an older published build is also ready", () => {
+    // b_old (sv-old) is the pinned/published live build; a newer publish (b_new,
+    // sv-new = current head) failed. latestFinishedCanvasBuild returns the first
+    // finished build in array order, which here is the ready b_old — position,
+    // not version identity — so it would hide the failure. currentHeadBuildFailure
+    // keys off the current version's own build.
+    const value = lifecycle([
+      build("b_old", "ready"),
+      build("b_new", "failed"),
+    ]);
+    value.builds[0].sourceVersionId = "sv-old";
+    value.builds[1].sourceVersionId = "sv-new";
+    value.publishedBuildId = "b_old";
+    value.currentVersionId = "sv-new";
+
+    expect(latestFinishedCanvasBuild(value)?.id).toBe("b_old");
+    expect(currentHeadBuildFailure(value)?.id).toBe("b_new");
+  });
+
+  it("returns null when the current head built fine or is still in flight", () => {
+    const value = lifecycle([build("b1", "building")]);
+    value.currentVersionId = "sv-1";
+
+    expect(currentHeadBuildFailure(value)).toBeNull();
+  });
+
   it("maps the builds endpoint's snake_case body to the client shape", async () => {
     const fetchMock = vi.fn(
       async () =>

packages/core/src/canvas/canvasBuildSchemas.ts

@@ -71,3 +71,23 @@ export function latestFinishedCanvasBuild(
     ) ?? null
   );
 }
+
+/**
+ * The failed build of the canvas's CURRENT head, if it failed. This is the
+ * build whose outcome the author actually cares about — a failed newest
+ * publish must surface even when an older (e.g. pinned/published) build is the
+ * first finished row in the list, because `latestFinishedCanvasBuild` picks by
+ * array position, not version identity.
+ */
+export function currentHeadBuildFailure(
+  lifecycle: CanvasBuildLifecycle,
+): CanvasBuildRecord | null {
+  if (!lifecycle.currentVersionId) return null;
+  return (
+    lifecycle.builds.find(
+      (build) =>
+        build.sourceVersionId === lifecycle.currentVersionId &&
+        build.buildStatus === "failed",
+    ) ?? null
+  );
+}

packages/core/src/canvas/dashboardsService.test.ts

@@ -1,7 +1,7 @@
 import { transform } from "esbuild";
 import { describe, expect, it, vi } from "vitest";
 import { DashboardsService } from "./dashboardsService";
-import type { ProjectApiClient } from "./projectApiClient";
+import { type ProjectApiClient, ProjectApiError } from "./projectApiClient";
 
 // A canvas as the PostHog canvases API returns it.
 function apiCanvas(overrides: Record<string, unknown> = {}) {
@@ -172,6 +172,80 @@ describe("DashboardsService.ensureHomeCanvas", () => {
   });
 });
 
+describe("DashboardsService.ensureHomeCanvas races", () => {
+  it("reuses the winner's canvas when create loses the is_home uniqueness race (409)", async () => {
+    let lookups = 0;
+    const { api } = fakeApi({
+      // First lookup: none. After the 409, the winner's home canvas exists.
+      "canvases/?channel=chan-1&is_home=true": () => {
+        lookups += 1;
+        return lookups === 1
+          ? []
+          : [
+              apiCanvas({
+                id: "home-winner",
+                is_home: true,
+                current_version_id: "v1",
+              }),
+            ];
+      },
+      "canvases/": () => {
+        throw new ProjectApiError("Failed to create canvas (409)", 409);
+      },
+    });
+    const service = new DashboardsService(api);
+
+    const record = await service.ensureHomeCanvas("chan-1");
+
+    expect(record.id).toBe("home-winner");
+  });
+
+  it("rethrows a non-409 create failure instead of masking it as a race", async () => {
+    const { api } = fakeApi({
+      "canvases/?channel=chan-1&is_home=true": [],
+      "canvases/": () => {
+        throw new ProjectApiError("Failed to create canvas (403)", 403);
+      },
+    });
+    const service = new DashboardsService(api);
+
+    await expect(service.ensureHomeCanvas("chan-1")).rejects.toMatchObject({
+      status: 403,
+    });
+  });
+
+  it("retries the seed publish once on a 409 version conflict", async () => {
+    const home = apiCanvas({
+      id: "home-1",
+      is_home: true,
+      current_version_id: null,
+    });
+    let publishCalls = 0;
+    const { api } = fakeApi({
+      "canvases/?channel=chan-1&is_home=true": [home],
+      "canvases/home-1/publish/": (init?: RequestInit) => {
+        publishCalls += 1;
+        if (publishCalls === 1) {
+          throw new ProjectApiError("Failed to seed home canvas (409)", 409);
+        }
+        const body = JSON.parse(String(init?.body));
+        return { current_version_id: body.expected_current_version_id ?? "v1" };
+      },
+      "canvases/home-1/": apiCanvas({
+        id: "home-1",
+        is_home: true,
+        current_version_id: "v-fresh",
+      }),
+    });
+    const service = new DashboardsService(api);
+
+    const record = await service.ensureHomeCanvas("chan-1");
+
+    expect(publishCalls).toBe(2);
+    expect(record.id).toBe("home-1");
+  });
+});
+
 describe("DashboardsService.resetHomeCanvas", () => {
   it("publishes a fresh default guarded on the current head", async () => {
     const home = apiCanvas({

packages/core/src/canvas/dashboardsService.ts

@@ -18,7 +18,11 @@ import type {
   DashboardRecord,
 } from "./dashboardSchemas";
 import { FREEFORM_TEMPLATE_ID } from "./freeformSchemas";
-import { PROJECT_API_CLIENT, type ProjectApiClient } from "./projectApiClient";
+import {
+  apiErrorStatus,
+  PROJECT_API_CLIENT,
+  type ProjectApiClient,
+} from "./projectApiClient";
 
 // Display name (canvas h1) of a channel's auto-created home canvas.
 const HOME_CANVAS_NAME = "Home";
@@ -308,8 +312,11 @@ export class DashboardsService {
           templateId: FREEFORM_TEMPLATE_ID,
           isHome: true,
         });
-      } catch {
-        // Lost the uniqueness race — another client created it; reuse theirs.
+      } catch (error) {
+        // Only the is_home uniqueness race (409) means another client created
+        // it; reuse theirs. Any other failure (auth, capacity, network) must
+        // surface, not be masked as a race.
+        if (apiErrorStatus(error) !== 409) throw error;
         record = await this.findHomeCanvas(channelId);
         if (!record) throw new Error("Failed to create home canvas");
       }
@@ -360,19 +367,30 @@ export class DashboardsService {
         network: { origins: [] },
       },
     };
-    await this.api.json<unknown>(
-      `canvases/${encodeURIComponent(record.id)}/publish/`,
-      "seed home canvas",
-      {
-        method: "POST",
-        headers: { "Content-Type": "application/json" },
-        body: JSON.stringify({
-          project,
-          prompt: "Default home board",
-          expected_current_version_id: record.currentVersionId ?? null,
-        }),
-      },
-    );
+    const publish = (expectedVersionId: string | null) =>
+      this.api.json<unknown>(
+        `canvases/${encodeURIComponent(record.id)}/publish/`,
+        "seed home canvas",
+        {
+          method: "POST",
+          headers: { "Content-Type": "application/json" },
+          body: JSON.stringify({
+            project,
+            prompt: "Default home board",
+            expected_current_version_id: expectedVersionId,
+          }),
+        },
+      );
+    try {
+      await publish(record.currentVersionId ?? null);
+    } catch (error) {
+      // A concurrent seed can win the guarded publish between our read and
+      // POST. On the 409 version conflict, re-read the head and retry once
+      // against the fresh version id rather than failing the channel open.
+      if (apiErrorStatus(error) !== 409) throw error;
+      const conflicted = await this.get(record.id);
+      await publish(conflicted?.currentVersionId ?? null);
+    }
     const fresh = await this.get(record.id);
     return fresh ?? record;
   }

packages/core/src/canvas/projectApiClient.ts

@@ -8,6 +8,22 @@ export const PROJECT_API_CLIENT = Symbol.for(
 
 const MAX_PAGES = 50;
 
+/** An API call that failed with an HTTP status (so callers can branch on it). */
+export class ProjectApiError extends Error {
+  constructor(
+    message: string,
+    public readonly status: number,
+  ) {
+    super(message);
+    this.name = "ProjectApiError";
+  }
+}
+
+/** The status code of a ProjectApiError, or null for a non-API error. */
+export function apiErrorStatus(error: unknown): number | null {
+  return error instanceof ProjectApiError ? error.status : null;
+}
+
 /**
  * Thin shared client for the current PostHog project's REST API. Resolves the
  * project + auth, then forwards to authenticated fetch. Owners of typed
@@ -36,7 +52,11 @@ export class ProjectApiClient {
     init?: RequestInit,
   ): Promise<T> {
     const res = await this.fetch(path, init);
-    if (!res.ok) throw new Error(`Failed to ${errorLabel} (${res.status})`);
+    if (!res.ok)
+      throw new ProjectApiError(
+        `Failed to ${errorLabel} (${res.status})`,
+        res.status,
+      );
     return (await res.json()) as T;
   }
 

packages/ui/src/features/canvas/freeform/CanvasBuildStatus.tsx

@@ -6,7 +6,10 @@ import {
   WarningCircleIcon,
   XIcon,
 } from "@phosphor-icons/react";
-import { latestFinishedCanvasBuild } from "@posthog/core/canvas/canvasBuildSchemas";
+import {
+  currentHeadBuildFailure,
+  latestFinishedCanvasBuild,
+} from "@posthog/core/canvas/canvasBuildSchemas";
 import { useHostTRPC } from "@posthog/host-router/react";
 import { Button } from "@posthog/quill";
 import type { CanvasDiagnostic } from "@posthog/shared";
@@ -114,7 +117,11 @@ export function CanvasBuildStatus({
     );
   }
 
-  const latest = latestFinishedCanvasBuild(lifecycle);
+  // Surface a failed build of the CURRENT head even when an older (pinned /
+  // published) finished build appears first in the list — array position must
+  // not hide a failed newest publish.
+  const failedHead = currentHeadBuildFailure(lifecycle);
+  const latest = failedHead ?? latestFinishedCanvasBuild(lifecycle);
   if (!latest) return null;
 
   if (latest.buildStatus === "failed") {

packages/ui/src/features/canvas/freeform/FreeformCanvasView.tsx

@@ -9,6 +9,7 @@ import {
   WarningIcon,
 } from "@phosphor-icons/react";
 import {
+  currentHeadBuildFailure,
   hasActiveCanvasBuild,
   latestFinishedCanvasBuild,
   publishedCanvasBuild,
@@ -89,6 +90,10 @@ interface PinnedArtifact {
   url: string;
   /** Epoch ms the pinned URL was minted (the builds fetch that produced it). */
   mintedAt: number;
+  /** The refresh nonce the pin was adopted under, so a remount also re-stamps
+   * the pin's mint time (otherwise the expiry timer would keep firing on a URL
+   * that's already been recovered). */
+  refreshKey: number;
 }
 
 // A freeform (React-in-iframe) canvas. The rendered output is, in priority
@@ -111,6 +116,16 @@ export function FreeformCanvasView({
   const setBrowseVersion = useFreeformChatStore((s) => s.setBrowseVersion);
   const setRuntimeError = useFreeformChatStore((s) => s.setRuntimeError);
 
+  // Protect this thread from LRU eviction while the view is open — a burst of
+  // background patches must never drop the canvas the user is looking at.
+  useEffect(() => {
+    const store = useFreeformChatStore.getState();
+    store.setThreadMounted(threadId, true);
+    return () => {
+      useFreeformChatStore.getState().setThreadMounted(threadId, false);
+    };
+  }, [threadId]);
+
   // Right-hand panel state (persisted minimize + width). `startedTaskId` is a
   // local bridge so the composer floats to the side immediately on submit,
   // before the canvas record's polled generationTaskId catches up.
@@ -237,25 +252,28 @@ export function FreeformCanvasView({
   // Pin the artifact to one signed URL per build: every lifecycle refetch mints
   // a fresh URL for the same artifact, and adopting each one would reload the
   // iframe on every 2s poll while a build runs. Adopt only when the published
-  // build itself changes — or when a refresh was explicitly requested because
-  // the pinned URL expired. Adjusted during render (not an effect) so the swap
+  // build itself changes. Adjusted during render (not an effect) so the swap
   // can't flash a stale frame.
   const [pinnedArtifact, setPinnedArtifact] = useState<PinnedArtifact | null>(
     null,
   );
-  const wantFreshArtifactUrlRef = useRef(false);
+  // A nonce that, when bumped, remounts the artifact frame so it revalidates
+  // against the live token endpoint (ETag/304 makes this cheap) — the recovery
+  // path when the pinned URL expired. Remounting, not URL-string compare, is
+  // what guarantees a wedged iframe actually retries: the token endpoint is the
+  // authority, and a new URL for the same bucket would be byte-identical.
+  const [artifactRefreshKey, setArtifactRefreshKey] = useState(0);
   if (publishedBuild?.artifactUrl) {
-    const shouldAdopt =
+    const adoptFresh =
       !pinnedArtifact ||
       pinnedArtifact.buildId !== publishedBuild.id ||
-      (wantFreshArtifactUrlRef.current &&
-        pinnedArtifact.url !== publishedBuild.artifactUrl);
-    if (shouldAdopt) {
-      wantFreshArtifactUrlRef.current = false;
+      pinnedArtifact.refreshKey !== artifactRefreshKey;
+    if (adoptFresh) {
       setPinnedArtifact({
         buildId: publishedBuild.id,
         url: publishedBuild.artifactUrl,
         mintedAt: buildsUpdatedAt || Date.now(),
+        refreshKey: artifactRefreshKey,
       });
     }
   } else if (lifecycle && pinnedArtifact) {
@@ -280,10 +298,15 @@ export function FreeformCanvasView({
       if (Date.now() - renderedArtifact.mintedAt < ARTIFACT_URL_FRESH_MS) {
         return;
       }
-      wantFreshArtifactUrlRef.current = true;
-      void queryClient.invalidateQueries({
-        queryKey: trpc.dashboards.builds.queryKey({ id: dashboardId }),
-      });
+      // Refetch mints the current bucket's URL (re-checking the token server-
+      // side even when the browser would reframe from cache), then remount the
+      // frame so it revalidates against those endpoints. The remount, not a URL
+      // string change, is what un-wedges a frame whose module fetches hung.
+      void queryClient
+        .invalidateQueries({
+          queryKey: trpc.dashboards.builds.queryKey({ id: dashboardId }),
+        })
+        .then(() => setArtifactRefreshKey((k) => k + 1));
     }, ARTIFACT_READY_GRACE_MS);
     return () => clearTimeout(timer);
   }, [renderedArtifact, dashboardId, queryClient, trpc]);
@@ -480,6 +503,7 @@ export function FreeformCanvasView({
     !!lifecycle &&
     lifecycle.builds.length > 0 &&
     (hasActiveCanvasBuild(lifecycle) ||
+      !!currentHeadBuildFailure(lifecycle) ||
       latestFinishedCanvasBuild(lifecycle)?.buildStatus === "failed");
   const showToolbar = interactive || hasBuildSignal;
 
@@ -707,6 +731,7 @@ export function FreeformCanvasView({
           ) : pinnedArtifact ? (
             <Box className="h-full w-full">
               <BuiltCanvas
+                key={`${pinnedArtifact.buildId}:${artifactRefreshKey}`}
                 artifactUrl={pinnedArtifact.url}
                 onDataRequest={onDataRequest}
                 onError={onError}