From 7dec92abe193aa455a9a528f97677596dd19a9e7 Mon Sep 17 00:00:00 2001 From: grypez <143971198+grypez@users.noreply.github.com> Date: Thu, 6 Aug 2026 07:17:49 -0400 Subject: [PATCH] test: pin the transaction invariants #1005 left broken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight tests, all currently failing, for three defects that landed with #1005. They change no production code: each one states the invariant the fix has to restore, so the diff that repairs them is the specification being met rather than a claim about it. `releaseSavepoint` was never hardened the way `rollbackSavepoint` was in that PR. A RELEASE that throws leaves the savepoint on the stack and the transaction open with nothing that will ever commit or abort it, so every later write on the connection joins it, reports success, and vanishes on close — verbatim the failure mode #1005 documents for the other door. The driver tests sit beside their rollback counterparts so the asymmetry is visible in place. `endCrank` gets the companion case: it now settles its waiters in a `finally`, which is right, but it also leaves the savepoint listed, so the next crank numbers its savepoint `t1` against a database that still has `t0`. `#processCrankResult` does fallible work after the crank's transactional boundary has already been crossed. On the success path `#flushCrankBuffer` settles the promise `enqueueMessage` handed an external caller, and only then can `#terminateVat` throw and have the new catch roll the crank back — so the caller keeps an answer computed from state the store discarded, and a restart delivers the message again. On the abort path the rollback ends the transaction, so `#terminateVat` and `collectGarbage` autocommit piecemeal and the second rollback the flag correctly suppresses would have had nothing left to undo either way. The invariant is stated as "the rollback is the last thing the crank asks of the store", which leaves the choice of remedy open. The wasm driver tracks `_inTx` itself rather than reading it from SQLite, so a failed abort inside the new catch is the one case that can leave it disagreeing with the database. Left true, `beginIfNeeded` is a no-op from then on and the next `createSavepoint` runs in autocommit mode, where the matching RELEASE commits (Agoric/agoric-sdk#8423, already cited two lines above the code) and no rollback can undo the delivery. The second test runs that next `createSavepoint` and asserts the BEGIN, so the corruption path is observable instead of argued. Co-Authored-By: Claude Opus 5 --- .../kernel-store/src/sqlite/nodejs.test.ts | 23 ++++ packages/kernel-store/src/sqlite/wasm.test.ts | 69 ++++++++++++ packages/ocap-kernel/src/KernelQueue.test.ts | 106 ++++++++++++++++++ .../src/store/methods/crank.test.ts | 17 +++ 4 files changed, 215 insertions(+) diff --git a/packages/kernel-store/src/sqlite/nodejs.test.ts b/packages/kernel-store/src/sqlite/nodejs.test.ts index a62392fe1..3bf0db5f6 100644 --- a/packages/kernel-store/src/sqlite/nodejs.test.ts +++ b/packages/kernel-store/src/sqlite/nodejs.test.ts @@ -360,6 +360,29 @@ describe('makeSQLKernelDatabase', () => { expect(mockDb._spStack).toStrictEqual([]); }); + // The same hazard `rollbackSavepoint` guards against, by the other door: a + // RELEASE that throws leaves the savepoint on the stack and the transaction + // open with nothing to ever commit or abort it, so every later write on this + // connection joins it, reports success, and vanishes on close. + it('releaseSavepoint discards the transaction when the release fails', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb.inTransaction = true; + mockDb._spStack = ['point1']; + mockStatement.run.mockClear(); + mockDb.exec.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + + expect(() => db.releaseSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + expect(mockDb._spStack).toStrictEqual([]); + // The abort is the only prepared statement this path runs. + expect(mockStatement.run).toHaveBeenCalledOnce(); + mockDb.inTransaction = false; + }); + it('supports nested savepoints', async () => { const db = await makeSQLKernelDatabase({}); db.createSavepoint('outer'); diff --git a/packages/kernel-store/src/sqlite/wasm.test.ts b/packages/kernel-store/src/sqlite/wasm.test.ts index 2cbc96d65..e0e6be1c4 100644 --- a/packages/kernel-store/src/sqlite/wasm.test.ts +++ b/packages/kernel-store/src/sqlite/wasm.test.ts @@ -518,6 +518,75 @@ describe('makeSQLKernelDatabase', () => { expect(mockDb._inTx).toBe(false); }); + // The same hazard `rollbackSavepoint` guards against, by the other door: a + // RELEASE that throws leaves the savepoint on the stack and the transaction + // open with nothing to ever commit or abort it, so every later write on this + // connection joins it, reports success, and vanishes on close. + it('releaseSavepoint discards the transaction when the release fails', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + mockDb.exec.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + + expect(() => db.releaseSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + expect(mockDb._spStack).toStrictEqual([]); + expect(mockDb._inTx).toBe(false); + }); + + // `_inTx` is tracked here rather than read from SQLite, so a failed abort is + // the one case that can leave it disagreeing with the database. Left true, + // `beginIfNeeded` becomes a no-op forever after. + it('stops believing it is in a transaction when the abort fails too', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + mockDb.exec.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + mockStatement.step.mockImplementationOnce(() => { + throw new Error('cannot rollback'); + }); + + expect(() => db.rollbackSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + expect(mockDb._inTx).toBe(false); + }); + + // The consequence of the above, and the reason it is worth asserting: a + // savepoint created outside a transaction autocommits when released + // (Agoric/agoric-sdk#8423), so no later rollback can undo the delivery — an + // aborted crank would silently keep its writes. + it('begins a transaction for the next savepoint after a failed abort', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + mockDb.exec.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + mockStatement.step.mockImplementationOnce(() => { + throw new Error('cannot rollback'); + }); + expect(() => db.rollbackSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + mockDb.exec.mockClear(); + mockStatement.step.mockClear(); + db.createSavepoint('next'); + + // BEGIN is the only prepared statement `createSavepoint` runs; the + // SAVEPOINT itself goes through `exec`. + expect(mockStatement.step).toHaveBeenCalledOnce(); + expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT next'); + }); + it('supports nested savepoints', async () => { const db = await makeSQLKernelDatabase({}); db.createSavepoint('outer'); diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index d7beb0e2f..073c08d31 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -194,6 +194,112 @@ describe('KernelQueue', () => { expect(kernelStore.collectGarbage).toHaveBeenCalled(); expect(kernelStore.endCrank).toHaveBeenCalled(); }); + + // `#flushCrankBuffer` settles the promise `enqueueMessage` handed an external + // caller, reading the resolution out of the store on the way. Rolling the + // crank back afterwards un-resolves that promise in the store and restores + // the run queue item, so a restart delivers the message a second time and + // notifies every other subscriber again — while the original caller has + // already been told the first answer. + it('does not roll back a crank whose result the caller already received', async () => { + const mockItem: RunQueueItem = { + type: 'send', + target: 'ko123', + message: { result: 'kp1' } as KernelMessage, + }; + (kernelStore.runQueueLength as unknown as MockInstance) + .mockReturnValueOnce(1) + .mockReturnValue(0); + (kernelStore.dequeueRun as unknown as MockInstance).mockReturnValueOnce( + mockItem, + ); + + // A caller is awaiting this message's result. + const resolve = vi.fn(); + const reject = vi.fn(); + kernelQueue.subscriptions.set('kp1', { resolve, reject }); + + // The crank succeeds, so the flush hands that caller its answer... + ( + kernelStore.flushCrankBuffer as unknown as MockInstance + ).mockReturnValueOnce([ + { type: 'notify', endpointId: 'v1', kpid: 'kp1' }, + ]); + (kernelStore.getKernelPromise as unknown as MockInstance).mockReturnValue( + { + state: 'fulfilled', + value: { body: '"answer"', slots: [] }, + }, + ); + + // ...and only then does the kernel die, in work that runs after the flush. + const terminationError = new Error('vat worker already gone'); + (terminateVat as unknown as MockInstance).mockRejectedValueOnce( + terminationError, + ); + const deliver = vi.fn().mockResolvedValue({ + terminate: { vatId: 'v1', info: { body: '"exit"', slots: [] } }, + }); + + await expect(kernelQueue.run(deliver)).rejects.toBe(terminationError); + + expect(resolve).toHaveBeenCalledWith({ body: '"answer"', slots: [] }); + expect(kernelStore.rollbackCrank).not.toHaveBeenCalled(); + }); + + // `rollbackCrank('start')` rolls back the crank's outermost savepoint, which + // ends the transaction — so anything the crank does to the store afterwards + // autocommits piecemeal and no rollback can reach it. Whatever the ordering, + // the rollback has to be the last thing the crank asks of the store. + it.each([ + { label: 'an abort', crankResult: { abort: true } }, + { + label: 'an abort that also terminates', + crankResult: { + abort: true, + terminate: { vatId: 'v1', info: { body: '"exit"', slots: [] } }, + }, + }, + ])( + 'does no store work after rolling back $label', + async ({ crankResult }) => { + const mockItem: RunQueueItem = { + type: 'send', + target: 'ko123', + message: { result: 'kp99' } as KernelMessage, + }; + (kernelStore.runQueueLength as unknown as MockInstance) + .mockReturnValueOnce(1) + .mockReturnValue(0); + (kernelStore.dequeueRun as unknown as MockInstance).mockReturnValueOnce( + mockItem, + ); + + const storeCalls: string[] = []; + ( + kernelStore.rollbackCrank as unknown as MockInstance + ).mockImplementation(() => { + storeCalls.push('rollbackCrank'); + }); + (terminateVat as unknown as MockInstance).mockImplementation( + async () => { + storeCalls.push('terminateVat'); + }, + ); + ( + kernelStore.collectGarbage as unknown as MockInstance + ).mockImplementation(() => { + storeCalls.push('collectGarbage'); + throw new Error(STOP_RUN_LOOP); + }); + + const deliver = vi.fn().mockResolvedValue(crankResult); + await expect(kernelQueue.run(deliver)).rejects.toThrow(STOP_RUN_LOOP); + + expect(storeCalls).toContain('rollbackCrank'); + expect(storeCalls.at(-1)).toBe('rollbackCrank'); + }, + ); }); describe('getRunLoopStatus', () => { diff --git a/packages/ocap-kernel/src/store/methods/crank.test.ts b/packages/ocap-kernel/src/store/methods/crank.test.ts index db3de8645..8fd6cd1fe 100644 --- a/packages/ocap-kernel/src/store/methods/crank.test.ts +++ b/packages/ocap-kernel/src/store/methods/crank.test.ts @@ -206,6 +206,23 @@ describe('crank methods', () => { expect(context.resolveCrank).toBeUndefined(); expect(await waiter).toBeUndefined(); }); + + // What `rollbackCrank` already does in its own `finally`. Settling the crank + // regardless means callers proceed, so a savepoint left listed here has the + // next crank number its savepoint `t1` while the database still has `t0`: + // from then on `releaseAllSavepoints` releases the wrong one and every + // rollback aims past the crank it meant to undo. + it('forgets its savepoints even if releasing them fails', () => { + crankMethods.startCrank(); + context.savepoints = ['test']; + vi.mocked(kdb.releaseSavepoint).mockImplementationOnce(() => { + throw new Error('database is gone'); + }); + + expect(() => crankMethods.endCrank()).toThrow('database is gone'); + + expect(context.savepoints).toStrictEqual([]); + }); }); describe('releaseAllSavepoints', () => {