test(cycle): a retry the browser walk can actually pass

frontend/app/cycle/_workspace.ts

@@ -0,0 +1,86 @@
+/**
+ * Empty this suite's workspace, through the routes the product publishes.
+ *
+ * The workspace is built once per *server* start — `scripts/cycle_server.sh`
+ * `rm -rf`s it and runs `visionset init`, and `playwright.cycle.config.ts` starts
+ * that script once as its `webServer`. So every attempt after the first inherits
+ * the one before it, and the walk's third step asserts Home's first-run
+ * invitation, which is gated on the workspace holding no projects at all.
+ * Without this, a retry could never reach whatever failed: it died on that
+ * assertion, three steps in, naming a screen that had nothing to do with the
+ * failure — and the trace and screenshot a person opens are the retry's.
+ *
+ * Deleting through the API rather than the filesystem is what makes it possible
+ * at all. The server owns the workspace for as long as it runs, and `webServer`
+ * is Playwright's to start and stop.
+ *
+ * A project delete takes its dataset, batches, jobs, sources and releases with
+ * it; connections are the only other workspace-level row this walk creates.
+ * Content blobs survive both, deliberately — they are shared and the delete
+ * route never removes them — so a repeated walk re-ingests the same three
+ * fixture images into a new dataset rather than into a new store.
+ *
+ * Paths here are origin-relative on purpose. This suite's `baseURL` ends in
+ * `/app/`, because the bundle is mounted under a prefix while the API owns the
+ * root — and a leading-slash path resolves against that base's *origin*, so
+ * `/projects` reaches the API where `/app/projects` would not.
+ */
+import { expect, type APIRequestContext } from "@playwright/test";
+
+/** The shape both listings answer with. Neither takes a paging parameter. */
+interface Listing {
+  items: { id: string; name: string }[];
+  total: number;
+}
+
+async function listing(
+  request: APIRequestContext,
+  path: string,
+  headers: Record<string, string>,
+): Promise<Listing> {
+  const response = await request.get(path, { headers });
+  expect(response.ok(), `GET ${path} answered ${response.status()}`).toBe(true);
+  const page = (await response.json()) as Listing;
+  // A cast is not a check. A 200 whose body carries no `items` — a route
+  // reshaped under this helper, which is the case running on every attempt
+  // exists to catch — would otherwise throw a bare TypeError on the next line,
+  // before any message could name the route it came from.
+  expect(Array.isArray(page.items), `GET ${path} answered without an items array`).toBe(true);
+  /*
+   * One read is the whole collection, and that is asserted rather than assumed.
+   * A reset that quietly cleared only the first page would put this suite back
+   * exactly where it started — the next attempt failing on a stale screen three
+   * steps later, which is the failure this file exists to remove and is harder
+   * to see the second time.
+   */
+  expect(page.items.length, `GET ${path} returned a partial page`).toBe(page.total);
+  return page;
+}
+
+/**
+ * Delete every project and every inference connection, and assert each answer.
+ *
+ * Loud on purpose: a reset that half-succeeds must fail at the reset, where the
+ * cause is on screen, rather than leaving the walk to fail somewhere that cannot
+ * name it.
+ */
+export async function emptyWorkspace(request: APIRequestContext, bearer: string): Promise<void> {
+  const headers = { Authorization: `Bearer ${bearer}` };
+
+  for (const project of (await listing(request, "/projects", headers)).items) {
+    // The kernel refuses to destroy data without `confirm`, and answers 409.
+    const response = await request.delete(`/projects/${project.id}`, {
+      headers,
+      params: { confirm: true },
+    });
+    expect(response.status(), `DELETE project ${project.name}`).toBe(204);
+  }
+
+  for (const connection of (await listing(request, "/inference/connections", headers)).items) {
+    // No confirmation gate here, unlike a project: nothing holds a key to a
+    // connection, because an annotation copies its model's identity at write
+    // time. What is destroyed is a configuration.
+    const response = await request.delete(`/inference/connections/${connection.id}`, { headers });
+    expect(response.status(), `DELETE connection ${connection.name}`).toBe(204);
+  }
+}

