Repair databases that applied migration 39 in its pre-review shape

apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.test.ts

@@ -313,6 +313,7 @@ projectionSnapshotLayer("ProjectionSnapshotQuery", (it) => {
           snoozedUntil: null,
           snoozedAt: null,
           titleRegeneration: null,
+          titleRegenerationFailure: null,
           deletedAt: null,
           messages: [
             {
@@ -428,6 +429,7 @@ projectionSnapshotLayer("ProjectionSnapshotQuery", (it) => {
           snoozedUntil: null,
           snoozedAt: null,
           titleRegeneration: null,
+          titleRegenerationFailure: null,
           session: {
             threadId: ThreadId.make("thread-1"),
             status: "running",

apps/server/src/persistence/Migrations.ts

@@ -59,6 +59,7 @@ import Migration0036 from "./Migrations/033_ProjectionThreadsSettled.ts";
 import Migration0037 from "./Migrations/034_ProjectionThreadsSnoozed.ts";
 import Migration0038 from "./Migrations/035_ProjectionThreadTitleRegeneration.ts";
 import Migration0039 from "./Migrations/036_ProjectionThreadTitleRegenerationFailure.ts";
+import Migration0040 from "./Migrations/037_RepairProjectionThreadTitleRegenerationFailure.ts";
 
 /**
  * Migration loader with all migrations defined inline.
@@ -110,6 +111,7 @@ export const migrationEntries = [
   [37, "ProjectionThreadsSnoozed", Migration0037],
   [38, "ProjectionThreadTitleRegeneration", Migration0038],
   [39, "ProjectionThreadTitleRegenerationFailure", Migration0039],
+  [40, "RepairProjectionThreadTitleRegenerationFailure", Migration0040],
 ] as const;
 
 export const makeMigrationLoader = (throughId?: number) =>

apps/server/src/persistence/Migrations/036_ProjectionThreadTitleRegenerationFailure.test.ts

@@ -1,47 +1,67 @@
 import { assert, it } from "@effect/vitest";
 import * as Effect from "effect/Effect";
-import * as Layer from "effect/Layer";
 import * as SqlClient from "effect/unstable/sql/SqlClient";
+import { describe } from "vite-plus/test";
 
 import { runMigrations } from "../Migrations.ts";
-import * as NodeSqliteClient from "../NodeSqliteClient.ts";
+import { columnNames, onFreshDatabase } from "./migrationTestSupport.ts";
 
-const layer = it.layer(Layer.mergeAll(NodeSqliteClient.layerMemory()));
+const FAILURE_COLUMNS = [
+  "title_regeneration_failure_request_id",
+  "title_regeneration_failure_at",
+  "title_regeneration_failure_error",
+] as const;
 
-layer("036_ProjectionThreadTitleRegenerationFailure", (it) => {
+const projectionThreadColumns = columnNames("projection_threads");
+
+describe("036_ProjectionThreadTitleRegenerationFailure", () => {
   it.effect("adds the title regeneration failure columns", () =>
-    Effect.gen(function* () {
-      const sql = yield* SqlClient.SqlClient;
-
-      yield* runMigrations({ toMigrationInclusive: 38 });
-      yield* runMigrations({ toMigrationInclusive: 39 });
-
-      const columns = yield* sql<{ readonly name: string }>`
-        PRAGMA table_info(projection_threads)
-      `;
-      const names = new Set(columns.map((column) => column.name));
-      assert.ok(names.has("title_regeneration_failure_request_id"));
-      assert.ok(names.has("title_regeneration_failure_at"));
-      assert.ok(names.has("title_regeneration_failure_error"));
-      // The pending columns stay separate so "in flight" keeps its meaning.
-      assert.ok(names.has("title_regeneration_request_id"));
-      assert.ok(names.has("title_regeneration_started_at"));
-    }),
+    onFreshDatabase(
+      Effect.gen(function* () {
+        yield* runMigrations({ toMigrationInclusive: 38 });
+
+        const before = yield* projectionThreadColumns;
+        for (const column of FAILURE_COLUMNS) {
+          assert.ok(!before.includes(column), `${column} should not exist before 39`);
+        }
+
+        yield* runMigrations({ toMigrationInclusive: 39 });
+
+        const after = yield* projectionThreadColumns;
+        for (const column of FAILURE_COLUMNS) {
+          assert.ok(after.includes(column), `${column} should exist after 39`);
+        }
+        // The pending columns stay separate so "in flight" keeps its meaning.
+        assert.ok(after.includes("title_regeneration_request_id"));
+        assert.ok(after.includes("title_regeneration_started_at"));
+      }),
+    ),
   );
 
-  it.effect("is idempotent when the columns already exist", () =>
-    Effect.gen(function* () {
-      yield* runMigrations({ toMigrationInclusive: 39 });
-      yield* runMigrations({ toMigrationInclusive: 39 });
-
-      const sql = yield* SqlClient.SqlClient;
-      const columns = yield* sql<{ readonly name: string }>`
-        PRAGMA table_info(projection_threads)
-      `;
-      assert.strictEqual(
-        columns.filter((column) => column.name === "title_regeneration_failure_error").length,
-        1,
-      );
-    }),
+  it.effect("skips columns that already exist", () =>
+    onFreshDatabase(
+      Effect.gen(function* () {
+        const sql = yield* SqlClient.SqlClient;
+
+        yield* runMigrations({ toMigrationInclusive: 38 });
+        yield* sql`
+          ALTER TABLE projection_threads
+          ADD COLUMN title_regeneration_failure_at TEXT
+        `;
+
+        // Exercises the PRAGMA guard rather than the migrator's id bookkeeping:
+        // without it this dies on "duplicate column name: …_failure_at".
+        yield* runMigrations({ toMigrationInclusive: 39 });
+
+        const columns = yield* projectionThreadColumns;
+        for (const column of FAILURE_COLUMNS) {
+          assert.strictEqual(
+            columns.filter((name) => name === column).length,
+            1,
+            `${column} should exist exactly once`,
+          );
+        }
+      }),
+    ),
   );
 });

apps/server/src/persistence/Migrations/037_RepairProjectionThreadTitleRegenerationFailure.test.ts

@@ -0,0 +1,101 @@
+import { assert, it } from "@effect/vitest";
+import * as Effect from "effect/Effect";
+import * as SqlClient from "effect/unstable/sql/SqlClient";
+import { describe } from "vite-plus/test";
+
+import { runMigrations } from "../Migrations.ts";
+import { columnNames, onFreshDatabase } from "./migrationTestSupport.ts";
+
+const FAILURE_COLUMNS = [
+  "title_regeneration_failure_request_id",
+  "title_regeneration_failure_at",
+  "title_regeneration_failure_error",
+] as const;
+
+const projectionThreadColumns = columnNames("projection_threads");
+
+describe("037_RepairProjectionThreadTitleRegenerationFailure", () => {
+  it.effect("adds the failure columns to a database that applied the pre-review 39", () =>
+    onFreshDatabase(
+      Effect.gen(function* () {
+        const sql = yield* SqlClient.SqlClient;
+
+        // Reproduce the shape migration 39 had before it was rewritten: one
+        // `title_regeneration_error` column, recorded under the old name. The
+        // migrator skips by id without comparing names, so the rewritten 39 is
+        // treated as already applied and never runs.
+        yield* runMigrations({ toMigrationInclusive: 38 });
+        yield* sql`
+          ALTER TABLE projection_threads
+          ADD COLUMN title_regeneration_error TEXT
+        `;
+        yield* sql`
+          INSERT INTO effect_sql_migrations (migration_id, name)
+          VALUES (39, 'ProjectionThreadTitleRegenerationError')
+        `;
+
+        // The defect itself: 39 is skipped, so the columns are genuinely absent.
+        const beforeRepair = yield* projectionThreadColumns;
+        for (const column of FAILURE_COLUMNS) {
+          assert.ok(
+            !beforeRepair.includes(column),
+            `${column} should be missing before the repair`,
+          );
+        }
+
+        const executed = yield* runMigrations({ toMigrationInclusive: 40 });
+        assert.deepStrictEqual(
+          executed.map(([id]) => id),
+          [40],
+        );
+
+        const afterRepair = yield* projectionThreadColumns;
+        for (const column of FAILURE_COLUMNS) {
+          assert.ok(afterRepair.includes(column), `${column} should exist after the repair`);
+        }
+      }),
+    ),
+  );
+
+  it.effect("no-ops on a fresh database where 39 already added the columns", () =>
+    onFreshDatabase(
+      Effect.gen(function* () {
+        // Without the PRAGMA guard this dies on "duplicate column name": on a
+        // fresh install 39 and 40 run in the same pass, 39 having just added
+        // every column 40 adds.
+        const executed = yield* runMigrations({ toMigrationInclusive: 40 });
+        assert.ok(executed.some(([id]) => id === 40));
+
+        const columns = yield* projectionThreadColumns;
+        for (const column of FAILURE_COLUMNS) {
+          assert.strictEqual(
+            columns.filter((name) => name === column).length,
+            1,
+            `${column} should exist exactly once`,
+          );
+        }
+      }),
+    ),
+  );
+
+  it.effect("applies 39 then 40 in order from a database sitting at 38", () =>
+    onFreshDatabase(
+      Effect.gen(function* () {
+        yield* runMigrations({ toMigrationInclusive: 38 });
+
+        // The mid-sequence case: neither id applied yet, so both must run, in
+        // ascending order — 40 assumes 39 has already had its chance.
+        const executed = yield* runMigrations({ toMigrationInclusive: 40 });
+        assert.deepStrictEqual(
+          executed.map(([id]) => id),
+          [39, 40],
+        );
+
+        const columns = yield* projectionThreadColumns;
+        for (const column of FAILURE_COLUMNS) {
+          assert.ok(columns.includes(column), `${column} should exist`);
+        }
+      }),
+    ),
+  );
+});

apps/server/src/persistence/Migrations/037_RepairProjectionThreadTitleRegenerationFailure.ts

@@ -0,0 +1,53 @@
+/**
+ * Repairs databases that applied migration 39 while it still carried its
+ * pre-review shape.
+ *
+ * Migration 39 originally added a single `title_regeneration_error` column, and
+ * was then rewritten in place to add the three `title_regeneration_failure_*`
+ * columns instead. The migrator records applied migrations by numeric id and
+ * skips anything at or below the latest recorded id without comparing names, so
+ * a database that ran the earlier shape treats the rewritten 39 as done and
+ * never grows the failure columns — every thread and shell snapshot query then
+ * fails on the missing columns.
+ *
+ * Both shapes of 39 reached `main` in the same merge, so no released build ever
+ * carried the earlier one — the affected databases are those on machines that
+ * ran the PR branch mid-review. Narrow, but the failure is a hard one: every
+ * thread and shell snapshot query selects the missing columns.
+ *
+ * Adding them again under a fresh id fixes those databases and no-ops on any
+ * database that got them from 39. The orphaned `title_regeneration_error`
+ * column is left in place: it is nullable and unreferenced, and dropping it
+ * would rewrite the table for no benefit.
+ */
+import * as Effect from "effect/Effect";
+import * as SqlClient from "effect/unstable/sql/SqlClient";
+
+export default Effect.gen(function* () {
+  const sql = yield* SqlClient.SqlClient;
+  const columns = yield* sql<{ readonly name: string }>`
+    PRAGMA table_info(projection_threads)
+  `;
+  const names = new Set(columns.map((column) => column.name));
+
+  if (!names.has("title_regeneration_failure_request_id")) {
+    yield* sql`
+      ALTER TABLE projection_threads
+      ADD COLUMN title_regeneration_failure_request_id TEXT
+    `;
+  }
+
+  if (!names.has("title_regeneration_failure_at")) {
+    yield* sql`
+      ALTER TABLE projection_threads
+      ADD COLUMN title_regeneration_failure_at TEXT
+    `;
+  }
+
+  if (!names.has("title_regeneration_failure_error")) {
+    yield* sql`
+      ALTER TABLE projection_threads
+      ADD COLUMN title_regeneration_failure_error TEXT
+    `;
+  }
+});

apps/server/src/persistence/Migrations/migrationTestSupport.ts

@@ -0,0 +1,36 @@
+/**
+ * Shared helpers for migration tests.
+ *
+ * The `it.layer` idiom used by most tests in this directory hands every case in
+ * a block the same in-memory database. Once one case migrates to the head id,
+ * the migrator skips every later `runMigrations` call — later cases then assert
+ * against a database no migration body touched and pass vacuously. Migration
+ * tests therefore need a database per case, which is what `onFreshDatabase`
+ * provides.
+ *
+ * @module migrationTestSupport
+ */
+import * as Effect from "effect/Effect";
+import * as SqlClient from "effect/unstable/sql/SqlClient";
+
+import * as NodeSqliteClient from "../NodeSqliteClient.ts";
+
+/** Run one test case against its own empty in-memory database. */
+export const onFreshDatabase = <A, E>(effect: Effect.Effect<A, E, SqlClient.SqlClient>) =>
+  Effect.provide(effect, NodeSqliteClient.layerMemory());
+
+/**
+ * Column names of a table, in declaration order.
+ *
+ * An array rather than a `Set` so callers can assert on ordering and counts.
+ * Note that SQLite rejects a duplicate `ADD COLUMN` outright, so a count
+ * assertion here cannot fail on its own — an unguarded migration errors first.
+ */
+export const columnNames = (table: string) =>
+  Effect.gen(function* () {
+    const sql = yield* SqlClient.SqlClient;
+    const columns = yield* sql<{ readonly name: string }>`
+      SELECT name FROM pragma_table_info(${table})
+    `;
+    return columns.map((column) => column.name);
+  });