diff --git a/.changeset/sql-driver-dialect-connect-timeout.md b/.changeset/sql-driver-dialect-connect-timeout.md new file mode 100644 index 0000000000..5dde8daf0d --- /dev/null +++ b/.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. diff --git a/content/docs/data-modeling/drivers.mdx b/content/docs/data-modeling/drivers.mdx index 0465370314..08261a07ac 100644 --- a/content/docs/data-modeling/drivers.mdx +++ b/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. diff --git a/packages/plugins/driver-sql/src/sql-driver-connect-bound.test.ts b/packages/plugins/driver-sql/src/sql-driver-connect-bound.test.ts index f1b0c0d9bc..6b3fb4a25c 100644 --- a/packages/plugins/driver-sql/src/sql-driver-connect-bound.test.ts +++ b/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 }); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index b83e850568..ef99e5b2fb 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/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 = { + 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): Record { const pool = knexConfig.pool as Record | undefined; - if (pool?.createTimeoutMillis !== undefined) return knexConfig; // host chose its own - return { - ...knexConfig, - pool: { ...(pool ?? {}), createTimeoutMillis: SqlDriver.DEFAULT_CREATE_TIMEOUT_MS }, - }; + const bounded: Record = + 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}"`);