From 93ba844130841913624d0a1963e4430e50be95bd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 10:13:36 +0000 Subject: [PATCH] fix(datasource): a `memory` datasource is ephemeral, and each pool gets its own store (#4083) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared driver factory built `new InMemoryDriver()` for `driver: 'memory'` with no config, so the pool inherited that driver's own `persistence: 'auto'` default — in Node, a file adapter at the RELATIVE, process-global path `.objectstack/data/memory-driver.json`. Two consequences, neither intended: - It was not ephemeral. The pool flushed its whole store into the server's working directory (on an unref'd 2s autosave timer, and again at teardown) and reloaded it on the next boot — the opposite of what the driver id promises the operator who asks for it (`storage-driver.ts` documents `memory` as "ephemeral, not real SQL"). - Every memory pool in a process shared one destination. The default path carries no per-datasource component, so two `driver: 'memory'` datasources loaded and saved the same file: each saw the other's tables, and the last teardown to flush clobbered the other's rows. Both surfaced as an intermittent test failure. The ADR-0062 D1 federated-read acceptance seeds 2 rows into an auto-connected external memory datasource and reads them back; it returned 2 rows on a clean checkout and 2xN on the Nth run in the same tree — green in CI (always run #1, always a fresh checkout) and failing locally for anyone who ran it twice. Whether a given run leaked came down to the autosave timer, which is what made it look flaky rather than wrong. - The factory now builds the memory pool with `persistence: false` by default. - It also honors the datasource's own `config`, previously dropped entirely: `initialData` and `strictMode` never reached the driver. - When an author does opt into persistence, the default destination is scoped to the datasource (`.objectstack/data/memory-.json` / `objectstack:memory-db:`) so pools stay independent. An explicit `path`/`key`, or a custom `adapter`, is left exactly as written. - The dev-only sqlite step-down's last-resort in-memory driver is built the same way, making its own "not persistent" comment true. `InMemoryDriver`'s documented defaults are unchanged — constructing one directly still auto-detects persistence. Only the datasource-scoped pools this factory builds changed. Tests: four factory tests pin ephemerality, pool independence, config passthrough and the scoped opt-in destination; a runtime test pins the property that actually broke — a federated in-memory pool writes no state file and a second boot in the same process starts empty. `flush()` stands in for the autosave timer so it fails deterministically without the fix rather than racing. The acceptance's own default driver is now explicitly non-persistent, so it no longer depends on ambient disk state. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SVUpPzE2cJCwGr6H9xjiDT --- .../memory-datasource-ephemeral-per-pool.md | 51 ++++++++++ .../src/datasource-autoconnect.test.ts | 65 ++++++++++++- .../default-datasource-driver-factory.test.ts | 93 ++++++++++++++++++- .../src/default-datasource-driver-factory.ts | 81 +++++++++++++++- .../src/sqlite-driver-fallback.ts | 10 +- 5 files changed, 294 insertions(+), 6 deletions(-) create mode 100644 .changeset/memory-datasource-ephemeral-per-pool.md diff --git a/.changeset/memory-datasource-ephemeral-per-pool.md b/.changeset/memory-datasource-ephemeral-per-pool.md new file mode 100644 index 0000000000..8d6175b01b --- /dev/null +++ b/.changeset/memory-datasource-ephemeral-per-pool.md @@ -0,0 +1,51 @@ +--- +"@objectstack/service-datasource": patch +--- + +fix(datasource): a `memory` datasource is ephemeral again, and each pool gets its own store (#4083) + +The shared driver factory built `new InMemoryDriver()` for `driver: 'memory'` with +no config, so the pool inherited that driver's own `persistence: 'auto'` default — +in Node, a file adapter at the **relative, process-global** path +`.objectstack/data/memory-driver.json`. Two consequences, neither intended: + +- **It was not ephemeral.** The pool flushed its whole store into the server's + working directory (on an unref'd 2s autosave timer, and again at teardown) and + reloaded it on the next boot. That is the opposite of what the driver id + promises the operator who asks for it — `OS_DATABASE_DRIVER=memory` is + documented as *ephemeral, not real SQL* — and it means a "throwaway" datasource + left state in the deploy directory. +- **Every memory pool in a process shared one destination.** The default path + carries no per-datasource component, so two `driver: 'memory'` datasources + loaded and saved the same file: each saw the other's tables, and the last + teardown to flush clobbered the other's rows. + +Both were visible as an intermittent test failure. The ADR-0062 D1 federated-read +acceptance seeds 2 rows into an auto-connected external memory datasource and +reads them back; it returned 2 rows on a clean checkout and 2×N on the Nth run in +the same tree — passing in CI (always run #1, always a fresh checkout) and +failing locally for anyone who ran it twice. Whether a given run leaked depended +on the autosave timer, which is what made it look flaky rather than wrong. + +- The factory now builds the memory pool with **`persistence: false` by default**. +- It also **honors the datasource's own `config`**, which was previously dropped + entirely: `initialData` and `strictMode` never reached the driver. +- When an author *does* opt into persistence (`config.persistence`), the default + destination is **scoped to the datasource** — + `.objectstack/data/memory-.json` / `objectstack:memory-db:` — so + pools stay independent. An explicit `path`/`key`, or a custom `adapter`, is + left exactly as written. +- The dev-only sqlite step-down's last-resort in-memory driver + (`resolveSqliteDriver`, #2229) is built the same way, making its own + "not persistent" contract true. + +`InMemoryDriver`'s documented defaults are unchanged — constructing one directly +still auto-detects persistence. Only the datasource-scoped pools this factory +builds changed. + +**Migration.** A deployment relying on `driver: 'memory'` state surviving a +restart was relying on a bug, and should declare it: set +`config: { persistence: 'file' }` on the datasource (now written to a +per-datasource file), or use a real driver — `sqlite`/`sqlite-wasm` give durable +storage with real SQL. Existing `.objectstack/data/memory-driver.json` files are +no longer read; delete them. diff --git a/packages/runtime/src/datasource-autoconnect.test.ts b/packages/runtime/src/datasource-autoconnect.test.ts index 99505205f8..a7d6e87cd0 100644 --- a/packages/runtime/src/datasource-autoconnect.test.ts +++ b/packages/runtime/src/datasource-autoconnect.test.ts @@ -12,6 +12,8 @@ // without any native driver dependency. import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach } from 'vitest'; +import { existsSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; import { Runtime } from './runtime.js'; import { DriverPlugin } from './driver-plugin.js'; import { AppPlugin } from './app-plugin.js'; @@ -71,7 +73,10 @@ async function boot(opts: { connectPolicy?: DatasourceConnectPolicy } = {}) { const runtime = new Runtime({ cluster: false }); const kernel = runtime.getKernel(); - await kernel.use(new DriverPlugin(new InMemoryDriver())); // default driver + // `persistence: false` keeps the acceptance hermetic. The driver's own default + // is `'auto'` — in Node a file adapter at `.objectstack/data/…` under the CWD, + // which both reloads and rewrites ambient state between runs (#4083). + await kernel.use(new DriverPlugin(new InMemoryDriver({ persistence: false }))); // default driver await kernel.use(new ObjectQLPlugin()); await kernel.use(new AppPlugin(artifact())); await kernel.use( @@ -131,6 +136,64 @@ describe('ADR-0062 declared-datasource auto-connect', () => { }); }); +// #4083 — the acceptance above passed on a clean checkout and failed on every +// subsequent run, reading 2×N rows on the Nth: the auto-connected `memory` +// datasource inherited `InMemoryDriver`'s `persistence: 'auto'` default, so it +// flushed `ext_note` into `.objectstack/data/memory-driver.json` under the CWD +// and the next boot's connect() loaded those rows back before this file seeded +// its own. CI never caught it because CI always runs #1 on a fresh checkout. +// +// The intermittency ("passes once in four") came from WHEN the flush lands: the +// file adapter writes on a 2s unref'd autosave timer, so a run short enough to +// finish first left nothing behind. `flush()` below stands in for that timer, so +// this pins the property that was actually broken — a federated in-memory pool +// leaves nothing behind and does not outlive its kernel — without a timing race +// and without depending on run-to-run state. +describe('ADR-0062 D1 — the auto-connected in-memory pool leaves nothing behind (#4083)', () => { + const STATE_DIR = join(process.cwd(), '.objectstack'); + const clearState = () => { try { rmSync(STATE_DIR, { recursive: true, force: true }); } catch { /* noop */ } }; + // Clear on both sides: a leftover from elsewhere would make this pass for the + // wrong reason, and a failure that DID write must not leak into the next run + // (that leak is the bug under test). + beforeAll(clearState); + afterAll(clearState); + + async function seedAndRead(kernel: Awaited>) { + const engine = kernel.getService<{ + getDriverByName(n: string): any; + find(object: string, query?: any): Promise; + }>('data'); + const driver = engine.getDriverByName('autoconn_ext'); + await driver.bulkCreate('ext_note', [ + { id: 'n1', title: 'first' }, + { id: 'n2', title: 'second' }, + ]); + const titles = (await engine.find('ext_note')).map((r) => r.title).sort(); + // Whatever the autosave timer would have written, written now. + await driver.flush?.(); + return titles; + } + + it('writes no state file, and a second boot in the same process starts empty', async () => { + const first = await boot(); + try { + expect(await seedAndRead(first)).toEqual(['first', 'second']); + // The seeded rows must not have reached the host filesystem at all. + expect(existsSync(STATE_DIR)).toBe(false); + } finally { + try { await (first as any)?.stop?.(); } catch { /* noop */ } + } + + const second = await boot(); + try { + // Was ['first','first','second','second'] — the first boot's rows, reloaded. + expect(await seedAndRead(second)).toEqual(['first', 'second']); + } finally { + try { await (second as any)?.stop?.(); } catch { /* noop */ } + } + }, BOOT_TIMEOUT); +}); + describe('ADR-0062 credentials fail-closed (D3)', () => { // An external datasource that declares a credentialsRef the host cannot // resolve (no matching sys_secret row) must FAIL CLOSED — never connect with diff --git a/packages/services/service-datasource/src/__tests__/default-datasource-driver-factory.test.ts b/packages/services/service-datasource/src/__tests__/default-datasource-driver-factory.test.ts index ad7cc7e6b0..6020a0d547 100644 --- a/packages/services/service-datasource/src/__tests__/default-datasource-driver-factory.test.ts +++ b/packages/services/service-datasource/src/__tests__/default-datasource-driver-factory.test.ts @@ -6,7 +6,7 @@ // kind. These are the first direct tests of the factory's id → driver mapping. import { describe, it, expect, beforeAll, afterAll } from 'vitest'; -import { mkdtempSync, rmSync } from 'node:fs'; +import { existsSync, mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { createDefaultDatasourceDriverFactory } from '../default-datasource-driver-factory.js'; @@ -76,3 +76,94 @@ describe('createDefaultDatasourceDriverFactory — mysql construction', () => { try { await handle.disconnect?.(); } catch { /* pool never opened */ } }); }); + +// #4083 — the `memory` id built a bare `new InMemoryDriver()`, inheriting that +// driver's own `persistence: 'auto'` default: in Node a file adapter at the +// RELATIVE, process-global path `.objectstack/data/memory-driver.json`. So a +// "memory" datasource flushed its store into the server's CWD at teardown and +// reloaded it on the next boot, and every memory pool in the process aliased the +// same file. That is what made the ADR-0062 D1 federated-read acceptance read 2 +// rows on a clean checkout and 2×N on the Nth run. +describe('createDefaultDatasourceDriverFactory — memory construction (#4083)', () => { + const STATE_DIR = join(process.cwd(), '.objectstack'); + + // Nothing here may write to the CWD. Asserting that directory's absence is the + // point, so clear it on both sides: a leftover from another test file would + // make this pass for the wrong reason, and a failure that DID create it must + // not leak into the next run (that leak is the very bug under test). + const clearState = () => { try { rmSync(STATE_DIR, { recursive: true, force: true }); } catch { /* noop */ } }; + beforeAll(clearState); + afterAll(clearState); + + async function pool(spec: { name?: string; config?: Record } = {}) { + const handle: any = await factory().create({ + driver: 'memory', + config: spec.config ?? {}, + ...(spec.name ? { name: spec.name } : {}), + }); + const driver = handle.driver ?? handle; + expect(driver?.constructor?.name).toMatch(/InMemoryDriver$/); + await handle.connect?.(); + return { handle, driver }; + } + + it('is ephemeral: a pool writes nothing to disk and a restart starts empty', async () => { + const first = await pool({ name: 'ext' }); + await first.driver.bulkCreate('ext_note', [{ id: 'n1', title: 'first' }, { id: 'n2', title: 'second' }]); + expect(await first.driver.find('ext_note', {})).toHaveLength(2); + // Teardown is where the file adapter flushed. It must produce no file… + await first.handle.disconnect?.(); + expect(existsSync(STATE_DIR)).toBe(false); + + // …and the next boot of the SAME datasource must not inherit those rows. + const second = await pool({ name: 'ext' }); + try { + expect(await second.driver.find('ext_note', {})).toEqual([]); + } finally { + await second.handle.disconnect?.(); + } + }); + + it('gives two memory datasources independent stores', async () => { + const a = await pool({ name: 'ext_a' }); + const b = await pool({ name: 'ext_b' }); + try { + await a.driver.create('ext_note', { id: 'a1', title: 'only-in-a' }); + expect(await a.driver.find('ext_note', {})).toHaveLength(1); + expect(await b.driver.find('ext_note', {})).toEqual([]); + } finally { + await a.handle.disconnect?.(); + await b.handle.disconnect?.(); + } + }); + + it("honors the datasource's own memory config (previously dropped entirely)", async () => { + const { handle, driver } = await pool({ + name: 'seeded', + config: { initialData: { ext_note: [{ id: 'seed', title: 'from-config' }] }, strictMode: true }, + }); + try { + const rows = (await driver.find('ext_note', {})) as Array<{ title?: string }>; + expect(rows.map((r) => r.title)).toEqual(['from-config']); + } finally { + await handle.disconnect?.(); + } + }); + + it('scopes an OPT-IN persistence destination to the datasource, not the process', async () => { + // The one white-box assertion here, and deliberately so: what needs pinning + // is the destination handed to the driver, and the alternative (letting two + // pools actually write) means writing into the repo checkout's CWD. + const { handle, driver } = await pool({ name: 'warehouse', config: { persistence: 'file' } }); + const persistence = (driver as { config: { persistence: { type?: string; path?: string; key?: string } } }).config.persistence; + expect(persistence.type).toBe('file'); + expect(persistence.path).toBe(join('.objectstack', 'data', 'memory-warehouse.json')); + expect(persistence.key).toBe('objectstack:memory-db:warehouse'); + // Author-supplied destinations are theirs — never rewritten. + const explicit = await pool({ name: 'warehouse', config: { persistence: { type: 'file', path: join(tmpdir(), 'os-4083-explicit.json') } } }); + expect((explicit.driver as { config: { persistence: { path?: string } } }).config.persistence.path) + .toBe(join(tmpdir(), 'os-4083-explicit.json')); + await handle.disconnect?.(); + await explicit.handle.disconnect?.(); + }); +}); 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 d7cbc3e619..72c5ec14b5 100644 --- a/packages/services/service-datasource/src/default-datasource-driver-factory.ts +++ b/packages/services/service-datasource/src/default-datasource-driver-factory.ts @@ -16,7 +16,8 @@ * - `sqlite-wasm` / `wasm-sqlite` → `@objectstack/driver-sqlite-wasm` (pure-JS) * - `mysql` / `mysql2` → `@objectstack/driver-sql` (client `mysql2`) * - `mongodb` / `mongo` → `@objectstack/driver-mongodb` (peer dep) - * - `memory` / `inmemory` → `@objectstack/driver-memory` + * - `memory` / `inmemory` → `@objectstack/driver-memory` (ephemeral, + * per-datasource — see {@link buildMemoryConfig}) * * `sqlite-wasm` joined for ADR-0062 D1 (#3826): the standalone stack's * `default` datasource is a *declared definition* connected through the shared @@ -30,6 +31,7 @@ * is never persisted or logged here. */ +import { join } from 'node:path'; import type { IDatasourceDriverFactory, DatasourceConnectionSpec, @@ -126,6 +128,78 @@ function buildMysqlConnection(spec: DatasourceConnectionSpec): unknown { }; } +/** + * Host-composition keys the CLI/standalone stack stamps into a `default` + * datasource's `config` for the SQL builders (#3826). They are not memory-driver + * config, so they are stripped before the rest of `config` is handed through. + */ +const NON_MEMORY_CONFIG_KEYS = ['schemaMode', 'autoMigrate', 'persist'] as const; + +/** `.objectstack/data/memory-.json` — one file per pool, never one for all. */ +function memoryStatePath(datasource: string): string { + return join('.objectstack', 'data', `memory-${datasource}.json`); +} + +/** `objectstack:memory-db:` — the localStorage equivalent of the above. */ +function memoryStateKey(datasource: string): string { + return `objectstack:memory-db:${datasource}`; +} + +/** + * Scope a REQUESTED persistence mode to one datasource. + * + * `InMemoryDriver`'s own persistence defaults are process-global — one file + * (`.objectstack/data/memory-driver.json`), one localStorage key — so two + * `driver: 'memory'` datasources in the same process load and save the SAME + * store: each sees the other's tables, and the last teardown to flush clobbers + * the other's rows. Expanding the string forms (`'auto'`/`'file'`/`'local'`) to + * the object form is what lets the default path/key carry the datasource name. + * An author-supplied `path`/`key` is theirs and is left alone, as is a custom + * `adapter` — they chose the destination. + */ +function scopeMemoryPersistence(persistence: unknown, datasource: string): unknown { + if (persistence === false) return false; + if (typeof persistence === 'string') { + return { type: persistence, path: memoryStatePath(datasource), key: memoryStateKey(datasource) }; + } + if (persistence && typeof persistence === 'object' && !('adapter' in persistence)) { + const p = persistence as { path?: string; key?: string }; + return { + ...p, + ...(p.path ? {} : { path: memoryStatePath(datasource) }), + ...(p.key ? {} : { key: memoryStateKey(datasource) }), + }; + } + return persistence; +} + +/** + * Build the `InMemoryDriver` config for a `memory` datasource (#4083). + * + * Two things the bare `new InMemoryDriver()` this replaces got wrong: + * + * - **It was not ephemeral.** `InMemoryDriver`'s own `persistence` default is + * `'auto'`, which in Node resolves to a file adapter at the *relative* path + * `.objectstack/data/memory-driver.json`. Every memory datasource therefore + * flushed its whole store into the server's CWD at teardown and reloaded it + * on the next boot — the opposite of what this driver id promises the + * operator who asks for it ("ephemeral, not real SQL", see + * `cli/src/utils/storage-driver.ts`), and why the ADR-0062 D1 federated-read + * acceptance read 2 rows on a clean checkout and 2×N on the Nth run (#4083). + * - **Every pool shared one destination.** See {@link scopeMemoryPersistence}. + * + * So: default to no persistence, honor the datasource's own `config` (dropped on + * the floor entirely before — `initialData`/`strictMode` never reached the + * driver), and scope the destination per datasource when an author *does* ask + * for persistence without naming a path/key of their own. + */ +function buildMemoryConfig(spec: DatasourceConnectionSpec): Record { + const cfg = { ...((spec.config ?? {}) as Record) }; + for (const key of NON_MEMORY_CONFIG_KEYS) delete cfg[key]; + if (cfg.persistence === undefined) return { ...cfg, persistence: false }; + return { ...cfg, persistence: scopeMemoryPersistence(cfg.persistence, spec.name ?? 'default') }; +} + /** Build a mongodb connection URL from a spec's config + secret. */ function buildMongoUrl(spec: DatasourceConnectionSpec): string { const cfg = (spec.config ?? {}) as Record; @@ -256,9 +330,10 @@ export function createDefaultDatasourceDriverFactory( return toHandle(driver); } - // memory + // memory — ephemeral per datasource unless the author opts into + // persistence, and then into a destination of its own (#4083). const { InMemoryDriver } = await import('@objectstack/driver-memory'); - return toHandle(new InMemoryDriver()); + return toHandle(new InMemoryDriver(buildMemoryConfig(spec))); }, }; } diff --git a/packages/services/service-datasource/src/sqlite-driver-fallback.ts b/packages/services/service-datasource/src/sqlite-driver-fallback.ts index aba044b993..f84e337a5b 100644 --- a/packages/services/service-datasource/src/sqlite-driver-fallback.ts +++ b/packages/services/service-datasource/src/sqlite-driver-fallback.ts @@ -189,7 +189,15 @@ export async function resolveSqliteDriver( } // 3. In-memory (mingo) — dev-only last resort. Not real SQL, not persistent. + // `persistence: false` is what makes that second half true: the driver's own + // default is `'auto'`, which in Node flushes the whole store to + // `.objectstack/data/memory-driver.json` in the CWD and reloads it next boot — + // a shared file that every memory pool in the process would alias (#4083). const { InMemoryDriver } = await import('@objectstack/driver-memory'); warn(NATIVE_SQLITE_MEMORY_FALLBACK_WARNING); - return { driver: new InMemoryDriver(), engine: 'memory', label: 'InMemoryDriver' }; + return { + driver: new InMemoryDriver({ persistence: false }), + engine: 'memory', + label: 'InMemoryDriver', + }; }