test: pin the transaction invariants #1005 left broken

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 <noreply@anthropic.com>

Author: 2 people

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

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

packages/ocap-kernel/src/KernelQueue.test.ts

@@ -195,6 +195,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', () => {

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