From e661c9bb4b92422763e278006bd109d1a11d0cc2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 04:17:12 +0000 Subject: [PATCH] fix(driver-sql): bound a connection attempt at 10s, correct the "no reconnection" claim (#3769, #3759) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related corrections, both from measuring what #3741/#3751/#3765 asserted. ## The claim was wrong #3751 and #3765 shipped several statements that drivers never reconnect — "there is no lazy reconnection", "NOT retried and NOT reconnected", "stays disconnected for the process lifetime". Measured, both drivers recover: - driver-mongodb: killing a real mongod and restarting it on the same port, the SAME driver instance served the next write successfully (13ms) with no reconnect call from us — the official driver's topology monitor handles it. - driver-sql: a knex/pg pool is not poisoned by an outage. Its error tracks live server state (ECONNREFUSED while down → a handshake error once a listener is back → ECONNREFUSED again), i.e. every acquire opens a fresh connection. storage-driver.ts also sets pool.min = 0, so no stale idle connections. The original reasoning grepped this repo for `reconnect`, found nothing, and concluded recovery does not happen — but the recovery lives in the client libraries, not in our code. "We didn't write it" is not "it doesn't happen". ## Fail-fast at boot is unchanged — the reason is different It is not that the connection can never return; it is that the BOOT SEQUENCE never re-runs. A driver that missed init() also missed syncRegisteredSchemas(), so its tables can simply not exist even after the database comes back. The DEGRADED BOOT banner now says that instead of claiming a dead connection. ## The real defect underneath SqlDriver passed its config to knex untouched, so an endpoint that accepts TCP but never completes the handshake (overloaded instance, half-open firewall, LB mid-failover) made every query wait out tarn's 30s default, then fail with "Timeout acquiring a connection. The pool is probably full" — pointing an operator at pool sizing instead of the network. With a small pool.max a few such queries saturate the pool and everything else queues behind them. SqlDriver now defaults pool.createTimeoutMillis to 10s, matching driver-mongodb's existing `connectTimeoutMS ?? 10_000`. A host that sets its own createTimeoutMillis is left alone. Measured (black-holing listener, real knex/pg): today (no bound) 30024ms Knex: Timeout acquiring a connection… acquireConnectionTimeout 3s 3003ms Knex: Timeout acquiring a connection… pool.createTimeoutMillis 3s 3004ms Knex: Timeout acquiring a connection… connectionTimeoutMillis 3s 3006ms timeout expired Message accuracy needs the dialect-specific knob (last row), which changes the shape of `connection` and would regress serve.ts's startup banner — left to #3769. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01T9VBCNvg9Zixrf1A9Rnwdd --- ...-connect-bound-and-reconnect-correction.md | 58 ++++++++++++++ content/docs/data-modeling/drivers.mdx | 23 ++++-- content/docs/deployment/self-hosting.mdx | 15 ++-- .../objectql/src/driver-connect-errors.ts | 8 +- .../engine-driver-connect-failfast.test.ts | 4 +- packages/objectql/src/engine.ts | 19 +++-- .../src/sql-driver-connect-bound.test.ts | 79 +++++++++++++++++++ packages/plugins/driver-sql/src/sql-driver.ts | 34 +++++++- packages/types/src/env.ts | 8 +- 9 files changed, 216 insertions(+), 32 deletions(-) create mode 100644 .changeset/driver-connect-bound-and-reconnect-correction.md create mode 100644 packages/plugins/driver-sql/src/sql-driver-connect-bound.test.ts diff --git a/.changeset/driver-connect-bound-and-reconnect-correction.md b/.changeset/driver-connect-bound-and-reconnect-correction.md new file mode 100644 index 0000000000..90e5ed32c2 --- /dev/null +++ b/.changeset/driver-connect-bound-and-reconnect-correction.md @@ -0,0 +1,58 @@ +--- +"@objectstack/driver-sql": minor +"@objectstack/objectql": patch +"@objectstack/types": patch +--- + +fix(driver-sql): bound a connection attempt at 10s, and correct the "no reconnection" claim (#3769, #3759) + +Two related corrections, both from measuring what #3741/#3751/#3765 had only asserted. + +**The claim was wrong.** #3751 and #3765 shipped several statements that drivers +never reconnect — "there is no lazy reconnection", "NOT retried and NOT +reconnected", "stays disconnected for the process lifetime". Measured, both +drivers recover on their own: + +- driver-mongodb: killing a real `mongod` and restarting it on the same port, + the *same* driver instance served the next write successfully (13ms), with no + reconnect call from us — the official driver's topology monitor handles it. +- driver-sql: a knex/pg pool is not poisoned by an outage. Its error tracks live + server state (`ECONNREFUSED` while down → a handshake error once a listener is + back → `ECONNREFUSED` again), i.e. every acquire opens a fresh connection. + `storage-driver.ts` also configures `pool.min: 0`, so no stale idle + connections are held. + +The original reasoning grepped this repo for `reconnect`, found nothing, and +concluded recovery does not happen — but the recovery lives in the client +libraries, not in our code. The claims are now corrected in `DriverConnectError`, +the `DEGRADED BOOT` banner, `resolveAllowDriverConnectFailure`'s docs, and the +drivers / self-hosting pages. + +**Fail-fast at boot is unchanged and still correct** — the reason is just +different. It is not that the connection can never return; it is that the *boot +sequence* never re-runs. A driver that missed `init()` also missed +`syncRegisteredSchemas()`, so its tables can simply not exist even after the +database comes back. The banner now says that. + +**The real defect underneath.** `SqlDriver` passed its config to knex untouched, +so a database endpoint that accepts TCP but never completes the handshake — an +overloaded instance, a half-open firewall, a load balancer mid-failover — made +every query wait out tarn's 30s default, then fail with `Timeout acquiring a +connection. The pool is probably full`, pointing an operator at pool sizing +instead of the network. With a small `pool.max` a few such queries saturate the +pool and everything else queues. + +`SqlDriver` now defaults `pool.createTimeoutMillis` to **10s**, matching +driver-mongodb's existing `connectTimeoutMS ?? 10_000` so both drivers give up on +an unreachable server at the same point. A host that sets its own +`createTimeoutMillis` is left alone. + +**Migration.** None for a healthy datasource. A deployment that deliberately +relies on connection establishment taking longer than 10s (a slow cross-region +replica) should set `pool.createTimeoutMillis` explicitly on its `SqlDriver` +config. + +Not fixed here, tracked in #3769: knex still reports the bounded wait as "the +pool is probably full". An accurate message needs a dialect-specific connect +timeout (pg's `connectionTimeoutMillis`), which changes the shape of `connection` +and would regress the startup banner's URL display. diff --git a/content/docs/data-modeling/drivers.mdx b/content/docs/data-modeling/drivers.mdx index 2ed7381b33..0465370314 100644 --- a/content/docs/data-modeling/drivers.mdx +++ b/content/docs/data-modeling/drivers.mdx @@ -78,17 +78,24 @@ bootstrap. If any `connect()` rejects, the boot is **refused** ([#3741](https://github.com/objectstack-ai/objectstack/issues/3741)) — the error names each failed driver and its cause, and `objectstack serve` exits 1. -There is no lazy reconnection. A driver that did not connect at startup stays -disconnected for the process lifetime, so booting anyway would produce a server -that reports itself started, answers health checks, and then fails every request -with an error nothing like *the database is unreachable* — while the schema sync -that follows `init()` issues DDL against a datasource that isn't there. +Booting without a reachable datasource would produce a server that reports +itself started, answers health checks, and then fails every request with an +error nothing like *the database is unreachable* — while the schema sync that +follows `init()` issues DDL against a datasource that isn't there. + +The reason is **not** that the connection can never come back. Client libraries +do re-establish connections on their own — the MongoDB driver's topology monitor +reconnects, and knex/pg opens a fresh connection per acquire (verified in +[#3759](https://github.com/objectstack-ai/objectstack/issues/3759)). What never +comes back is the **boot sequence that was skipped**: nothing re-runs the schema +sync, so those objects can be left with no tables even after the database +returns. `OS_ALLOW_DRIVER_CONNECT_FAILURE=1` boots anyway, in an explicitly degraded -state announced by a `DEGRADED BOOT` banner. The failed drivers are never -retried and never reconnected: every query and every schema sync routed to them -fails until the process restarts. Do not set it in production. +state announced by a `DEGRADED BOOT` banner. Queries to a failed driver fail +until the datasource becomes reachable, and its boot-time schema sync is skipped +for good. Do not set it in production. ### Writing a driver: `connect()` is where you refuse to start diff --git a/content/docs/deployment/self-hosting.mdx b/content/docs/deployment/self-hosting.mdx index 61a508ed28..695f987bd4 100644 --- a/content/docs/deployment/self-hosting.mdx +++ b/content/docs/deployment/self-hosting.mdx @@ -298,13 +298,14 @@ cleanly and a replica that lost its database stops receiving traffic instead of serving 500s. Before setting `replicas > 1`, read the multi-node note below. -Drivers do **not** reconnect on their own -([#3759](https://github.com/objectstack-ai/objectstack/issues/3759)). Once a -connection is lost the process stays broken until it restarts, so on Kubernetes -the readiness probe above is what removes the replica — pair it with your -normal restart policy. Without an orchestrator (bare `os serve`, systemd, a -single container with no health check), plan for a supervisor that restarts on -a failing `/api/v1/ready`. +A replica **does** recover on its own once the database returns — client +libraries re-establish connections without help (the MongoDB driver's topology +monitor; knex/pg opening a fresh connection per acquire, verified in +[#3759](https://github.com/objectstack-ai/objectstack/issues/3759)). So a +transient outage — a failover, a maintenance window — needs no restart: the +readiness probe drains the replica while the database is away and re-admits it +afterwards. Restart only when the process is genuinely stuck, and note that a +replica which *booted* without its database never re-runs its schema sync. ## Reverse proxy & TLS diff --git a/packages/objectql/src/driver-connect-errors.ts b/packages/objectql/src/driver-connect-errors.ts index 3d2fa83287..10a5297cf9 100644 --- a/packages/objectql/src/driver-connect-errors.ts +++ b/packages/objectql/src/driver-connect-errors.ts @@ -67,10 +67,10 @@ export class DriverConnectError extends Error { super( `${failures.length} of ${totalDrivers} data driver(s) failed to connect — refusing to boot.\n` + `${detail}\n` + - `A driver that did not connect cannot serve queries (there is no lazy reconnection) and the ` + - `schema sync that runs right after init would issue DDL against it. Fix the datasource ` + - `configuration (e.g. OS_DATABASE_URL), or set OS_ALLOW_DRIVER_CONNECT_FAILURE=1 to boot anyway ` + - `in an explicitly degraded state where every query to those drivers fails.`, + `A driver that did not connect cannot serve queries, and the schema sync that runs right ` + + `after init would issue DDL against it. Fix the datasource configuration (e.g. ` + + `OS_DATABASE_URL), or set OS_ALLOW_DRIVER_CONNECT_FAILURE=1 to boot anyway and serve errors ` + + `until the datasource becomes reachable.`, ); this.name = 'DriverConnectError'; if (failures[0]?.error instanceof Error) { diff --git a/packages/objectql/src/engine-driver-connect-failfast.test.ts b/packages/objectql/src/engine-driver-connect-failfast.test.ts index 987f3b2937..9012bc71d2 100644 --- a/packages/objectql/src/engine-driver-connect-failfast.test.ts +++ b/packages/objectql/src/engine-driver-connect-failfast.test.ts @@ -132,7 +132,9 @@ describe('ObjectQL.init() — driver connect fail-fast (framework#3741)', () => await expect(engine.init()).resolves.toBeUndefined(); const warned = warnings.join('\n'); expect(warned).toContain('DEGRADED BOOT'); - expect(warned).toContain('NOT reconnected'); + // The durable consequence is the SKIPPED SCHEMA SYNC, not a dead connection: + // the client reconnects on its own, but nothing re-runs the DDL (#3759). + expect(warned).toContain('SKIPPED FOR GOOD'); expect(warned).toContain('sql'); }); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 2d4eb2abdd..c3ed1c629a 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1891,13 +1891,15 @@ export class ObjectQL implements IDataEngine { * `connect()` rejects, this throws {@link DriverConnectError} and kernel * bootstrap aborts. Two reasons, both load-bearing: * - * 1. A driver that did not connect never recovers — there is no lazy - * reconnection anywhere in `driver-sql` / `driver-mongodb`. Booting - * anyway produces a server that reports itself started, may even answer - * health checks, and then 500s every request with an error that reads + * 1. Booting without a reachable datasource produces a server that reports + * itself started and then 500s every request with an error that reads * nothing like "the database is unreachable". The caller immediately * makes it worse: `ObjectQLPlugin.start()` runs `syncRegisteredSchemas()` - * right after this, issuing DDL against a driver that isn't there. + * right after this, issuing DDL against a driver that isn't there — so + * the boot leaves the schema half-applied even if the database appears + * a second later. Recovery is not the point: the underlying clients DO + * re-establish connections on their own (framework#3759 verified this), + * but nothing re-runs the boot sequence that was skipped. * 2. Swallowing the rejection removed a driver's ability to REFUSE STARTUP. * Any fatal startup check — licence, server version, incompatible * configuration, missing capability, not just an unreachable socket — is @@ -1935,9 +1937,10 @@ export class ObjectQL implements IDataEngine { const banner = `⚠️ DEGRADED BOOT: ${failures.length} of ${this.drivers.size} driver(s) failed to connect ` + `(${failedDrivers.join(', ')}), but OS_ALLOW_DRIVER_CONNECT_FAILURE is set — starting anyway. ` + - `These drivers are NOT retried and NOT reconnected: every query and every schema sync routed ` + - `to them fails for the lifetime of this process. Unset OS_ALLOW_DRIVER_CONNECT_FAILURE to ` + - `restore fail-fast boot.`; + `Every query routed to them fails until the datasource becomes reachable, and the boot-time ` + + `schema sync is SKIPPED FOR GOOD — the client will reconnect on its own, but nothing re-runs ` + + `the DDL, so those objects may have no tables even after the database comes back. Unset ` + + `OS_ALLOW_DRIVER_CONNECT_FAILURE to restore fail-fast boot.`; this.logger.warn(banner, { failedDrivers }); // …and again on a channel the host cannot silence — see the helper's note // on `os serve`'s boot-quiet stdout capture. 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 new file mode 100644 index 0000000000..f1b0c0d9bc --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-connect-bound.test.ts @@ -0,0 +1,79 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +// +// framework#3769: a database endpoint that accepts the TCP connection but never +// completes the handshake (overloaded instance, half-open firewall, LB mid- +// failover) makes every query WAIT rather than fail. Left to tarn's default that +// wait is 30s per query on the request path. SqlDriver now bounds it by default, +// while leaving a host's own choice alone. + +import { describe, it, expect, afterEach } from 'vitest'; +import net from 'node:net'; +import { SqlDriver } from './sql-driver.js'; + +/** A listener that accepts sockets and then says nothing, ever. */ +function blackHole() { + const held: net.Socket[] = []; + const server = net.createServer((sock) => { sock.on('error', () => {}); held.push(sock); }); + return { + listen: () => new Promise((resolve) => { + server.listen(0, '127.0.0.1', () => resolve((server.address() as net.AddressInfo).port)); + }), + close: () => { held.forEach((s) => s.destroy()); server.close(); }, + }; +} + +const drivers: SqlDriver[] = []; +const make = (cfg: any) => { const d = new SqlDriver(cfg); drivers.push(d); return d; }; + +afterEach(async () => { + await Promise.all(drivers.splice(0).map((d) => d.disconnect().catch(() => {}))); +}); + +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); + }); + + it('applies it alongside a host pool config without clobbering min/max', () => { + const d = make({ + client: 'pg', + connection: 'postgres://u:p@127.0.0.1:1/d', + pool: { min: 0, max: 5 }, + }); + const pool = (d as any).knex.client.config.pool; + expect(pool).toMatchObject({ min: 0, max: 5, createTimeoutMillis: 10_000 }); + }); + + it("leaves a host's explicit createTimeoutMillis alone", () => { + const d = make({ + client: 'pg', + connection: 'postgres://u:p@127.0.0.1:1/d', + pool: { min: 0, max: 5, createTimeoutMillis: 45_000 }, + }); + expect((d as any).knex.client.config.pool.createTimeoutMillis).toBe(45_000); + }); + + 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. + const d = make({ + client: 'pg', + connection: `postgres://u:p@127.0.0.1:${port}/d`, + pool: { min: 0, max: 2, createTimeoutMillis: 700 }, + }); + const started = Date.now(); + await expect((d as any).knex.raw('SELECT 1')).rejects.toThrow(); + expect(Date.now() - started).toBeLessThan(5_000); + } finally { + bh.close(); + } + }, 20_000); + + it('does not disturb a working sqlite connection', async () => { + const d = make({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + await expect(d.checkHealth()).resolves.toBe(true); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index f0c6674b14..33e4725d1c 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -569,10 +569,42 @@ export class SqlDriver implements IDataDriver { this.schemaMode = schemaMode ?? 'managed'; this.autoMigrate = autoMigrate ?? 'off'; this.config = knexConfig; - this.knex = knex(knexConfig); + this.knex = knex(SqlDriver.withConnectBound(knexConfig)); this.installQueryTiming(); } + /** + * Default bound on establishing ONE connection (framework#3769). + * + * 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 — is the failure mode that hurts most, because nothing fails: + * the query just waits. Left to tarn's default that wait is **30 seconds**, + * per query, on the request path; with a small `pool.max` a handful of them + * saturate the pool and everything queues behind it. + * + * 10s matches driver-mongodb's existing `connectTimeoutMS ?? 10_000`, so both + * drivers give up on an unreachable server at the same point. A host that + * 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. + */ + private static readonly DEFAULT_CREATE_TIMEOUT_MS = 10_000; + + 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 }, + }; + } + /** * Per-request SQL query timing (perf-tuning mode). Correlates knex's * `query` → `query-response` / `query-error` events by `__knexQueryUid` and diff --git a/packages/types/src/env.ts b/packages/types/src/env.ts index c507351260..9b810fcd5e 100644 --- a/packages/types/src/env.ts +++ b/packages/types/src/env.ts @@ -179,9 +179,11 @@ export function resolveAllowDegradedTenancy(): boolean { * * Setting this to a truthy value (`true`/`1`/`on`/`yes`, case-insensitive) * boots anyway, in an explicitly degraded state that is logged loudly at - * startup. There is NO reconnection: the drivers that failed stay dead for the - * process lifetime and every query routed to them fails. Defaults OFF — an - * unset flag means "fail fast". + * startup. Every query routed to a failed driver fails until the datasource + * becomes reachable — the underlying clients do re-establish connections on + * their own (framework#3759) — but the boot-time schema sync those drivers + * missed is never re-run, so their tables may simply not exist afterwards. + * Defaults OFF — an unset flag means "fail fast". */ export function resolveAllowDriverConnectFailure(): boolean { const raw = readEnvWithDeprecation('OS_ALLOW_DRIVER_CONNECT_FAILURE', [], { silent: true });