test(nextly): fail integration tests that leave a transaction aborted

packages/nextly/src/__tests__/setup.ts

@@ -1,10 +1,34 @@
-import { beforeAll, afterAll } from "vitest";
+import { beforeAll, afterAll, afterEach, expect } from "vitest";
+
+import { describeAbortedTransactions } from "../plugins/aborted-transaction-sightings";
 
 beforeAll(() => {
   // Setup test environment
   console.log("🧪 Setting up nextly tests...");
 });
 
+/**
+ * Fail any test that leaves a PostgreSQL transaction aborted.
+ *
+ * `current transaction is aborted` is always a SECONDARY error: some earlier statement in the
+ * transaction failed and was swallowed, and every statement after it reports this instead. The
+ * pattern that produces it is an existence check written as "run a query and catch the
+ * failure" — valid on SQLite and MySQL, fatal on PostgreSQL, and therefore invisible to a suite
+ * that passes on the first two.
+ *
+ * Asserted centrally rather than per test, because the failure surfaces far from its cause and
+ * no individual test knows to look for it.
+ *
+ * Runs after EVERY test so the failure is attributed to the test that caused it rather than to
+ * whichever one happens to run last. The message comes from the same function published through
+ * `nextly/testing`, so this suite and a plugin author's suite report the identical diagnosis;
+ * only the assertion differs, because each runner reports its own best.
+ */
+afterEach(() => {
+  const aborted = describeAbortedTransactions();
+  if (aborted) expect.fail(aborted);
+});
+
 afterAll(() => {
   // Cleanup
   console.log("✅ nextly tests complete");

packages/nextly/src/plugins/__tests__/aborted-transaction-guard.integration.test.ts

@@ -0,0 +1,155 @@
+/**
+ * The aborted-transaction guard has to be seen failing, or it is just a green check.
+ *
+ * `current transaction is aborted` is PostgreSQL's report that some EARLIER statement in the
+ * transaction failed and was swallowed. The shape that produces it — an existence check written
+ * as "run a query and catch the failure" — is valid on SQLite and MySQL and fatal on PostgreSQL,
+ * so a suite green on two dialects can be quietly broken on the third. The harness records any
+ * sighting and `__tests__/setup.ts` fails the test that caused it.
+ *
+ * This proves the mechanism end to end by inducing exactly that state on purpose: swallow a
+ * query against a missing relation inside a transaction, then issue another statement. The test
+ * consumes its own sighting, which both asserts the guard saw it and stops the shared `afterEach`
+ * from failing this deliberately-broken case.
+ *
+ * PostgreSQL only. SQLite and MySQL do not poison a transaction on a failed statement, which is
+ * the entire reason the guard exists.
+ */
+import { createTestNextly as createThroughPublishedEntry } from "nextly/testing";
+import { afterEach, describe, expect, it } from "vitest";
+
+import { createAdapter } from "../../database/factory";
+import { clearServices } from "../../di/register";
+import { takeAbortedTransactionSightings } from "../aborted-transaction-sightings";
+import {
+  createTestNextly,
+  getConfiguredTestDialects,
+  type TestNextly,
+} from "../test-nextly";
+
+let current: TestNextly | undefined;
+
+afterEach(async () => {
+  await current?.destroy();
+  current = undefined;
+});
+
+const onPostgres = getConfiguredTestDialects().includes("postgresql");
+
+describe.skipIf(!onPostgres)("aborted-transaction guard (integration)", () => {
+  it("records the abort so the shared assertion can fail the test that caused it", async () => {
+    current = await createTestNextly({ dialect: "postgresql" });
+
+    // Swallow a failure inside the transaction exactly as a naive existence probe would,
+    // then carry on. PostgreSQL has already marked the transaction aborted by this point.
+    await expect(
+      current.adapter.transaction(async tx => {
+        try {
+          await tx.execute("SELECT 1 FROM a_relation_that_does_not_exist");
+        } catch {
+          // Swallowed on purpose: this is the pattern being guarded against.
+        }
+        await tx.execute("SELECT 1");
+      })
+    ).rejects.toThrow();
+
+    // Consuming the sighting is both the assertion and the cleanup: the shared `afterEach`
+    // in setup.ts reads the same buffer, so leaving it full would fail this test for doing
+    // precisely what it set out to demonstrate.
+    const sightings = takeAbortedTransactionSightings();
+    expect(sightings.length).toBeGreaterThan(0);
+    expect(sightings[0]).toMatch(/current transaction is aborted/);
+  });
+
+  it("records the abort even when the callback swallows it and returns normally", async () => {
+    current = await createTestNextly({ dialect: "postgresql" });
+
+    // No rejection to observe from out here: the callback catches its own failure and returns,
+    // so PostgreSQL accepts the COMMIT, downgrades it to a rollback, and `transaction()`
+    // resolves over a transaction that kept nothing. This is the shape the bulk write paths
+    // produce when they record a per-item error and move to the next item.
+    await current.adapter.transaction(async tx => {
+      try {
+        await tx.execute("SELECT 1 FROM a_relation_that_does_not_exist");
+      } catch {
+        // Swallowed on purpose: this is the pattern being guarded against.
+      }
+    });
+
+    const sightings = takeAbortedTransactionSightings();
+    expect(sightings.length).toBeGreaterThan(0);
+    expect(sightings[0]).toMatch(/current transaction is aborted/);
+  });
+
+  it("records an abort raised through the published entry point", async () => {
+    // The harness reaches tests two ways: this file imports the source module, while suites that
+    // import `nextly/testing` get the copy bundled into `dist/testing.mjs`. Those are separate
+    // module instances, so a buffer held in module scope would give them one array each — the
+    // bundled harness would record an abort that the shared assertion, reading the source array,
+    // never sees. That failure is silent and it fails open, which is the one outcome a guard
+    // must not have. Holding the buffer on `globalThis` gives both instances the same array.
+    const viaPublished = await createThroughPublishedEntry({
+      dialect: "postgresql",
+    });
+    try {
+      await expect(
+        viaPublished.adapter.transaction(async tx => {
+          try {
+            await tx.execute("SELECT 1 FROM a_relation_that_does_not_exist");
+          } catch {
+            // Swallowed on purpose: this is the pattern being guarded against.
+          }
+          await tx.execute("SELECT 1");
+        })
+      ).rejects.toThrow();
+    } finally {
+      await viaPublished.destroy();
+    }
+
+    // Read through the source module. Seeing the sighting here is the assertion: it can only
+    // have arrived from the bundled harness, so the two instances share one buffer.
+    const sightings = takeAbortedTransactionSightings();
+    expect(sightings.length).toBeGreaterThan(0);
+  });
+
+  it("stays silent when nothing aborts", async () => {
+    current = await createTestNextly({ dialect: "postgresql" });
+
+    await current.adapter.transaction(async tx => {
+      await tx.execute("SELECT 1");
+    });
+
+    expect(takeAbortedTransactionSightings()).toEqual([]);
+  });
+});
+
+describe("instrumenting an adapter more than once", () => {
+  it("installs the guard exactly once across boots", async () => {
+    // Handing the same adapter back to `createTestNextly` is how a test keeps a database alive
+    // across boots. Each boot instruments the adapter, so without a marker the second boot would
+    // wrap the first wrapper: one abort would then report twice, and every transaction would carry
+    // a probe for every boot that ever happened.
+    //
+    // Asserted on the identity of the installed method rather than by counting sightings, because
+    // re-wrapping necessarily replaces it with a new closure. Reboots the adapter the way the
+    // builder suites do — a caller-owned in-memory adapter, and `clearServices` rather than
+    // `destroy`, since the latter disconnects the adapter the second boot needs.
+    process.env.DB_DIALECT = "sqlite";
+    const adapter = await createAdapter({
+      type: "sqlite",
+      memory: true,
+    } as Parameters<typeof createAdapter>[0]);
+
+    const first = await createTestNextly({ adapter });
+    const afterFirstBoot = first.adapter.transaction;
+    expect(first.adapter).toBe(adapter);
+
+    clearServices();
+    const second = await createTestNextly({ adapter });
+    try {
+      expect(second.adapter.transaction).toBe(afterFirstBoot);
+    } finally {
+      await second.destroy();
+    }
+  });
+});

packages/nextly/src/plugins/__tests__/aborted-transaction-sightings.test.ts

@@ -0,0 +1,146 @@
+/**
+ * Detection has to be structural, because the message is not stable.
+ *
+ * PostgreSQL translates its human-readable error text according to the server's `lc_messages`, so
+ * an English substring match fails to fire on a localized server. That failure is silent and it
+ * fails open: nothing is recorded, the shared assertion sees an empty buffer, and the suite
+ * reports green over a transaction that discarded its writes. SQLSTATE `25P02` is fixed by the
+ * wire protocol and carries the same meaning on every server.
+ *
+ * No database needed — these are the shapes the adapter and the driver actually produce, asserted
+ * directly.
+ */
+import { describe, expect, it } from "vitest";
+
+import {
+  PG_ABORTED_TRANSACTION_SQLSTATE,
+  describeAbortedTransactions,
+  isAbortedTransactionError,
+  recordAbortedTransaction,
+  takeAbortedTransactionSightings,
+} from "../aborted-transaction-sightings";
+
+/** The `DatabaseError` shape `createDatabaseError` produces, minus the fields not read here. */
+function classified(fields: {
+  message: string;
+  code?: string;
+  cause?: unknown;
+}): Error & { code?: string } {
+  const error = new Error(fields.message) as Error & {
+    code?: string;
+    cause?: unknown;
+  };
+  if (fields.code !== undefined) error.code = fields.code;
+  if (fields.cause !== undefined) error.cause = fields.cause;
+  return error;
+}
+
+describe("isAbortedTransactionError", () => {
+  it("matches on SQLSTATE when the message is not English", () => {
+    // A German `lc_messages` server reporting 25P02. The English matcher cannot see this, which
+    // is the whole reason the code is consulted first.
+    const error = classified({
+      message:
+        "FEHLER: aktuelle Transaktion wurde abgebrochen, Befehle werden bis zum Ende des Transaktionsblocks ignoriert",
+      code: PG_ABORTED_TRANSACTION_SQLSTATE,
+    });
+
+    expect(isAbortedTransactionError(error)).toBe(true);
+  });
+
+  it("matches on the message when the code was stripped on the way up", () => {
+    const error = classified({
+      message:
+        "current transaction is aborted, commands ignored until end of transaction block",
+    });
+
+    expect(isAbortedTransactionError(error)).toBe(true);
+  });
+
+  it("finds the code on the cause when the wrapper does not carry it", () => {
+    // What the adapter produces: its own classified error wrapping the driver's, where only the
+    // inner one carries the SQLSTATE.
+    const error = classified({
+      message: "Query failed",
+      cause: classified({
+        message: "aktuelle Transaktion wurde abgebrochen",
+        code: PG_ABORTED_TRANSACTION_SQLSTATE,
+      }),
+    });
+
+    expect(isAbortedTransactionError(error)).toBe(true);
+  });
+
+  it("matches a bare string carrying the driver text", () => {
+    expect(
+      isAbortedTransactionError(
+        "current transaction is aborted, commands ignored"
+      )
+    ).toBe(true);
+  });
+
+  it("ignores unrelated database errors", () => {
+    const unique = classified({
+      message:
+        'duplicate key value violates unique constraint "posts_slug_key"',
+      code: "23505",
+    });
+
+    expect(isAbortedTransactionError(unique)).toBe(false);
+    expect(isAbortedTransactionError(new Error("connection terminated"))).toBe(
+      false
+    );
+    expect(isAbortedTransactionError(undefined)).toBe(false);
+    expect(isAbortedTransactionError(null)).toBe(false);
+    expect(isAbortedTransactionError({})).toBe(false);
+  });
+
+  it("terminates on a self-referencing cause chain", () => {
+    // A wrapper that points at itself would spin a naive walk forever, taking the suite with it.
+    const looping = classified({ message: "wrapped" });
+    (looping as { cause?: unknown }).cause = looping;
+
+    expect(isAbortedTransactionError(looping)).toBe(false);
+  });
+});
+
+describe("the sightings buffer", () => {
+  it("hands back what was recorded and clears itself", () => {
+    // Held on globalThis so the source module and the copy bundled into `dist/testing.mjs` share
+    // one array; a per-module array would let a recorded abort go unseen by the assertion.
+    recordAbortedTransaction("first");
+    recordAbortedTransaction("second");
+
+    expect(takeAbortedTransactionSightings()).toEqual(["first", "second"]);
+    // Cleared on read, so one test's failure is never re-reported against the next.
+    expect(takeAbortedTransactionSightings()).toEqual([]);
+  });
+});
+
+describe("describeAbortedTransactions", () => {
+  it("returns null when nothing was recorded", () => {
+    // Returning rather than throwing is what lets the same message serve this package's vitest
+    // setup and a plugin author's runner, without shipping a dependency on either.
+    expect(describeAbortedTransactions()).toBeNull();
+  });
+
+  it("names every sighting and says the message is not the defect", () => {
+    recordAbortedTransaction("current transaction is aborted (one)");
+    recordAbortedTransaction("current transaction is aborted (two)");
+
+    const described = describeAbortedTransactions();
+
+    expect(described).toContain("Seen 2 time(s)");
+    expect(described).toContain("(one)");
+    expect(described).toContain("(two)");
+    // The point of the message: whoever reads it must go looking for the swallowed error.
+    expect(described).toContain("swallowed");
+  });
+
+  it("consumes the buffer so the next test starts clean", () => {
+    recordAbortedTransaction("current transaction is aborted");
+
+    expect(describeAbortedTransactions()).not.toBeNull();
+    expect(describeAbortedTransactions()).toBeNull();
+  });
+});