Skip to content

Commit 01faeb1

Browse files
hotlongclaude
andauthored
fix(service-datasource): reject a pool block the memory arm cannot honour (#5931) (#6237)
#5714 made an unhonourable `pool` block a loud authoring error, but its ruling was scoped to the two sqlite arms — the `memory` arm kept dropping it. It hands `InMemoryDriver` nothing but `buildMemoryConfig(spec)`, which reads `spec.config` and never `spec.pool`, so a sized pool reached nothing and said nothing. `memory` now joins `POOL_UNSUPPORTED_DRIVER_IDS`, so all three doors that already rejected sqlite reject it: the Setup wizard's create/update, the boot-time auto-connect pre-pass, and the driver factory itself. Its own explanation, not SQLite's, per the maintainer ruling of 2026-08-07: SQLite is rejected because a second connection to `:memory:` opens a separate, empty database, which would split one datasource across several stores. That reasoning is false for `memory` — there is no connection at all — so reusing it would send the author looking for a connection-strategy knob that does not exist. Reasons are keyed by driver id, which makes an arm joining the set without writing one a type error. The sqlite arms' text is byte-for-byte unchanged (pinned whole). The pin #5954 deliberately left green (`leaves 'memory' out of the rejected set (#5931), deliberately`) is flipped rather than deleted — the same fact, re-judged — and the module note that named this issue as a known, deliberately drawn boundary now records the hole as closed. Out of scope, by the dispatch's red line: the spec half of the ruling (the four driver-qualified `pool` rows in `packages/spec/liveness/datasource.json`) goes to the spec seat on its own issue. Claude-Session: https://claude.ai/code/session_015a5qkLzpGXhLL2F5gvJ7dD Co-authored-by: Claude <noreply@anthropic.com>
1 parent 72847c5 commit 01faeb1

4 files changed

Lines changed: 346 additions & 36 deletions

File tree

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
---
2+
"@objectstack/service-datasource": patch
3+
---
4+
5+
fix(service-datasource): a `pool` block on a `memory` datasource is rejected, not dropped in silence (#5931)
6+
7+
#5714 made a `pool` block the driver cannot honour a loud authoring error, but
8+
its ruling was scoped to the two sqlite arms — `memory` kept dropping it. The
9+
`memory` arm hands `InMemoryDriver` nothing but `buildMemoryConfig(spec)`, which
10+
reads `spec.config` and never `spec.pool`, so a sized pool reached nothing and
11+
said nothing. Measured through the real factory:
12+
13+
```text
14+
memory + pool{min:3,max:9} driver config {"persistence":false} pool undefined
15+
sqlite + pool{min:3,max:9} rejected (since #5714)
16+
postgres + pool{min:3,max:9} knex config.pool {"min":3,"max":9} live {min:3,max:9}
17+
```
18+
19+
`memory` now joins `POOL_UNSUPPORTED_DRIVER_IDS`, so the same three doors that
20+
already rejected sqlite reject it: the Setup wizard's create/update, the
21+
boot-time auto-connect pre-pass, and the driver factory itself.
22+
23+
**Behaviour change.** A datasource declaring `driver: 'memory'` (or `inmemory` /
24+
`in-memory` / `mingo`) together with a non-empty `pool` block used to load and
25+
run; it now throws at whichever door it arrives through. The fix is the one edit
26+
the message names — delete the `pool` block. Nothing is lost by deleting it: it
27+
configured nothing before. An absent or empty `pool` is unchanged, and every
28+
`memory` datasource without one builds exactly as it did. No declaration in this
29+
repo, the example apps included, carried the combination.
30+
31+
**Its own explanation, not SQLite's.** SQLite is rejected because a second
32+
connection to `:memory:` opens a separate, empty database, so sizing the pool
33+
would split one datasource across several stores. That reasoning is false for
34+
`memory`: there is no connection at all — the store is a plain data structure in
35+
this process — so the message says that instead. Telling an author their driver
36+
picked a connection strategy for them would send them looking for a knob that
37+
does not exist. Reasons are now keyed by driver id, which makes an arm joining
38+
the set without writing one a type error.
39+
40+
Maintainer ruling 2026-08-07, which also set the default for the next sister
41+
arm: when a declared key is silently dropped on one arm and an earlier ruling
42+
already made it a loud authoring error on a sibling, the new arm joins the
43+
existing rejection set rather than queueing for a ruling of its own — unless the
44+
original rationale was measured to be arm-specific.
45+
46+
No API surface is added — `POOL_UNSUPPORTED_DRIVER_IDS`,
47+
`driverReadsDeclaredPool`, `unsupportedPoolIssue`, `unsupportedPoolMessage` and
48+
`assertDatasourcePoolSupported` keep the signatures #5714 published, and the
49+
sqlite arms' rejection text is byte-for-byte unchanged.

packages/services/service-datasource/src/__tests__/datasource-pool-support.test.ts

Lines changed: 203 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,25 @@
1111
// takes effect or is rejected out loud — never dropped. These tests pin the
1212
// rejection at each door it can come in through, and pin that the arms which
1313
// DO honour the block still do.
14+
//
15+
// #5931 — the SISTER ARM. `memory` dropped `pool` exactly as silently
16+
// (`buildMemoryConfig` reads `spec.config` only, and `InMemoryDriver` has no
17+
// pool concept whatsoever), but it was left out of the rejected set because
18+
// #5714's ruling was scoped to the two sqlite arms. Measured the same way:
19+
//
20+
// memory + pool{min:3,max:9} driver config {"persistence":false} pool undefined
21+
//
22+
// The maintainer ruling of 2026-08-07 folds it in, with its own explanation
23+
// rather than SQLite's, and sets the default for the next sister arm (#6140).
24+
// The pin that used to hold `memory` OUT of the set is flipped below, not
25+
// deleted — it is the same fact, re-judged.
1426

1527
import { describe, it, expect, vi } from 'vitest';
1628
import {
1729
POOL_UNSUPPORTED_DRIVER_IDS,
1830
driverReadsDeclaredPool,
1931
unsupportedPoolIssue,
32+
unsupportedPoolMessage,
2033
assertDatasourcePoolSupported,
2134
} from '../datasource-pool-support.js';
2235
import { createDefaultDatasourceDriverFactory } from '../default-datasource-driver-factory.js';
@@ -29,8 +42,8 @@ import { DatasourceAdminService, type StoredDatasource } from '../datasource-adm
2942
import type { IDatasourceDriverFactory } from '../contracts/datasource-driver-factory.js';
3043

3144
describe('#5714 — which driver arms read a declared `pool`', () => {
32-
it('names exactly the two sqlite arms as unable to honour it', () => {
33-
expect([...POOL_UNSUPPORTED_DRIVER_IDS]).toEqual(['sqlite', 'sqlite-wasm']);
45+
it('names the two sqlite arms and `memory` as unable to honour it (#5931)', () => {
46+
expect([...POOL_UNSUPPORTED_DRIVER_IDS]).toEqual(['memory', 'sqlite', 'sqlite-wasm']);
3447
});
3548

3649
it('rejects every spelling of the sqlite arms, case-insensitively', () => {
@@ -53,17 +66,24 @@ describe('#5714 — which driver arms read a declared `pool`', () => {
5366
expect(unsupportedPoolIssue({ driver: 'com.vendor.snowflake', pool: { min: 3, max: 9 } })).toBeUndefined();
5467
});
5568

56-
// Deliberate, and filed rather than silently widened: `memory` reads no pool
57-
// either, but the #5714 ruling authorised this tightening for the sqlite arms
58-
// only. #5931 carries the decision.
59-
it('leaves `memory` out of the rejected set (#5931), deliberately', () => {
60-
expect(driverReadsDeclaredPool('memory')).toBe(true);
69+
// THE FLIPPED PIN (#5931). This case used to assert the opposite — `memory`
70+
// deliberately OUT of the rejected set, pending a ruling — and #5954 left it
71+
// green on purpose so the hole would read as drawn rather than overlooked.
72+
// The ruling of 2026-08-07 folded the arm in, so the same fact now has the
73+
// opposite verdict. Every spelling, because `mingo` and `in-memory` build the
74+
// very same driver.
75+
it('rejects `memory` and every spelling of it (#5931 — the flipped pin)', () => {
76+
for (const id of ['memory', 'inmemory', 'in-memory', 'mingo', 'Memory', ' MEMORY ']) {
77+
expect(driverReadsDeclaredPool(id), id).toBe(false);
78+
}
6179
});
6280

6381
it('treats an absent or empty block as no declaration', () => {
64-
expect(unsupportedPoolIssue({ driver: 'sqlite' })).toBeUndefined();
65-
expect(unsupportedPoolIssue({ driver: 'sqlite', pool: {} })).toBeUndefined();
66-
expect(unsupportedPoolIssue({ driver: 'sqlite', pool: undefined })).toBeUndefined();
82+
for (const driver of ['sqlite', 'memory']) {
83+
expect(unsupportedPoolIssue({ driver }), driver).toBeUndefined();
84+
expect(unsupportedPoolIssue({ driver, pool: {} }), driver).toBeUndefined();
85+
expect(unsupportedPoolIssue({ driver, pool: undefined }), driver).toBeUndefined();
86+
}
6787
});
6888

6989
it('names the datasource and the one edit that fixes it', () => {
@@ -77,14 +97,73 @@ describe('#5714 — which driver arms read a declared `pool`', () => {
7797
// authoring mistake has a correction; suggesting an env var that boots past
7898
// it (or a different driver) sends the author away from the fix.
7999
it('offers no escape hatch and no "use another driver" advice', () => {
80-
const msg = unsupportedPoolIssue({ driver: 'sqlite-wasm', pool: { max: 9 }, name: 'ds' }) ?? '';
81-
expect(msg).not.toMatch(/OS_ALLOW_DRIVER_CONNECT_FAILURE/);
82-
expect(msg).not.toMatch(/OS_[A-Z_]*=1/);
83-
expect(msg).not.toMatch(/switch|instead use|change the driver/i);
100+
for (const driver of ['sqlite-wasm', 'memory']) {
101+
const msg = unsupportedPoolIssue({ driver, pool: { max: 9 }, name: 'ds' }) ?? '';
102+
expect(msg, driver).not.toMatch(/OS_ALLOW_DRIVER_CONNECT_FAILURE/);
103+
expect(msg, driver).not.toMatch(/OS_[A-Z_]*=1/);
104+
expect(msg, driver).not.toMatch(/switch|instead use|change the driver/i);
105+
}
106+
});
107+
108+
// #5931 — the memory arm gets its OWN explanation. The ruling was explicit
109+
// that SQLite's sentence must not be reused, and the reason is not style:
110+
// "the driver owns the connection strategy" would send the author looking for
111+
// a strategy knob, when the truth is that there is no connection to pool.
112+
it('explains `memory` in its own terms, never SQLite\'s (#5931)', () => {
113+
const msg = unsupportedPoolIssue({ driver: 'memory', pool: { min: 3, max: 9 }, name: 'scratch' }) ?? '';
114+
expect(msg).toContain(`Datasource 'scratch'`);
115+
expect(msg).toContain(`the 'memory' driver does not read it`);
116+
expect(msg).toMatch(/no pool to size, and no connection to pool/);
117+
expect(msg).toMatch(/plain data structure inside this process/);
118+
// …and not one part of SQLite's reasoning, which is about a DIFFERENT
119+
// failure (one datasource split across several stores).
120+
expect(msg).not.toMatch(/SQLite/);
121+
expect(msg).not.toMatch(/connection strategy is owned by the driver/);
122+
expect(msg).not.toMatch(/SEPARATE, empty database/);
123+
expect(msg).not.toMatch(/split one datasource's data/);
124+
// The shared frame survives: the one edit that fixes it, and where the key
125+
// stays meaningful.
126+
expect(msg).toMatch(/Remove `pool` from this datasource declaration/);
127+
expect(msg).toMatch(/postgres \/ mysql \/ mongo/);
128+
});
129+
130+
it('quotes the spelling the author actually wrote, with the memory reason behind it', () => {
131+
const msg = unsupportedPoolIssue({ driver: 'mingo', pool: { max: 2 }, name: 'scratch' }) ?? '';
132+
expect(msg).toContain(`the 'mingo' driver does not read it`);
133+
expect(msg).toMatch(/no pool to size, and no connection to pool/);
134+
});
135+
136+
// The two sqlite arms' text is UNCHANGED by #5931 — pinned whole, against the
137+
// literal as it stood on `origin/main` before this change, because "we only
138+
// added an arm" is a claim about bytes.
139+
it('leaves the sqlite arms\' message byte-for-byte as #5714 wrote it', () => {
140+
const expected =
141+
"Datasource 'crm_primary' declares a `pool` block, but the 'sqlite' driver does not read " +
142+
'it: a SQLite connection strategy is owned by the driver, not by the datasource — one ' +
143+
'connection per database, because a second connection to `:memory:` opens a SEPARATE, ' +
144+
"empty database. Sizing it here would therefore split one datasource's data across " +
145+
'several stores, so the block is rejected instead of dropped. Remove `pool` from this ' +
146+
'datasource declaration; it stays meaningful on the pooled drivers ' +
147+
'(postgres / mysql / mongo).';
148+
expect(unsupportedPoolMessage('sqlite', 'crm_primary')).toBe(expected);
149+
expect(unsupportedPoolMessage('sqlite-wasm', 'crm_primary'))
150+
.toBe(expected.replace("the 'sqlite' driver", "the 'sqlite-wasm' driver"));
151+
});
152+
153+
// Unreachable through `unsupportedPoolIssue` (nothing produces a message for a
154+
// driver that reads the block), but the helper is exported and takes a plain
155+
// string. The honest answer there is the part true of every rejected arm —
156+
// never another arm's specific reasoning.
157+
it('borrows no arm\'s reasoning for a driver that is not in the set', () => {
158+
const msg = unsupportedPoolMessage('postgres');
159+
expect(msg).toMatch(/nothing in the block reaches a connection/);
160+
expect(msg).not.toMatch(/SQLite/);
161+
expect(msg).not.toMatch(/plain data structure inside this process/);
84162
});
85163

86164
it('assert throws exactly when the issue is reported', () => {
87165
expect(() => assertDatasourcePoolSupported({ driver: 'sqlite', pool: { max: 5 } })).toThrow(/does not read it/);
166+
expect(() => assertDatasourcePoolSupported({ driver: 'memory', pool: { max: 5 } })).toThrow(/does not read it/);
88167
expect(() => assertDatasourcePoolSupported({ driver: 'postgres', pool: { max: 5 } })).not.toThrow();
89168
});
90169
});
@@ -119,6 +198,43 @@ describe('#5714 — the driver factory rejects a pool it cannot honour', () => {
119198
).rejects.toThrow(/does not read it/);
120199
});
121200

201+
// #5931 — the arm this file used to pin as deliberately unguarded. Before the
202+
// ruling this call RESOLVED, handing back an `InMemoryDriver` built from
203+
// `buildMemoryConfig(spec)` alone, with `pool` nowhere in it.
204+
it('memory + pool is rejected instead of built with the block dropped (#5931)', async () => {
205+
await expect(
206+
factory().create({
207+
name: 'scratch',
208+
driver: 'memory',
209+
config: {},
210+
pool: { min: 3, max: 9 },
211+
}),
212+
).rejects.toThrow(/Datasource 'scratch' declares a `pool` block/);
213+
});
214+
215+
it('rejects the `inmemory` alias too, and says why in memory terms', async () => {
216+
const err = await Promise.resolve(
217+
factory().create({ name: 'scratch', driver: 'inmemory', config: {}, pool: { max: 9 } }),
218+
).then(() => undefined, (e: Error) => e);
219+
expect(err?.message).toMatch(/no pool to size, and no connection to pool/);
220+
expect(err?.message).not.toMatch(/SQLite/);
221+
});
222+
223+
it('memory WITHOUT a pool still builds exactly as before', async () => {
224+
const handle: any = await factory().create({ name: 'scratch', driver: 'memory', config: {} });
225+
const driver = handle.driver ?? handle;
226+
expect(driver?.constructor?.name).toMatch(/InMemoryDriver$/);
227+
// The #4083 shape is untouched: ephemeral unless the author opts in.
228+
expect(driver.config?.persistence).toBe(false);
229+
try { await handle.disconnect?.(); } catch { /* never connected */ }
230+
});
231+
232+
it('memory with an EMPTY pool block still builds — nothing was declared', async () => {
233+
const handle: any = await factory().create({ name: 'scratch', driver: 'memory', config: {}, pool: {} });
234+
expect((handle.driver ?? handle)?.constructor?.name).toMatch(/InMemoryDriver$/);
235+
try { await handle.disconnect?.(); } catch { /* never connected */ }
236+
});
237+
122238
it('sqlite WITHOUT a pool still builds exactly as before', async () => {
123239
const handle: any = await factory().create({
124240
name: 'crm_primary',
@@ -240,6 +356,46 @@ describe('#5714 — boot refuses a declared pool the driver cannot honour', () =
240356
).resolves.toEqual([]);
241357
});
242358

359+
// #5931 — the same door, the sister arm. A memory datasource carrying a pool
360+
// used to boot silently and run on a store no pool setting ever touched.
361+
it('refuses a memory datasource carrying a pool, before anything is connected (#5931)', async () => {
362+
const { service, factory, engine } = svc();
363+
await expect(
364+
service.connectDeclared({
365+
datasources: [{ name: 'scratch', driver: 'memory', config: {}, pool: { min: 3, max: 9 } }],
366+
objects: [],
367+
}),
368+
).rejects.toThrow(/Datasource 'scratch' declares a `pool` block/);
369+
expect((factory.create as any).mock.calls.length).toBe(0);
370+
expect(engine.drivers.size).toBe(0);
371+
});
372+
373+
it('names a memory offender alongside a sqlite one in the same throw', async () => {
374+
const { service } = svc();
375+
const err = await service
376+
.connectDeclared({
377+
datasources: [sqliteWithPool, { name: 'scratch', driver: 'memory', pool: { max: 4 } }],
378+
objects: [],
379+
})
380+
.then(() => undefined, (e: Error) => e);
381+
expect(err?.message).toMatch(/2 declared datasource\(s\)/);
382+
expect(err?.message).toContain(`Datasource 'crm_primary'`);
383+
expect(err?.message).toContain(`Datasource 'scratch'`);
384+
// Each offender keeps its own explanation in the aggregate.
385+
expect(err?.message).toMatch(/a SQLite connection strategy is owned by the driver/);
386+
expect(err?.message).toMatch(/no pool to size, and no connection to pool/);
387+
});
388+
389+
it('leaves a memory datasource with no pool block connecting as before', async () => {
390+
const { service, engine } = svc();
391+
const results = await service.connectDeclared({
392+
datasources: [{ name: 'scratch', driver: 'memory', config: {}, autoConnect: true }],
393+
objects: [],
394+
});
395+
expect(results.map((r) => r.status)).toEqual(['connected']);
396+
expect(engine.drivers.has('scratch')).toBe(true);
397+
});
398+
243399
it('leaves a sqlite datasource with no pool block connecting as before', async () => {
244400
const { service, engine } = svc();
245401
const results = await service.connectDeclared({
@@ -328,6 +484,39 @@ describe('#5714 — the Setup wizard rejects it before the record is stored', ()
328484
expect(records[0]?.pool).toEqual({ min: 3, max: 9 });
329485
});
330486

487+
// #5931 — the wizard is the door an author is most likely to come through
488+
// with `driver: memory`, since it is the dev/test choice the Setup UI offers.
489+
it('create: a memory draft carrying a pool never reaches the store (#5931)', async () => {
490+
const { service, records, registered } = adminHarness();
491+
await expect(
492+
service.createDatasource({
493+
name: 'scratch',
494+
driver: 'memory',
495+
config: {},
496+
pool: { min: 1, max: 5 },
497+
}),
498+
).rejects.toThrow(/no pool to size, and no connection to pool/);
499+
expect(records).toHaveLength(0);
500+
expect(registered).toHaveLength(0);
501+
});
502+
503+
it('create: a memory draft with no pool is stored as before', async () => {
504+
const { service, records, registered } = adminHarness();
505+
await service.createDatasource({ name: 'scratch', driver: 'memory', config: {} });
506+
expect(records[0]?.name).toBe('scratch');
507+
expect(records[0]?.pool).toBeUndefined();
508+
expect(registered).toEqual(['scratch']);
509+
});
510+
511+
it('update: switching a pooled datasource TO memory is rejected on the merged record (#5931)', async () => {
512+
const { service } = adminHarness([
513+
{ name: 'reporting', driver: 'postgres', config: {}, pool: { min: 3, max: 9 }, origin: 'runtime' },
514+
]);
515+
await expect(
516+
service.updateDatasource('reporting', { driver: 'memory', config: {} }),
517+
).rejects.toThrow(/declares a `pool` block/);
518+
});
519+
331520
it('update: patching a pool onto a stored sqlite datasource is rejected', async () => {
332521
const { service } = adminHarness([
333522
{ name: 'local_cache', driver: 'sqlite', config: { filename: ':memory:' }, origin: 'runtime' },

0 commit comments

Comments
 (0)