diff --git a/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts b/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts index 57b0293d6..672313a02 100644 --- a/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts +++ b/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts @@ -1,3 +1,5 @@ +import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; +import { Logger } from '@metamask/logger'; import { Kernel } from '@metamask/ocap-kernel'; import { describe, expect, it, vi } from 'vitest'; @@ -8,7 +10,9 @@ vi.mock('@metamask/kernel-store/sqlite/nodejs', async () => { '../../../ocap-kernel/test/storage.ts' ); return { - makeSQLKernelDatabase: makeMapKernelDatabase, + // Wrapped so that a test can see what the database was constructed with, + // while still getting a real store back. + makeSQLKernelDatabase: vi.fn(makeMapKernelDatabase), }; }); @@ -18,4 +22,19 @@ describe('makeKernel', () => { expect(kernel).toBeInstanceOf(Kernel); }); + + // FAILING REPRO. + // + // The kernel store is the only collaborator `makeKernel` builds without + // handing it a logger, so every `logger?.` call inside the SQLite driver is + // dead code in production — including the four abort failures #1012 added + // logging for. `kernel-worker.ts` omits it too, which keeps the wasm driver's + // pair dead even once this passes. + it('gives the kernel store a logger', async () => { + await makeKernel({}); + + expect(vi.mocked(makeSQLKernelDatabase)).toHaveBeenCalledWith( + expect.objectContaining({ logger: expect.any(Logger) }), + ); + }); }); diff --git a/packages/kernel-store/src/sqlite/wasm.test.ts b/packages/kernel-store/src/sqlite/wasm.test.ts index 8e82a00bc..1cf496dc9 100644 --- a/packages/kernel-store/src/sqlite/wasm.test.ts +++ b/packages/kernel-store/src/sqlite/wasm.test.ts @@ -599,6 +599,44 @@ describe('makeSQLKernelDatabase', () => { expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT next'); }); + // FAILING REPRO. + // + // `commitIfNeeded` still steps the COMMIT before clearing `_inTx`, the exact + // ordering `rollbackIfNeeded` was corrected to avoid. A COMMIT that throws + // therefore leaves `_inTx` true against a database that may hold no + // transaction, `beginIfNeeded` is a no-op forever after, and the next + // savepoint is created outside a transaction — which commits when released + // (Agoric/agoric-sdk#8423). This is the crank's commit point, so the writes + // that leak are a whole crank's. + // + // A failed abort is therefore not, as the abort case above claims, the one + // case that can leave `_inTx` disagreeing with the database. This is the + // second. + it('stops believing it is in a transaction when the commit fails', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + // The RELEASE goes through `exec` and succeeds; COMMIT is the first + // prepared statement this path steps, and it is what fails. + mockStatement.step.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + + expect(() => db.releaseSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + expect(mockDb._inTx).toBe(false); + + // And so the next savepoint gets a transaction of its own rather than + // being created bare. + mockDb.exec.mockClear(); + mockStatement.step.mockClear(); + db.createSavepoint('next'); + 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 1af58d81f..9df06624c 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -25,6 +25,24 @@ vi.mock('@endo/promise-kit', () => ({ */ const STOP_RUN_LOOP = 'test: stop run loop'; +/** + * Collect an error and every error reachable through its `cause` chain, so that + * a test can assert a root cause survived without pinning how its reporter + * chose to wrap it. + * + * @param error - The error to walk. + * @returns The chain, outermost first. + */ +const causeChain = (error: unknown): Error[] => { + const chain: Error[] = []; + let current = error; + while (current instanceof Error) { + chain.push(current); + current = current.cause; + } + return chain; +}; + describe('KernelQueue', () => { let kernelStore: KernelStore; let kernelQueue: KernelQueue; @@ -539,6 +557,43 @@ describe('KernelQueue', () => { }); }); + // FAILING REPRO. + // + // The companion of the case above, at the other end of the crank. Since the + // delivery rollback now spares `crank`, `endCrank`'s release is a real + // RELEASE + COMMIT on the dying path where it used to be a no-op, and + // `#runLoop` calls it from a bare `finally` — so when it throws it replaces + // the error that killed the kernel instead of being reported alongside it. + it('reports both failures when endCrank also fails', async () => { + ( + kernelStore.runQueueLength as unknown as MockInstance + ).mockReturnValueOnce(1); + (kernelStore.dequeueRun as unknown as MockInstance).mockReturnValueOnce({ + type: 'send', + target: 'ko123', + message: {} as KernelMessage, + }); + (kernelStore.endCrank as unknown as MockInstance).mockImplementation( + () => { + throw new Error('database is gone'); + }, + ); + const crankError = new Error('crank exploded'); + const deliver = vi.fn().mockRejectedValue(crankError); + + const failure = await kernelQueue + .run(deliver) + .catch((error: unknown) => error); + + // However the release failure is named, the error that actually killed the + // kernel has to stay reachable — as the rollback path already manages. + expect(causeChain(failure)).toContain(crankError); + expect(kernelQueue.getRunLoopStatus()).toMatchObject({ + state: 'failed', + detail: expect.stringContaining('crank exploded'), + }); + }); + // `rollbackCrank` discards the savepoint even when its database call throws, // so a second attempt could only report a missing savepoint. Without the // `finally` that records the attempt, the abort path leaves the flag unset, diff --git a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts index c14539bc6..9dc0c9661 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts @@ -220,6 +220,56 @@ describe('RemoteHandle', () => { }); }); + // FAILING REPRO. + // + // `handleRemoteMessage` releases its savepoint inside the `try` and rolls + // back in the `catch`. #1012 made a failed `RELEASE` discard the whole + // savepoint stack, so that rollback now reports a savepoint that no longer + // exists, and it throws out of the `catch` in place of the failure that + // brought it there. + it('reports the release failure rather than a missing savepoint', async () => { + // The drivers' bookkeeping as #1012 leaves it, verified against both: a + // failed RELEASE clears `_spStack`, and rolling back a name that is not on + // it throws `No such savepoint`. Replacing the store wholesale rather than + // assigning over its methods because `makeKernelStore` hardens what it + // returns. + const savepoints: string[] = []; + const releaseFailure = new Error('database or disk is full'); + const rollbackSavepoint = vi.fn((name: string) => { + if (!savepoints.includes(name)) { + throw new Error(`No such savepoint: ${name}`); + } + }); + mockKernelStore = { + ...mockKernelStore, + createSavepoint: (name: string) => { + savepoints.push(name); + }, + releaseSavepoint: () => { + savepoints.length = 0; + throw releaseFailure; + }, + rollbackSavepoint, + }; + const remote = makeRemote(); + + const delivery = JSON.stringify({ + seq: 1, + method: 'deliver', + params: ['bringOutYourDead'], + }); + + // The error an operator needs is the one the database gave, not the + // bookkeeping artefact of trying to clean up after it. + await expect(remote.handleRemoteMessage(delivery)).rejects.toBe( + releaseFailure, + ); + // Still attempted, so that abandoning the rollback is not a way to pass + // this test: a release that failed for a reason of its own may well have + // left the savepoint standing. + expect(rollbackSavepoint).toHaveBeenCalledWith('receive_r0_1'); + }); + // A dead run loop will never deliver the message, and `handleRemoteMessage` // rolls back without advancing the received sequence number, so the peer // retries and gives up rather than being acknowledged by a black hole.