From 415027e64dda7fc4627f2e1cb151b91e500851ab Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 00:16:01 +0000 Subject: [PATCH] feat(datasource): size the SQL pool from OS_DATABASE_POOL_MAX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildSqlPool` gives every postgres/mysql datasource that declares no `pool` an explicit `{min:0,max:5}`. The primary datasource — the one behind `OS_DATABASE_URL` — is composed as a url and nothing else, so that `max: 5` was the per-replica ceiling on every self-hosted deployment with no operator knob for it. A driver-level env read would have been dead code behind this function's explicit object. Precedence: a declared `pool.max` > `OS_DATABASE_POOL_MAX` > today's `5`. With the env unset nothing changes, which is the upgrade path for every existing deployment and is pinned as such. A non-integer value refuses the boot naming the variable, the value and the sizing rule. Only the postgres/mysql arms call `buildSqlPool`, so the unsupported arms (`memory` / `sqlite` / `sqlite-wasm` / `turso`) structurally cannot see it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .changeset/primary-datasource-pool-max-env.md | 45 ++++++ .../docs/deployment/environment-variables.mdx | 1 + .../default-datasource-pool-env.test.ts | 149 ++++++++++++++++++ .../src/default-datasource-driver-factory.ts | 79 +++++++++- 4 files changed, 272 insertions(+), 2 deletions(-) create mode 100644 .changeset/primary-datasource-pool-max-env.md create mode 100644 packages/services/service-datasource/src/__tests__/default-datasource-pool-env.test.ts diff --git a/.changeset/primary-datasource-pool-max-env.md b/.changeset/primary-datasource-pool-max-env.md new file mode 100644 index 0000000000..2fc7e6ee67 --- /dev/null +++ b/.changeset/primary-datasource-pool-max-env.md @@ -0,0 +1,45 @@ +--- +"@objectstack/service-datasource": minor +--- + +feat(datasource): size the SQL connection pool from `OS_DATABASE_POOL_MAX` + +A multi-replica deployment had no supported way to raise the number of database +connections each replica opens. The reporter measured a live 3-replica cluster +whose authed data API plateaued at ~25 rps while Postgres held only ~9-21 of its +200 connections: the ceiling was the client pool, and the operator had no knob +for it. Adding replicas raised the ceiling; sizing the pool — the cheap half — +was not expressible at all. + +The pool for a `postgres` / `mysql` datasource is built by `buildSqlPool`, which +gives every datasource that declares no `pool` block an explicit `{min: 0, +max: 5}`. That includes the primary datasource: the one behind +`OS_DATABASE_URL` is composed as a url and nothing else, so `max: 5` per replica +was the effective ceiling on every self-hosted deployment, and it was reachable +only by hand-authoring a `pool` block onto a datasource the operator does not +write. + +`buildSqlPool` now reads `OS_DATABASE_POOL_MAX`. Precedence is a declared +`pool.max` first, then the env, then today's `5` — an operator knob does not +override what an author wrote about their own datasource, and it is the only +site that decides the unspecified case, so the ordering is expressed once. + +**Nothing changes when the variable is unset**, which is the upgrade path for +every existing deployment: the pool stays exactly `{min: 0, max: 5}`, pinned by +a test whose job is to stay red if that ever drifts. A blank value is read as +unset, so a declared-but-unfilled compose variable keeps today's behaviour too. + +A value that is not a positive integer refuses the boot, naming the variable, +the value it rejected and the sizing rule — rather than the lenient +`Number(process.env.X ?? default)` shape, where a typo becomes `NaN` and the +operator who was trying to raise the ceiling silently keeps the one they meant +to leave. A pool ceiling is only ever measured in production. + +Size it with `replicas × OS_DATABASE_POOL_MAX` below the database's +`max_connections`, leaving headroom for migrations and admin connections. + +Only `postgres` / `mysql` are affected — they are the two arms that build a +pool. `memory` / `sqlite` / `sqlite-wasm` / `turso` receive no pool parameter +and reject a declared one outright; the env is read inside a function those arms +never call, so it cannot reach them. `OS_DATABASE_POOL_MIN` is deliberately not +exposed: this path already runs `min: 0`. diff --git a/content/docs/deployment/environment-variables.mdx b/content/docs/deployment/environment-variables.mdx index 6a653457d2..a7939d36c9 100644 --- a/content/docs/deployment/environment-variables.mdx +++ b/content/docs/deployment/environment-variables.mdx @@ -49,6 +49,7 @@ read at startup unless noted otherwise. Boolean variables accept `true` / `false | `OS_DATABASE_AUTH_TOKEN` | string | — | Auth token for a libSQL/Turso connection (`--database-auth-token`). The vendor's own `TURSO_AUTH_TOKEN` is read as a fallback and is **not** renamed (see the third-party names note above). Ignored by every other driver — their credentials live in the URL. | | `OS_DATABASE_DRIVER` | enum | inferred | Force a specific driver when the URL is ambiguous. `memory` \| `sqlite` \| `sqlite-wasm` \| `postgres` \| `mysql` \| `mongodb` \| `turso`. `mysql` is a supported deployment target that carries three dialect caveats — two of them integrity guarantees MySQL cannot enforce at the database. Read [MySQL dialect caveats](/docs/data-modeling/drivers#mysql-dialect-caveats) before choosing it. | | `OS_DATABASE_SQLITE_JOURNAL_MODE` | enum | `wal` | Journal mode for **file-backed** SQLite. `wal` (default) lets a dev server and CLI commands share one file without blocking each other, and is what makes the `os migrate` occupancy check reliable. Set to `delete` for SQLite's rollback journal — required when the database lives on a **network filesystem** (NFS/SMB), where WAL cannot work. The setting is applied, not merely skipped: `delete` converts a database that already adopted WAL back. Ignored for `:memory:`, for the WASM SQLite driver, and for non-SQLite drivers. A per-datasource `sqliteJournalMode` in driver config outranks it. See [Journal mode](/docs/data-modeling/drivers#journal-mode-wal-and-cross-process-access). | +| `OS_DATABASE_POOL_MAX` | integer | `5` | Maximum pooled connections **each replica** opens to `postgres` / `mysql`. The pool, not the database, is usually what caps authed throughput on a multi-replica deployment: at the default of 5 a 3-replica cluster reaches at most ~15 connections, which can plateau the data API while the database still has most of its `max_connections` free. Size it so **`replicas × OS_DATABASE_POOL_MAX` stays below the database's `max_connections`**, leaving headroom for migrations and admin connections — e.g. 3 replicas × 40 = 120 against `max_connections=200`. A per-datasource `pool.max` in the datasource's own definition outranks it, and with the variable unset nothing changes. A value that is not a positive integer refuses the boot rather than falling back to the default. Ignored by `memory` / `sqlite` / `sqlite-wasm` / `turso`, which open no pool — declaring a `pool` block on those is an error, not a silent drop. | | `OS_ALLOW_DRIVER_CONNECT_FAILURE` | boolean | `false` | Escape hatch for the driver-connect boot guard. By default a data driver that fails to connect at startup **refuses the boot** — a server that cannot reach its database must not report itself started and then fail every request. The same guard covers a **declared datasource** that objects bind to via `datasource: '…'`, or an `external` one with `validation.onMismatch: 'fail'`: those objects have no fallback datasource, so an unconnected one means they are all dead. Set to `1` to boot anyway, in an explicitly degraded state logged loudly at startup. There is **no reconnection**: whatever failed stays dead for the process lifetime and every query and schema sync routed to it fails. | | `OS_STORAGE_LOCAL_ROOT` | path | `./.objectstack/data/uploads` | Root directory for the local file storage adapter, relative to the process cwd (used by `os serve`'s default `storage` capability wiring). This is the same value as **Setup → Settings → File Storage → Root directory**; setting it here pins that field (it shows as locked-by-env). Renamed from `OS_STORAGE_ROOT` — see below. | | `OS_STORAGE_ROOT` | path | — | **Deprecated alias for `OS_STORAGE_LOCAL_ROOT`.** Still read for one release, with a startup warning; it will be removed in a future major. Rename it now. Before the rename the two halves of the platform spelled this value differently — the CLI wrote `OS_STORAGE_ROOT` while the settings service read `OS_STORAGE_LOCAL_ROOT` — so **any value other than the default was silently discarded** at startup and uploads landed in `./.objectstack/data/uploads` regardless. If you set `OS_STORAGE_ROOT` on an older release, check where your uploads actually are before assuming a backup covered them. | diff --git a/packages/services/service-datasource/src/__tests__/default-datasource-pool-env.test.ts b/packages/services/service-datasource/src/__tests__/default-datasource-pool-env.test.ts new file mode 100644 index 0000000000..9efeca8f31 --- /dev/null +++ b/packages/services/service-datasource/src/__tests__/default-datasource-pool-env.test.ts @@ -0,0 +1,149 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #14176 — the primary datasource's knex pool had no operator-facing size. +// +// What the reporter measured (a live 3-replica EE cluster, Postgres 16 with +// `max_connections=200`, 2026-09-01) and what this file does NOT re-measure: +// authed data throughput plateaued at ~25 rps while Postgres held ~9-21 of its +// 200 connections, i.e. the ceiling was the client pool, not the server. Those +// are the REPORTER'S numbers. No test here reproduces them — there is no +// cluster and no live database in this suite, and a fabricated local rerun +// would be evidence of nothing. What is pinnable here is the MECHANISM the +// throughput number rests on: which pool size actually reaches knex. +// +// The card blamed `SqlDriver.withConnectBound` for setting no pool size. That +// is falsified for this path and the pins below say why: the CLI composes the +// primary datasource as `config: { url, ...autoMigrate }` with no `pool` block +// (`packages/cli/src/utils/storage-driver.ts`, the `postgres` arm), so +// `buildSqlPool` — not knex's own `{min:2,max:10}` default, and not the driver +// — decides the size, and it decided `{min:0,max:5}` for every deployment. +// An env read in the driver would have been dead code behind that explicit +// object. Maintainer ruling 2026-09-02 (「同意」, option A): read one variable, +// `OS_DATABASE_POOL_MAX`, in `buildSqlPool`, precedence declared `pool` > env > +// `{min:0,max:5}`. +// +// ⚠️ The first pin below is the load-bearing one. An unset env is the upgrade +// path for every existing deployment, so "unset means exactly what it meant +// before" is the whole safety argument for this change; if it ever goes red, +// the change is silently altering production connection counts on upgrade. + +import { describe, it, expect, afterEach } from 'vitest'; +import { createDefaultDatasourceDriverFactory } from '../default-datasource-driver-factory.js'; + +const factory = () => createDefaultDatasourceDriverFactory({ dev: false }); + +const POOL_MAX_ENV = 'OS_DATABASE_POOL_MAX'; + +/** The knex config a constructed SqlDriver was built from (as the sibling suite reads it). */ +function knexConfigOf(driver: any): any { + return driver?.config ?? driver?.knexConfig ?? driver?.options ?? {}; +} + +/** + * Build a datasource the way the CLI composes the PRIMARY one — a url and + * nothing else, in particular no `pool` block — and return its knex pool. + * Never connects: `create` constructs the driver, and the pool is read off the + * config it was built from. + */ +async function primaryPool( + extra: Record = {}, +): Promise> { + const handle: any = await factory().create({ + driver: 'postgres', + config: { url: 'postgres://app@db.internal:5432/app' }, + ...extra, + }); + try { + return knexConfigOf(handle.driver ?? handle).pool; + } finally { + try { await handle.disconnect?.(); } catch { /* pool never opened */ } + } +} + +function setEnv(value: string | undefined): void { + if (value === undefined) delete process.env[POOL_MAX_ENV]; + else process.env[POOL_MAX_ENV] = value; +} + +afterEach(() => { delete process.env[POOL_MAX_ENV]; }); + +describe('OS_DATABASE_POOL_MAX — the primary datasource pool ceiling (#14176)', () => { + // ⭐ THE DEFAULT-PRESERVATION PIN. Do not relax this one. + it('with the env UNSET, the pool is byte-identical to the pre-change default', async () => { + setEnv(undefined); + expect(await primaryPool()).toEqual({ min: 0, max: 5 }); + }); + + it('reads a blank value as unset rather than as garbage', async () => { + // `OS_DATABASE_POOL_MAX=` in a compose file is a declared-but-unfilled + // variable asking for today's behaviour, not a misconfiguration. + setEnv(''); + expect(await primaryPool()).toEqual({ min: 0, max: 5 }); + setEnv(' '); + expect(await primaryPool()).toEqual({ min: 0, max: 5 }); + }); + + it('raises the ceiling the operator asked for, leaving the floor alone', async () => { + setEnv('40'); + // The floor stays 0 — `OS_DATABASE_POOL_MIN` is deliberately NOT exposed + // (ruling 2026-09-02: today's path runs `min: 0`; a later patch if needed). + expect(await primaryPool()).toEqual({ min: 0, max: 40 }); + }); + + it("a datasource's own declared pool outranks the operator env", async () => { + setEnv('40'); + const pool = await primaryPool({ pool: { min: 2, max: 9 } }); + expect(pool).toMatchObject({ min: 2, max: 9 }); + }); + + it('refuses a non-integer value loudly instead of silently keeping the default', async () => { + // The failure this rejects is the lenient `Number(process.env.X ?? d)` + // shape: a typo becomes NaN, knex gets an unsizable pool, and the operator + // who was TRYING to raise the ceiling silently keeps the one they wanted + // to leave. A pool ceiling is only ever measured in production. + for (const bad of ['abc', '10.5', '-4', '0', '1e3', '0x10', '10 20']) { + setEnv(bad); + await expect( + factory().create({ driver: 'postgres', config: { url: 'postgres://app@db.internal:5432/app' } }), + ).rejects.toThrow(/OS_DATABASE_POOL_MAX must be a positive integer/); + } + }); + + it('names the variable, the value it rejected and the sizing rule in the refusal', async () => { + setEnv('lots'); + await expect( + factory().create({ driver: 'postgres', config: { url: 'postgres://app@db.internal:5432/app' } }), + ).rejects.toThrow(/got "lots"[\s\S]*max_connections/); + }); + + it('applies to the mysql arm on the same terms', async () => { + setEnv('12'); + const handle: any = await factory().create({ + driver: 'mysql', + config: { url: 'mysql://app@db.internal:3306/app' }, + }); + try { + expect(knexConfigOf(handle.driver ?? handle).pool).toEqual({ min: 0, max: 12 }); + } finally { + try { await handle.disconnect?.(); } catch { /* pool never opened */ } + } + }); + + // ⛔ The guard for the constraint this change must not breach. `sqlite` / + // `sqlite-wasm` / `memory` / `turso` REJECT a declared `pool` outright under + // three maintainer rulings (#5714 / #5931 / #7243), and knex's better-sqlite3 + // dialect pins `{min:1,max:1}` on purpose — two connections to `:memory:` are + // two separate databases. The env is read inside `buildSqlPool`, which only + // the `postgres` / `mysql` arms call, so those arms structurally cannot see + // it. This pin is what makes "structurally" a measurement. + it('does not leak a pool onto an arm that refuses to be pooled', async () => { + setEnv('40'); + const handle: any = await factory().create({ driver: 'sqlite', config: { filename: ':memory:' } }); + try { + const cfg = knexConfigOf(handle.driver ?? handle); + expect(cfg.pool?.max).not.toBe(40); + } finally { + try { await handle.disconnect?.(); } catch { /* pool never opened */ } + } + }); +}); diff --git a/packages/services/service-datasource/src/default-datasource-driver-factory.ts b/packages/services/service-datasource/src/default-datasource-driver-factory.ts index 7470571c30..5efad54874 100644 --- a/packages/services/service-datasource/src/default-datasource-driver-factory.ts +++ b/packages/services/service-datasource/src/default-datasource-driver-factory.ts @@ -614,6 +614,60 @@ function buildSqlConnection(spec: DatasourceConnectionSpec, client: 'pg' | 'bett }; } +/** + * The operator-facing ceiling for a factory-built SQL datasource's knex pool. + * + * Named per AGENTS.md Prime Directive #9 (`OS_{DOMAIN}_{NAME}`): the domain is + * `DATABASE`, which is the family this joins (`OS_DATABASE_URL`, + * `OS_DATABASE_DRIVER`, `OS_DATABASE_SQLITE_JOURNAL_MODE`). `OS_DB_*` is not a + * prefix this repo uses. + */ +const POOL_MAX_ENV = 'OS_DATABASE_POOL_MAX'; + +/** + * The pool a `postgres` / `mysql` datasource gets when it declares no `pool` + * and the operator sets no env. ⛔ Do not move these: an unset env is the + * upgrade path for every existing deployment, and changing either number here + * changes how many connections every running cluster opens on upgrade. + */ +const DEFAULT_SQL_POOL_MIN = 0; +const DEFAULT_SQL_POOL_MAX = 5; + +function invalidPoolMaxMessage(raw: string): string { + return ( + `${POOL_MAX_ENV} must be a positive integer, got ${JSON.stringify(raw)}. ` + + 'It caps the connections each replica opens to postgres / mysql; size it so ' + + `replicas × ${POOL_MAX_ENV} stays below the database's max_connections, leaving ` + + 'headroom for migrations and admin connections. ' + + `Unset ${POOL_MAX_ENV} to keep the default of ${DEFAULT_SQL_POOL_MAX}.` + ); +} + +/** + * `OS_DATABASE_POOL_MAX`, or `undefined` when the operator set nothing. + * + * Strict on purpose, and loud rather than lenient: the repo's older numeric env + * reads are `Number(process.env.X ?? default)`, which turns a typo into a + * silent `NaN` and hands knex a pool it cannot size. A pool ceiling is measured + * only in production, so a value that does not parse must refuse the boot + * instead of quietly reverting to the default the operator was trying to raise. + * Blank is read as unset, not as garbage — `OS_DATABASE_POOL_MAX=` in a compose + * file is a declared-but-unfilled variable, and refusing that would break boots + * that are asking for today's behaviour. + */ +function readPoolMaxEnv(): number | undefined { + const raw = process.env[POOL_MAX_ENV]; + if (raw === undefined) return undefined; + const trimmed = raw.trim(); + if (trimmed === '') return undefined; + // Reject before Number(): it accepts '0x10', '1e3', ' 12 ' and '12.0', none of + // which an operator meant to write as a connection count. + if (!/^[0-9]+$/.test(trimmed)) throw new Error(invalidPoolMaxMessage(raw)); + const value = Number(trimmed); + if (!Number.isSafeInteger(value) || value < 1) throw new Error(invalidPoolMaxMessage(raw)); + return value; +} + /** * Knex pool options for a SQL driver, from the datasource's own `pool` block. * @@ -622,12 +676,33 @@ function buildSqlConnection(spec: DatasourceConnectionSpec, client: 'pg' | 'bett * hardcoded `{ min: 0, max: 5 }` over the top, so an author who sized their pool * got the defaults and no indication. Those defaults are preserved for the * unspecified case, so nothing that did not set `pool` changes behaviour. + * + * ## Why the env is read HERE and not in the driver (#14176) + * + * The primary datasource — the one behind `OS_DATABASE_URL` — is composed by + * the CLI as `config: { url, ...autoMigrate }` with **no `pool` block**, so the + * `max: 5` fallback on this line is the number that reaches knex on every + * self-hosted deployment. `SqlDriver` therefore never sees an "unspecified" + * pool from this path: an env read in the driver would be dead code, because + * this function's explicit `{min, max}` always wins. This is the only site that + * decides the unspecified case, which is why it is the only site that needs to + * know the env exists — the precedence below is expressed once, here. + * + * Precedence: a declared `pool.max` > `OS_DATABASE_POOL_MAX` > `5`. An operator + * knob must not override what an author wrote about their own datasource, and + * with the env unset nothing changes at all. + * + * ⛔ Reaches only `postgres` / `mysql` — the two arms that call this function. + * `memory` / `sqlite` / `sqlite-wasm` / `turso` receive no pool parameter at all + * and reject a declared one outright (`POOL_UNSUPPORTED_DRIVER_IDS`, rulings + * #5714 / #5931 / #7243); the env cannot change that, because it is read inside + * a function those arms never call. */ function buildSqlPool(spec: DatasourceConnectionSpec): Record { const pool = (spec.pool ?? {}) as Record; return { - min: typeof pool.min === 'number' ? pool.min : 0, - max: typeof pool.max === 'number' ? pool.max : 5, + min: typeof pool.min === 'number' ? pool.min : DEFAULT_SQL_POOL_MIN, + max: typeof pool.max === 'number' ? pool.max : (readPoolMaxEnv() ?? DEFAULT_SQL_POOL_MAX), ...(typeof pool.idleTimeoutMillis === 'number' ? { idleTimeoutMillis: pool.idleTimeoutMillis } : {}), ...(typeof pool.connectionTimeoutMillis === 'number' ? { acquireTimeoutMillis: pool.connectionTimeoutMillis }