From 3aa9e1d14e38db72a970959f35c1bbec59f70600 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 08:28:11 +0000 Subject: [PATCH] =?UTF-8?q?fix(objectql):=20=E7=A6=BB=E6=95=A3=E4=BA=8B?= =?UTF-8?q?=E5=8A=A1=E4=B8=89=E4=BB=B6=E5=A5=97=E8=A1=A5=E9=BD=90=20ADR-00?= =?UTF-8?q?67=20D2=20=E7=9A=84=20ambient=20join=20(#6406)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #6168(PR #6403)给回调面(`ScopedContext.transaction(cb)`)补上了 D2 的 join, 够不到沙箱面:QuickJS body 里的 `ctx.api.transaction(fn)` 是 VM 侧糖,底下驱动 `__txBegin`/`__txCommit`/`__txRollback` 三个 host leaf,走的是 `ScopedContext` 的离散三件套 —— 另一个方法,自己没有 join 支。于是宿主 `engine.transaction()` 内的沙箱 body 依旧另开一个 driver 事务:多占一条连接(D2 要避开的单连接池死锁), 且自行 commit,写入**存活过外层回滚** —— 调用者被告知工作单元已撤销,而其中若干 行仍在,无报错无日志。 `beginTransaction()` 现在做与两个回调面同样的第一件事:在查驱动之前读引擎的 ambient store,有则返回**同一个句柄**的子 context,并在结果里报 `owned: false` (#5696 的信号,换成这一面能承载的形状)。`commitTransaction` / `rollbackTransaction` 对这种句柄**弃权**,commit/rollback 的唯一归属仍在外层。 joined 句柄上的显式 rollback 不做任何 driver rollback —— 这正是回调面给出的答案 (它的 joined 支同样没有自己的 rollback,抛错向外传播、由外层属主整体回滚)。沙箱 上这条映射是精确的:糖的 catch 分支调 `__txRollback` 后**原样重抛**,body 的失败 因此走出 VM、走出 hook,到达宿主属主。 弃权落在 `ScopedContext` 而非调用方,任何三件套调用方都不可能关掉不属于自己的 事务;QuickJS runner 另外在三条关闭路径(commit 叶、rollback 叶、以及超时 body 遗留事务的 teardown 清理)上尊重 `owned` 位 —— runner 只关自己开的。 实测而非假设:`__txBegin` 时刻沙箱叶**读得到**引擎 ambient store(叶跑在自宿主 `txStore.run` 一路 await 下来的链上),因此不需要额外的捕获机制。三件套仍然不能 **发布**(没有跨 begin→commit 的闭包可交给 `txStore.run`),自开的事务对 ambient 读者依旧不可见,#6167 的面不变。 测试:objectql `engine-ambient-transaction.test.ts` +8(join/commit 弃权/rollback 弃权/回滚持久性钉子/无 ambient 两条对照/已关闭不误 join/单连接池),runtime 新增 `sandbox/transaction-ambient-join.integration.test.ts` 6 例(真引擎 + 真 QuickJS 端到端), `quickjs-runner.test.ts` +3(手写 ctx.api 钉 runner 自身的 `owned` 接线)。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- .../sandbox-transaction-ambient-join.md | 46 +++ .../src/engine-ambient-transaction.test.ts | 184 ++++++++++++ packages/objectql/src/engine.ts | 150 ++++++++-- .../src/sandbox/quickjs-runner.test.ts | 127 +++++++++ .../runtime/src/sandbox/quickjs-runner.ts | 63 ++++- ...ansaction-ambient-join.integration.test.ts | 263 ++++++++++++++++++ 6 files changed, 804 insertions(+), 29 deletions(-) create mode 100644 .changeset/sandbox-transaction-ambient-join.md create mode 100644 packages/runtime/src/sandbox/transaction-ambient-join.integration.test.ts diff --git a/.changeset/sandbox-transaction-ambient-join.md b/.changeset/sandbox-transaction-ambient-join.md new file mode 100644 index 0000000000..196ee7a1cd --- /dev/null +++ b/.changeset/sandbox-transaction-ambient-join.md @@ -0,0 +1,46 @@ +--- +"@objectstack/objectql": patch +"@objectstack/runtime": patch +--- + +fix(objectql): the discrete transaction trio joins an open ambient transaction, so a sandbox body no longer opens a second one (#6406) + +#6168 taught the callback face (`ctx.api.transaction(fn)` on `ScopedContext`) +the ADR-0067 D2 join. It could not reach the SANDBOX face: a QuickJS hook or +action body's `ctx.api.transaction(fn)` is VM-side sugar over three host leaves +(`__txBegin` / `__txCommit` / `__txRollback`) that drive `ScopedContext`'s +discrete `beginTransaction` / `commitTransaction` / `rollbackTransaction` trio — +a different method, which had no join branch of its own. So a body running +inside a host `engine.transaction()` still opened a SECOND driver transaction: + +1. it asked the pool for a second connection — the deadlock D2 exists to avoid + on a single-connection pool (knex/SQLite); and +2. it committed itself, so its writes SURVIVED the outer rollback. The caller + was told the unit of work had been undone while some of its rows were still + there — no error, no log. + +`beginTransaction()` now makes the same first move as both callback faces: +before looking a driver up it reads the engine's ambient transaction store, and +where one is open it returns THAT handle in a child context, with `owned: false` +in its result (#5696's signal, in the shape this face can carry it). +`commitTransaction` and `rollbackTransaction` abstain for such a handle, so the +outer caller keeps the one and only commit/rollback. An explicit rollback of a +joined handle performs no driver rollback: that is the same answer the callback +faces give, where the joined branch has no rollback either and a throw +propagates to the outer owner, which rolls the whole unit back. In the sandbox +that path is exact — the sugar's catch reaches `__txRollback` and RE-THROWS, so +the body's failure travels out to the host owner. + +The abstention lives on `ScopedContext`, not in the caller, so no trio caller +can close a transaction it does not own. The QuickJS runner additionally +honours the `owned` bit at all three of its close paths (commit leaf, rollback +leaf, and the teardown cleanup that rolls back a transaction a timed-out body +left open) — the runner closes what the runner opened. + +Measured, not assumed: at `__txBegin` time the engine's ambient store IS +readable from the sandbox leaf (the leaf runs on a chain awaited down from the +host's `txStore.run`), which is why no separate capture mechanism is needed. +What the trio still cannot do is PUBLISH — with no closure spanning +begin→commit there is nothing to hand `txStore.run` — so a transaction it opens +itself stays invisible to ambient readers, exactly as before, and the #6167 +surface (handles the engine cannot attribute) is unchanged. diff --git a/packages/objectql/src/engine-ambient-transaction.test.ts b/packages/objectql/src/engine-ambient-transaction.test.ts index d06eda9c68..75d419a25e 100644 --- a/packages/objectql/src/engine-ambient-transaction.test.ts +++ b/packages/objectql/src/engine-ambient-transaction.test.ts @@ -403,3 +403,187 @@ describe('ScopedContext.transaction joins the ambient transaction (ADR-0067 D2, expect(seen.begins).toHaveLength(2); // the outer one, then a fresh one }); }); + +// --------------------------------------------------------------------------- +// #6406 — the THIRD implementation of the same primitive joins too +// --------------------------------------------------------------------------- +// +// The discrete `beginTransaction` / `commitTransaction` / `rollbackTransaction` +// trio is the face the QuickJS sandbox drives: a VM body's +// `ctx.api.transaction(fn)` is sugar over three host leaves, precisely because +// the body runs across many host event-loop turns with no closure spanning +// begin→commit. #6168 fixed the callback face and could not reach this one, so +// the ADR-0067 D2 violation stayed open here: inside a host +// `engine.transaction()` the trio opened a SECOND driver transaction, committed +// it itself, and its writes survived the outer rollback. +// +// These cases exercise the trio directly, which IS its contract (a caller holds +// the handle and closes it later). The end-to-end path through a real QuickJS +// body lives in @objectstack/runtime's +// `sandbox/transaction-ambient-join.integration.test.ts`, where the real caller +// is the real sandbox runner. +describe('ScopedContext trio joins the ambient transaction (ADR-0067 D2, #6406)', () => { + let engine: ObjectQL; + let seen: ReturnType['seen']; + let committedNames: ReturnType['committedNames']; + + beforeEach(async () => { + engine = new ObjectQL(); + const d = makeRollbackHonestDriver(); + seen = d.seen; + committedNames = d.committedNames; + engine.registerDriver(d.driver, true); + await engine.init(); + engine.registry.registerObject({ name: 'thing', fields: { name: { type: 'text' } } } as any, 'test'); + }); + + const scoped = () => (engine as any).createContext({ userId: 'u1' }) as ScopedContext; + + it('joins the outer transaction — same handle, owned: false, no second begin', async () => { + let begun: any; + let outerHandle: unknown; + + await engine.transaction(async (ctx: any) => { + outerHandle = ctx.transaction; + begun = await scoped().beginTransaction(); + }); + + expect(begun.owned).toBe(false); + expect(begun.handle).toBe(outerHandle); + expect(begun.ctx.transactionHandle).toBe(outerHandle); + // The whole point of D2: ONE begin, so ONE connection. + expect(seen.begins).toHaveLength(1); + }); + + it('commit ABSTAINS for a joined handle — the outer owner commits, once', async () => { + let commitsSeenInside = -1; + + await engine.transaction(async () => { + const s = scoped(); + const begun = (await s.beginTransaction())!; + await begun.ctx.object('thing').insert({ name: 'inner' }); + await s.commitTransaction(begun.handle); + // Before #6406 this committed the OUTER transaction from the inside: the + // inner writes landed early and the outer caller had nothing left to own. + commitsSeenInside = seen.commits.length; + }); + + expect(commitsSeenInside).toBe(0); + expect(seen.commits).toHaveLength(1); // the outer one, after its callback returned + expect(committedNames('thing')).toEqual(['inner']); + }); + + it('rollback ABSTAINS for a joined handle — mirroring the callback faces', async () => { + let rollbacksSeenInside = -1; + + await engine.transaction(async () => { + const s = scoped(); + const begun = (await s.beginTransaction())!; + await begun.ctx.object('thing').insert({ name: 'inner' }); + // An explicit rollback of a transaction this call JOINED. The callback + // faces have no rollback of their own on the joined branch — a throw + // propagates and the OUTER owner rolls the whole unit back — and this is + // the same answer in the shape the trio can express it: no driver + // rollback here, the outcome stays the outer caller's to decide. + await s.rollbackTransaction(begun.handle); + rollbacksSeenInside = seen.rollbacks.length; + }); + + expect(rollbacksSeenInside).toBe(0); + expect(seen.rollbacks).toHaveLength(0); + // The outer succeeded, so its unit of work — including the inner write — + // commits. Exactly what a joined callback that swallowed its own error gets. + expect(seen.commits).toHaveLength(1); + expect(committedNames('thing')).toEqual(['inner']); + }); + + it('the joined write is UNDONE by the outer rollback — the durability pin', async () => { + await expect( + engine.transaction(async () => { + await engine.insert('thing', { name: 'outer' }); + const s = scoped(); + const begun = (await s.beginTransaction())!; + await begun.ctx.object('thing').insert({ name: 'inner' }); + await s.commitTransaction(begun.handle); + throw new Error('outer boom'); + }), + ).rejects.toThrow('outer boom'); + + // Before #6406 the trio committed its own transaction, so 'inner' was still + // here after the outer rollback: a write the caller was told had been undone + // and had not been. Nothing failed, nothing was logged. + expect(committedNames('thing')).toEqual([]); + expect(seen.commits).toHaveLength(0); + expect(seen.rollbacks).toHaveLength(1); + // Both writes rode the ONE handle the outer call owns. + expect(seen.creates).toHaveLength(2); + expect(seen.creates[1].transaction).toBe(seen.creates[0].transaction); + }); + + it('with NO ambient transaction the trio still OPENS one — owned: true, commit lands', async () => { + const s = scoped(); + const begun = (await s.beginTransaction())!; + + expect(begun.owned).toBe(true); + await begun.ctx.object('thing').insert({ name: 'standalone' }); + await s.commitTransaction(begun.handle); + + expect(seen.begins).toHaveLength(1); + expect(seen.commits).toHaveLength(1); + expect(committedNames('thing')).toEqual(['standalone']); + }); + + it('with NO ambient transaction an explicit rollback still DISCARDS — owned, unchanged', async () => { + const s = scoped(); + const begun = (await s.beginTransaction())!; + + await begun.ctx.object('thing').insert({ name: 'discarded' }); + await s.rollbackTransaction(begun.handle); + + expect(seen.rollbacks).toHaveLength(1); + expect(committedNames('thing')).toEqual([]); + }); + + it('does not join a transaction that has already closed — the store does not leak', async () => { + await engine.transaction(async () => { + await engine.insert('thing', { name: 'covered' }); + }); + + const begun = (await scoped().beginTransaction())!; + expect(begun.owned).toBe(true); + expect(seen.begins).toHaveLength(2); // the outer one, then a fresh one + }); + + /** + * The OTHER half of D2's rationale — see the #6168 block above for why a + * refusing pool of size 1 stands in for the hang a real one would produce. + */ + it('never asks for a second connection — a single-connection pool survives the trio begin', async () => { + let checkedOut = false; + const checkouts: number[] = []; + const d = makeRollbackHonestDriver(); + d.driver.beginTransaction = async () => { + if (checkedOut) throw new Error('pool exhausted: no connection available (max=1)'); + checkedOut = true; + checkouts.push(checkouts.length + 1); + return { __trx: 'only' }; + }; + d.driver.commit = async () => { checkedOut = false; }; + d.driver.rollback = async () => { checkedOut = false; }; + + const oneConn = new ObjectQL(); + oneConn.registerDriver(d.driver, true); + await oneConn.init(); + oneConn.registry.registerObject({ name: 'thing', fields: { name: { type: 'text' } } } as any, 'test'); + + await expect( + oneConn.transaction(async () => { + const s = (oneConn as any).createContext({ userId: 'u1' }) as ScopedContext; + const begun = (await s.beginTransaction())!; + await s.commitTransaction(begun.handle); + }), + ).resolves.toBeUndefined(); + + expect(checkouts).toHaveLength(1); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 1f87bbd4f2..15aed66a56 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1135,9 +1135,11 @@ export class ObjectQL implements IObjectQLEngine { * happens to it: a business write is refused, a system ledger is carved out. * * 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. + * `beginTransaction`/`commit`/`rollback` trio never POPULATES this store — + * since #6406 it reads it, to join an ambient transaction, but a + * transaction the trio opens still cannot be published: there is no closure + * spanning begin→commit to hand `run`) and on any store entry an outside + * caller populated, so every reader must treat it as optional. */ scope?: TransactionScope; }>(); @@ -8182,9 +8184,12 @@ export class ScopedContext implements IScopedContext { * trio-held handle is invisible here and is NOT joined — which is what keeps * this branch from mistaking an explicitly-threaded handle for an ambient * one. The QuickJS sandbox drives `ctx.api.transaction(fn)` through that trio - * rather than through this method, so a VM-side body is outside this join; - * unattributable handles are the same surface #6167 tracks, and closing that - * needs handle ownership to become discoverable on `IDataDriver`. + * rather than through this method, so a VM-side body is outside THIS join — + * it gets its own, on the trio's `beginTransaction` since #6406, with the + * same semantics: same handle, `owned: false`, and commit/rollback abstaining + * in favour of the outer owner. Unattributable handles are the same surface + * #6167 tracks, and closing that needs handle ownership to become + * discoverable on `IDataDriver`. */ async transaction( callback: (trxCtx: ScopedContext, info: EngineTransactionInfo) => Promise, @@ -8275,6 +8280,45 @@ export class ScopedContext implements IScopedContext { } } + /** + * Handles this context JOINED (ADR-0067 D2) rather than opened, recorded by + * {@link beginTransaction} and consumed by {@link releaseJoinedHandle}. + * + * A joined handle belongs to an OUTER owner, so this context must not commit + * or roll it back. Keying by the handle object is what makes the abstention + * exact — a context that later opens one of its own gets a different handle + * and closes it normally. Entries are removed by the first commit/rollback + * that names them, so the set holds at most the transactions currently open + * through this (per-dispatch, short-lived) context. + */ + private readonly joinedHandles = new Set(); + + /** + * Was `handle` JOINED by this context rather than opened by it? Consumes the + * record, so the trio's terminal call is also what forgets the handle. + * + * Two independent signals, because the trio's callers are exactly the ones + * that cannot keep a closure on the stack and may not close on the same + * object they opened on: + * + * 1. this context's own {@link joinedHandles} record, and + * 2. identity with the CURRENT ambient handle — a handle the engine is + * holding open right now is, by construction, not one the trio opened + * (the trio never publishes into `txStore`, so a trio-owned handle is + * never the ambient one). + * + * Both point the same way and both fail SAFE: the ambiguous answer is + * "abstain", never "commit a transaction we do not own". + */ + private releaseJoinedHandle(handle: unknown): boolean { + if (this.joinedHandles.delete(handle)) return true; + if (handle == null) return false; + const ambient = (this.engine as any)?.txStore?.getStore?.() as + | { transaction?: unknown } + | undefined; + return ambient?.transaction === handle; + } + /** * Resolve the default driver, if it exposes transaction primitives. * Shared by {@link transaction} and the discrete begin/commit/rollback trio. @@ -8294,19 +8338,73 @@ export class ScopedContext implements IScopedContext { * This trio exists for callers that cannot keep a JS closure on the stack for * the lifetime of the transaction — chiefly the sandbox runner, where the * hook/action body's `ctx.api.transaction(fn)` is driven across many host - * event-loop turns via deferred promises. Across those `setImmediate` - * boundaries the engine's ambient `txStore` (AsyncLocalStorage) does NOT - * survive, so the transaction handle is threaded **explicitly**: `begin` - * returns a child ScopedContext carrying `transaction: trx` in its execution - * context, and `resolveTx` honors that explicit handle ahead of the ambient - * store. Every `object(...)` op on the returned context therefore reuses the - * one connection without relying on ALS. + * event-loop turns via deferred promises. With no closure spanning + * begin→commit there is nothing to hand `txStore.run`, so a transaction this + * trio opens can never be PUBLISHED into the engine's ambient store; the + * handle is threaded **explicitly** instead: `begin` returns a child + * ScopedContext carrying `transaction: trx` in its execution context, and + * `resolveTx` honors that explicit handle ahead of the ambient store. Every + * `object(...)` op on the returned context therefore reuses the one + * connection without relying on ALS — which is also what keeps it working + * across `setImmediate` boundaries an outside caller may schedule the + * commit from. * * Returns `null` when the driver has no transaction support — the caller then * runs non-transactionally against `this` (same graceful degrade as * {@link transaction}). - */ - async beginTransaction(): Promise<{ ctx: ScopedContext; handle: unknown } | null> { + * + * ## ADR-0067 D2 join (#6406) — the third face of one primitive + * + * `begin` JOINS an already-open ambient transaction instead of opening a + * nested driver one, exactly as `ObjectQL.transaction` always did and as + * {@link transaction} does since #6168. Without it, a QuickJS body's + * `ctx.api.transaction(fn)` — which reaches this trio, not {@link transaction} + * — opened a SECOND driver transaction inside a host `engine.transaction()`: + * a second connection (the deadlock D2 exists to avoid on a single-connection + * pool) whose `__txCommit` made its writes SURVIVE the outer rollback, with + * no error and no log. + * + * `owned` in the result is #5696's signal, in the shape this face can carry + * it: `false` says this call joined and the OUTER caller owns the one and + * only commit/rollback. Commit and rollback abstain for such a handle + * ({@link releaseJoinedHandle}) — the guarantee lives HERE rather than in + * each caller, so a caller that ignores the bit still cannot close a + * transaction it does not own. An explicit rollback of a joined handle + * therefore performs NO driver rollback here; it is the same answer the + * callback faces give, where the joined branch has no rollback of its own and + * a throw propagates to the outer owner, which rolls the whole unit back. + * + * DECLARED LIMIT, measured rather than assumed (#6406): the join reads the + * engine's ambient `txStore` at BEGIN time. On the sandbox path that store IS + * readable there — the leaf runs on a chain awaited down from the host's + * `txStore.run`, so the AsyncLocalStorage context is still current — which is + * why no separate capture mechanism is needed. What the trio still cannot do + * is PUBLISH: it has no closure spanning begin→commit to wrap in + * `txStore.run`, which is why its own handle is threaded explicitly and stays + * invisible to the ambient readers. A caller whose `begin` is scheduled from + * OUTSIDE the transaction's async context sees no ambient and opens its own, + * exactly as before — join is best-effort on visibility, on this face and on + * both callback faces alike. + */ + async beginTransaction(): Promise<{ ctx: ScopedContext; handle: unknown; owned: boolean } | null> { + // ADR-0067 D2 — JOIN before the driver lookup, the same first move and the + // same position as both callback faces (#6168 / #6406). An ambient + // transaction IS a transaction, so there is nothing to look a driver up for. + const ambient = (this.engine as any)?.txStore?.getStore?.() as + | { transaction?: unknown } + | undefined; + if (ambient?.transaction) { + // Threaded explicitly into the child context, identity-equal to the + // store's handle — so `buildDriverOptions` binds every op on it to the + // outer connection and `transactionCoversDriverFor` still attributes the + // handle to the OUTER owner (#5351 unchanged). + const ctx = new ScopedContext( + { ...this.executionContext, transaction: ambient.transaction }, + this.engine + ); + this.joinedHandles.add(ambient.transaction); + return { ctx, handle: ambient.transaction, owned: false }; + } const driver = this.txDriver(); if (!driver) return null; const trx = await driver.beginTransaction(); @@ -8314,19 +8412,35 @@ export class ScopedContext implements IScopedContext { { ...this.executionContext, transaction: trx }, this.engine ); - return { ctx, handle: trx }; + return { ctx, handle: trx, owned: true }; } - /** Commit a handle obtained from {@link beginTransaction}. */ + /** + * Commit a handle obtained from {@link beginTransaction}. + * + * ABSTAINS for a JOINED handle (#6406): committing a transaction this context + * did not open would land the inner writes early and take the outcome away + * from the outer owner — the durability half of the D2 defect. + */ async commitTransaction(handle: unknown): Promise { + if (this.releaseJoinedHandle(handle)) return; const driver = this.txDriver(); if (!driver) return; if (driver.commit) await driver.commit(handle); else if (driver.commitTransaction) await driver.commitTransaction(handle); } - /** Roll back a handle obtained from {@link beginTransaction}. */ + /** + * Roll back a handle obtained from {@link beginTransaction}. + * + * ABSTAINS for a JOINED handle (#6406), mirroring the callback faces: their + * joined branch issues no rollback either, and a throw inside it propagates + * to the outer owner, which rolls back the whole unit of work. Rolling the + * outer transaction back from here would be the mirror-image error — an inner + * failure silently discarding writes the outer caller has not finished with. + */ async rollbackTransaction(handle: unknown): Promise { + if (this.releaseJoinedHandle(handle)) return; const driver = this.txDriver(); if (!driver) return; if (driver.rollback) await driver.rollback(handle); diff --git a/packages/runtime/src/sandbox/quickjs-runner.test.ts b/packages/runtime/src/sandbox/quickjs-runner.test.ts index 434c7d22e9..c1815279a5 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.test.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.test.ts @@ -1079,6 +1079,133 @@ describe('QuickJSScriptRunner — ctx.api.transaction', () => { expect(r.value).toBe('ok'); expect(events).toEqual([{ op: 'insert', tx: null }]); }, 30000); + + // ------------------------------------------------------------------------- + // #6406 — `begin` may JOIN a transaction the host already had open + // (ADR-0067 D2). `owned: false` in its result says the OUTER caller owns the + // one and only commit/rollback, so every close path here abstains. + // + // `ScopedContext` abstains for a joined handle on its own side too, which is + // the guarantee that holds for every caller. These cases use a hand-written + // `ctx.api` that does NOT, so what they measure is this file's own wiring: + // whether the runner asks at all. The end-to-end behaviour against the real + // engine is `transaction-ambient-join.integration.test.ts`. + // ------------------------------------------------------------------------- + /** Like {@link makeTxApi}, but `begin` JOINS a handle the host already holds. */ + function makeJoinedTxApi() { + const events: Array<{ op: string; name?: string; tx: unknown }> = []; + const outerHandle = { __outer: true }; + const repoFor = (tx: unknown) => (name: string) => ({ + insert: async () => { events.push({ op: 'insert', name, tx }); return { id: 'r' }; }, + findOne: async () => { events.push({ op: 'findOne', name, tx }); return null; }, + }); + const api = { + object: repoFor(null), + beginTransaction: async () => { + events.push({ op: 'begin(joined)', tx: outerHandle }); + return { ctx: { object: repoFor(outerHandle) }, handle: outerHandle, owned: false }; + }, + commitTransaction: async (h: unknown) => { events.push({ op: 'commit', tx: h }); }, + rollbackTransaction: async (h: unknown) => { events.push({ op: 'rollback', tx: h }); }, + }; + return { api, events, outerHandle }; + } + + it('does NOT commit a JOINED transaction — the outer owner does', async () => { + const { api, events, outerHandle } = makeJoinedTxApi(); + const r = await runner.runScript( + { + language: 'js', + source: ` + return await ctx.api.transaction(async () => { + await ctx.api.object('a').insert({ x: 1 }); + return 'ok'; + }); + `, + capabilities: ['api.write', 'api.transaction'], + timeoutMs: 30000, + }, + ctx({ api }), + actionOpts, + ); + + expect(r.value).toBe('ok'); + // The in-tx op still rides the OUTER handle — joining is what puts it on + // the one connection — but nothing here closes that transaction. + expect(events).toEqual([ + { op: 'begin(joined)', tx: outerHandle }, + { op: 'insert', name: 'a', tx: outerHandle }, + ]); + }, 30000); + + it('does NOT roll back a JOINED transaction when the body throws — the error propagates instead', async () => { + const { api, events } = makeJoinedTxApi(); + await expect( + runner.runScript( + { + language: 'js', + source: ` + await ctx.api.transaction(async () => { + await ctx.api.object('a').insert({ x: 1 }); + throw new Error('boom'); + }); + `, + capabilities: ['api.write', 'api.transaction'], + timeoutMs: 30000, + }, + ctx({ api }), + actionOpts, + ), + ).rejects.toThrow(/boom/); + + // The sugar's catch reaches `__txRollback`, which abstains, and re-throws — + // so the host owner hears the failure and decides for the whole unit. + expect(events.map((e) => e.op)).toEqual(['begin(joined)', 'insert']); + }, 30000); + + it('does NOT roll back a JOINED transaction the body leaves open at the wall ceiling', async () => { + const events: Array<{ op: string; tx: unknown }> = []; + const outerHandle = { __outer: true }; + const api = { + object: () => ({ insert: () => new Promise(() => {}) }), + beginTransaction: async () => { + events.push({ op: 'begin(joined)', tx: outerHandle }); + return { + ctx: { object: () => ({ insert: () => new Promise(() => {}) }) }, + handle: outerHandle, + owned: false, + }; + }, + commitTransaction: async (h: unknown) => { events.push({ op: 'commit', tx: h }); }, + rollbackTransaction: async (h: unknown) => { events.push({ op: 'rollback', tx: h }); }, + }; + + const r = new QuickJSScriptRunner({ wallCeilingMs: 300 }); + await expect( + r.runScript( + { + language: 'js', + source: ` + await ctx.api.transaction(async () => { + await ctx.api.object('a').insert({ x: 1 }); + }); + `, + capabilities: ['api.write', 'api.transaction'], + timeoutMs: 300, + }, + ctx({ api }), + actionOpts, + ), + ).rejects.toThrow(/ceiling/i); + + // The owned case above rolls back here, to avoid leaking a half-applied + // transaction on a connection nobody else holds. A JOINED handle is the + // host's live transaction on the host's connection: rolling it back from a + // VM teardown would discard writes the outer caller has not finished with, + // and there is nothing to leak — the timeout error reaches that caller, + // which decides. + expect(events.map((e) => e.op)).toEqual(['begin(joined)']); + }, 10000); }); // --------------------------------------------------------------------------- diff --git a/packages/runtime/src/sandbox/quickjs-runner.ts b/packages/runtime/src/sandbox/quickjs-runner.ts index 5d29bcc619..c0ba1175c3 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.ts @@ -226,7 +226,7 @@ export class QuickJSScriptRunner implements ScriptRunner { // closes it on commit/rollback. The execute() finally consults it to roll // back a transaction the body left open (threw mid-tx, or timed out before // its commit/rollback settled). - const txState: TxState = { api: null, handle: null, open: false }; + const txState: TxState = { api: null, handle: null, open: false, owned: true }; // Every host call hands the VM a `vm.newPromise()` deferred; the newPromise // contract requires each be `dispose()`d. On the settled path the runner @@ -432,7 +432,14 @@ export class QuickJSScriptRunner implements ScriptRunner { // connection isn't leaked with a half-applied transaction. Best-effort: // the script result (success or the original error) is already decided; // a rollback failure here must not mask it. - if (txState.open && txState.handle != null) { + // + // Only for a transaction this runner OPENED (#6406). A JOINED one belongs + // to the host `engine.transaction()` that is still in flight above us: + // rolling it back from a VM teardown would discard writes the outer caller + // has not finished with. There is nothing to leak either — the connection + // is the outer one, and the error that cut this body off propagates to the + // outer owner, which decides commit vs rollback for the whole unit. + if (txState.open && txState.owned && txState.handle != null) { const apiTx = args.ctx.api as Record | undefined; const rollback = apiTx?.rollbackTransaction; if (typeof rollback === 'function') { @@ -533,10 +540,17 @@ export class QuickJSScriptRunner implements ScriptRunner { // // The handle is threaded EXPLICITLY through `txState` rather than via the // engine's ambient AsyncLocalStorage: the body runs across many host - // event-loop turns, and ALS context does not survive those `setImmediate` - // boundaries. While a tx is open, `installApiMethod` resolves its repository - // from `txState.api` (the tx-scoped ScopedContext) so every op reuses the - // one connection. + // event-loop turns with no single closure spanning begin→commit, so there + // is nothing to hand `txStore.run` and a transaction opened here can never + // be published into that store. While a tx is open, `installApiMethod` + // resolves its repository from `txState.api` (the tx-scoped ScopedContext) + // so every op reuses the one connection. + // + // Reading the store is a different matter, and is what `beginTransaction` + // does on the engine side to JOIN a host transaction already in flight + // (ADR-0067 D2, #6406) — a hook body that runs inside `engine.transaction()` + // must not open a second driver transaction. `txState.owned` carries that + // verdict back to the three close paths below. const apiTx = ctx.api as Record | undefined; const installTxLeaf = (name: string, run: () => Promise): void => { const fn = vm.newFunction(name, () => { @@ -569,11 +583,17 @@ export class QuickJSScriptRunner implements ScriptRunner { installTxLeaf('__txBegin', async () => { if (txState.open) throw new SandboxError('nested ctx.api.transaction is not supported'); const begin = apiTx?.beginTransaction; + txState.owned = true; if (typeof begin === 'function') { - const r = (await (begin as () => Promise<{ ctx: unknown; handle: unknown } | null>).call(apiTx)) ?? null; + const r = (await (begin as () => Promise<{ ctx: unknown; handle: unknown; owned?: boolean } | null>).call(apiTx)) ?? null; if (r) { txState.api = r.ctx as Record; txState.handle = r.handle; + // ADR-0067 D2 (#6406): `begin` JOINS a host transaction that is + // already open rather than nesting a second driver one, and says so. + // Absent (a foreign `ctx.api` that predates the signal) reads as + // owned — the same answer this file gave before the bit existed. + txState.owned = r.owned !== false; } } // else (or null result): driver without tx support → degrade to @@ -582,23 +602,32 @@ export class QuickJSScriptRunner implements ScriptRunner { }); installTxLeaf('__txCommit', async () => { - const { handle, open } = txState; + const { handle, open, owned } = txState; txState.api = null; txState.handle = null; txState.open = false; + txState.owned = true; const commit = apiTx?.commitTransaction; - if (open && handle != null && typeof commit === 'function') { + // A JOINED transaction is committed by whoever opened it, not here + // (#6406): committing early would land the body's writes outside the + // outer caller's control and let them survive its rollback. + if (open && owned && handle != null && typeof commit === 'function') { await (commit as (h: unknown) => Promise).call(apiTx, handle); } }); installTxLeaf('__txRollback', async () => { - const { handle, open } = txState; + const { handle, open, owned } = txState; txState.api = null; txState.handle = null; txState.open = false; + txState.owned = true; const rollback = apiTx?.rollbackTransaction; - if (open && handle != null && typeof rollback === 'function') { + // Joined: abstain here too, exactly as the callback faces do (#6406). + // This leaf is reached from the `ctx.api.transaction` sugar's catch + // branch, which RE-THROWS afterwards, so the body's failure travels out + // to the host and the outer owner rolls the whole unit of work back. + if (open && owned && handle != null && typeof rollback === 'function') { await (rollback as (h: unknown) => Promise).call(apiTx, handle); } }); @@ -756,6 +785,18 @@ interface TxState { api: Record | null; handle: unknown; open: boolean; + /** + * Did `__txBegin` OPEN this transaction, or JOIN one the host already had + * open (ADR-0067 D2, #6406)? `false` means the outer caller owns the one and + * only commit/rollback, so every close path here abstains — the commit leaf, + * the rollback leaf, and the teardown cleanup in `execute`'s finally. + * + * `ScopedContext` abstains for a joined handle on its own side too, so a + * caller that ignored this bit still could not close someone else's + * transaction. Honouring it here is what makes THIS file's intent readable: + * the runner closes what the runner opened. + */ + owned: boolean; } /** diff --git a/packages/runtime/src/sandbox/transaction-ambient-join.integration.test.ts b/packages/runtime/src/sandbox/transaction-ambient-join.integration.test.ts new file mode 100644 index 0000000000..862929daad --- /dev/null +++ b/packages/runtime/src/sandbox/transaction-ambient-join.integration.test.ts @@ -0,0 +1,263 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * End-to-end regression for #6406 — ADR-0067 D2 on the SANDBOX face. + * + * A QuickJS hook body's `ctx.api.transaction(fn)` is VM-side sugar over three + * host leaves (`__txBegin` / `__txCommit` / `__txRollback`), which drive + * `ScopedContext`'s discrete trio — NOT the callback `ScopedContext.transaction` + * that #6168 (PR #6403) taught to join. So the violation that PR closed on the + * callback face stayed open here: a body running inside a host + * `engine.transaction()` opened a SECOND driver transaction, which + * + * 1. asks the pool for a second connection — the deadlock D2 exists to avoid + * on a single-connection pool, and + * 2. committed itself, so its writes SURVIVED the outer rollback: the caller + * was told the unit of work was undone while some of its rows were still + * there, with no error and no log. + * + * Everything below wires a REAL {@link ObjectQL} engine to the REAL + * {@link QuickJSScriptRunner} through {@link hookBodyRunnerFactory}, so the + * whole host-transaction → write → hook → VM → host-leaf path is exercised. The + * driver double is ROLLBACK-HONEST (writes are staged per handle; `commit` + * flushes, `rollback` discards), which is what lets these cases pin the fact the + * issue is actually about — whether the row is still there afterwards — rather + * than merely "rollback was called". + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL, bindHooksToEngine } from '@objectstack/objectql'; +import { hookBodyRunnerFactory } from './body-runner.js'; +import { QuickJSScriptRunner } from './quickjs-runner.js'; + +function makeRollbackHonestDriver() { + const committed = new Map>(); + const staged = new Map>(); + const seen = { + begins: [] as unknown[], + commits: [] as unknown[], + rollbacks: [] as unknown[], + creates: [] as Array<{ object: string; transaction: unknown }>, + }; + let nextId = 0; + let nextTrx = 0; + const committedFor = (o: string) => { + let s = committed.get(o); + if (!s) { s = new Map(); committed.set(o, s); } + return s; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + // Reads see COMMITTED state only — nothing here reads back its own + // uncommitted write, and it keeps the durability assertion unambiguous. + async find(object: string) { return Array.from(committedFor(object).values()); }, + async findOne(object: string) { for (const r of committedFor(object).values()) return r; return null; }, + async create(object: string, data: Record, options: any) { + const trx = options?.transaction; + seen.creates.push({ object, transaction: trx }); + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + if (trx === undefined) committedFor(object).set(id, row); + else staged.set(trx, [...(staged.get(trx) ?? []), { object, row }]); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = committedFor(object); + const row = { ...s.get(id), ...data, id }; + s.set(id, row); + return row; + }, + async delete(object: string, id: string) { return committedFor(object).delete(id); }, + async count() { return 0; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r, undefined))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, async syncSchema() {}, + async beginTransaction() { + nextTrx += 1; + const handle = { __trx: nextTrx }; + seen.begins.push(handle); + staged.set(handle, []); + return handle; + }, + async commit(trx: unknown) { + seen.commits.push(trx); + for (const { object, row } of staged.get(trx) ?? []) committedFor(object).set(row.id, row); + staged.delete(trx); + }, + async rollback(trx: unknown) { seen.rollbacks.push(trx); staged.delete(trx); }, + }; + const committedNames = (object: string) => + Array.from(committedFor(object).values()).map((r) => r.name).sort(); + return { driver, seen, committedNames }; +} + +/** + * A hook whose BODY runs in QuickJS and opens a transaction — the real + * reachability path the issue names. The `name` guard matters: the body's own + * write fires this hook again, and without it the join would be measured + * against that recursion instead of against the host's transaction. + */ +const TX_BODY = ` + if (ctx.input.name !== 'outer') return; + await ctx.api.transaction(async () => { + await ctx.api.object('thing').insert({ name: 'inner' }); + }); +`; + +const THROWING_TX_BODY = ` + if (ctx.input.name !== 'outer') return; + await ctx.api.transaction(async () => { + await ctx.api.object('thing').insert({ name: 'inner' }); + throw new Error('body boom'); + }); +`; + +describe('#6406 sandbox ctx.api.transaction joins the host transaction (real engine + real QuickJS)', () => { + let engine: ObjectQL; + let seen: ReturnType['seen']; + let committedNames: ReturnType['committedNames']; + + const wire = async (source: string, driverOverrides?: (d: any) => void) => { + engine = new ObjectQL(); + const d = makeRollbackHonestDriver(); + driverOverrides?.(d.driver); + seen = d.seen; + committedNames = d.committedNames; + engine.registerDriver(d.driver, true); + await engine.init(); + engine.registry.registerObject({ name: 'thing', fields: { name: { type: 'text' } } } as any, 'test'); + engine.setDefaultBodyRunner( + hookBodyRunnerFactory(new QuickJSScriptRunner(), { ql: engine, appId: 'test' }), + ); + bindHooksToEngine( + engine, + [ + { + name: 'sandbox_opens_a_transaction', + object: 'thing', + events: ['afterInsert'], + body: { language: 'js', source, capabilities: ['api.write', 'api.transaction'] }, + } as any, + ], + { packageId: 'test' }, + ); + }; + + beforeEach(() => { seen = undefined as any; }); + + it('JOINS the host transaction — one begin, one handle, no commit of its own', async () => { + await wire(TX_BODY); + + await engine.transaction(async () => { + await engine.insert('thing', { name: 'outer' }); + }); + + // ONE driver transaction for the whole unit of work — the connection half + // of D2. Before #6406 there were two begins and two commits. + expect(seen.begins).toHaveLength(1); + expect(seen.commits).toHaveLength(1); + expect(seen.creates).toHaveLength(2); + expect(seen.creates[1].transaction).toBe(seen.creates[0].transaction); + expect(seen.creates[0].transaction).toBe(seen.begins[0]); + expect(committedNames('thing')).toEqual(['inner', 'outer']); + }, 30000); + + it('the sandbox write is UNDONE by the outer rollback — the durability pin', async () => { + await wire(TX_BODY); + + await expect( + engine.transaction(async () => { + await engine.insert('thing', { name: 'outer' }); + throw new Error('outer boom'); + }), + ).rejects.toThrow('outer boom'); + + // Before #6406: `committedNames('thing')` was `['inner']` — the VM's write + // committed on its own transaction and outlived the rollback the caller was + // told had undone the whole unit. Nothing failed, nothing was logged. + expect(committedNames('thing')).toEqual([]); + expect(seen.commits).toHaveLength(0); + expect(seen.rollbacks).toHaveLength(1); + expect(seen.begins).toHaveLength(1); + }, 30000); + + it('a throw inside the JOINED body rolls back the OUTER transaction, once', async () => { + await wire(THROWING_TX_BODY); + + // The sugar's catch branch reaches `__txRollback`, which ABSTAINS for a + // joined handle, and then RE-THROWS — so the failure travels out of the VM, + // out of the hook, and up to the host owner, which rolls the whole unit + // back. That is the callback face's answer to "explicit rollback of a + // joined transaction" (its joined branch has no rollback either), in the + // shape the trio can express it. + await expect( + engine.transaction(async () => { + await engine.insert('thing', { name: 'outer' }); + }), + ).rejects.toThrow(/body boom/); + + expect(seen.rollbacks).toHaveLength(1); // the OUTER one, not an inner one + expect(seen.rollbacks[0]).toBe(seen.begins[0]); + expect(seen.commits).toHaveLength(0); + expect(committedNames('thing')).toEqual([]); + }, 30000); + + it('with NO host transaction the sandbox still OPENS one — unchanged behaviour', async () => { + await wire(TX_BODY); + + await engine.insert('thing', { name: 'outer' }); + + // The outer write is not in any transaction; the body's is, and owns it. + expect(seen.begins).toHaveLength(1); + expect(seen.commits).toHaveLength(1); + expect(seen.commits[0]).toBe(seen.begins[0]); + expect(committedNames('thing')).toEqual(['inner', 'outer']); + }, 30000); + + it('with NO host transaction a throwing body still ROLLS BACK its own — unchanged', async () => { + await wire(THROWING_TX_BODY); + + await expect(engine.insert('thing', { name: 'outer' })).rejects.toThrow(/body boom/); + + expect(seen.begins).toHaveLength(1); + expect(seen.rollbacks).toHaveLength(1); + expect(seen.commits).toHaveLength(0); + // The body's write is discarded; the outer one was never transactional. + expect(committedNames('thing')).toEqual(['outer']); + }, 30000); + + /** + * The other half of D2's rationale. A real single-connection pool BLOCKS on + * the second checkout — that is the deadlock — and a test modelling the block + * faithfully could only fail by timing out. This double refuses instead, so + * what is measured is the thing that causes both: whether a second connection + * is asked for at all. + */ + it('never asks for a second connection — a single-connection pool survives the body', async () => { + let checkedOut = false; + const checkouts: number[] = []; + await wire(TX_BODY, (driver) => { + const staged = new Map(); + driver.beginTransaction = async () => { + if (checkedOut) throw new Error('pool exhausted: no connection available (max=1)'); + checkedOut = true; + checkouts.push(checkouts.length + 1); + const h = { __trx: 'only' }; + staged.set(h, []); + return h; + }; + driver.commit = async () => { checkedOut = false; }; + driver.rollback = async () => { checkedOut = false; }; + }); + + await expect( + engine.transaction(async () => { await engine.insert('thing', { name: 'outer' }); }), + ).resolves.toBeUndefined(); + + expect(checkouts).toHaveLength(1); + }, 30000); +});