frontend/app/cycle/cycle.spec.ts

@@ -39,6 +39,7 @@ import { readFileSync, readdirSync } from "node:fs";
 import path from "node:path";
 
 import { saveNow } from "../e2e/_frame";
+import { emptyWorkspace } from "./_workspace";
 
 const CYCLE_DIR = process.env["VISIONSET_CYCLE_DIR"] ?? "";
 
@@ -84,37 +85,47 @@ const REPINNED = "built-in stand-in, repinned";
  * A project name nothing else in the workspace will collide with — **including
  * this same spec on another repetition**.
  *
- * A project name is unique per workspace, case-insensitively, and the workspace
- * outlives a repetition: `scripts/cycle_server.sh` rebuilds it once per *server
- * start*, and `--repeat-each` reuses that one server. So the fixed literal that
- * used to live here made the flag useless — repeat 2 died on
- * `POST /projects → 409`, a wall standing in front of everything the suite is
- * about, and the only way to run the cycle twice was two whole invocations at
- * about ninety seconds of rebuild each.
+ * A project name is unique per workspace, case-insensitively, and a fixed
+ * literal here used to make `--repeat-each` useless: the workspace outlived a
+ * repetition, so repeat 2 died on `POST /projects → 409` before reaching
+ * anything the suite is about. The connection name had the same problem one
+ * screen later and nobody had noticed it, which is the argument against curing
+ * a collision one name at a time.
  *
- * `repeatEachIndex` **and `retry`**, because a retry is the same repetition run
- * again into the same workspace. Scoping only the first turns one readable
- * failure into two unreadable ones: a genuine failure leaves its project behind,
- * the retry dies on `POST /projects → 409`, and the report names the 409 — which
- * is the exact wall the scoping exists to remove. The workspace really is fresh
- * per invocation (the script
- * `rm -rf`s it before `init`) and `workers: 1` means two repetitions never
- * overlap, so those two indices are the whole of the uniqueness needed.
+ * Neither needs curing now. Every attempt begins on an empty workspace — see
+ * the `beforeEach` below — so no name in this walk collides with a name from
+ * another attempt, and none of them has to move.
  *
- * The suffix is unconditional rather than omitted on the first, so every run's
- * names have one shape and a failure message reads the same way whether or not
- * somebody passed the flag.
- *
- * The project is the only name that has to move, and that is worth stating so
- * the next collision is looked for rather than assumed: a release tag is unique
- * per dataset, a batch name is not unique at all, and a source's idempotency key
- * `(project, kind, path, fps)` leads with the project. All three are already
- * scoped by a project that is new.
+ * The suffix stays anyway, and is unconditional, because it costs nothing and
+ * it makes a failure message name the attempt it came from. That is worth more
+ * now than it was: a retry can finally produce a second, *different* failure,
+ * and two reports that name the same project are two reports somebody has to
+ * tell apart by hand.
  */
 function projectFor(info: TestInfo): string {
   return `browser-cycle-${info.repeatEachIndex}-${info.retry}`;
 }
 
