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', + }; }