test: failing repros for four defects found reviewing #1012

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) }),
+    );
+  });
 });

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');

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,

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.