diff --git a/.changeset/transaction-degrade-and-cross-datasource-observability.md b/.changeset/transaction-degrade-and-cross-datasource-observability.md new file mode 100644 index 0000000000..f9667af761 --- /dev/null +++ b/.changeset/transaction-degrade-and-cross-datasource-observability.md @@ -0,0 +1,53 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): say out loud when `transaction()` is not giving you a transaction (#4619) + +`ObjectQL.transaction()` carries two caveats that are part of its **declared** +meaning (ADR-0119 D1, `packages/spec/src/contracts/objectql-engine.ts`), not +hidden behaviour: + +1. when the default driver has no `beginTransaction`, the callback runs with no + transaction and no rollback; +2. the transaction covers the **default** datasource only, so an object routed + elsewhere by `setDatasourceMapping` is written outside it. + +Declaring them is not the same as being able to observe them, and both were +completely mute. A caller asking for atomicity and not getting it had no way to +find out; a multi-datasource "atomic" unit of work that partially committed +reported nothing at all — one store reverted, the other kept its rows, and the +caller saw only that the whole thing failed. That is the same shape as +`batchData`'s `atomic` flag being a lie for as long as it was (ADR-0119 D4). + +**Nothing about what the engine does has changed.** Both caveats still hold +exactly as declared; this release only makes them discoverable. + +- **`warn`, once per driver per engine instance**, when `transaction()` (or + `ctx.api.transaction()` in a sandboxed hook/action body) degrades because the + driver has no `beginTransaction`. The line names the driver, the consequence + — writes commit as they execute, so a later throw leaves the earlier ones + persisted while the call rejects as if nothing had landed — and the fix. + `warn` rather than `error` on purpose: at that moment nothing has been lost, + a capability is simply absent, which is the functional-degradation branch + AGENTS.md keeps at `warn`. Once per driver because the drivers that reach + this path (test doubles, foreign engines) reach it on *every* call. + +- **`error`, once per transaction per datasource**, when an `insert`/`update`/ + `delete` inside an open `transaction()` is routed to a driver that + transaction does not cover. The line names the object, the datasource it went + to, the datasource the transaction was opened on, and says the write commits + on its own and will survive the rollback. `error` per AGENTS.md's judgment + question: afterwards the system looks entirely normal from the outside while + a write it claimed was part of an atomic unit has landed by itself — the + durability class, not the functional one. + +Both diagnostics are reported by the engine, so the direct +(`engine.transaction`) and sandboxed (`ScopedContext.transaction`) surfaces +share one budget and one wording rather than drifting apart. + +Tightening either caveat — an `opts.require` that throws instead of degrading, +refusing a cross-driver write, or surfacing an owned-vs-joined signal to the +callback — would change the contract's declared semantics and is deliberately +**not** done here; that half of #4619 is tracked separately against +`packages/spec`. diff --git a/packages/objectql/src/engine-transaction-observability.test.ts b/packages/objectql/src/engine-transaction-observability.test.ts new file mode 100644 index 0000000000..7e1fff6c12 --- /dev/null +++ b/packages/objectql/src/engine-transaction-observability.test.ts @@ -0,0 +1,399 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #4619 (ADR-0119 D1 follow-up, engine half) — `transaction()` carries two +// caveats that are part of its DECLARED meaning, not hidden behaviour: +// +// 1. when the default driver has no `beginTransaction`, the callback runs +// with no transaction and no rollback; +// 2. the transaction covers the DEFAULT datasource only, so an object routed +// elsewhere by `setDatasourceMapping` is written outside it. +// +// Declaring them is not the same as being able to observe them. Both used to be +// completely mute: a caller asking for atomicity and not getting it had no way +// to find out, and a multi-datasource "atomic" unit of work that partially +// committed reported nothing at all. These tests pin the diagnostics that make +// the two DISCOVERABLE. +// +// What they deliberately do NOT pin is any change of behaviour. Refusing the +// degrade (`opts.require`) or refusing the cross-driver write would tighten the +// declared contract in `packages/spec/src/contracts/objectql-engine.ts`; that +// half of #4619 is spec-lane work and is not done here. So every assertion +// below is of the form "the same thing happens, and now it is also said". + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; + +interface Recorded { + level: 'debug' | 'info' | 'warn' | 'error'; + message: string; + /** + * Everything after the message. The engine's logger contract is + * `(msg, Error | undefined, meta)` for `error` and `(msg, meta)` for `warn`, + * so the recorder keeps the raw tail and each assertion says which slot it + * means — same reasoning as `schema-sync-durability-log-level.test.ts`. + */ + args: unknown[]; +} + +function recordingLogger() { + const records: Recorded[] = []; + const push = (level: Recorded['level']) => (message: string, ...args: unknown[]) => + void records.push({ level, message: String(message), args }); + return { + records, + logger: { debug: push('debug'), info: push('info'), warn: push('warn'), error: push('error') }, + at(level: Recorded['level']) { + return records.filter((r) => r.level === level); + }, + }; +} + +/** Records that mention `needle` anywhere in the message. */ +function matching(records: Recorded[], needle: string): Recorded[] { + return records.filter((r) => r.message.includes(needle)); +} + +/** Meta object the engine passes in the trailing slot. */ +function meta(r: Recorded): Record { + return (r.args.find((a) => a !== undefined && typeof a === 'object' && !(a instanceof Error)) ?? + {}) as Record; +} + +/** + * A driver that records the writes it sees. `transactional: false` makes it a + * driver WITHOUT `beginTransaction` — the shape the degrade path exists for + * (test doubles and foreign engines; every in-tree driver implements it). + */ +function makeDriver(name: string, opts: { transactional?: boolean } = {}) { + const writes: Array<{ object: string; op: 'create' | 'update' | 'delete'; transaction: unknown }> = []; + const rows = new Map>(); + let nextId = 0; + const driver: any = { + name, + version: '0.0.0', + supports: {}, + writes, + async connect() {}, + async disconnect() {}, + async checkHealth() { return true; }, + async execute() { return null; }, + async find() { return Array.from(rows.values()); }, + async findOne(_o: string, ast: any) { + const id = ast?.where?.find?.((c: any) => c?.field === 'id')?.value; + if (id !== undefined) return rows.get(String(id)) ?? null; + for (const r of rows.values()) return r; + return null; + }, + async create(object: string, data: Record, options: any) { + writes.push({ object, op: 'create', transaction: options?.transaction }); + nextId += 1; + const id = (data.id as string) ?? `${name}_${nextId}`; + const row = { ...data, id }; + rows.set(id, row); + return row; + }, + async update(object: string, id: string, data: Record, options: any) { + writes.push({ object, op: 'update', transaction: options?.transaction }); + const row = { ...rows.get(String(id)), ...data, id }; + rows.set(String(id), row); + return row; + }, + async delete(object: string, id: string, options: any) { + writes.push({ object, op: 'delete', transaction: options?.transaction }); + return rows.delete(String(id)); + }, + async count() { return 0; }, + async bulkCreate(object: string, batch: Record[]) { + return Promise.all(batch.map((r) => this.create(object, r, undefined))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async syncSchema() {}, + }; + if (opts.transactional !== false) { + driver.beginTransaction = async () => ({ __trx: name }); + driver.commit = async () => {}; + driver.rollback = async () => {}; + } + return driver; +} + +// --------------------------------------------------------------------------- +// 1. Silent degrade → warn-once +// --------------------------------------------------------------------------- + +describe('transaction() degrade with no beginTransaction warns once (#4619)', () => { + async function engineWithoutTransactions() { + const rec = recordingLogger(); + const engine = new ObjectQL({ logger: rec.logger } as any); + const driver = makeDriver('memory', { transactional: false }); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject({ name: 'thing', fields: { name: { type: 'text' } } } as any); + return { rec, engine, driver }; + } + + it('warns — naming the driver, the consequence, and the fix — and still runs the callback', async () => { + const { rec, engine } = await engineWithoutTransactions(); + + let ran = false; + const result = await engine.transaction(async () => { + ran = true; + return 'value'; + }); + + // BEHAVIOUR IS UNCHANGED: the callback still runs and still returns. + // Tightening this into a throw is the spec half of #4619, not this PR. + expect(ran).toBe(true); + expect(result).toBe('value'); + + const warns = matching(rec.at('warn'), 'has no beginTransaction'); + expect(warns).toHaveLength(1); + // Names the driver it is talking about. + expect(warns[0].message).toContain("driver 'memory'"); + // The consequence, concretely (AGENTS.md: an operator log owes both). + expect(warns[0].message).toContain('WITHOUT transaction or rollback'); + expect(warns[0].message).toMatch(/PERSISTED/); + // The fix / the deliberate opt-out. + expect(warns[0].message).toContain('beginTransaction for this datasource'); + expect(warns[0].message).toContain('fail'); + expect(meta(warns[0])).toMatchObject({ datasource: 'memory' }); + }); + + it('is a WARN, not an error — nothing has been lost at the moment it fires', async () => { + const { rec, engine } = await engineWithoutTransactions(); + await engine.transaction(async () => undefined); + // AGENTS.md "Degradation log levels": this is the `if (!capability)` + // composition branch — the capability is absent and every write still + // lands. Escalating it to `error` is the mirror-image failure the same + // section names (it trains readers to skim `error`). + expect(matching(rec.at('error'), 'beginTransaction')).toHaveLength(0); + expect(matching(rec.at('warn'), 'has no beginTransaction')).toHaveLength(1); + }); + + it('does not repeat itself — five more calls add no further warnings', async () => { + const { rec, engine } = await engineWithoutTransactions(); + + for (let i = 0; i < 6; i += 1) { + await engine.transaction(async () => { + await engine.insert('thing', { name: `row_${i}` }); + }); + } + + // Six degrades, one line. The drivers that reach this path reach it on + // EVERY call, so per-call would be unreadable — which is the #4420 `warn` + // failure mode, not a fix for it. + expect(matching(rec.at('warn'), 'has no beginTransaction')).toHaveLength(1); + }); + + it('says nothing when the driver DOES support transactions', async () => { + const rec = recordingLogger(); + const engine = new ObjectQL({ logger: rec.logger } as any); + engine.registerDriver(makeDriver('memory'), true); + await engine.init(); + engine.registry.registerObject({ name: 'thing', fields: { name: { type: 'text' } } } as any); + + await engine.transaction(async () => { + await engine.insert('thing', { name: 'A' }); + }); + + expect(matching(rec.at('warn'), 'beginTransaction')).toHaveLength(0); + expect(matching(rec.at('error'), 'beginTransaction')).toHaveLength(0); + }); + + it('shares one budget with ScopedContext.transaction (ctx.api.transaction) — said once, not once per surface', async () => { + const { rec, engine } = await engineWithoutTransactions(); + + // The sandbox surface (`ctx.api.transaction(fn)`) is a SECOND + // implementation of the same degrade in the same file. It reports through + // the same engine-side helper, so "say it once" holds across both. + const scoped = (engine as any).createContext({ userId: 'u1' }); + let scopedRan = false; + await scoped.transaction(async () => { scopedRan = true; }); + expect(scopedRan).toBe(true); + expect(matching(rec.at('warn'), 'has no beginTransaction')).toHaveLength(1); + + await engine.transaction(async () => undefined); + expect(matching(rec.at('warn'), 'has no beginTransaction')).toHaveLength(1); + }); +}); + +// --------------------------------------------------------------------------- +// 2. Default-driver-only → loud cross-datasource routing diagnostic +// --------------------------------------------------------------------------- + +describe('a write inside transaction() routed off the default datasource is reported at error (#4619)', () => { + async function twoDatasourceEngine() { + const rec = recordingLogger(); + const engine = new ObjectQL({ logger: rec.logger } as any); + const primary = makeDriver('primary'); + const ledgerDs = makeDriver('ledger_db'); + const archiveDs = makeDriver('archive_db'); + engine.registerDriver(ledgerDs); + engine.registerDriver(archiveDs); + engine.registerDriver(primary, true); // default + await engine.init(); + // `ledger` and `archive` live elsewhere; `thing` stays on the default. + engine.setDatasourceMapping([ + { objectPattern: 'ledger', datasource: 'ledger_db' }, + { objectPattern: 'archive', datasource: 'archive_db' }, + ]); + engine.registry.registerObject({ name: 'thing', fields: { name: { type: 'text' } } } as any); + engine.registry.registerObject({ name: 'ledger', fields: { name: { type: 'text' } } } as any); + engine.registry.registerObject({ name: 'archive', fields: { name: { type: 'text' } } } as any); + const splits = () => matching(rec.at('error'), 'running OUTSIDE the transaction'); + return { rec, engine, primary, ledgerDs, archiveDs, splits }; + } + + it('reports the split — which object, which datasource, and that it is outside the transaction', async () => { + const { engine, ledgerDs, splits } = await twoDatasourceEngine(); + + await engine.transaction(async () => { + await engine.insert('thing', { name: 'covered' }); + await engine.insert('ledger', { name: 'NOT covered' }); + }); + + const found = splits(); + expect(found).toHaveLength(1); + expect(found[0].message).toContain("insert of 'ledger'"); + expect(found[0].message).toContain("datasource 'ledger_db'"); + expect(found[0].message).toContain("default datasource 'primary'"); + // The consequence, concretely, and the fix — both owed by an `error`. + expect(found[0].message).toContain('rolling the transaction back will NOT undo it'); + expect(found[0].message).toContain('datasourceMapping'); + expect(meta(found[0])).toMatchObject({ + object: 'ledger', + operation: 'insert', + datasource: 'ledger_db', + transactionDatasource: 'primary', + }); + + // BEHAVIOUR IS UNCHANGED: the write still went to the mapped datasource, + // exactly as before. This PR reports; refusing is the spec half. + expect(ledgerDs.writes).toHaveLength(1); + expect(ledgerDs.writes[0]).toMatchObject({ object: 'ledger', op: 'create' }); + + // And it is worse than "written without a transaction", which is what the + // contract's caveat says. `buildDriverOptions` reads the ambient handle + // with no idea which driver is about to receive it, so `ledger_db`'s driver + // is handed `primary`'s transaction object. Pinned here as OBSERVED, not + // endorsed — it predates this change (nothing here touches + // `buildDriverOptions`) and is filed separately; the assertion exists so + // that whoever fixes it sees this test, rather than a silent shift under a + // `toBeUndefined()` that was only ever a guess. + expect(ledgerDs.writes[0].transaction).toEqual({ __trx: 'primary' }); + }); + + it('does not fire for writes that stay on the default datasource', async () => { + const { engine, primary, splits } = await twoDatasourceEngine(); + + await engine.transaction(async () => { + await engine.insert('thing', { name: 'A' }); + await engine.insert('thing', { name: 'B' }); + }); + + expect(splits()).toHaveLength(0); + // and those writes really did ride the transaction + expect(primary.writes).toHaveLength(2); + expect(primary.writes[0].transaction).toBeTruthy(); + }); + + it('does not fire for a mapped write made OUTSIDE any transaction — no false positive', async () => { + const { engine, splits } = await twoDatasourceEngine(); + + // Routing an object to another datasource is a normal, supported thing to + // do. It is only a problem while a transaction is open and claiming to + // cover the work, so an ordinary write must stay silent. + const seeded = await engine.insert('ledger', { name: 'plain write' }); + await engine.update('ledger', { id: seeded.id, name: 'renamed' }); + await engine.delete('ledger', { where: { id: seeded.id } } as any); + + expect(splits()).toHaveLength(0); + }); + + it('says it once per transaction per datasource, and separately for a second datasource', async () => { + const { engine, splits } = await twoDatasourceEngine(); + + await engine.transaction(async () => { + await engine.insert('ledger', { name: 'a' }); + await engine.insert('ledger', { name: 'b' }); + await engine.insert('ledger', { name: 'c' }); + await engine.insert('archive', { name: 'd' }); + await engine.insert('archive', { name: 'e' }); + }); + + // Three writes to `ledger_db` + two to `archive_db` = two splits, not five. + // AGENTS.md: "say it once, at the first degradation, not once per failed + // write" — a 500-row batch off the default datasource is ONE split. + const found = splits(); + expect(found).toHaveLength(2); + expect(found.map((r) => meta(r).datasource).sort()).toEqual(['archive_db', 'ledger_db']); + }); + + it('re-reports in a NEW transaction — the budget is per transaction, not per engine', async () => { + const { engine, splits } = await twoDatasourceEngine(); + + await engine.transaction(async () => { await engine.insert('ledger', { name: 'first' }); }); + await engine.transaction(async () => { await engine.insert('ledger', { name: 'second' }); }); + + // Two units of work, two partial commits, two reports: each one is a + // separate atomicity claim that did not hold. + expect(splits()).toHaveLength(2); + }); + + it('covers update and delete, not just insert', async () => { + const { engine, splits } = await twoDatasourceEngine(); + + const seeded = await engine.insert('ledger', { name: 'seed' }); + + await engine.transaction(async () => { + await engine.update('ledger', { id: seeded.id, name: 'renamed' }); + }); + await engine.transaction(async () => { + await engine.delete('ledger', { where: { id: seeded.id } } as any); + }); + + const found = splits(); + expect(found).toHaveLength(2); + expect(found.map((r) => meta(r).operation)).toEqual(['update', 'delete']); + }); + + it('fires for a nested transaction() that JOINED the outer one (ADR-0067 D2)', async () => { + const { engine, splits } = await twoDatasourceEngine(); + + // A joined nested call does not open its own transaction — it runs inside + // the outer one's ambient scope, so the outer owner is what the write is + // measured against. The write is just as uncovered as at the top level. + await engine.transaction(async () => { + await engine.transaction(async () => { + await engine.insert('ledger', { name: 'nested' }); + }); + }); + + expect(splits()).toHaveLength(1); + expect(meta(splits()[0])).toMatchObject({ transactionDatasource: 'primary' }); + }); + + it('fires from ScopedContext.transaction too (ctx.api.transaction in a sandboxed hook body)', async () => { + const { engine, splits } = await twoDatasourceEngine(); + + const scoped = (engine as any).createContext({ userId: 'u1' }); + await scoped.transaction(async () => { + await engine.insert('ledger', { name: 'from sandbox' }); + }); + + expect(splits()).toHaveLength(1); + expect(meta(splits()[0])).toMatchObject({ datasource: 'ledger_db', transactionDatasource: 'primary' }); + }); + + it('stays silent after the transaction closes — the scope does not leak', async () => { + const { engine, splits } = await twoDatasourceEngine(); + + await engine.transaction(async () => { + await engine.insert('thing', { name: 'covered' }); + }); + await engine.insert('ledger', { name: 'after' }); + + expect(splits()).toHaveLength(0); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 173b3e7c46..ecc073165b 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -824,6 +824,33 @@ function eventMatchedCount(value: unknown): number | undefined { return value; } +/** + * What the engine knows about the transaction it opened, beyond the handle + * itself (#4619, ADR-0119 D1 follow-up). + * + * Purely an OBSERVABILITY record: nothing here changes which driver a write is + * routed to, whether a transaction is opened, or what is committed. It exists + * so the write path can tell a caller that a write it believes is inside the + * transaction is not — the one thing today's engine cannot say. + */ +interface TransactionScope { + /** + * The driver instance the open transaction belongs to. Compared by IDENTITY + * rather than by name: two drivers can transiently claim one name (see + * `registerDriver`'s collision branch), and identity is what actually decides + * whether a write rides this transaction's connection. + */ + readonly driver: IDataDriver; + /** The datasource name that driver is registered under — for the message. */ + readonly datasource: string; + /** + * Datasources already reported for THIS transaction. AGENTS.md's + * "say it once, at the first degradation, not once per failed write" — a + * 500-row batch routed elsewhere is one split, not 500. + */ + readonly reportedOutOfScope: Set; +} + export class ObjectQL implements IObjectQLEngine { /** * Ambient transaction store (ADR-0034). While a `transaction()` callback @@ -833,12 +860,38 @@ export class ObjectQL implements IObjectQLEngine { * instead of asking the pool for another one and deadlocking on the * single-connection SQLite pool. */ - private readonly txStore = new AsyncLocalStorage<{ transaction: unknown }>(); + private readonly txStore = new AsyncLocalStorage<{ + transaction: unknown; + /** + * Which driver actually owns this transaction, when the engine opened it + * (#4619). `transaction()` covers the DEFAULT datasource only — a caveat + * that is part of the declared contract (ADR-0119 D1) — so a write routed + * elsewhere by `setDatasourceMapping` runs OUTSIDE it and cannot be rolled + * back with it. Carrying the owner here is what lets the write path SAY so + * ({@link reportWriteOutsideTransaction}); it changes no routing. + * + * Absent on the sandbox runner's explicitly-threaded handles (the + * `beginTransaction`/`commit`/`rollback` trio does not use this store at + * all) and on any store entry an outside caller populated, so every reader + * must treat it as optional. + */ + scope?: TransactionScope; + }>(); private drivers = new Map(); private defaultDriver: string | null = null; private logger: Logger; + /** + * Datasources already reported by {@link warnTransactionUnsupported}, so the + * "no `beginTransaction`" degrade says its piece ONCE per engine instance per + * driver instead of once per `transaction()` call (#4619). Test doubles and + * foreign engines hit that path on every call; a per-call warning would be + * skimmed, which is the same unreadability that made the #4420 `warn` + * worthless. + */ + private readonly transactionUnsupportedReported = new Set(); + // Datasource mapping rules (imported from defineStack) private datasourceMapping: Array<{ namespace?: string; @@ -4859,6 +4912,8 @@ export class ObjectQL implements IObjectQLEngine { this.logger.debug('Insert operation starting', { object, isBatch: Array.isArray(data) }); this.assertWriteAllowed(object, 'insert'); const driver = this.getDriver(object); + // #4619 — diagnostic only, changes nothing about where this write goes. + this.reportWriteOutsideTransaction(object, driver, 'insert'); const opCtx: OperationContext = { object, @@ -5188,6 +5243,8 @@ export class ObjectQL implements IObjectQLEngine { this.logger.debug('Update operation starting', { object }); this.assertWriteAllowed(object, 'update'); const driver = this.getDriver(object); + // #4619 — diagnostic only, changes nothing about where this write goes. + this.reportWriteOutsideTransaction(object, driver, 'update'); // Fold the `filter` alias into `where` FIRST (#4346): everything below — // token resolution, the by-id fast path, the #2982 AST seeding — reads @@ -5744,6 +5801,8 @@ export class ObjectQL implements IObjectQLEngine { this.logger.debug('Delete operation starting', { object }); this.assertWriteAllowed(object, 'delete'); const driver = this.getDriver(object); + // #4619 — diagnostic only, changes nothing about where this write goes. + this.reportWriteOutsideTransaction(object, driver, 'delete'); // Fold the `filter` alias into `where` first — same reasoning as update() // above (#4346): unfolded, a `multi: true` delete with `{ filter }` had no @@ -6228,9 +6287,18 @@ export class ObjectQL implements IObjectQLEngine { * - If the default driver does not support `beginTransaction`, the callback * runs directly with the supplied base context (no rollback). This keeps * the API safe to call on drivers without ACID support (e.g. the - * in-memory driver in tests). + * in-memory driver in tests). It is DECLARED behaviour (ADR-0119 D1), not + * a bug to be discovered — but since v17 it is no longer *silent*: the + * degrade warns once per driver (#4619, {@link warnTransactionUnsupported}). * - On callback success the transaction is committed; on any thrown error * it is rolled back and the original error is re-thrown. + * - The transaction covers the DEFAULT datasource only — also declared + * (ADR-0119 D1). A write that `setDatasourceMapping` routes elsewhere runs + * OUTSIDE it and survives the rollback; that split is now reported at + * `error` from the write path (#4619, + * {@link reportWriteOutsideTransaction}). Reporting it does not fix it: + * refusing, or committing across drivers, would change the declared + * contract and is tracked by #4619's spec half. * * Use case: multi-step operations that must be atomic (e.g. CRM * `convertLead`, which creates an account + contact + opportunity + flips @@ -6255,6 +6323,9 @@ export class ObjectQL implements IObjectQLEngine { const driver = this.defaultDriver ? this.drivers.get(this.defaultDriver) : undefined; const drv = driver as any; if (!drv?.beginTransaction) { + // Declared degrade (ADR-0119 D1) — behaviour unchanged, but no longer + // mute: the caller asked for atomicity and is not getting it (#4619). + this.warnTransactionUnsupported(this.defaultDriver ?? drv?.name); return callback(baseContext); } const trx = await drv.beginTransaction(); @@ -6262,7 +6333,10 @@ export class ObjectQL implements IObjectQLEngine { try { // Run the callback inside the ambient transaction store so internal // queries during writes reuse this transaction's connection (ADR-0034). - const result = await this.txStore.run({ transaction: trx }, () => callback(trxCtx)); + const result = await this.txStore.run( + { transaction: trx, scope: this.newTransactionScope(driver!) }, + () => callback(trxCtx), + ); if (drv.commit) await drv.commit(trx); else if (drv.commitTransaction) await drv.commitTransaction(trx); return result; @@ -6277,6 +6351,121 @@ export class ObjectQL implements IObjectQLEngine { } } + /** + * Build the observability record for a transaction this engine just opened + * (#4619). See {@link TransactionScope} — records only, routes nothing. + */ + private newTransactionScope(owner: IDataDriver): TransactionScope { + return { + driver: owner, + datasource: this.datasourceNameOf(owner), + reportedOutOfScope: new Set(), + }; + } + + /** + * Which datasource name is `driver` registered under? + * + * `registerDriver` keys {@link drivers} by `driver.name`, so the two normally + * agree — but the map is the routing authority (`getDriver` resolves through + * it), so the map is what is searched, and `driver.name` is only the fallback + * for a driver that was never registered. Runs at most once per transaction + * (and once per out-of-scope report), never on the hot path. + */ + private datasourceNameOf(driver: IDataDriver): string { + for (const [name, registered] of this.drivers) { + if (registered === driver) return name; + } + return driver?.name ?? 'unknown'; + } + + /** + * A caller asked for a transaction and the driver cannot give it one (#4619). + * + * The behaviour is unchanged and DECLARED (ADR-0119 D1: "when that driver has + * no `beginTransaction` the callback runs with NO transaction and NO + * rollback"). What was missing is that a caller had no way to find out — + * the same shape as `batchData`'s `atomic` flag being a lie for as long as it + * was (ADR-0119 D4). Tightening this into a throw would change the declared + * contract and is deliberately NOT done here. + * + * `warn`, not `error`, on purpose. AGENTS.md's judgment question asks whether + * the system looks normal *while something it claims is persisted has not + * landed*; at this moment nothing has been lost — a capability simply is not + * there, and every write in the callback still lands. This is the + * `if (!capability)` composition branch the same section names as the + * usual `warn`; escalating it would train readers to skim `error`, which is + * exactly what made the #4420 `warn` unreadable. + * + * Once per engine instance per driver: the drivers that reach this path (test + * doubles, foreign engines) reach it on EVERY call. + */ + private warnTransactionUnsupported(datasource: string | undefined): void { + const name = datasource ?? ''; + if (this.transactionUnsupportedReported.has(name)) return; + this.transactionUnsupportedReported.add(name); + this.logger.warn( + `transaction() requested a transaction but driver '${name}' has no beginTransaction — ` + + 'running WITHOUT transaction or rollback. Every write the callback makes commits as it executes, ' + + 'so a later throw leaves the earlier ones PERSISTED even though the call rejects as if the whole ' + + 'unit of work had been undone; no caller is told, and the records stay behind. ' + + 'Register a driver that implements beginTransaction for this datasource, or have the caller fail ' + + "closed itself when it cannot tolerate losing atomicity (batchData's atomic gate, ADR-0119 D4, is " + + 'the pattern). Reported once per driver per engine instance.', + { datasource: name }, + ); + } + + /** + * A write inside an open `transaction()` was routed to a driver that + * transaction does not cover (#4619). + * + * `transaction()` opens on the DEFAULT datasource only — declared behaviour + * (ADR-0119 D1) — so an object that `setDatasourceMapping` (or an explicit + * `datasource:` binding, or lifecycle-class separation) routes elsewhere is + * written on another connection entirely. It commits immediately, the + * transaction's rollback cannot reach it, and today NOTHING says so: a failed + * "atomic" multi-datasource write reverts one store, keeps the other, and + * returns a clean rejection either way. + * + * `error`, per AGENTS.md's judgment question — after the degradation the + * system looks entirely normal from the outside while a write it claimed was + * part of an atomic unit has landed on its own. This is the durability class, + * not the functional one. + * + * Diagnostic ONLY: the write still goes exactly where routing sent it. + * Refusing the cross-driver write would change the declared contract and + * belongs to #4619's spec half. + */ + private reportWriteOutsideTransaction( + objectName: string, + driver: IDataDriver, + operation: 'insert' | 'update' | 'delete', + ): void { + const scope = this.txStore.getStore()?.scope; + // No engine-owned transaction in scope (or a handle threaded explicitly by + // the sandbox trio, which this store never sees) — nothing to be outside of. + if (!scope) return; + // Identity, not name: this is about riding the same connection. + if (driver === scope.driver) return; + const target = this.datasourceNameOf(driver); + if (scope.reportedOutOfScope.has(target)) return; + scope.reportedOutOfScope.add(target); + this.logger.error( + `${operation} of '${objectName}' inside transaction() is routed to datasource '${target}', but the ` + + `transaction was opened on the default datasource '${scope.datasource}' and covers only that one — ` + + 'so this write is running OUTSIDE the transaction. It commits on its own the moment it executes, and ' + + "rolling the transaction back will NOT undo it: a failed \"atomic\" unit of work reverts " + + `'${scope.datasource}' while these rows stay behind in '${target}', and the caller is told only that ` + + 'the whole thing failed. Keep every object written inside one transaction() on the default ' + + 'datasource (move the object, or drop the datasourceMapping rule that routes it away), or split the ' + + 'work into per-datasource units and have the caller reconcile them explicitly — cross-driver ' + + 'atomicity is not something this engine provides. Reported once per transaction per datasource.', + undefined, + { object: objectName, operation, datasource: target, transactionDatasource: scope.datasource }, + ); + } + // ============================================ // Compatibility / Convenience API // ============================================ @@ -6730,6 +6919,15 @@ export class ScopedContext { * * Falls back to non-transactional execution if the driver * does not support transactions. + * + * Carries BOTH of `ObjectQL.transaction`'s declared caveats (ADR-0119 D1) — + * default-datasource-only, and a silent degrade when that driver has no + * `beginTransaction` — because it is a second implementation of the same + * thing, reached from `ctx.api.transaction(fn)` in sandboxed hook and action + * bodies. Behaviour is unchanged, and so is the split: since #4619 both + * caveats report through the SAME engine-side helpers the engine's own + * `transaction()` uses, so the sandbox surface is no quieter than the direct + * one and "say it once" holds across both. */ async transaction(callback: (trxCtx: ScopedContext) => Promise): Promise { const engine = this.engine as any; @@ -6740,7 +6938,10 @@ export class ScopedContext { : undefined; if (!driver?.beginTransaction) { - // No transaction support — execute directly + // No transaction support — execute directly. Declared (ADR-0119 D1), but + // said out loud since #4619: the caller asked for atomicity and the + // callback is about to run without any. + engine.warnTransactionUnsupported?.(engine.defaultDriver ?? driver?.name); return callback(this); } @@ -6750,12 +6951,16 @@ export class ScopedContext { this.engine ); // Share the engine's ambient transaction store so internal queries during - // writes reuse this transaction's connection (ADR-0034). + // writes reuse this transaction's connection (ADR-0034). The store entry + // also carries WHICH driver owns the transaction (#4619) so the write path + // can report a write routed off it; `newTransactionScope` is the engine's, + // reached the same `as any` way as `txStore` itself. const txStore = (this.engine as any)?.txStore as - | { run(s: { transaction: unknown }, fn: () => R): R } + | { run(s: { transaction: unknown; scope?: unknown }, fn: () => R): R } | undefined; + const scope = engine.newTransactionScope?.(driver); const runIn = (fn: () => Promise): Promise => - txStore ? txStore.run({ transaction: trx }, fn) : fn(); + txStore ? txStore.run({ transaction: trx, scope }, fn) : fn(); try { const result = await runIn(() => callback(trxCtx));