fix(driver-sql): give the bounded connection attempt an accurate error message (#3769)

.changeset/sql-driver-dialect-connect-timeout.md

@@ -0,0 +1,46 @@
+---
+"@objectstack/driver-sql": minor
+---
+
+fix(driver-sql): give the bounded connection attempt an accurate error message (#3769)
+
+#3781 bounded a connection attempt at 10s via `pool.createTimeoutMillis`, which
+stopped the 30s hang but kept knex's own wording: `Timeout acquiring a
+connection. The pool is probably full`. The pool is not full — the server never
+completed the handshake — so that message sends an operator to tune `pool.max`
+while the network is what is broken. This is the same defect class the boot
+guard in #3741 was about: an error that reads nothing like its cause.
+
+`SqlDriver` now also sets the **dialect's own** connect timeout, which fails with
+a message that names what happened:
+
+| client | key | message |
+|---|---|---|
+| `pg` / `postgres` / `postgresql` / `cockroachdb` | `connectionTimeoutMillis` | `timeout expired` |
+| `mysql` / `mysql2` | `connectTimeout` | `connect ETIMEDOUT` |
+
+Carrying the timeout requires `connection` to be an object, so a URL string is
+moved into the dialect's URL slot (`connectionString` for pg, `uri` for mysql2).
+Verified against a black-holing listener that both forms still reach the URL's
+own host/port and still honour `?sslmode=require`. SQLite is untouched — opening
+a file has no handshake to time out.
+
+**The two bounds are deliberately unequal.** They race and knex wins a tie, so
+equal values would let the pool timeout fire first and the accurate message would
+never be seen. The dialect timeout is the effective bound at **10s**; the pool
+timeout is a strictly looser backstop, raised from 10s to **15s**, reached only
+by a dialect with no connect-timeout knob or one that ignores the one we set.
+
+`driver.config` keeps the shape the author passed — the rewrite applies only to
+what knex receives. Two existing readers depend on that: `serve.ts`'s startup
+banner and `createDatabase()`, which parses the URL to swap in the maintenance
+database. A test pins it.
+
+`createDatabase()`'s own admin connection now gets the same bound; it is opened
+during boot against the very server we already suspect is unreachable, so it must
+not be the one place that still waits 30s.
+
+**Migration.** None for a healthy datasource. A deployment that deliberately
+needs longer than 10s to establish a connection (a slow cross-region replica)
+sets `connection.connectionTimeoutMillis` (pg) or `connection.connectTimeout`
+(mysql2) explicitly, and it is left alone.

content/docs/data-modeling/drivers.mdx

@@ -127,7 +127,9 @@ even earlier — that is where `MongoDBDriver` puts its
 pnpm add @objectstack/driver-sql pg
 ```
 
-`SqlDriver`'s constructor accepts a [Knex `Knex.Config`](https://knexjs.org/guide/#configuration-options) object verbatim.
+`SqlDriver`'s constructor accepts a [Knex `Knex.Config`](https://knexjs.org/guide/#configuration-options)
+object, passing it through unchanged except for two connect-timeout defaults
+described [below](#connect-timeouts).
 
 ```typescript
 import { SqlDriver } from '@objectstack/driver-sql';
@@ -152,6 +154,29 @@ Or via env var alone (driver inferred from `postgres://` scheme):
 export OS_DATABASE_URL=postgres://admin:secret@db.example.com:5432/myapp
 ```
 
+### Connect timeouts
+
+A database endpoint that accepts the TCP connection but never completes the
+handshake — an overloaded instance, a half-open firewall, a load balancer
+mid-failover — makes every query *wait* rather than fail. Left to Knex's own
+defaults that wait is 30 seconds per query on the request path, and with a small
+`pool.max` a handful of them saturate the pool. So `SqlDriver` supplies two
+defaults ([#3769](https://github.com/objectstack-ai/objectstack/issues/3769)):
+
+| Setting | Default | Purpose |
+| :--- | :--- | :--- |
+| `connection.connectionTimeoutMillis` (pg) / `connection.connectTimeout` (mysql2) | `10_000` | The effective bound. Fails with the driver's own wording — `timeout expired` / `connect ETIMEDOUT` — which names the network. |
+| `pool.createTimeoutMillis` | `15_000` | Backstop, only reached by a client that has no connect-timeout option or ignores it. Deliberately looser: the two race, and Knex wins a tie, so an equal value would mask the accurate message with Knex's misleading "the pool is probably full". |
+
+Set either explicitly and it is left untouched — do that when a datasource
+legitimately takes longer to connect (a distant cross-region replica). SQLite
+gets neither: opening a file has no handshake to time out.
+
+Carrying a connect timeout requires `connection` to be an object, so a URL string
+is moved into the client's URL slot (`connectionString` for pg, `uri` for
+mysql2) before reaching Knex. This affects only what Knex receives — the config
+you passed is preserved as-is on the driver.
+
 ## MongoDB
 
 Configuration properties for the MongoDB driver.

packages/plugins/driver-sql/src/sql-driver-connect-bound.test.ts

@@ -32,7 +32,7 @@ afterEach(async () => {
 describe('SqlDriver — connection-attempt bound (framework#3769)', () => {
   it('applies a default createTimeoutMillis when the host sets no pool config', () => {
     const d = make({ client: 'pg', connection: 'postgres://u:p@127.0.0.1:1/d' });
-    expect((d as any).knex.client.config.pool.createTimeoutMillis).toBe(10_000);
+    expect((d as any).knex.client.config.pool.createTimeoutMillis).toBe(15_000);
   });
 
   it('applies it alongside a host pool config without clobbering min/max', () => {
@@ -42,7 +42,7 @@ describe('SqlDriver — connection-attempt bound (framework#3769)', () => {
       pool: { min: 0, max: 5 },
     });
     const pool = (d as any).knex.client.config.pool;
-    expect(pool).toMatchObject({ min: 0, max: 5, createTimeoutMillis: 10_000 });
+    expect(pool).toMatchObject({ min: 0, max: 5, createTimeoutMillis: 15_000 });
   });
 
   it("leaves a host's explicit createTimeoutMillis alone", () => {
@@ -54,23 +54,97 @@ describe('SqlDriver — connection-attempt bound (framework#3769)', () => {
     expect((d as any).knex.client.config.pool.createTimeoutMillis).toBe(45_000);
   });
 
+  // The pool bound stops the wait but knex blames "the pool is probably full".
+  // Each network dialect also gets its OWN connect timeout so the message names
+  // the network instead — `timeout expired` (pg) / `connect ETIMEDOUT` (mysql2).
+  describe('per-dialect connect timeout (accurate diagnosis)', () => {
+    it('moves a pg URL into connectionString and rides the timeout alongside it', () => {
+      const d = make({ client: 'pg', connection: 'postgres://u:p@host:5432/d' });
+      expect((d as any).knex.client.config.connection).toEqual({
+        connectionString: 'postgres://u:p@host:5432/d',
+        connectionTimeoutMillis: 10_000,
+      });
+    });
+
+    it('moves a mysql2 URL into uri with the mysql2 spelling of the timeout', () => {
+      const d = make({ client: 'mysql2', connection: 'mysql://u:p@host:3306/d' });
+      expect((d as any).knex.client.config.connection).toEqual({
+        uri: 'mysql://u:p@host:3306/d',
+        connectTimeout: 10_000,
+      });
+    });
+
+    it('adds the timeout to an object connection without disturbing its fields', () => {
+      const d = make({
+        client: 'pg',
+        connection: { host: 'db.example.com', port: 5432, user: 'admin', database: 'app', ssl: true },
+      });
+      expect((d as any).knex.client.config.connection).toEqual({
+        host: 'db.example.com', port: 5432, user: 'admin', database: 'app', ssl: true,
+        connectionTimeoutMillis: 10_000,
+      });
+    });
+
+    it("leaves a host's explicit connect timeout alone", () => {
+      const d = make({
+        client: 'pg',
+        connection: { host: 'db.example.com', connectionTimeoutMillis: 60_000 },
+      });
+      expect((d as any).knex.client.config.connection.connectionTimeoutMillis).toBe(60_000);
+    });
+
+    it('injects nothing for sqlite — a file open has no handshake to time out', () => {
+      const d = make({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true });
+      expect((d as any).knex.client.config.connection).toEqual({ filename: ':memory:' });
+    });
+
+    it('leaves a function-valued connection alone — the host builds each one itself', () => {
+      const provider = () => ({ host: 'x' });
+      const d = make({ client: 'pg', connection: provider });
+      expect((d as any).knex.client.config.connection).toBe(provider);
+    });
+  });
+
+  // `driver.config` is load-bearing for two readers that predate this change:
+  // serve.ts's startup banner (`describeRegisteredDriver` reads conn.host /
+  // conn.filename) and `createDatabase()` (parses the URL to swap in the
+  // maintenance database). Both would break on the rewritten shape, so the
+  // rewrite must apply to what knex receives — never to what the author gave us.
+  it('keeps driver.config in the shape the author passed', () => {
+    const d = make({ client: 'pg', connection: 'postgres://u:p@host:5432/d', pool: { min: 0, max: 5 } });
+    expect((d as any).config.connection).toBe('postgres://u:p@host:5432/d');
+    expect((d as any).config.pool).toEqual({ min: 0, max: 5 });
+  });
+
   it('actually bounds a query against a black-holing endpoint', async () => {
     const bh = blackHole();
     const port = await bh.listen();
     try {
-      // 700ms so the assertion is decisive: unbounded is 30s.
+      // The default is 10s; unbounded would be knex/tarn's 30s.
       const d = make({
         client: 'pg',
         connection: `postgres://u:p@127.0.0.1:${port}/d`,
-        pool: { min: 0, max: 2, createTimeoutMillis: 700 },
+        pool: { min: 0, max: 2 },
       });
       const started = Date.now();
-      await expect((d as any).knex.raw('SELECT 1')).rejects.toThrow();
-      expect(Date.now() - started).toBeLessThan(5_000);
+      const err = await (d as any).knex.raw('SELECT 1').then(
+        () => { throw new Error('query resolved but should have failed'); },
+        (e: Error) => e,
+      );
+      const elapsed = Date.now() - started;
+
+      // The pg-level timeout fires first (10s), so the message names the
+      // connection attempt rather than blaming pool sizing. Guarding against
+      // the regression is the point: knex's own wording would send an operator
+      // to tune `pool.max` while the network is what is broken.
+      expect(err.message).toContain('timeout expired');
+      expect(err.message).not.toContain('pool is probably full');
+      expect(elapsed).toBeGreaterThan(8_000);
+      expect(elapsed).toBeLessThan(20_000);
     } finally {
       bh.close();
     }