+/**
+ * Every attempt starts on an empty workspace — a retry included, and every
+ * repetition of `--repeat-each`.
+ *
+ * On the first attempt of a freshly built workspace this deletes nothing and
+ * costs two reads. It exists for the second: the workspace is rebuilt once per
+ * *server* start, not once per attempt, so without this a retry inherits the
+ * previous attempt's projects and dies on Home's first-run invitation — an
+ * assertion about the workspace, three steps in, unrelated to whatever actually
+ * failed.
+ *
+ * Unconditional rather than guarded on `retry`, because a repair that only runs
+ * on the rare attempt has the same property as the defect it repairs: nothing
+ * exercises it until the day it matters. Run every time, its two reads are
+ * proved by every run of this suite.
+ */
+test.beforeEach(async ({ request }) => {
+  await emptyWorkspace(request, token());
+});
+
 test("the whole cycle, from opening the app to a downloaded export", async ({ page }, info) => {
   test.slow();
 
@@ -532,11 +543,31 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa
     // Born not set up, like every local connection, and made ready by the same
     // action — the lifecycle here is the real one, not a shortcut written for a
     // suite. What is different is only that there is nothing to fetch.
-    await expect(page.getByTestId("connection-status")).toContainText(/not set up/i);
-    await page.getByTestId("download-weights").click();
-    await expect(page.getByTestId("connection-status")).toContainText(/ready/i, {
+
+    // Read inside the row rather than off the screen. Both ids live inside a
+    // connection's own row, so an unscoped read is sound only while the
+    // workspace holds exactly one — and Playwright's strict mode then refuses
+    // the locator, naming the selector instead of the assumption.
+    const row = page.getByTestId(`connection-${STAND_IN}`);
+    await expect(row.getByTestId("connection-status")).toContainText(/not set up/i);
+    await row.getByTestId("download-weights").click();
+    await expect(row.getByTestId("connection-status")).toContainText(/ready/i, {
       timeout: 15_000,
     });
+
+    /*
+     * One connection, asserted rather than assumed.
+     *
+     * The suggest panel further down names this connection in a sentence and
+     * offers a picker from two upwards, so a second connection breaks that
+     * assertion as a *missing string* — pointing at the suggest panel while the
+     * cause is an extra row on this screen. Stated here, where the count is
+     * decided, a violation names the count.
+     *
+     * The type badge is what is counted because it is unconditional inside a
+     * row, while the download button appears only before setup.
+     */
+    await expect(page.getByTestId("connection-type")).toHaveCount(1);
   });
 
   await test.step("a click in the editor comes back as a shape, from a real server", async () => {
@@ -1380,7 +1411,10 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa
 
     await page.getByTestId("rail-inference").click();
     await expect(page.getByTestId("inference-screen")).toBeVisible();
-    await expect(page.getByTestId("connection-status")).toContainText(/ready/i);
+    // Two locators because the row's id is its name and the rename moves it.
+    const before = page.getByTestId(`connection-${STAND_IN}`);
+    const after = page.getByTestId(`connection-${REPINNED}`);
+    await expect(before.getByTestId("connection-status")).toContainText(/ready/i);
 
     // A rename, which sends the model reference the row already has. That it is
     // *accepted* is the half that would have caught a PATCH carrying the kind;
@@ -1395,8 +1429,8 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa
     // was typed — so its absence is the first thing that says the server took
     // the body.
     await expect(page.getByTestId("connection-dialog")).toHaveCount(0);
-    await expect(page.getByTestId(`connection-${REPINNED}`)).toBeVisible();
-    await expect(page.getByTestId("connection-status")).toContainText(/ready/i);
+    await expect(after).toBeVisible();
+    await expect(after.getByTestId("connection-status")).toContainText(/ready/i);
 
     // And a reference that really does move. The revision is free text on a
     // connection — the commit-hash rule belongs to catalog entries, which is why
@@ -1408,11 +1442,11 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa
     await page.getByTestId("connection-submit").click();
 
     await expect(page.getByTestId("connection-dialog")).toHaveCount(0);
-    await expect(page.getByTestId("connection-status")).toContainText(/not set up/i);
+    await expect(after.getByTestId("connection-status")).toContainText(/not set up/i);
 
     // The remedy is offered on the row it happened to, which is the other half
     // of what "undoes its setup" is allowed to mean.
-    await expect(page.getByTestId("download-weights")).toBeVisible();
+    await expect(after.getByTestId("download-weights")).toBeVisible();
   });
 
   await test.step("the whole walk produced a clean console", async () => {