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', () => {