fix(test): close leaked database pools in server/tests

package.json

@@ -21,6 +21,7 @@
     "test": "bun test",
     "typecheck": "bun run --filter '*' typecheck",
     "test:ci": "bun scripts/test-ci.ts",
+    "test:server:sharded": "bun scripts/test-server-sharded.ts",
     "pretest": "bun run generate:app-config",
     "test:smoke": "OPENBOT_SMOKE=1 bun test tests/smoke",
     "test:live-screen": "OPENBOT_LIVE_SCREEN=1 bun test agent-computer/tests/live-screen.test.ts agent-computer/tests/browser-close-announcement.test.ts agent-computer/tests/reset-run.test.ts",

scripts/test-server-sharded.ts

@@ -0,0 +1,142 @@
+export {};
+
+/**
+ * `server/tests` in shards — useful for wall-clock, and historically the only way a full
+ * sweep stayed under PostgreSQL's connection ceiling.
+ *
+ * Around a hundred of those files open a module-scope connection pool. PostgreSQL's default
+ * `max_connections` is 100. Construction alone opens nothing; the first query on a pool takes
+ * that pool's full `max` (two under TEST_POOL). Pools are never reaped while idle, so exhaustion
+ * tracks how many files have *run*, not how many have been imported. A single
+ * `bun test server/tests` that leaves pools open dies with `sorry, too many clients already`.
+ *
+ * Closing each file's pool in afterAll (see #785) is the right fix so an unsharded run can
+ * finish. Shards remain worth keeping for parallel wall-clock: each group runs in its own
+ * `bun test` invocation, so each process opens its own pools and gives them back when it exits.
+ *
+ * Why this script exists at all, rather than a remembered shell incantation: a sharded sweep is the
+ * only thing that has ever caught a test file that went red and stayed red. A targeted run passes,
+ * `typecheck` passes (`server/tests` is outside `tsc`), a reviewer reads the diff and sees nothing,
+ * and the branch is red the whole time. The sweep belongs at every quality gate, and a check that
+ * has to be retyped from memory at every gate is a check that gets skipped.
+ *
+ * Two deliberate properties:
+ *
+ * - **Every shard's summary line is read, and the exit code comes from `bun test` itself.** Piping
+ *   bun's output through `grep` or `tail` overwrites `$?` with the pipe's status, which is how a
+ *   failing shard reports success. Each shard's stderr is captured and echoed VERBATIM so a failure
+ *   message survives in full for diagnosis.
+ * - **A shard whose summary cannot be parsed fails the run.** A pass that cannot be counted is not
+ *   a pass; an import-time throw takes its file's tests with it and reports no failure.
+ *
+ * Usage: `bun run test:server:sharded`, with `TEST_DATABASE_URL` set for the integration suites.
+ * `SHARD_SIZE` overrides the files-per-shard count.
+ */
+
+const SHARD_SIZE = Number.parseInt(process.env.SHARD_SIZE ?? "55", 10);
+
+async function writeStderr(text: string) {
+  if (!text) return;
+  if (process.stderr.write(text)) return;
+  await new Promise<void>((resolve) => process.stderr.once("drain", resolve));
+}
+
+if (!process.env.TEST_DATABASE_URL?.trim()) {
+  await writeStderr(
+    "TEST_DATABASE_URL is empty or unset. The integration suites would each fail to connect, which\n" +
+      "looks like a hundred broken tests rather than one missing variable. Run this as\n" +
+      "TEST_DATABASE_URL=postgres://openbot:openbot@localhost:5432/openbot_test bun run test:server:sharded\n",
+  );
+  process.exit(1);
+}
+
+const files = (
+  await Array.fromAsync(
+    new Bun.Glob("server/tests/**/*.test.ts").scan({ cwd: process.cwd() }),
+  )
+).sort();
+
+if (files.length === 0) {
+  await writeStderr(
+    "No test files matched server/tests/**/*.test.ts. Refusing to report a pass.\n",
+  );
+  process.exit(1);
+}
+
+const shards: string[][] = [];
+for (let index = 0; index < files.length; index += SHARD_SIZE) {
+  shards.push(files.slice(index, index + SHARD_SIZE));
+}
+
+// `bun run generate:app-config` once, up front, rather than per shard: the `pretest` hook that
+// normally produces it fires for `bun run test`, and these shards invoke `bun test` directly.
+const config = Bun.spawn(["bun", "run", "generate:app-config"], {
+  stdout: "inherit",
+  stderr: "inherit",
+});
+if ((await config.exited) !== 0) {
+  await writeStderr(
+    "\nCould not generate the application config, so route imports would fail.\n",
+  );
+  process.exit(1);
+}
+
+await writeStderr(
+  `\nRunning ${files.length} files in ${shards.length} shards of up to ${SHARD_SIZE}.\n`,
+);
+
+let passed = 0;
+let failed = 0;
+let skipped = 0;
+const unreadable: number[] = [];
+const red: number[] = [];
+
+for (const [index, shard] of shards.entries()) {
+  await writeStderr(
+    `\n--- shard ${index + 1}/${shards.length} (${shard.length} files) ---\n`,
+  );
+
+  const proc = Bun.spawn(["bun", "test", ...shard], {
+    stdout: "inherit",
+    stderr: "pipe",
+  });
+
+  // Bun writes its summary to stderr, so it is captured to be parsed, then echoed unmodified.
+  const stderr = await new Response(proc.stderr).text();
+  await writeStderr(stderr);
+
+  // Read the status from the process, never from the text: a parse failure and a test failure are
+  // different outcomes and both have to be reported.
+  const exitStatus = await proc.exited;
+  if (exitStatus !== 0) red.push(index + 1);
+
+  const passMatch = stderr.match(/(\d+) pass/);
+  const failMatch = stderr.match(/(\d+) fail/);
+  const skipMatch = stderr.match(/(\d+) skip/);
+
+  if (!passMatch) {
+    unreadable.push(index + 1);
+  } else {
+    passed += Number.parseInt(passMatch[1] as string, 10);
+    failed += failMatch ? Number.parseInt(failMatch[1] as string, 10) : 0;
+    skipped += skipMatch ? Number.parseInt(skipMatch[1] as string, 10) : 0;
+  }
+}
+
+await writeStderr(
+  `\n=== ${shards.length} shards: ${passed} pass / ${failed} fail / ${skipped} skip ===\n`,
+);
+
+if (red.length > 0) {
+  await writeStderr(`Shards that exited non-zero: ${red.join(", ")}.\n`);
+  process.exitCode = 1;
+}
+
+if (unreadable.length > 0) {
+  await writeStderr(
+    `Could not read a summary line from shard(s) ${unreadable.join(", ")}. Refusing to report a\n` +
+      "pass on a run that cannot be counted: the usual cause is a file that threw while being\n" +
+      "imported, which takes its tests with it and reports nothing.\n",
+  );
+  process.exitCode = 1;
+}

