test: pin the transaction invariants #1005 left broken

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

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

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