-  }, 20_000);
+  }, 40_000);
 
   it('does not disturb a working sqlite connection', async () => {
     const d = make({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true });

packages/plugins/driver-sql/src/sql-driver.ts

@@ -598,21 +598,68 @@ export class SqlDriver implements IDataDriver {
    * knows better (a deliberately slow cross-region replica) sets its own
    * `pool.createTimeoutMillis` and this leaves it alone.
    *
-   * NOTE this bounds the wait but not the diagnosis: knex reports it as
-   * "Timeout acquiring a connection. The pool is probably full", which points
-   * at pool sizing rather than the network. Making the message accurate needs a
-   * dialect-specific connect timeout (pg's `connectionTimeoutMillis`), which
-   * changes the shape of `connection` — tracked in #3769.
+   * Applied in two layers, because the outer one bounds the wait but misstates
+   * the cause. Knex reports its own timeout as "Timeout acquiring a connection.
+   * The pool is probably full" — which sends an operator to tune `pool.max`
+   * while the actual problem is the network. So each network dialect ALSO gets
+   * its own connect timeout, which fails with a message that names what really
+   * happened (`timeout expired` from pg, `connect ETIMEDOUT` from mysql2).
+   *
+   * **The two bounds must not be equal.** They race, and knex wins a tie — set
+   * to the same value, the pool timeout fires first and the accurate message is
+   * never seen (caught by the black-hole test, which asserts the wording). So
+   * the dialect timeout is the effective bound at 10s and the pool timeout is a
+   * strictly looser backstop at 15s, reached only by a dialect that has no
+   * connect-timeout knob (SQLite) or ignores the one we set.
+   */
+  private static readonly DEFAULT_CONNECT_TIMEOUT_MS = 10_000;
+  private static readonly DEFAULT_CREATE_TIMEOUT_MS = 15_000;
+
+  /**
+   * Per-dialect connect timeout: how the driver spells "how long may
+   * establishing ONE connection take", and where a URL goes once `connection`
+   * has to become an object to carry it.
+   *
+   * SQLite (`better-sqlite3` / `sqlite3`) is deliberately absent — it opens a
+   * file, so there is no handshake to time out and nothing to inject.
    */
-  private static readonly DEFAULT_CREATE_TIMEOUT_MS = 10_000;
+  private static readonly DIALECT_CONNECT_TIMEOUT: Record<string, { key: string; urlKey: string }> = {
+    pg: { key: 'connectionTimeoutMillis', urlKey: 'connectionString' },
+    postgres: { key: 'connectionTimeoutMillis', urlKey: 'connectionString' },
+    postgresql: { key: 'connectionTimeoutMillis', urlKey: 'connectionString' },
+    cockroachdb: { key: 'connectionTimeoutMillis', urlKey: 'connectionString' },
+    mysql: { key: 'connectTimeout', urlKey: 'uri' },
+    mysql2: { key: 'connectTimeout', urlKey: 'uri' },
+  };
 
   private static withConnectBound(knexConfig: Record<string, any>): Record<string, any> {
     const pool = knexConfig.pool as Record<string, any> | undefined;
-    if (pool?.createTimeoutMillis !== undefined) return knexConfig; // host chose its own
-    return {
-      ...knexConfig,
-      pool: { ...(pool ?? {}), createTimeoutMillis: SqlDriver.DEFAULT_CREATE_TIMEOUT_MS },
-    };
+    const bounded: Record<string, any> =
+      pool?.createTimeoutMillis === undefined
+        ? {
+          ...knexConfig,
+          pool: { ...(pool ?? {}), createTimeoutMillis: SqlDriver.DEFAULT_CREATE_TIMEOUT_MS },
+        }
+        : { ...knexConfig }; // host chose its own bound — respect it
+
+    const dialect = SqlDriver.DIALECT_CONNECT_TIMEOUT[String(knexConfig.client ?? '')];
+    if (!dialect) return bounded; // sqlite / unknown client — nothing to inject
+
+    const conn = knexConfig.connection;
+    if (typeof conn === 'string') {
+      // The URL must move into the dialect's own URL slot so the timeout can
+      // ride alongside it. Verified for both dialects: the connection attempt
+      // still goes to the URL's host/port, `?sslmode=` is still honoured.
+      bounded.connection = {
+        [dialect.urlKey]: conn,
+        [dialect.key]: SqlDriver.DEFAULT_CONNECT_TIMEOUT_MS,
+      };
+    } else if (conn && typeof conn === 'object' && (conn as any)[dialect.key] === undefined) {
+      bounded.connection = { ...(conn as object), [dialect.key]: SqlDriver.DEFAULT_CONNECT_TIMEOUT_MS };
+    }
+    // A function-valued `connection` (knex's per-acquire provider) is left
+    // alone: the host is building each connection itself and owns its timeouts.
+    return bounded;
   }
 
   /**
@@ -4123,7 +4170,10 @@ export class SqlDriver implements IDataDriver {
       return; // Unsupported dialect for auto-creation
     }
 
-    const adminKnex = knex(adminConfig);
+    // Same connect bound as the main pool (#3769) — this admin connection is
+    // opened during boot against the very server we already suspect might be
+    // unreachable, so it must not be the one place that waits 30s.
+    const adminKnex = knex(SqlDriver.withConnectBound(adminConfig));
     try {
       if (this.isPostgres) {
         await adminKnex.raw(`CREATE DATABASE "${dbName}"`);