server/tests/account-mode.integration.test.ts

@@ -1,5 +1,12 @@
 // server/tests/account-mode.integration.test.ts
-import { afterEach, beforeEach, describe, expect, test } from "bun:test";
+import {
+  afterAll,
+  afterEach,
+  beforeEach,
+  describe,
+  expect,
+  test,
+} from "bun:test";
 import { randomUUID } from "node:crypto";
 import { eq } from "drizzle-orm";
 import { createAuditStore } from "../src/audit";
@@ -398,3 +405,7 @@ describe("switching back to Personal", () => {
     expect(rules[0]?.revokedAt).not.toBeNull();
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/audit-cursor.integration.test.ts

@@ -1,4 +1,4 @@
-import { describe, expect, test } from "bun:test";
+import { afterAll, describe, expect, test } from "bun:test";
 import { sql } from "drizzle-orm";
 import { createApp } from "../src/app";
 import { createAuditReader } from "../src/audit";
@@ -158,3 +158,7 @@ describe("a cursor over rows written within one millisecond", () => {
     expect(await walk(targetId)).toEqual(expected);
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/audit-initiator.integration.test.ts

@@ -1,4 +1,4 @@
-import { describe, expect, test } from "bun:test";
+import { afterAll, describe, expect, test } from "bun:test";
 import { eq } from "drizzle-orm";
 import {
   auditQueryFromUrl,
@@ -291,3 +291,7 @@ describe("what started a run", () => {
     expect(reattribute()).rejects.toThrow();
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/audit-retention.integration.test.ts

@@ -1,4 +1,4 @@
-import { afterEach, describe, expect, test } from "bun:test";
+import { afterAll, afterEach, describe, expect, test } from "bun:test";
 import { eq, sql } from "drizzle-orm";
 import postgres from "postgres";
 import { sweepAuditTrail } from "../src/audit-retention";
@@ -309,3 +309,7 @@ describe("keeping the audit trail to a retention policy", () => {
     expect(deleted).toBe(1);
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/bot-lifecycle.integration.test.ts

@@ -706,3 +706,7 @@ describe("reset", () => {
     ).toEqual([]);
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/component-store.integration.test.ts

@@ -404,3 +404,7 @@ describe("what a component may read", () => {
     expect(store.callFunction(`nothing_${suite}`, {})).rejects.toThrow();
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/composio-connection-kinds.test.ts

@@ -2126,3 +2126,7 @@ describe("what a brokered connection writes into the append-only trail", () => {
     ]).toEqual([""]);
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/composio-connections.test.ts

@@ -5276,3 +5276,7 @@ test("a connect spends its check on the account it just made", async () => {
  * action the check spent — is asserted by the check-does-not-come-back-clean test above, which now
  * reaches that state by the ordinary path instead of only through a vendor that would not act.
  */
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/computer-page-frame-store.integration.test.ts

@@ -1,4 +1,4 @@
-import { afterEach, describe, expect, test } from "bun:test";
+import { afterAll, afterEach, describe, expect, test } from "bun:test";
 import { eq } from "drizzle-orm";
 import { createPageFrameStore } from "../src/computer/page-frames";
 import { createDatabase } from "../src/db/client";
@@ -208,3 +208,7 @@ describe("kept page frames", () => {
     expect(await store.load(id, "")).toBe(null);
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/computer-session-guard-replicas.integration.test.ts

@@ -142,3 +142,7 @@ test("CONTROL: replica B allows the same ref while the run is still current", as
   });
   expect(result).toMatchObject({ action: "click" });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/computer-snapshot-store.integration.test.ts

@@ -1,4 +1,4 @@
-import { afterEach, describe, expect, test } from "bun:test";
+import { afterAll, afterEach, describe, expect, test } from "bun:test";
 import type { SnapshotElement } from "../src/computer/schema";
 import {
   createSnapshotStore,
@@ -147,3 +147,7 @@ describe("a snapshot taken on one server", () => {
     );
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/credential-rotation-leak.integration.test.ts

@@ -162,3 +162,7 @@ describe("a rotation the database refuses", () => {
     expect(sentence).not.toContain("ciphertext");
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/credentials.test.ts

@@ -1,4 +1,4 @@
-import { afterEach, describe, expect, test } from "bun:test";
+import { afterAll, afterEach, describe, expect, test } from "bun:test";
 import { randomUUID } from "node:crypto";
 import { eq } from "drizzle-orm";
 import { createApp } from "../src/app";
@@ -865,3 +865,7 @@ describe("admin credential API", () => {
     expect(await rotate.text()).not.toContain("rotated-secret");
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/database-pool-pairing.test.ts

@@ -0,0 +1,49 @@
+/**
+ * Pairing rule for server/tests: a file that opens a database pool must close it.
+ *
+ * Every file that calls createDatabase holds a pool for the rest of the process.
+ * PostgreSQL's default max_connections is 100; with TEST_POOL max: 2, a file costs
+ * two connections from its first query. Leaving pools open exhausts the server
+ * during a full `bun test server/tests` run (see #785).
+ *
+ * The close must be the last work in that file's teardown (Bun runs afterAll FIFO),
+ * so deletes still have a live client. This guard only checks that a close exists;
+ * hook order stays a human review concern.
+ */
+import { expect, test } from "bun:test";
+import { readdir } from "node:fs/promises";
+import { join } from "node:path";
+
+const testsRoot = join(import.meta.dir);
+
+async function listTestFiles(directory: string): Promise<string[]> {
+  const entries = await readdir(directory, { withFileTypes: true });
+  const files: string[] = [];
+  for (const entry of entries) {
+    const path = join(directory, entry.name);
+    if (entry.isDirectory()) {
+      files.push(...(await listTestFiles(path)));
+      continue;
+    }
+    if (entry.isFile() && entry.name.endsWith(".test.ts")) {
+      files.push(path);
+    }
+  }
+  return files;
+}
+
+test("a file that opens a database closes it", async () => {
+  const files = await listTestFiles(testsRoot);
+  const offenders: string[] = [];
+
+  for (const path of files) {
+    const source = await Bun.file(path).text();
+    if (!source.includes("createDatabase(")) continue;
+    if (source.includes("$client.close(") || source.includes("$client.end(")) {
+      continue;
+    }
+    offenders.push(path.slice(testsRoot.length + 1));
+  }
+
+  expect(offenders).toEqual([]);
+});

server/tests/database.test.ts

@@ -2,11 +2,12 @@ import { expect, test } from "bun:test";
 import { createDatabase } from "../src/db/client";
 import { TEST_POOL } from "./support/database";
 
-test("creates a typed database boundary without opening a query", () => {
+test("creates a typed database boundary without opening a query", async () => {
   const database = createDatabase(
     "postgres://openbot:openbot@localhost:5432/openbot",
     TEST_POOL,
   );
 
   expect(database.query.users).toBeDefined();
+  await database.$client.close();
 });

server/tests/headless-components.integration.test.ts

@@ -361,3 +361,7 @@ describe("an unattended routine turn answering with a governed chart", () => {
     }
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/jsonb-encoding.integration.test.ts

@@ -79,3 +79,7 @@ describe("what a jsonb column actually stores", () => {
     expect(row?.strings).toBe(0);
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});

server/tests/last-sign-in-backfill.integration.test.ts

@@ -1,4 +1,4 @@
-import { describe, expect, test } from "bun:test";
+import { afterAll, describe, expect, test } from "bun:test";
 import { inArray, sql } from "drizzle-orm";
 import { createDatabase } from "../src/db/client";
 import { sessions, users } from "../src/db/schema";
@@ -112,3 +112,7 @@ describe("the backfill migration", () => {
     expect(observed.get(neverHere)).toBeNull();
   });
 });
+
+afterAll(async () => {
+  await database.$client.close();
+});