fix(kernel-store): ask SQLite whether a transaction is open

packages/kernel-store/CHANGELOG.md

@@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- After SQLite rolls a transaction back on its own, as it can on a full disk or an I/O error, the wasm driver no longer throws `cannot commit - no transaction is active` from later savepoint releases ([#1089](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1089))
+- A read or write that fails in the wasm driver no longer makes every later call to the same operation throw `SQLITE_MISUSE` ([#1089](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1089))
+- After SQLite rolls a transaction back on its own, the Node driver commits the writes under a later savepoint instead of leaving them in a transaction that never commits ([#1089](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1089))
 - Neither SQLite driver logs the contents of the store any more: the Node driver opens the database without `verbose`, and the wasm driver no longer logs kv keys and values. Both wrote vat state in plain text to whatever logger the embedder supplied ([#1086](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1086))
 - `rollbackSavepoint` discards the enclosing transaction when `ROLLBACK TO` itself fails, instead of leaving the savepoint on its stack and the transaction open ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005))
   - Nothing would ever commit or abort that transaction, so every later write on the connection silently joined it, reported success, and vanished on close. Discarding it is no wider than the caller asked for: the transaction begins with the outermost savepoint, so it holds only the work the rollback was abandoning

packages/kernel-store/src/sqlite/nodejs.test.ts

@@ -17,24 +17,59 @@ const mockKVDataForMap: [string, string][] = [
   ['key2', 'value2'],
 ];
 
-const mockStatement = {
+const makeMockStatement = () => ({
   run: vi.fn(),
   get: vi.fn(),
   all: vi.fn(),
   pluck: vi.fn(),
   iterate: vi.fn(() => mockKVData),
-};
+});
+
+const mockStatement = makeMockStatement();
+const mockBegin = makeMockStatement();
+const mockCommit = makeMockStatement();
+const mockAbort = makeMockStatement();
 
 const mockDb = {
-  prepare: vi.fn(() => mockStatement),
+  prepare: vi.fn((sql: string) => {
+    switch (sql) {
+      case SQL_QUERIES.BEGIN_TRANSACTION:
+        return mockBegin;
+      case SQL_QUERIES.COMMIT_TRANSACTION:
+        return mockCommit;
+      case SQL_QUERIES.ABORT_TRANSACTION:
+        return mockAbort;
+      default:
+        return mockStatement;
+    }
+  }),
   transaction: vi.fn((fn) => fn),
   exec: vi.fn(),
+  // better-sqlite3 reports this live, so the mock has to move it the way SQLite
+  // would: running BEGIN, COMMIT or ABORT is what changes it.
   inTransaction: false,
 
   _spStack: [] as string[],
   close: vi.fn(),
 };
 
+const resetStatements = (): void => {
+  [mockStatement, mockBegin, mockCommit, mockAbort].forEach((statement) =>
+    Object.values(statement).forEach((mock) => mock.mockReset()),
+  );
+  mockStatement.iterate.mockReturnValue(mockKVData);
+  mockBegin.run.mockImplementation(() => {
+    mockDb.inTransaction = true;
+  });
+  mockCommit.run.mockImplementation(() => {
+    mockDb.inTransaction = false;
+  });
+  mockAbort.run.mockImplementation(() => {
+    mockDb.inTransaction = false;
+  });
+};
+resetStatements();
+
 vi.mock('better-sqlite3', () => ({
   default: vi.fn(function () {
     return mockDb;
@@ -53,7 +88,9 @@ describe('makeSQLKernelDatabase', () => {
   const mockMkdir = vi.mocked(mkdir).mockResolvedValue('');
 
   beforeEach(() => {
-    Object.values(mockStatement).forEach((mock) => mock.mockReset());
+    resetStatements();
+    mockDb.inTransaction = false;
+    mockDb._spStack = [];
   });
 
   it('creates kv table', async () => {
@@ -253,8 +290,6 @@ describe('makeSQLKernelDatabase', () => {
   describe('savepoint functionality', () => {
     beforeEach(() => {
       mockDb.exec.mockClear();
-      mockDb.inTransaction = false;
-      mockDb._spStack = [];
     });
 
     it('creates a savepoint using sanitized name', async () => {
@@ -302,6 +337,7 @@ describe('makeSQLKernelDatabase', () => {
     it('createSavepoint begins transaction if needed', async () => {
       const db = await makeSQLKernelDatabase({});
       db.createSavepoint('test_point');
+      expect(mockBegin.run).toHaveBeenCalled();
       expect(mockDb._spStack).toContain('test_point');
       expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT test_point');
     });
@@ -330,6 +366,8 @@ describe('makeSQLKernelDatabase', () => {
       mockDb._spStack = ['point1'];
       db.rollbackSavepoint('point1');
       expect(mockDb._spStack).toStrictEqual([]);
+      expect(mockAbort.run).toHaveBeenCalled();
+      expect(mockDb.inTransaction).toBe(false);
     });
 
     // Otherwise every later write on this connection joins a transaction nothing
@@ -348,9 +386,8 @@ describe('makeSQLKernelDatabase', () => {
       );
 
       expect(mockDb._spStack).toStrictEqual([]);
-      // The abort is the only prepared statement this path runs.
-      expect(mockStatement.run).toHaveBeenCalledOnce();
-      mockDb.inTransaction = false;
+      expect(mockAbort.run).toHaveBeenCalledOnce();
+      expect(mockDb.inTransaction).toBe(false);
     });
 
     // The rollback failure is the diagnosis; a failed abort on top of it only
@@ -362,7 +399,7 @@ describe('makeSQLKernelDatabase', () => {
       mockDb.exec.mockImplementationOnce(() => {
         throw new Error('disk I/O error');
       });
-      mockStatement.run.mockImplementationOnce(() => {
+      mockAbort.run.mockImplementationOnce(() => {
         throw new Error('cannot rollback');
       });
 
@@ -371,7 +408,7 @@ describe('makeSQLKernelDatabase', () => {
       );
 
       expect(mockDb._spStack).toStrictEqual([]);
-      mockDb.inTransaction = false;
+      expect(mockAbort.run).toHaveBeenCalledOnce();
     });
 
     it('releaseSavepoint validates savepoint exists', async () => {
@@ -398,17 +435,33 @@ describe('makeSQLKernelDatabase', () => {
       mockDb._spStack = ['point1'];
       db.releaseSavepoint('point1');
       expect(mockDb._spStack).toStrictEqual([]);
+      expect(mockCommit.run).toHaveBeenCalled();
+      expect(mockDb.inTransaction).toBe(false);
     });
 
     it('supports nested savepoints', async () => {
       const db = await makeSQLKernelDatabase({});
       db.createSavepoint('outer');
       db.createSavepoint('inner');
+      expect(mockBegin.run).toHaveBeenCalledOnce();
       expect(mockDb._spStack).toStrictEqual(['outer', 'inner']);
       db.rollbackSavepoint('inner');
       expect(mockDb._spStack).toStrictEqual(['outer']);
       db.releaseSavepoint('outer');
       expect(mockDb._spStack).toStrictEqual([]);
+      expect(mockCommit.run).toHaveBeenCalledOnce();
+    });
+
+    it('drops savepoints SQLite discarded with the transaction', async () => {
+      const db = await makeSQLKernelDatabase({});
+      db.createSavepoint('t0');
+      mockDb.inTransaction = false;
+
+      db.createSavepoint('t1');
+      db.releaseSavepoint('t1');
+
+      expect(mockDb._spStack).toStrictEqual([]);
+      expect(mockCommit.run).toHaveBeenCalledOnce();
     });
   });
 

packages/kernel-store/src/sqlite/nodejs.transactions.test.ts

@@ -0,0 +1,30 @@
+import { mkdtemp, rm } from 'node:fs/promises';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { describe, it, expect, onTestFinished } from 'vitest';
+
+import { makeSQLKernelDatabase } from './nodejs.ts';
+
+describe('the Node driver on real SQLite', () => {
+  it('commits a savepoint opened after SQLite ends the transaction itself', async () => {
+    const dir = await mkdtemp(join(tmpdir(), 'kernel-store-'));
+    onTestFinished(async () => rm(dir, { recursive: true, force: true }));
+    const dbFilename = join(dir, 'store.db');
+    const kdb = await makeSQLKernelDatabase({ dbFilename });
+    kdb.createSavepoint('t0');
+    kdb.executeQuery('PRAGMA max_page_count = 1');
+    expect(() => kdb.kernelKVStore.set('key', 'x'.repeat(100_000))).toThrow(
+      'database or disk is full',
+    );
+    kdb.executeQuery('PRAGMA max_page_count = 1073741823');
+
+    kdb.createSavepoint('t1');
+    kdb.kernelKVStore.set('key', 'value');
+    kdb.releaseSavepoint('t1');
+    kdb.close();
+
+    const reopened = await makeSQLKernelDatabase({ dbFilename });
+    expect(reopened.kernelKVStore.get('key')).toBe('value');
+    reopened.close();
+  });
+});

packages/kernel-store/src/sqlite/nodejs.ts

@@ -157,6 +157,7 @@ export async function makeSQLKernelDatabase({
       return false;
     }
     sqlBeginTransaction.run();
+    db._spStack.length = 0;
     return true;
   }