fix(db): make the teardown regression test detect the ordering defect

The test used to pass even when closeRegisteredClients did nothing: it
waited a fixed number of ticks before sending the late query, and by
then the driver had not yet cleared its socket reference, so the
query always rejected cleanly regardless of the fix. Wait for the
driver's own close callback instead of guessing a tick count, so the
late query lands exactly in the race the fix closes.

Also close a gap the new test surfaced: hostPortKey() throws on a URL
new URL() cannot parse, while the underlying driver does not. Skip the
registry entry in that case instead of throwing, and add a small test
for it.

Co-authored-by: Paperclip <noreply@paperclip.ing>

packages/db/src/client-teardown-registry.test.ts

@@ -50,44 +50,68 @@ describe("closeRegisteredClients", () => {
     server = null;
   });
 
-  it("ends a reserved connection before its backend dies, so no query can reach a null socket", async () => {
-    const started = await startFakePostgresServer();
-    server = started.server;
-    const url = `postgres://test:test@127.0.0.1:${started.port}/test`;
+  it(
+    "ends a reserved connection before its backend dies, so no query can reach a null socket",
+    async () => {
+      const started = await startFakePostgresServer();
+      server = started.server;
+      const url = `postgres://test:test@127.0.0.1:${started.port}/test`;
 
-    const db = createDb(url, { connectTimeoutSeconds: 5 });
-    // `sql.reserve()` pins one physical connection. Drizzle `db.transaction()`
-    // reaches the same surface through `sql.begin()`, so this stands in for a
-    // suite that left a transaction connection open.
-    const reserved = await db.$client.reserve();
+      const db = createDb(url, { connectTimeoutSeconds: 5 });
+      // `sql.reserve()` pins one physical connection. Drizzle `db.transaction()`
+      // reaches the same surface through `sql.begin()`, so this stands in for a
+      // suite that left a transaction connection open.
+      const reserved = await db.$client.reserve();
 
-    const order: string[] = [];
+      // The driver calls this only after it has fully processed a connection
+      // close: its socket reference cleared and any in-flight query failed.
+      // Waiting for it, instead of a fixed number of ticks, is what the
+      // historical crash reproduction does — it is real observed state from
+      // the driver, not a guess at timing.
+      const driverProcessedClose = new Promise<void>((resolve) => {
+        db.$client.options.onclose = () => resolve();
+      });
 
-    // This is the order our fixture owns: end every registered client for
-    // this host and port before a caller stops the cluster it points at.
-    await closeRegisteredClients(url);
-    order.push("clients-closed");
+      // This is the order our fixture owns: end every registered client for
+      // this host and port before a caller stops the cluster it points at.
+      await closeRegisteredClients(url);
 
-    // Simulate the cluster stop that follows in the real fixture. Before the
-    // fix, killing the backend here while a client still held the reserved
-    // connection open crashed the process on a later deferred write.
-    for (const socket of started.backendSockets) socket.destroy();
-    order.push("cluster-stopped");
+      // Simulate the cluster stop that follows in the real fixture. Before the
+      // fix, killing the backend here while a client still held the reserved
+      // connection open crashed the process on a later deferred write.
+      for (const socket of started.backendSockets) socket.destroy();
+      await driverProcessedClose;
 
-    expect(order).toEqual(["clients-closed", "cluster-stopped"]);
+      // A query sent only after the driver finished processing the close still
+      // buffers its frame for a deferred flush one tick later. If the fix let
+      // the reserved connection outlive the backend, that flush reaches a
+      // cleared socket reference and throws from inside the timer callback —
+      // this specific promise then never settles, because nothing on that
+      // path ever calls its resolve or reject. This test's own timeout (set
+      // below, on the `it` call) is what turns that hang into a reported
+      // failure, alongside the unhandled exception the crash raises
+      // separately.
+      const settled = await reserved`select 1`.catch((error: unknown) => error);
+      expect(settled).toBeInstanceOf(Error);
 
-    // Let any timer the driver's teardown scheduled run to completion. If a
-    // deferred write still fired against a null socket, it would surface here
-    // as an unhandled error and fail this test file.
-    await new Promise((resolve) => setImmediate(() => setImmediate(resolve)));
-
-    // A query on the reserved connection rejects through the ordinary
-    // closed-connection path; it does not throw past this call.
-    const settled = await reserved`select 1`.catch((error: unknown) => error);
-    expect(settled).toBeInstanceOf(Error);
-  });
+      // Let the deferred flush actually run. If it still fires against a null
+      // socket, it surfaces here as an unhandled error and fails this test
+      // file — the exact signature the fix protects against.
+      await new Promise((resolve) => setImmediate(() => setImmediate(resolve)));
+    },
+    2_000,
+  );
 
   it("does nothing when no client is registered for a host and port", async () => {
     await expect(closeRegisteredClients("postgres://test:test@127.0.0.1:1/test")).resolves.toBeUndefined();
   });
+
+  it("does not throw when createDb receives a URL that new URL() cannot parse", async () => {
+    let db: ReturnType<typeof createDb> | undefined;
+    expect(() => {
+      db = createDb("", { connectTimeoutSeconds: 1 });
+    }).not.toThrow();
+
+    await db?.$client.end({ timeout: 0 }).catch(() => {});
+  });
 });

packages/db/src/client.ts

@@ -25,6 +25,22 @@ function hostPortKey(url: string): string {
   return `${parsed.hostname}:${parsed.port || "5432"}`;
 }
 
+/**
+ * Same as `hostPortKey`, but returns `null` instead of throwing when the URL
+ * does not parse. `postgres(url)` tolerates a value `new URL()` rejects (an
+ * empty string falls back to the `PG*` environment variables), so `createDb`
+ * must tolerate it too: skip the registry entry and let the driver decide
+ * the outcome, instead of throwing an error the driver itself would not.
+ */
+function hostPortKeyOrNull(url: string): string | null {
+  try {
+    return hostPortKey(url);
+  } catch (error) {
+    if (error instanceof TypeError && (error as NodeJS.ErrnoException).code === "ERR_INVALID_URL") return null;
+    throw error;
+  }
+}
+
 // Tracks every client `createDb` hands out, keyed by host and port, so a test
 // fixture can end them before it stops the Postgres cluster they point at. A
 // `WeakRef` plus `FinalizationRegistry` means a long-lived process (a real
@@ -177,7 +193,8 @@ export function postgresJsOptions(options: DatabaseClientOptions): Record<string
 export function createDb(url: string, options?: DatabaseClientOptions) {
   const resolved = options ?? databaseClientOptionsFromEnv();
   const sql = postgres(url, postgresJsOptions(resolved));
-  registerClient(hostPortKey(url), sql);
+  const key = hostPortKeyOrNull(url);
+  if (key) registerClient(key, sql);
   return drizzlePg(sql, { schema });
 }