From 8412cd8b499f1de4ea903fa2092e88b103ac77ee Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 12:53:39 +0000 Subject: [PATCH] fix(service-datasource): reject a `pool` block the sqlite arms cannot honour instead of dropping it (#5714) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `datasource.pool` reached a driver only from the arms that build a pooled client (`postgres` / `mysql` via `buildSqlPool`, `mongo` via `minPoolSize`/`maxPoolSize`). The `sqlite` and `sqlite-wasm` arms passed no pool at all, so an author who sized their pool got the driver's own single connection with no indication: sqlite + pool{min:3,max:9} knex.client.config.pool {"createTimeoutMillis":15000} live {min:1,max:1} postgres + pool{min:3,max:9} knex config.pool {"min":3,"max":9} live {min:3,max:9} Wiring it through would be wrong rather than merely more work: knex's better-sqlite3 dialect pins {min:1,max:1} on purpose, because two connections to `:memory:` are two separate, mutually invisible databases. So the declaration is rejected — maintainer ruling on #5714, option B. New `datasource-pool-support.ts` holds the predicate and the message (a fix instruction: delete the block; no escape hatch, no "change your driver"), and three doors enforce it: - boot auto-connect (`connectDeclared`) refuses before any connection is attempted, naming every offender in one throw. It is an AUTHORING verdict, never routed through `handleFailure`, so OS_ALLOW_DRIVER_CONNECT_FAILURE does not apply and is not suggested. `active: false` is skipped. - the Setup wizard (`createDatasource`/`updateDatasource`) rejects the draft before the record is stored, with the same "only when this write touches the pairing" carve-out the #4410 config gate uses. - the driver factory rejects it as the last door. `examples/app-crm` (the live specimen) and `examples/app-showcase` drop their inert declarations; `content/docs/data-modeling/drivers.mdx` no longer claims `pool` is honoured by every SQL driver. The `memory` arm reads no pool either and is deliberately NOT in the rejected set — widening the ruling's authoring-surface tightening is a triage decision; filed as #5931 and named in the module comment. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015a5qkLzpGXhLL2F5gvJ7dD --- .../datasource-sqlite-pool-loud-reject.md | 75 ++++ content/docs/data-modeling/drivers.mdx | 13 +- .../app-crm/src/datasources/crm.datasource.ts | 10 +- .../src/system/datasources/index.ts | 10 +- .../__tests__/datasource-pool-support.test.ts | 365 ++++++++++++++++++ .../src/datasource-admin-service.ts | 15 + .../src/datasource-connection-service.ts | 56 +++ .../src/datasource-pool-support.ts | 138 +++++++ .../src/default-datasource-driver-factory.ts | 9 + .../services/service-datasource/src/index.ts | 12 + 10 files changed, 695 insertions(+), 8 deletions(-) create mode 100644 .changeset/datasource-sqlite-pool-loud-reject.md create mode 100644 packages/services/service-datasource/src/__tests__/datasource-pool-support.test.ts create mode 100644 packages/services/service-datasource/src/datasource-pool-support.ts diff --git a/.changeset/datasource-sqlite-pool-loud-reject.md b/.changeset/datasource-sqlite-pool-loud-reject.md new file mode 100644 index 0000000000..2343f67d72 --- /dev/null +++ b/.changeset/datasource-sqlite-pool-loud-reject.md @@ -0,0 +1,75 @@ +--- +"@objectstack/service-datasource": minor +--- + +fix(service-datasource): a `pool` block on a sqlite datasource is rejected, not dropped in silence (#5714) + +`datasource.pool` is declared, strict and documented, and until now it reached a +driver only from the arms that build a pooled client: `postgres` / `mysql` hand +`buildSqlPool(spec)` to `SqlDriver`, `mongo` maps `min`/`max` onto the client's +`minPoolSize`/`maxPoolSize`. The `sqlite` and `sqlite-wasm` arms passed no pool +at all — `resolveSqliteDriver` has no such option and `SqliteWasmDriver` does +not take one — so an author who sized their pool got the driver's own single +connection and nothing said otherwise. Measured through the real factory: + +```text +sqlite + pool{min:3,max:9} knex.client.config.pool {"createTimeoutMillis":15000} live {min:1,max:1} +postgres + pool{min:3,max:9} knex config.pool {"min":3,"max":9} live {min:3,max:9} +``` + +`examples/app-crm` was the live specimen: `CrmDatasource` asked for +`{ min: 1, max: 5 }` and ran on one connection. + +**Wiring it through would be wrong, not merely more work.** Knex's +better-sqlite3 dialect pins `{min:1,max:1}` on purpose: every pool acquire runs +`new Database(filename)`, so two connections to `:memory:` are two separate, +mutually invisible databases. Honouring `max: 5` there would split one +datasource's data across five stores. Sizing a SQLite pool is not a knob the +platform can offer, so the declaration is rejected at authoring/publish instead +— Prime Directive #12: fix the metadata at the producer, reject it loudly, never +tolerate it in the consumer. + +**Observable behaviour change — read this if any datasource declares `pool`.** +A `sqlite` / `sqlite-wasm` datasource carrying a `pool` block now **fails** +where it used to boot with the block ignored: + +- **Boot** (`DatasourceConnectionService.connectDeclared`) refuses before a + single connection is attempted, naming every offending datasource in one + throw. Every *declared, active* datasource is judged, including the ones the + ADR-0062 D2 gate leaves unconnected — a pool block on a datasource nobody + connects is exactly as dropped as a connected one's. `active: false` is + skipped, so switching a datasource off remains the way out. +- **Setup → Datasources** (`createDatasource` / `updateDatasource`) rejects the + draft before the record is stored. An update that touches neither `pool` nor + `driver` is not re-judged, so a record written before this gate stays editable + — including the `active: false` that takes it out of service. +- **The driver factory** (`createDefaultDatasourceDriverFactory`) rejects it as + the last door, for hosts that build drivers directly. + +The fix is to delete the block: `pool` is a no-op on SQLite either way, so +removing it changes nothing about how the datasource runs. + +```diff + export const CrmDatasource = defineDatasource({ + name: 'crm_primary', + driver: 'sqlite', + config: { filename: ':memory:' }, +- pool: { min: 1, max: 5 }, + active: true, + }); +``` + +`pool` is unchanged and still honoured on `postgres` / `mysql` / `mongo`, and a +plugin-contributed driver id (`com.vendor.snowflake`) is not judged at all — +the same boundary the `datasource.config` gate draws in #4410: the platform +validates what it can construct. + +This verdict is an **authoring** error, not a connect failure: it never goes +through the ADR-0062 D5 degradation path, so `OS_ALLOW_DRIVER_CONNECT_FAILURE` +does not apply to it and is not suggested. That hatch exists for a database that +is unreachable — a fact about the world that may resolve itself. A `pool` the +driver cannot read is a fact about the metadata. + +Hosts that inject their own driver factory can hold the same contract with the +newly exported `assertDatasourcePoolSupported` / `driverReadsDeclaredPool` / +`unsupportedPoolIssue` / `POOL_UNSUPPORTED_DRIVER_IDS`. diff --git a/content/docs/data-modeling/drivers.mdx b/content/docs/data-modeling/drivers.mdx index c64fceffd9..be6116f717 100644 --- a/content/docs/data-modeling/drivers.mdx +++ b/content/docs/data-modeling/drivers.mdx @@ -117,8 +117,17 @@ save, the connection probe — reported success. Two things live **outside** `config`, because they are not driver-specific: - **Pool sizing** — the `pool` block on the datasource (`min`, `max`, - `idleTimeoutMillis`, `connectionTimeoutMillis`), honoured for every SQL driver - and mapped onto the Mongo client's `minPoolSize` / `maxPoolSize`. + `idleTimeoutMillis`, `connectionTimeoutMillis`), honoured by the pooled + drivers: `postgres` and `mysql` pass it to Knex, `mongo` maps `min` / `max` + onto the client's `minPoolSize` / `maxPoolSize`. Declaring it on a **sqlite** + or **sqlite-wasm** datasource is rejected — by the Setup wizard when you save, + and by the boot when a declared datasource carries one: a SQLite connection + strategy is owned by the driver (one connection per database, because a second + connection to `:memory:` opens a separate, empty one), so a pool declared + there could never take effect. It used to be dropped in silence — an `app-crm` + datasource asking for `max: 5` measurably ran on one connection + ([#5714](https://github.com/objectstack-ai/objectstack/issues/5714)). The fix + is to delete the block; it is a no-op on SQLite either way. - **TLS certificates** — the `ssl` block on the datasource (`enabled`, `rejectUnauthorized`, `ca`, `cert`, `key`). Inside `config`, `ssl` is the on/off boolean shorthand. diff --git a/examples/app-crm/src/datasources/crm.datasource.ts b/examples/app-crm/src/datasources/crm.datasource.ts index a3af9cc680..7f6b560285 100644 --- a/examples/app-crm/src/datasources/crm.datasource.ts +++ b/examples/app-crm/src/datasources/crm.datasource.ts @@ -5,6 +5,12 @@ import { defineDatasource } from '@objectstack/spec/data'; /** * Primary CRM datasource — in-memory SQLite for the example. * In production, swap `driver` to 'postgres' and supply real `config`. + * + * No `pool` block: SQLite's connection strategy is owned by the driver (one + * connection per database — a second connection to `:memory:` would open a + * separate, empty one). This example declared `pool: { min: 1, max: 5 }` and + * measurably ran on `{min:1,max:1}` with no indication at all, which is what + * #5714 turned from a silent drop into a loud rejection. */ export const CrmDatasource = defineDatasource({ name: 'crm_primary', @@ -13,10 +19,6 @@ export const CrmDatasource = defineDatasource({ config: { filename: ':memory:', }, - pool: { - min: 1, - max: 5, - }, active: true, }); diff --git a/examples/app-showcase/src/system/datasources/index.ts b/examples/app-showcase/src/system/datasources/index.ts index 2365e098d3..2afa1a5ed1 100644 --- a/examples/app-showcase/src/system/datasources/index.ts +++ b/examples/app-showcase/src/system/datasources/index.ts @@ -1,12 +1,18 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -/** Primary datasource — in-memory SQLite for the example. */ +/** + * Primary datasource — in-memory SQLite for the example. + * + * No `pool` block: a SQLite connection strategy is owned by the driver, so the + * `pool: { min: 1, max: 5 }` this used to declare reached nothing and the + * datasource ran on the dialect's single connection. Declaring it is a loud + * rejection since #5714 rather than a silent drop. + */ export const ShowcaseDatasource = { name: 'showcase_primary', label: 'Showcase Primary Database', driver: 'sqlite', config: { filename: ':memory:' }, - pool: { min: 1, max: 5 }, active: true, }; diff --git a/packages/services/service-datasource/src/__tests__/datasource-pool-support.test.ts b/packages/services/service-datasource/src/__tests__/datasource-pool-support.test.ts new file mode 100644 index 0000000000..162f9c2fa2 --- /dev/null +++ b/packages/services/service-datasource/src/__tests__/datasource-pool-support.test.ts @@ -0,0 +1,365 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #5714 — `datasource.pool` was honoured by the `postgres` / `mysql` / `mongo` +// arms and DROPPED IN SILENCE by `sqlite` / `sqlite-wasm`, which take no pool +// option at all. Measured on `origin/main` before this change: +// +// sqlite + pool{min:3,max:9} knex.client.config.pool {"createTimeoutMillis":15000} live {min:1,max:1} +// postgres + pool{min:3,max:9} knex config.pool {"min":3,"max":9} live {min:3,max:9} +// +// The maintainer ruling (2026-08-06, option B) is that a declaration either +// takes effect or is rejected out loud — never dropped. These tests pin the +// rejection at each door it can come in through, and pin that the arms which +// DO honour the block still do. + +import { describe, it, expect, vi } from 'vitest'; +import { + POOL_UNSUPPORTED_DRIVER_IDS, + driverReadsDeclaredPool, + unsupportedPoolIssue, + assertDatasourcePoolSupported, +} from '../datasource-pool-support.js'; +import { createDefaultDatasourceDriverFactory } from '../default-datasource-driver-factory.js'; +import { + DatasourceConnectionService, + type ConnectableDatasource, + type ConnectionEngineLike, +} from '../datasource-connection-service.js'; +import { DatasourceAdminService, type StoredDatasource } from '../datasource-admin-service.js'; +import type { IDatasourceDriverFactory } from '../contracts/datasource-driver-factory.js'; + +describe('#5714 — which driver arms read a declared `pool`', () => { + it('names exactly the two sqlite arms as unable to honour it', () => { + expect([...POOL_UNSUPPORTED_DRIVER_IDS]).toEqual(['sqlite', 'sqlite-wasm']); + }); + + it('rejects every spelling of the sqlite arms, case-insensitively', () => { + for (const id of ['sqlite', 'sqlite3', 'better-sqlite3', 'SQLite', 'sqlite-wasm', 'wasm-sqlite']) { + expect(driverReadsDeclaredPool(id), id).toBe(false); + } + }); + + it('leaves the pooled built-ins alone', () => { + for (const id of ['postgres', 'pg', 'postgresql', 'mysql', 'mysql2', 'mariadb', 'mongo', 'mongodb']) { + expect(driverReadsDeclaredPool(id), id).toBe(true); + } + }); + + // The same boundary the `datasource.config` gate draws (#4410): we judge what + // we can construct. A plugin driver may well pool, and rejecting a key + // against a contract we do not ship would be worse than the silence. + it('does not judge a driver id the platform ships no contract for', () => { + expect(driverReadsDeclaredPool('com.vendor.snowflake')).toBe(true); + expect(unsupportedPoolIssue({ driver: 'com.vendor.snowflake', pool: { min: 3, max: 9 } })).toBeUndefined(); + }); + + // Deliberate, and filed rather than silently widened: `memory` reads no pool + // either, but the #5714 ruling authorised this tightening for the sqlite arms + // only. #5931 carries the decision. + it('leaves `memory` out of the rejected set (#5931), deliberately', () => { + expect(driverReadsDeclaredPool('memory')).toBe(true); + }); + + it('treats an absent or empty block as no declaration', () => { + expect(unsupportedPoolIssue({ driver: 'sqlite' })).toBeUndefined(); + expect(unsupportedPoolIssue({ driver: 'sqlite', pool: {} })).toBeUndefined(); + expect(unsupportedPoolIssue({ driver: 'sqlite', pool: undefined })).toBeUndefined(); + }); + + it('names the datasource and the one edit that fixes it', () => { + const msg = unsupportedPoolIssue({ driver: 'sqlite', pool: { min: 1, max: 5 }, name: 'crm_primary' }); + expect(msg).toContain(`Datasource 'crm_primary'`); + expect(msg).toMatch(/Remove `pool` from this datasource declaration/); + expect(msg).toMatch(/postgres \/ mysql \/ mongo/); + }); + + // #5794's lesson: a fix instruction, never an escape-hatch instruction. An + // authoring mistake has a correction; suggesting an env var that boots past + // it (or a different driver) sends the author away from the fix. + it('offers no escape hatch and no "use another driver" advice', () => { + const msg = unsupportedPoolIssue({ driver: 'sqlite-wasm', pool: { max: 9 }, name: 'ds' }) ?? ''; + expect(msg).not.toMatch(/OS_ALLOW_DRIVER_CONNECT_FAILURE/); + expect(msg).not.toMatch(/OS_[A-Z_]*=1/); + expect(msg).not.toMatch(/switch|instead use|change the driver/i); + }); + + it('assert throws exactly when the issue is reported', () => { + expect(() => assertDatasourcePoolSupported({ driver: 'sqlite', pool: { max: 5 } })).toThrow(/does not read it/); + expect(() => assertDatasourcePoolSupported({ driver: 'postgres', pool: { max: 5 } })).not.toThrow(); + }); +}); + +// ── Door 1: the driver factory — the site that dropped it ──────────────────── +describe('#5714 — the driver factory rejects a pool it cannot honour', () => { + const factory = () => createDefaultDatasourceDriverFactory({ dev: false }); + + function knexConfigOf(driver: any): any { + return driver?.config ?? driver?.knexConfig ?? driver?.options ?? {}; + } + + it('sqlite + pool is rejected instead of built with the block dropped', async () => { + await expect( + factory().create({ + name: 'crm_primary', + driver: 'sqlite', + config: { filename: ':memory:' }, + pool: { min: 3, max: 9 }, + }), + ).rejects.toThrow(/Datasource 'crm_primary' declares a `pool` block/); + }); + + it('sqlite-wasm + pool is rejected the same way', async () => { + await expect( + factory().create({ + name: 'wasm_ds', + driver: 'sqlite-wasm', + config: { filename: ':memory:' }, + pool: { max: 9 }, + }), + ).rejects.toThrow(/does not read it/); + }); + + it('sqlite WITHOUT a pool still builds exactly as before', async () => { + const handle: any = await factory().create({ + name: 'crm_primary', + driver: 'sqlite', + config: { filename: ':memory:' }, + }); + const driver = handle.driver ?? handle; + expect(driver?.constructor?.name).toMatch(/SqlDriver$/); + try { await handle.disconnect?.(); } catch { /* pool never opened */ } + }); + + // The regression nails for the arms that DO honour it — the half of the + // contract this change must not disturb. + it('postgres still receives the declared pool', async () => { + const handle: any = await factory().create({ + driver: 'postgres', + config: { host: 'db.internal', database: 'analytics' }, + pool: { min: 3, max: 9, idleTimeoutMillis: 45_000 }, + }); + expect(knexConfigOf(handle.driver ?? handle).pool).toMatchObject({ min: 3, max: 9, idleTimeoutMillis: 45_000 }); + try { await handle.disconnect?.(); } catch { /* pool never opened */ } + }); + + it('mysql still receives the declared pool', async () => { + const handle: any = await factory().create({ + driver: 'mysql', + config: { url: 'mysql://user:pw@localhost:3306/db' }, + pool: { min: 3, max: 9 }, + }); + expect(knexConfigOf(handle.driver ?? handle).pool).toMatchObject({ min: 3, max: 9 }); + try { await handle.disconnect?.(); } catch { /* pool never opened */ } + }); +}); + +// ── Door 2: boot-time auto-connect ─────────────────────────────────────────── +function fakeEngine() { + const drivers = new Map(); + const engine: ConnectionEngineLike & { drivers: typeof drivers } = { + drivers, + registerDriver: (driver: any) => { drivers.set(driver.name, driver); }, + registerDatasourceDef: () => {}, + getDriverByName: (name) => drivers.get(name), + }; + return engine; +} + +function fakeFactory(): IDatasourceDriverFactory { + return { + supports: () => true, + create: vi.fn(async () => { + const driver: any = { name: 'com.fake.driver' }; + return { driver, connect: async () => { driver.connected = true; } }; + }), + }; +} + +function svc() { + const engine = fakeEngine(); + const factory = fakeFactory(); + const warnings: string[] = []; + const service = new DatasourceConnectionService({ + factory: () => factory, + engine: () => engine, + logger: { warn: (msg: string) => { warnings.push(msg); } }, + }); + return { service, engine, factory, warnings }; +} + +const sqliteWithPool: ConnectableDatasource = { + name: 'crm_primary', + driver: 'sqlite', + config: { filename: ':memory:' }, + pool: { min: 1, max: 5 }, +}; + +describe('#5714 — boot refuses a declared pool the driver cannot honour', () => { + it('throws before anything is connected, even for a datasource the D2 gate would skip', async () => { + const { service, factory, engine } = svc(); + // Managed + nothing bound: `isDatasourceAddressed` is false, so this + // datasource never reaches a connect. Its `pool` block is exactly as + // dropped as a connected one's — which is the app-crm specimen's shape. + await expect(service.connectDeclared({ datasources: [sqliteWithPool], objects: [] })) + .rejects.toThrow(/Datasource 'crm_primary' declares a `pool` block/); + expect((factory.create as any).mock.calls.length).toBe(0); + expect(engine.drivers.size).toBe(0); + }); + + // The verdict is about the metadata, not about the world, so the D5 + // degradation policy must not be able to swallow it. + it('is an authoring verdict, not a connect failure: no degradation escape hatch', async () => { + const { service } = svc(); + const err = await service + .connectDeclared({ datasources: [sqliteWithPool], objects: [] }) + .then(() => undefined, (e: Error) => e); + expect(err?.message).not.toMatch(/OS_ALLOW_DRIVER_CONNECT_FAILURE/); + expect(err?.message).not.toMatch(/connect failed/); + }); + + it('names every offender in one throw', async () => { + const { service } = svc(); + const err = await service + .connectDeclared({ + datasources: [sqliteWithPool, { name: 'wasm_ds', driver: 'sqlite-wasm', pool: { max: 4 } }], + objects: [], + }) + .then(() => undefined, (e: Error) => e); + expect(err?.message).toMatch(/2 declared datasource\(s\)/); + expect(err?.message).toContain(`Datasource 'crm_primary'`); + expect(err?.message).toContain(`Datasource 'wasm_ds'`); + }); + + // `active: false` is the operator's way to take a misconfigured datasource + // out of service. A boot that refuses to start over one already switched off + // would break the remedy itself. + it('skips a datasource that is switched off', async () => { + const { service } = svc(); + await expect( + service.connectDeclared({ datasources: [{ ...sqliteWithPool, active: false }], objects: [] }), + ).resolves.toEqual([]); + }); + + it('leaves a sqlite datasource with no pool block connecting as before', async () => { + const { service, engine } = svc(); + const results = await service.connectDeclared({ + datasources: [{ name: 'crm_primary', driver: 'sqlite', config: { filename: ':memory:' }, autoConnect: true }], + objects: [], + }); + expect(results.map((r) => r.status)).toEqual(['connected']); + expect(engine.drivers.has('crm_primary')).toBe(true); + }); + + it('leaves a postgres datasource WITH a pool block connecting as before', async () => { + const { service, engine } = svc(); + const results = await service.connectDeclared({ + datasources: [{ + name: 'reporting', + driver: 'postgres', + config: { host: 'db.internal', database: 'analytics' }, + pool: { min: 3, max: 9 }, + autoConnect: true, + }], + objects: [], + }); + expect(results.map((r) => r.status)).toEqual(['connected']); + expect(engine.drivers.has('reporting')).toBe(true); + }); + + // The runtime-admin path (`registerPool`) calls `connect()` directly rather + // than through the boot pre-pass, so it carries its own guard. + it('rejects on the direct connect() path too, without registering anything', async () => { + const { service, engine, factory } = svc(); + await expect( + service.connect(sqliteWithPool, { context: { origin: 'runtime', trigger: 'runtime-admin' } }), + ).rejects.toThrow(/declares a `pool` block/); + expect((factory.create as any).mock.calls.length).toBe(0); + expect(engine.drivers.size).toBe(0); + }); +}); + +// ── Door 3: the Setup wizard (runtime authoring) ───────────────────────────── +function adminHarness(seed: StoredDatasource[] = []) { + const records: StoredDatasource[] = seed.map((r) => ({ ...r })); + const registered: string[] = []; + const service = new DatasourceAdminService({ + probe: async () => ({ ok: true }), + listDatasourceRecords: async () => records.map((r) => ({ ...r })), + getDatasourceRecord: async (n) => { + const r = records.find((x) => x.name === n); + return r ? { ...r } : undefined; + }, + putDatasourceRecord: async (record) => { + const idx = records.findIndex((r) => r.name === record.name && r.origin === 'runtime'); + if (idx >= 0) records[idx] = { ...record }; + else records.push({ ...record }); + }, + deleteDatasourceRecord: async () => {}, + writeSecret: async () => 'sys_secret://x#1', + countBoundObjects: async () => 0, + registerPool: (record) => { registered.push(record.name); }, + }); + return { service, records, registered }; +} + +describe('#5714 — the Setup wizard rejects it before the record is stored', () => { + it('create: a sqlite draft carrying a pool never reaches the store', async () => { + const { service, records, registered } = adminHarness(); + await expect( + service.createDatasource({ + name: 'local_cache', + driver: 'sqlite', + config: { filename: ':memory:' }, + pool: { min: 1, max: 5 }, + }), + ).rejects.toThrow(/declares a `pool` block/); + expect(records).toHaveLength(0); + expect(registered).toHaveLength(0); + }); + + it('create: a postgres draft carrying a pool is stored as before', async () => { + const { service, records } = adminHarness(); + await service.createDatasource({ + name: 'reporting', + driver: 'postgres', + config: { database: 'analytics' }, + pool: { min: 3, max: 9 }, + }); + expect(records[0]?.pool).toEqual({ min: 3, max: 9 }); + }); + + it('update: patching a pool onto a stored sqlite datasource is rejected', async () => { + const { service } = adminHarness([ + { name: 'local_cache', driver: 'sqlite', config: { filename: ':memory:' }, origin: 'runtime' }, + ]); + await expect( + service.updateDatasource('local_cache', { pool: { min: 1, max: 5 } }), + ).rejects.toThrow(/declares a `pool` block/); + }); + + it('update: switching a pooled datasource TO sqlite is rejected on the merged record', async () => { + const { service } = adminHarness([ + { name: 'reporting', driver: 'postgres', config: {}, pool: { min: 3, max: 9 }, origin: 'runtime' }, + ]); + await expect( + service.updateDatasource('reporting', { driver: 'sqlite', config: { filename: ':memory:' } }), + ).rejects.toThrow(/declares a `pool` block/); + }); + + // A record written before this gate must stay editable — including the + // `active: false` that takes it out of service. Same carve-out the #4410 + // config gate makes, for the same reason. + it('update: a write that touches neither pool nor driver is not re-judged', async () => { + const { service } = adminHarness([ + { + name: 'local_cache', + driver: 'sqlite', + config: { filename: ':memory:' }, + pool: { min: 1, max: 5 }, + origin: 'runtime', + }, + ]); + const summary = await service.updateDatasource('local_cache', { active: false }); + expect(summary.active).toBe(false); + }); +}); diff --git a/packages/services/service-datasource/src/datasource-admin-service.ts b/packages/services/service-datasource/src/datasource-admin-service.ts index 09a12a64a1..29345036a4 100644 --- a/packages/services/service-datasource/src/datasource-admin-service.ts +++ b/packages/services/service-datasource/src/datasource-admin-service.ts @@ -22,6 +22,7 @@ */ import { validateDriverConfig } from '@objectstack/spec/data'; +import { assertDatasourcePoolSupported } from './datasource-pool-support.js'; import type { IDatasourceAdminService, DatasourceDraft, @@ -235,6 +236,11 @@ export class DatasourceAdminService implements IDatasourceAdminService { this.assertValidName(input?.name); if (!input.driver) throw new Error('A driver is required to create a datasource.'); this.assertValidConfig(input.driver, input.config); + // The wizard is a publish door for `pool` too (#5714). Rejected BEFORE the + // record is persisted: `tryRegisterPool` swallows its failures into a + // warning, so a datasource saved with an unhonourable pool would sit in the + // store with the block still in it, exactly as silently as before. + assertDatasourcePoolSupported({ driver: input.driver, pool: input.pool, name: input.name }); const existing = await this.config.getDatasourceRecord(input.name); if (existing) { @@ -298,6 +304,15 @@ export class DatasourceAdminService implements IDatasourceAdminService { if (patch.config !== undefined || patch.driver !== undefined) { this.assertValidConfig(merged.driver, merged.config); } + // Same judgement, same "only when this write touches the pairing" rule + // (#5714): a new `pool`, or a new `driver` that reinterprets the stored + // one. An edit that renames a datasource or flips `active` must not be + // blocked by a pool block it is not touching — otherwise a record written + // before this gate becomes uneditable, including the `active: false` that + // takes it out of service. + if (patch.pool !== undefined || patch.driver !== undefined) { + assertDatasourcePoolSupported({ driver: merged.driver, pool: merged.pool, name }); + } if (secret) { const prevRef = existing.external?.credentialsRef; diff --git a/packages/services/service-datasource/src/datasource-connection-service.ts b/packages/services/service-datasource/src/datasource-connection-service.ts index 77a0dda750..2500a6582d 100644 --- a/packages/services/service-datasource/src/datasource-connection-service.ts +++ b/packages/services/service-datasource/src/datasource-connection-service.ts @@ -39,6 +39,10 @@ import { type DatasourceConnectContext, type DatasourceConnectDecision, } from './contracts/connect-policy.js'; +import { + assertDatasourcePoolSupported, + unsupportedPoolIssue, +} from './datasource-pool-support.js'; import type { Logger } from './logger.js'; /** A datasource definition this service can connect (code- or runtime-origin). */ @@ -315,6 +319,7 @@ export class DatasourceConnectionService { }): Promise { const objects = input.objects ?? []; const mappedObjects = input.mappedObjects ?? {}; + this.assertDeclaredPoolsAreHonoured(input.datasources); const results: ConnectResult[] = []; const fatal: Error[] = []; for (const ds of input.datasources) { @@ -348,6 +353,46 @@ export class DatasourceConnectionService { return results; } + /** + * Reject every declared `pool` block the datasource's driver cannot honour, + * BEFORE a single connection is attempted (#5714). + * + * Three deliberate properties: + * + * - **It is an authoring verdict, not a connect failure.** It never goes + * through {@link handleFailure}, so the D5 degradation policy and its + * `OS_ALLOW_DRIVER_CONNECT_FAILURE` escape hatch do not apply and are not + * suggested: that hatch exists for a database that is unreachable — a fact + * about the world, which may resolve itself. A `pool` the driver cannot + * read is a fact about the metadata, and no env var should boot past it. + * - **Every declared datasource is judged, not just the connected ones.** + * The ADR-0062 D2 gate leaves a managed, unrouted datasource unconnected; + * its `pool` block is exactly as dropped as a connected one's, and + * `examples/app-crm`'s specimen was of precisely that shape. + * - **`active: false` is skipped.** That flag is the operator's way to take + * a misconfigured datasource out of service; a boot that refuses to start + * over a datasource already switched off would break the remedy itself. + * + * All offenders are reported in one throw, mirroring the aggregate connect + * failure below: one boot names everything to fix, not one per restart. + */ + private assertDeclaredPoolsAreHonoured(datasources: readonly ConnectableDatasource[]): void { + const issues: string[] = []; + for (const ds of datasources) { + if (!ds?.name || ds.active === false) continue; + const issue = unsupportedPoolIssue({ driver: ds.driver, pool: ds.pool, name: ds.name }); + if (issue) issues.push(issue); + } + if (issues.length === 1) throw new Error(issues[0]); + if (issues.length > 1) { + throw new Error( + `${issues.length} declared datasource(s) declare a \`pool\` block their driver cannot ` + + `honour — refusing to boot.\n` + + issues.map((m) => ` • ${m}`).join('\n'), + ); + } + } + /** * Build + connect + register a single datasource's live driver. The shared * core used by both auto-connect and the runtime-admin pool registration. @@ -450,6 +495,17 @@ export class DatasourceConnectionService { return { name, status: 'already-registered' }; } + // From here on THIS service is the thing that would build the driver, so it + // refuses to build from a declaration it cannot honour (#5714). Placed + // after the idempotency guard — a driver someone else already registered + // (the D8 `onEnable` escape hatch) is not this path's to re-judge — and + // before the policy gate, because an unhonourable `pool` is a property of + // the declaration rather than of the host's connect decision. Boot-declared + // datasources have already been judged in bulk by + // {@link assertDeclaredPoolsAreHonoured}; this covers the runtime-admin + // (`registerPool`) path and any host calling `connect()` directly. + assertDatasourcePoolSupported({ driver: record.driver, pool: record.pool, name }); + // Policy gate (fail-closed on throw). let decision: DatasourceConnectDecision; try { diff --git a/packages/services/service-datasource/src/datasource-pool-support.ts b/packages/services/service-datasource/src/datasource-pool-support.ts new file mode 100644 index 0000000000..b0a994fe38 --- /dev/null +++ b/packages/services/service-datasource/src/datasource-pool-support.ts @@ -0,0 +1,138 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Which driver arms actually READ `datasource.pool` — and the loud rejection + * for the ones that do not (#5714). + * + * ## The failure this exists for + * + * `datasource.pool` is declared, strict, documented, and honoured by exactly + * the arms that build a pooled client: `postgres` / `mysql` hand + * `buildSqlPool(spec)` to `SqlDriver`, and `mongo` maps `min`/`max` onto the + * MongoClient's `minPoolSize`/`maxPoolSize`. The `sqlite` and `sqlite-wasm` + * arms never received it at all — `resolveSqliteDriver` has no pool option and + * `SqliteWasmDriver` does not take one — so a datasource that sized its pool + * got the driver's own single connection and no indication whatsoever. + * Measured on `origin/main` before this module existed: + * + * ```text + * sqlite + pool{min:3,max:9} knex.client.config.pool = {"createTimeoutMillis":15000} live {min:1,max:1} + * postgres + pool{min:3,max:9} knex config.pool = {"min":3,"max":9} live {min:3,max:9} + * ``` + * + * `examples/app-crm` was the live specimen: `CrmDatasource` declared + * `pool: { min: 1, max: 5 }` and ran on `{min:1,max:1}`. + * + * ## Why rejection rather than wiring it up + * + * Wiring the block through to knex would be wrong, not merely more work: knex's + * better-sqlite3 dialect pins `{min:1,max:1}` **on purpose**, because every + * pool acquire runs `new Database(filename)` and two connections to `:memory:` + * are two SEPARATE, mutually invisible databases. Honouring `max: 5` there + * would silently split one datasource's data across five stores. Sizing a + * SQLite pool is not a knob the platform can offer, so the honest answer to a + * declaration it cannot serve is to reject it — Prime Directive #12: fix the + * metadata at the producer and reject it at authoring/publish, never tolerate + * it in the consumer. Maintainer ruling on #5714 (2026-08-06), option B. + * + * ## Where the boundary is, deliberately + * + * - A driver id the platform ships no contract for (`com.vendor.snowflake`) is + * NOT judged. Same line the `datasource.config` gate draws: "we validate what + * we can construct" — a plugin driver may well pool, and rejecting a key + * against a shape we do not have would be worse than the silence it replaces. + * - `memory` is a built-in that does not read `pool` either, and it is + * deliberately NOT in the rejected set: the #5714 ruling authorised this + * authoring-surface tightening for the two sqlite arms, and widening it is a + * contract decision for triage rather than for this module. Filed as #5931 so + * the hole is known rather than overlooked. + */ + +import { resolveDriverId } from '@objectstack/spec/data'; + +/** + * Canonical driver ids whose connection strategy is decided by the driver, so a + * declared `datasource.pool` can never reach anything. + * + * Both are SQLite: `sqlite` (better-sqlite3, via `resolveSqliteDriver`) and + * `sqlite-wasm` (`SqliteWasmDriver`). Neither takes a pool option, and neither + * could honour one — see the module note on `:memory:`. + */ +export const POOL_UNSUPPORTED_DRIVER_IDS = ['sqlite', 'sqlite-wasm'] as const; + +export type PoolUnsupportedDriverId = (typeof POOL_UNSUPPORTED_DRIVER_IDS)[number]; + +/** + * Does this driver id read a declared `datasource.pool`? + * + * `true` for the pooled built-ins (`postgres` / `mysql` / `mongo`) **and** for + * every id outside the built-in table — an unknown id is not ours to judge, so + * it is left alone rather than rejected against a contract we do not ship. + */ +export function driverReadsDeclaredPool(driver: unknown): boolean { + const id = resolveDriverId(driver); + if (!id) return true; + return !(POOL_UNSUPPORTED_DRIVER_IDS as readonly string[]).includes(id); +} + +/** + * Is there a `pool` block here at all? An absent block — and an empty one, + * which declares nothing and therefore loses nothing — is not a declaration. + */ +function isPoolDeclared(pool: unknown): boolean { + return ( + typeof pool === 'object' && pool !== null && Object.keys(pool as Record).length > 0 + ); +} + +/** + * The rejection text for a `pool` block on a driver that cannot honour it. + * + * It is a FIX instruction, deliberately: it names the one edit that resolves it + * (delete the block) and says where the key stays meaningful. It offers no + * escape hatch and does not suggest changing the driver — an authoring mistake + * has a correction, not a bypass. + */ +export function unsupportedPoolMessage(driver: string, datasourceName?: string): string { + const subject = datasourceName ? `Datasource '${datasourceName}'` : 'This datasource'; + return ( + `${subject} declares a \`pool\` block, but the '${driver}' driver does not read it: a SQLite ` + + `connection strategy is owned by the driver, not by the datasource — one connection per ` + + `database, because a second connection to \`:memory:\` opens a SEPARATE, empty database. ` + + `Sizing it here would therefore split one datasource's data across several stores, so the ` + + `block is rejected instead of dropped. Remove \`pool\` from this datasource declaration; it ` + + `stays meaningful on the pooled drivers (postgres / mysql / mongo).` + ); +} + +/** + * The rejection for one datasource declaration, or `undefined` when there is + * nothing to reject. Never throws — callers that want the throw use + * {@link assertDatasourcePoolSupported}. + */ +export function unsupportedPoolIssue(input: { + driver: string; + pool?: unknown; + name?: string; +}): string | undefined { + if (!isPoolDeclared(input.pool)) return undefined; + if (driverReadsDeclaredPool(input.driver)) return undefined; + return unsupportedPoolMessage(input.driver, input.name); +} + +/** + * Throw when a datasource declares a `pool` its driver cannot honour. + * + * Called at every door a `pool` block can come in through — the Setup wizard's + * create/update, the boot-time auto-connect pre-pass, and the driver factory + * itself — so the declaration is rejected before anything is built, rather than + * dropped after. + */ +export function assertDatasourcePoolSupported(input: { + driver: string; + pool?: unknown; + name?: string; +}): void { + const issue = unsupportedPoolIssue(input); + if (issue) throw new Error(issue); +} 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 bb018413a4..e11be8854d 100644 --- a/packages/services/service-datasource/src/default-datasource-driver-factory.ts +++ b/packages/services/service-datasource/src/default-datasource-driver-factory.ts @@ -41,6 +41,7 @@ import type { DatasourceConnectionSpec, DatasourceDriverHandle, } from './contracts/index.js'; +import { assertDatasourcePoolSupported } from './datasource-pool-support.js'; /** * Driver-id resolution comes from the spec since #4410 — this file used to keep @@ -335,6 +336,14 @@ export function createDefaultDatasourceDriverFactory( throw new Error(`Unsupported driver id '${spec.driver}'.`); } + // A `pool` block this driver cannot honour is rejected here rather than + // dropped on the floor two arms down (#5714). This is the LAST door — the + // wizard's create/update and the boot-time pre-pass in + // `DatasourceConnectionService` reject it earlier and with better context + // — but it is the one every host that builds through this factory passes + // through, so it is where "declared = honoured" is actually guaranteed. + assertDatasourcePoolSupported({ driver: spec.driver, pool: spec.pool, name: spec.name }); + // ADR-0015's ownership mode. `spec.schemaMode` — the datasource's own // declared key — is FIRST since #4410; before that the first two arms // were all there was, and neither could ever hold it: `external` is the diff --git a/packages/services/service-datasource/src/index.ts b/packages/services/service-datasource/src/index.ts index ac99ba5f5b..60832cc570 100644 --- a/packages/services/service-datasource/src/index.ts +++ b/packages/services/service-datasource/src/index.ts @@ -75,6 +75,18 @@ export type { SecretBinder, } from './datasource-admin-plugin.js'; +// Which driver arms read `datasource.pool`, and the loud rejection for the ones +// that do not (#5714) — exported so a host that injects its OWN driver factory +// can hold the same contract instead of re-deriving (or silently dropping) it. +export { + POOL_UNSUPPORTED_DRIVER_IDS, + driverReadsDeclaredPool, + unsupportedPoolIssue, + unsupportedPoolMessage, + assertDatasourcePoolSupported, +} from './datasource-pool-support.js'; +export type { PoolUnsupportedDriverId } from './datasource-pool-support.js'; + // Host glue: dev driver factory + fail-closed secret binder. export { createDefaultDatasourceDriverFactory } from './default-datasource-driver-factory.js'; // The "adopt a host-built driver instance" seam (ADR-0062 D1, #3826) — for