From 499b3a3b0478fc1efab13d389b49bd1d32ef95ee Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 03:20:47 +0000 Subject: [PATCH] =?UTF-8?q?fix(driver-sqlite-wasm):=20persist=20RETURNING?= =?UTF-8?q?=20writes=20=E2=80=94=20unblock=20cold-boot=20e2e=20(#4518)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A file-backed sqlite-wasm database flushed its schema at boot and then recorded nothing else: every table on disk, every subsequent row only in the WASM heap. `bootStack({ databaseFile })` therefore could not cold-boot, which blocked #4470's third minimal form. Root cause is in the Knex dialect, not the harness. `_query` picked its execution branch from "does this statement return rows" and then set the dirty flag only on the other, row-less branch. `INSERT ... RETURNING *` returns rows — and that is the shape ObjectQL writes with — so it executed on the row-returning branch and never marked the database dirty. The `on-disconnect` flush is gated on the same flag, so both persist strategies dropped the write; `knex.raw('INSERT ...')` (no Knex `method`) was lost the same way. "Does this statement change the database?" is now one exported predicate, `statementMutatesDatabase(sql, method)`, classifying by method AND SQL text and applied at a single funnel after execution — independent of which branch ran it. Transaction control still routes to `noteTransactionControl` so flushes stay deferred until a transaction closes (#1494); mutating PRAGMA assignments now count as writes. `WasmSqliteConnection.markDirty()` loses its method argument: re-filtering there made the same decision in two places that could disagree, which is precisely how the branches diverged. Tests: a new driver-level suite pins every execution branch (all six fail when the fix is reverted), and `flow-durable-suspend.dogfood.test.ts` loses its KNOWN GAP — it now suspends, shuts the kernel down, cold-boots a second kernel over the same file, resumes there, and proves the result survives a third boot, plus the plain-record assertion that identified this as a driver defect rather than a suspended-run one. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012C2cd7tL8QDoZ2QKN3djJ5 --- .../wasm-sqlite-returning-writes-persist.md | 44 +++++ .../src/knex-wasm-dialect.ts | 129 ++++++++++---- ...lite-wasm-driver-returning-persist.test.ts | 161 +++++++++++++++++ .../driver-sqlite-wasm/src/wasm-connection.ts | 27 +-- .../test/flow-durable-suspend.dogfood.test.ts | 164 ++++++++++++++++-- packages/verify/src/harness.ts | 7 + 6 files changed, 471 insertions(+), 61 deletions(-) create mode 100644 .changeset/wasm-sqlite-returning-writes-persist.md create mode 100644 packages/plugins/driver-sqlite-wasm/src/sqlite-wasm-driver-returning-persist.test.ts diff --git a/.changeset/wasm-sqlite-returning-writes-persist.md b/.changeset/wasm-sqlite-returning-writes-persist.md new file mode 100644 index 0000000000..151f7840c3 --- /dev/null +++ b/.changeset/wasm-sqlite-returning-writes-persist.md @@ -0,0 +1,44 @@ +--- +"@objectstack/driver-sqlite-wasm": patch +--- + +fix(driver-sqlite-wasm): a `RETURNING` write is a write — persist it (#4518) + +A file-backed `sqlite-wasm` database flushed its schema at boot and then +recorded nothing else. Every table was on disk; every row written after schema +sync lived only in the WASM heap and died with the process. Reopening the file +found a complete, empty database. + +**Cause.** The Knex dialect picked its execution branch from *"does this +statement return rows"* — and then marked the database dirty only on the other, +row-less branch. `INSERT … RETURNING *` returns rows, so it executed on the +row-returning branch and never set the flag. Since the `on-disconnect` flush is +gated on the same flag, nothing rescued it afterwards either: **both** persist +strategies dropped the write. ObjectQL writes through `RETURNING *` (it hands +the stored row back to the caller), so this covered essentially all business +data, along with `knex.raw('INSERT …')` and any other mutation arriving without +a Knex `method`. + +**Fix.** "Does this statement change the database?" is now one exported +predicate — `statementMutatesDatabase(sql, method)` — classifying by Knex method +*and* SQL text, applied at a single funnel after execution. It is independent of +which branch executed the statement, so a mutation can no longer slip through by +returning rows, by arriving without a method, or by taking a branch that forgot +to say so. Transaction control still routes to `noteTransactionControl`, which +keeps deferring flushes until the transaction closes (#1494), and mutating +`PRAGMA` assignments (`auto_vacuum`, `user_version`) now count as writes too. + +**What changes for you.** Nothing to author. File-backed wasm SQLite now +actually persists under `on-write` / `debounced:*`, and `disconnect()` is a real +durability boundary: when it returns, committed data is on disk. This is what +`bootStack({ databaseFile })` in `@objectstack/verify` needed to make `stop()` → +second `bootStack` a genuine cold boot — the suspended-run restart proof +ADR-0019 promises is now asserted end to end in the dogfood gate. Expect more +disk writes than before on a file-backed dev database, because previously there +were almost none. + +**One internal signature moved.** `WasmSqliteConnection.markDirty(method?)` is +now `markDirty()`. It used to re-filter the caller's Knex method against its own +allowlist, which made "did this mutate?" a decision taken in two places that +could — and did — disagree. If you call it directly, drop the argument; the +dialect classifies, the connection obeys. diff --git a/packages/plugins/driver-sqlite-wasm/src/knex-wasm-dialect.ts b/packages/plugins/driver-sqlite-wasm/src/knex-wasm-dialect.ts index 9fc8b39c8b..3d84eb9535 100644 --- a/packages/plugins/driver-sqlite-wasm/src/knex-wasm-dialect.ts +++ b/packages/plugins/driver-sqlite-wasm/src/knex-wasm-dialect.ts @@ -92,13 +92,69 @@ function formatBindings(bindings: unknown[] | undefined): unknown[] { * Everything else — `select`, `first`, `pluck`, `columnInfo`, raw PRAGMA, * DDL with no `method` — is read with `all`/row iteration so Knex sees the * same response shape it would from better-sqlite3. + * + * ⚠️ This answers "how do I EXECUTE this statement", never "does this statement + * change the database" — an `INSERT … RETURNING *` is executed down the + * row-returning branch and mutates. Persistence is classified separately by + * {@link statementMutatesDatabase}; conflating the two is #4518. */ -function isReadMethod(method?: string, returning?: unknown): boolean { +function isRowReturningExecution(method?: string, returning?: unknown): boolean { if (method === 'insert' || method === 'update') return !!returning ? true : false; if (method === 'counter' || method === 'del') return false; return true; } +/** Knex `method` values that always denote a mutation. */ +const MUTATING_METHODS = new Set(['insert', 'update', 'del', 'counter']); + +/** Statement-control forms whose persistence is owned by the transaction lifecycle. */ +const TRANSACTION_CONTROL_RE = /^\s*(BEGIN|COMMIT|END|ROLLBACK|SAVEPOINT|RELEASE)\b/i; + +/** + * DDL / schema statements. `BEGIN…RELEASE` share this prefix set in SQLite's + * grammar but are transaction control, so they are matched (and routed) first. + */ +const DDL_RE = + /^\s*(CREATE|ALTER|DROP|BEGIN|COMMIT|ROLLBACK|SAVEPOINT|RELEASE|REINDEX|VACUUM|ATTACH|DETACH|TRUNCATE)\b/i; + +/** DML that changes rows, whatever execution branch it happens to run down. */ +const MUTATING_DML_RE = /^\s*(INSERT|UPDATE|DELETE|REPLACE|UPSERT)\b/i; + +/** + * PRAGMA forms that change bytes in the database file: any assignment + * (`PRAGMA auto_vacuum = INCREMENTAL`, `PRAGMA user_version = 3` — both + * persistent header state) and `incremental_vacuum`, which actually moves + * pages. Introspection PRAGMAs (`table_info`, `index_list`, …) are reads. + */ +const MUTATING_PRAGMA_RE = /^\s*PRAGMA\b(?:[^;]*=|\s+incremental_vacuum\b)/i; + +/** + * THE single answer to "did this statement change the database, so that the + * in-memory image must eventually be written back to disk?" + * + * It is deliberately independent of which execution branch {@link + * isRowReturningExecution} picks, because those are different questions and + * answering them with one predicate is what broke persistence in #4518: the + * ObjectQL engine writes through `INSERT … RETURNING *` / `UPDATE … RETURNING *` + * (it needs the stored row back), those run down the row-returning branch, and + * the dirty flag was only ever set on the other branch. The result was a + * file-backed database that flushed its schema and then silently stopped + * recording anything — a cold boot found every table present and every row + * gone, and `on-disconnect` did not save it either, because the final flush + * also keys off the same flag. + * + * Classifying by BOTH the Knex `method` and the SQL text means a mutation + * cannot slip through by arriving without a method (`knex.raw('INSERT …')`, + * seed/migration SQL) or by taking an unexpected branch. + */ +export function statementMutatesDatabase(sql: string, method?: string): boolean { + if (TRANSACTION_CONTROL_RE.test(sql)) return false; // owned by noteTransactionControl + if (method && MUTATING_METHODS.has(method)) return true; + if (DDL_RE.test(sql)) return true; + if (MUTATING_DML_RE.test(sql)) return true; + return MUTATING_PRAGMA_RE.test(sql); +} + /** * Resolve the upstream `knex/lib/dialects/sqlite3` class at runtime. * @@ -179,35 +235,27 @@ export function getClient_WasmSqlite(): any { const db = connection.raw; const bindings = formatBindings(obj.bindings); - // DDL / transactional control statements have no Knex `method`. sql.js's + // ── 1. EXECUTE ──────────────────────────────────────────────────────── + // Three execution shapes. None of them decides persistence: that is + // settled once, below, so a statement cannot mutate the database on a + // branch that forgot to say so (#4518). + + // DDL / transaction control have no Knex `method`. sql.js's // `prepare`+`step` silently no-ops on many of these (e.g. CREATE TABLE), // so route them through `run` which is implemented via `exec` and // actually mutates the database. PRAGMA is intentionally excluded — many // PRAGMA forms (e.g. `PRAGMA table_info(...)`, `foreign_key_list(...)`) // return rows used by Knex's schema introspection/columnInfo, and // `db.run` discards those rows. - const isDdl = - /^\s*(CREATE|ALTER|DROP|BEGIN|COMMIT|ROLLBACK|SAVEPOINT|RELEASE|REINDEX|VACUUM|ATTACH|DETACH|TRUNCATE)\b/i.test( - obj.sql, - ); - if (isDdl) { + if (DDL_RE.test(obj.sql)) { db.run(obj.sql, bindings as any); obj.response = []; - // Transaction-control statements are routed through - // `noteTransactionControl`, which owns flushing for the transaction - // lifecycle: it suppresses flushes while a transaction is open (sql.js - // `export()` closes+reopens the db, which would abort the txn) and - // performs a single flush once the transaction fully closes. Routing - // them away from `markDirty` avoids a second, racing flush on COMMIT. - if (/^\s*(BEGIN|COMMIT|END|ROLLBACK|SAVEPOINT|RELEASE)\b/i.test(obj.sql)) { - connection.noteTransactionControl(obj.sql); - } else { - connection.markDirty('run'); - } - return obj; - } - - if (isReadMethod(obj.method, obj.returning) || /^\s*PRAGMA\b/i.test(obj.sql)) { + } else if ( + isRowReturningExecution(obj.method, obj.returning) || + /^\s*PRAGMA\b/i.test(obj.sql) + ) { + // Row-returning branch. NOTE this is also where `INSERT … RETURNING *` + // and `UPDATE … RETURNING *` land — statements that very much write. const stmt = db.prepare(obj.sql); try { if (bindings.length) stmt.bind(bindings as any); @@ -219,21 +267,34 @@ export function getClient_WasmSqlite(): any { } finally { stmt.free(); } - return obj; + } else { + // Row-less write path: execute via `run` and capture SQLite's + // per-connection lastID / changes counters. + db.run(obj.sql, bindings as any); + const changes = db.getRowsModified(); + let lastID: number | bigint = 0; + if (obj.method === 'insert') { + const r = db.exec('SELECT last_insert_rowid() AS id'); + lastID = (r?.[0]?.values?.[0]?.[0] as number) ?? 0; + } + obj.response = []; + obj.context = { lastID, changes }; } - // Write path: execute via `run` (no row iteration needed) and capture - // SQLite's per-connection lastID / changes counters. - db.run(obj.sql, bindings as any); - const changes = db.getRowsModified(); - let lastID: number | bigint = 0; - if (obj.method === 'insert') { - const r = db.exec('SELECT last_insert_rowid() AS id'); - lastID = (r?.[0]?.values?.[0]?.[0] as number) ?? 0; + // ── 2. PERSIST ──────────────────────────────────────────────────────── + // Exactly one place decides whether the on-disk image is now stale. + // + // Transaction-control statements are routed to `noteTransactionControl`, + // which owns flushing across the transaction lifecycle: it suppresses + // flushes while a transaction is open (sql.js `export()` closes+reopens + // the db, which would abort the txn) and performs a single flush once the + // transaction fully closes. Routing them away from `markDirty` avoids a + // second, racing flush on COMMIT. + if (TRANSACTION_CONTROL_RE.test(obj.sql)) { + connection.noteTransactionControl(obj.sql); + } else if (statementMutatesDatabase(obj.sql, obj.method)) { + connection.markDirty(); } - obj.response = []; - obj.context = { lastID, changes }; - connection.markDirty(obj.method); return obj; } } diff --git a/packages/plugins/driver-sqlite-wasm/src/sqlite-wasm-driver-returning-persist.test.ts b/packages/plugins/driver-sqlite-wasm/src/sqlite-wasm-driver-returning-persist.test.ts new file mode 100644 index 0000000000..b4d4746de9 --- /dev/null +++ b/packages/plugins/driver-sqlite-wasm/src/sqlite-wasm-driver-returning-persist.test.ts @@ -0,0 +1,161 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { SqliteWasmDriver } from '../src/index.js'; +import { statementMutatesDatabase } from './knex-wasm-dialect.js'; + +/** + * #4518 — a mutation must mark the database dirty on EVERY execution branch. + * + * The dialect picks its execution branch from "does this statement return + * rows", and `INSERT … RETURNING *` returns rows — so it ran down the branch + * that never called `markDirty`. Since the final `on-disconnect` flush keys off + * the same flag, nothing rescued it either: a file-backed database flushed its + * schema at boot and then recorded nothing for the rest of its life. Every + * table present, every row missing. + * + * That is not a corner case for this stack. ObjectQL writes through + * `RETURNING *` because it hands the stored row back to the caller, so the + * defect covered essentially all business data — which is why it surfaced as + * "`bootStack({ databaseFile })` cannot cold-boot" rather than as a driver bug. + * + * These tests read the database back through a SECOND driver over the same + * file, so they assert what is on disk rather than what the live WASM heap + * still remembers. + */ +describe('SqliteWasmDriver persists mutations from every execution branch (#4518)', () => { + const dirs: string[] = []; + const drivers: SqliteWasmDriver[] = []; + + function newFile(): string { + const dir = mkdtempSync(join(tmpdir(), 'wasm-returning-')); + dirs.push(dir); + return join(dir, 'db.sqlite'); + } + + function track(d: SqliteWasmDriver): SqliteWasmDriver { + drivers.push(d); + return d; + } + + /** Reopen the on-disk image in a fresh driver — a cold read, in miniature. */ + async function readBack(file: string): Promise[]> { + const reopened = track(new SqliteWasmDriver({ filename: file, persist: 'on-disconnect' })); + return (reopened as any).knex('acct').orderBy('id'); + } + + afterEach(async () => { + await Promise.all(drivers.splice(0).map((d) => d.disconnect().catch(() => {}))); + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + it('classifies a statement by what it DOES, not by which branch executes it', () => { + // The row-returning branch and the mutating set overlap; that overlap is + // the whole defect, so it is pinned directly. + expect(statementMutatesDatabase('insert into "acct" ("id") values (?) returning *', 'insert')).toBe(true); + expect(statementMutatesDatabase('update "acct" set "name" = ? returning *', 'update')).toBe(true); + expect(statementMutatesDatabase("INSERT INTO acct (id) VALUES ('x')")).toBe(true); + expect(statementMutatesDatabase('DELETE FROM acct WHERE id = ?')).toBe(true); + expect(statementMutatesDatabase('CREATE TABLE acct (id text)')).toBe(true); + expect(statementMutatesDatabase('PRAGMA auto_vacuum = INCREMENTAL')).toBe(true); + + expect(statementMutatesDatabase('select * from "acct"', 'select')).toBe(false); + expect(statementMutatesDatabase('PRAGMA table_info(acct)')).toBe(false); + // Transaction control is owned by the transaction lifecycle, which must NOT + // flush mid-transaction: sql.js `export()` closes and reopens the database, + // aborting the open transaction (#1494). + expect(statementMutatesDatabase('BEGIN')).toBe(false); + expect(statementMutatesDatabase('COMMIT')).toBe(false); + expect(statementMutatesDatabase('SAVEPOINT sp1')).toBe(false); + }); + + it('flushes an `INSERT … RETURNING` under on-write — the shape ObjectQL writes with', async () => { + const file = newFile(); + const driver = track(new SqliteWasmDriver({ filename: file, persist: 'on-write' })); + await driver.initObjects([{ name: 'acct', fields: { name: { type: 'string' } } }]); + + const returned = await (driver as any) + .knex('acct') + .insert({ id: 'a1', name: 'returning-insert' }) + .returning('*'); + expect(returned.length).toBe(1); + + // `flush()` awaits the queued write; `on-write` schedules it fire-and-forget. + await (driver as any).flush(); + + const onDisk = await readBack(file); + expect(onDisk.map((r) => r.id)).toEqual(['a1']); + expect(onDisk[0].name).toBe('returning-insert'); + }); + + it('flushes an `UPDATE … RETURNING` under on-write', async () => { + const file = newFile(); + const driver = track(new SqliteWasmDriver({ filename: file, persist: 'on-write' })); + await driver.initObjects([{ name: 'acct', fields: { name: { type: 'string' } } }]); + await (driver as any).knex('acct').insert({ id: 'a1', name: 'before' }); + await (driver as any).flush(); + + await (driver as any).knex('acct').where('id', 'a1').update({ name: 'after' }).returning('*'); + await (driver as any).flush(); + + expect((await readBack(file))[0].name).toBe('after'); + }); + + it('flushes a raw mutation that carries no Knex `method`', async () => { + const file = newFile(); + const driver = track(new SqliteWasmDriver({ filename: file, persist: 'on-write' })); + await driver.initObjects([{ name: 'acct', fields: { name: { type: 'string' } } }]); + + await (driver as any).knex.raw("INSERT INTO acct (id, name) VALUES ('r1', 'raw')"); + await (driver as any).flush(); + + expect((await readBack(file)).map((r) => r.id)).toEqual(['r1']); + }); + + it('`on-disconnect` still saves RETURNING writes — disconnect() is the durability contract', async () => { + // The second half of the same defect: the final flush is gated on the same + // dirty flag, so an unmarked write was lost by BOTH persist strategies. + // This is the invariant `bootStack({ databaseFile }).stop()` rests on. + // + // The explicit `flush()` after schema creation is what makes this test + // DISCRIMINATING rather than incidentally green. Schema DDL did mark the + // database dirty even before the fix, and under `on-disconnect` that flag + // simply sat there until close — so the final export happened to carry the + // unmarked row along with it. Clearing the flag first reproduces what + // `on-write` did in production: every earlier flush reset it, and from then + // on nothing set it again. + const file = newFile(); + const driver = new SqliteWasmDriver({ filename: file, persist: 'on-disconnect' }); + drivers.push(driver); + await driver.initObjects([{ name: 'acct', fields: { name: { type: 'string' } } }]); + await (driver as any).flush(); + + await (driver as any).knex('acct').insert({ id: 'd1', name: 'durable' }).returning('*'); + + await driver.disconnect(); + + const onDisk = await readBack(file); + expect(onDisk.map((r) => r.id)).toEqual(['d1']); + expect(onDisk[0].name).toBe('durable'); + }); + + it('persists RETURNING writes committed inside a transaction (no mid-transaction export)', async () => { + const file = newFile(); + const driver = track(new SqliteWasmDriver({ filename: file, persist: 'on-write' })); + await driver.initObjects([{ name: 'acct', fields: { name: { type: 'string' } } }]); + const knex = (driver as any).knex; + + await knex.transaction(async (trx: any) => { + await trx('acct').insert({ id: 't1', name: 'in-tx' }).returning('*'); + await trx('acct').where('id', 't1').first(); + await trx('acct').insert({ id: 't2', name: 'in-tx-2' }).returning('*'); + }); + await (driver as any).flush(); + + expect((await readBack(file)).map((r) => r.id)).toEqual(['t1', 't2']); + }); +}); diff --git a/packages/plugins/driver-sqlite-wasm/src/wasm-connection.ts b/packages/plugins/driver-sqlite-wasm/src/wasm-connection.ts index c421dc04c2..f1dbaffb3a 100644 --- a/packages/plugins/driver-sqlite-wasm/src/wasm-connection.ts +++ b/packages/plugins/driver-sqlite-wasm/src/wasm-connection.ts @@ -36,15 +36,6 @@ export interface WasmConnectionOptions { logger?: { warn: (msg: string, meta?: unknown) => void }; } -/** Mutation method names that should trigger a persistence cycle. */ -const WRITE_METHODS = new Set([ - 'run', - 'insert', - 'update', - 'del', - 'counter', -]); - /** * Detect whether a Node-style `fs` module is available. WebContainer * (StackBlitz) provides Node `fs`; pure-browser environments do not. @@ -339,10 +330,22 @@ export class WasmSqliteConnection { } } - /** Hint that a mutation just executed; schedule a flush if needed. */ - markDirty(method?: string): void { + /** + * Record that the statement just executed CHANGED the database, and schedule + * a flush according to {@link persist}. + * + * Deliberately takes no argument. It used to filter the caller's Knex + * `method` against a local write-method allowlist, which made "did this + * mutate?" a decision taken in TWO places — here and in the dialect's + * execution-path branch — and the two disagreed: an `INSERT … RETURNING` + * runs down the dialect's ROW-returning branch (it has rows to return), that + * branch never called this method at all, and so a whole class of committed + * writes was never marked dirty and never reached disk (#4518). One decision, + * one owner: {@link statementMutatesDatabase} in the dialect classifies the + * statement, and this method just does what it is told. + */ + markDirty(): void { if (this.isEphemeral || !this.fs) return; - if (method && !WRITE_METHODS.has(method)) return; this.dirty = true; if (this.persist === 'on-write') { diff --git a/packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts b/packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts index 206a70a9e3..0159820af2 100644 --- a/packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts +++ b/packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts @@ -26,14 +26,16 @@ // CONSUME that row (leaving the `run_`-prefixed history row in its place), and // the screen contract is enforced against the persisted `screen_json`. // -// KNOWN GAP — the literal cold boot. #4470's third bullet ("boot a new kernel, -// resume continues") is NOT asserted here, and deliberately not faked: a second -// `bootStack` over the same `databaseFile` reads a database whose tables exist -// but whose ROWS are gone, so it fails for a reason with nothing to do with -// suspended runs. Ordinary records do not survive it either, which is what -// identifies it as a harness/driver persistence gap rather than a defect in the -// suspended-run store. Filed as #4518; when it is fixed, the natural next test -// here is the one this file was originally written around. +// The literal cold boot — #4470's third bullet — is the second `describe` at +// the bottom of this file. It was a KNOWN GAP for one release: a second +// `bootStack` over the same `databaseFile` used to read a database whose tables +// existed but whose ROWS were gone, and ordinary records did not survive it +// either, which is what identified it as a driver persistence defect rather +// than a suspended-run one. #4518 found it: the wasm SQLite dialect marked the +// database dirty only on its row-LESS execution branch, so every +// `INSERT … RETURNING *` — the shape ObjectQL writes with — mutated the WASM +// heap and never reached disk, under either persist strategy. With that fixed, +// the test this file was originally written around is finally assertable. import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { mkdtempSync, rmSync } from 'node:fs'; @@ -114,13 +116,10 @@ describe('objectstack verify FLOW: suspended runs really reach the database (#44 }); it('the durable row is what a rehydration would read — the whole SuspendedRun round-trips', async () => { - // The cold-boot half of #4470's ask cannot be asserted from this harness - // yet: file-backed sqlite-wasm data does not survive an in-process kernel - // restart here, so a second `bootStack` over the same file reads created - // tables with no rows (filed as a blocker — see the note at the end of this - // file). Rather than assert nothing, this pins the thing that failure mode - // would actually destroy: that every field `AutomationEngine` needs to - // REBUILD the pause is present and correctly shaped in the persisted row. + // The cold boot itself is asserted in the second describe below; this pins + // the thing a persistence failure would destroy one layer earlier: that + // every field `AutomationEngine` needs to REBUILD the pause is present and + // correctly shaped in the persisted row. // // That is exactly what #4420 lacked. Its store wrote into a table that did // not exist, so the row was absent entirely; a row carrying the full @@ -203,3 +202,138 @@ describe('objectstack verify FLOW: suspended runs really reach the database (#44 expect(good.status, await good.clone().text()).toBeLessThan(300); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// THE COLD BOOT (#4470 third bullet, unblocked by #4518). +// +// Everything above proves the pause reaches the database. That is only half of +// ADR-0019's promise: a suspended run exists to outlive the process that +// created it. So this suite writes in one kernel, SHUTS IT DOWN, boots a second +// kernel over the same file — one that has never seen the run — and resumes +// there. +// +// It asserts the plain record first, deliberately. When this was filed as +// #4518 the symptom presented as "suspended runs don't survive a restart", and +// the fact that identified it as a driver defect instead was that an ordinary +// business row did not survive either. Keeping both assertions here means a +// regression tells you WHICH layer broke instead of just that the cold boot +// failed. +// ───────────────────────────────────────────────────────────────────────────── +describe('objectstack verify FLOW: a suspended run survives a real cold boot (#4470 / #4518)', () => { + let dir: string; + let dbFile: string; + /** Kernel #1 — writes, suspends, then dies. */ + let hot: VerifyStack | undefined; + /** Kernel #2 — boots over kernel #1's file having never seen the run. */ + let cold: VerifyStack | undefined; + let coldToken: string; + let noteId: string; + let runId: string; + + beforeAll(async () => { + dir = mkdtempSync(join(tmpdir(), 'os-cold-boot-')); + dbFile = join(dir, 'verify.sqlite'); + + hot = await bootStack(durableSuspendStack, { automation: true, databaseFile: dbFile }); + const hotToken = await hot.signIn(); + + const created = await hot.apiAs(hotToken, 'POST', '/data/suspend_note', { + name: 'survives-the-restart', + status: 'new', + }); + expect(created.status, await created.clone().text()).toBeLessThan(300); + const cj = (await created.json()) as { id?: string; record?: { id?: string } }; + noteId = (cj.id ?? cj.record?.id) as string; + expect(noteId).toBeTruthy(); + + const triggered = await hot.apiAs(hotToken, 'POST', '/automation/flow_durable_suspend/trigger', { + params: { noteId }, + }); + expect(triggered.status, await triggered.clone().text()).toBeLessThan(300); + const tr = (await triggered.json()) as any; + runId = (tr.result ?? tr.data ?? tr).runId; + expect(runId, 'no runId on the paused result').toBeTruthy(); + + // The restart. `stop()` runs the real kernel shutdown — the plugin + // `destroy()` → datasource `disconnect()` → driver flush chain — and the + // contract this suite rests on is that when it RETURNS, the data is on + // disk. Nothing else is done to help it: no explicit flush, no sleep. + await hot.stop(); + hot = undefined; + + cold = await bootStack(durableSuspendStack, { automation: true, databaseFile: dbFile }); + coldToken = await cold.signIn(); + }, 180_000); + + afterAll(async () => { + await hot?.stop().catch(() => {}); + await cold?.stop().catch(() => {}); + if (dir) rmSync(dir, { recursive: true, force: true }); + }); + + it('ordinary business rows survive the restart — `stop()` returning means DURABLE', async () => { + const res = await cold!.apiAs(coldToken, 'GET', `/data/suspend_note/${noteId}`); + expect(res.status, `the note written by the first kernel is gone: ${await res.clone().text()}`).toBe(200); + const rec = ((await res.json()) as any).record ?? {}; + expect(rec.name).toBe('survives-the-restart'); + // Still mid-flow — the resume below is what moves it. + expect(rec.status).toBe('new'); + }); + + it('the `paused` row survives the restart with its continuation intact', async () => { + const row = await cold!.apiAs(coldToken, 'GET', `/data/sys_automation_run/${runId}`); + expect(row.status, `no sys_automation_run row for ${runId} after the cold boot`).toBe(200); + const rec = ((await row.json()) as any).record ?? {}; + expect(rec.status).toBe('paused'); + expect(rec.flow_name).toBe('flow_durable_suspend'); + expect(rec.node_id).toBe('ask'); + expect(rec.node_type).toBe('screen'); + expect(JSON.parse(String(rec.variables_json)).noteId).toBe(noteId); + }); + + it('the cold kernel RESUMES the run and takes the right branch', async () => { + // The one assertion #4470 was written for. The second kernel never + // executed a node of this run: it has to rebuild the continuation from + // `sys_automation_run` alone, and prove it by moving the RIGHT row. + const resumed = await cold!.apiAs( + coldToken, 'POST', `/automation/flow_durable_suspend/runs/${runId}/resume`, + { inputs: { resolution: 'resolved after a cold boot' } }, + ); + expect(resumed.status, await resumed.clone().text()).toBeLessThan(300); + + const note = await cold!.apiAs(coldToken, 'GET', `/data/suspend_note/${noteId}`); + const rec = ((await note.json()) as any).record ?? {}; + expect(rec.status).toBe('resolved'); + expect(rec.resolution).toBe('resolved after a cold boot'); + + // …and the run is consumed in the new process exactly as it would be in the + // old one: the `paused` row gone, the terminal history row in its place. + const gone = await cold!.apiAs(coldToken, 'GET', `/data/sys_automation_run/${runId}`); + expect(gone.status).toBe(404); + const history = await cold!.apiAs(coldToken, 'GET', `/data/sys_automation_run/run_${runId}`); + expect(history.status).toBe(200); + expect((((await history.json()) as any).record ?? {}).status).toBe('completed'); + }); + + it('the resumed result is itself durable — a THIRD boot still reads it', async () => { + // Otherwise "it resumed" could be true only of the WASM heap, which is the + // exact illusion #4518 was: writes that every in-process read confirmed and + // no restart ever saw. + await cold!.stop(); + cold = undefined; + + const third = await bootStack(durableSuspendStack, { automation: true, databaseFile: dbFile }); + try { + const token = await third.signIn(); + const note = await third.apiAs(token, 'GET', `/data/suspend_note/${noteId}`); + expect(note.status).toBe(200); + expect((((await note.json()) as any).record ?? {}).resolution).toBe('resolved after a cold boot'); + + const history = await third.apiAs(token, 'GET', `/data/sys_automation_run/run_${runId}`); + expect(history.status).toBe(200); + expect((((await history.json()) as any).record ?? {}).status).toBe('completed'); + } finally { + await third.stop().catch(() => {}); + } + }, 120_000); +}); diff --git a/packages/verify/src/harness.ts b/packages/verify/src/harness.ts index 8f41dafbf1..7ba38d0c14 100644 --- a/packages/verify/src/harness.ts +++ b/packages/verify/src/harness.ts @@ -131,6 +131,13 @@ export interface BootOptions { * path and the second is a genuine COLD BOOT over the first's data — the * restart a durable suspended run has to survive (ADR-0019). Callers own the * file's lifetime (create it under a temp dir, delete it after). + * + * **`stop()` is the durability boundary.** When it returns, everything + * committed before it is on disk: the kernel shutdown runs the datasource + * plugin's `destroy()` → `disconnect()` → the driver's final flush. Nothing + * else is needed — no explicit flush, no sleep — and a cold boot that finds + * tables but no rows is a driver bug, not a fixture that forgot to wait + * (which is exactly what #4518 turned out to be). */ databaseFile?: string; /**