fix: keep a crank's store work inside one transaction

packages/kernel-store/CHANGELOG.md

@@ -12,6 +12,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `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
   - The rollback failure is still what gets thrown, even if aborting the transaction fails too
+- `releaseSavepoint` discards the enclosing transaction when `RELEASE` fails, as `rollbackSavepoint` already did for `ROLLBACK TO` ([#1012](https://github.com/MetaMask/ocap-kernel/pull/1012))
+  - Otherwise the savepoint stayed on the stack and the transaction open with nothing left to commit or abort it. The release failure is still what gets thrown
+- The wasm driver clears `_inTx` when aborting a transaction throws, instead of believing it is still in one ([#1012](https://github.com/MetaMask/ocap-kernel/pull/1012))
+  - Left true, `beginIfNeeded` became a permanent no-op and later writes autocommitted. The nodejs driver reads `db.inTransaction` and was never affected
+- An abort that fails while recovering from a failed savepoint operation is now logged in both drivers ([#1012](https://github.com/MetaMask/ocap-kernel/pull/1012))
 
 ## [0.6.0]
 

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

@@ -360,6 +360,45 @@ describe('makeSQLKernelDatabase', () => {
       expect(mockDb._spStack).toStrictEqual([]);
     });
 
+    // The hazard `rollbackSavepoint` guards against, by the other door.
+    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('releaseSavepoint reports the release failure even if the abort fails too', async () => {
+      const db = await makeSQLKernelDatabase({});
+      mockDb.inTransaction = true;
+      mockDb._spStack = ['point1'];
+      mockDb.exec.mockImplementationOnce(() => {
+        throw new Error('disk I/O error');
+      });
+      mockStatement.run.mockImplementationOnce(() => {
+        throw new Error('cannot rollback');
+      });
+
+      expect(() => db.releaseSavepoint('point1')).toThrowError(
+        'disk I/O error',
+      );
+
+      expect(mockDb._spStack).toStrictEqual([]);
+      mockDb.inTransaction = false;
+    });
+
     it('supports nested savepoints', async () => {
       const db = await makeSQLKernelDatabase({});
       db.createSavepoint('outer');

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

@@ -298,8 +298,14 @@ export async function makeSQLKernelDatabase({
       db._spStack.length = 0;
       try {
         rollbackIfNeeded();
-      } catch {
-        // The rollback failure below is the one worth reporting.
+      } catch (abortError) {
+        // The rollback failure below is the one worth reporting, but a failed
+        // abort leaves SQLite holding a transaction the next crank would
+        // silently write into. Nothing here can repair that.
+        logger?.error(
+          'failed to discard transaction after rollback',
+          abortError,
+        );
       }
       throw error;
     }
@@ -321,7 +327,28 @@ export async function makeSQLKernelDatabase({
       throw new Error(`No such savepoint: ${name}`);
     }
     const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', name);
-    db.exec(query);
+    try {
+      db.exec(query);
+    } catch (error) {
+      // The hazard `rollbackSavepoint` guards against, by the other door: left as
+      // it was, the savepoint stays 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. There is no
+      // committing this transaction now, so discard it.
+      db._spStack.length = 0;
+      try {
+        rollbackIfNeeded();
+      } catch (abortError) {
+        // The release failure below is the one worth reporting, but a failed
+        // abort leaves SQLite holding a transaction the next crank would
+        // silently write into. Nothing here can repair that.
+        logger?.error(
+          'failed to discard transaction after release',
+          abortError,
+        );
+      }
+      throw error;
+    }
     db._spStack.splice(idx);
     if (db._spStack.length === 0) {
       commitIfNeeded();

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

@@ -488,7 +488,6 @@ describe('makeSQLKernelDatabase', () => {
       );
 
       expect(mockDb._spStack).toStrictEqual([]);
-      mockDb._inTx = false;
     });
 
     it('releaseSavepoint validates savepoint exists', async () => {
@@ -518,6 +517,88 @@ describe('makeSQLKernelDatabase', () => {
       expect(mockDb._inTx).toBe(false);
     });
 
+    // The hazard `rollbackSavepoint` guards against, by the other door.
+    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);
+    });
+
+    it('releaseSavepoint reports the release failure even if 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.releaseSavepoint('point1')).toThrowError(
+        'disk I/O error',
+      );
+
+      expect(mockDb._inTx).toBe(false);
+    });
+
+    // A failed abort is the one case that can leave `_inTx` disagreeing with the
+    // database. Left true, `beginIfNeeded` is 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);
+    });
+
+    // Why that matters: a savepoint created outside a transaction commits when
+    // released (Agoric/agoric-sdk#8423), so an aborted crank would 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/kernel-store/src/sqlite/wasm.ts

@@ -210,10 +210,16 @@ export async function makeSQLKernelDatabase({
    */
   function rollbackIfNeeded(): void {
     if (db._inTx) {
-      sqlAbortTransaction.step();
-      sqlAbortTransaction.reset();
+      // Cleared before the abort is attempted, because the abort can throw and
+      // `_inTx` is tracked here rather than read from SQLite as the nodejs driver
+      // does. Left true, `beginIfNeeded` is a no-op forever after and writes
+      // autocommit one statement at a time (see `createSavepoint`). Cleared, a
+      // still-open transaction surfaces as a failed `BEGIN` — the louder
+      // failure.
       db._inTx = false;
       db._spStack.length = 0;
+      sqlAbortTransaction.step();
+      sqlAbortTransaction.reset();
     }
   }
 
@@ -380,8 +386,14 @@ export async function makeSQLKernelDatabase({
       db._spStack.length = 0;
       try {
         rollbackIfNeeded();
-      } catch {
-        // The rollback failure below is the one worth reporting.
+      } catch (abortError) {
+        // The rollback failure below is the one worth reporting. The next
+        // `BEGIN` will fail if SQLite really is still in a transaction, but that
+        // is a crank away and this is where the evidence is.
+        logger?.error(
+          'failed to discard transaction after rollback',
+          abortError,
+        );
       }
       throw error;
     }
@@ -403,7 +415,28 @@ export async function makeSQLKernelDatabase({
       throw new Error(`No such savepoint: ${name}`);
     }
     const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', name);
-    db.exec(query);
+    try {
+      db.exec(query);
+    } catch (error) {
+      // The hazard `rollbackSavepoint` guards against, by the other door: left as
+      // it was, the savepoint stays 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. There is no
+      // committing this transaction now, so discard it.
+      db._spStack.length = 0;
+      try {
+        rollbackIfNeeded();
+      } catch (abortError) {
+        // The release failure below is the one worth reporting. The next
+        // `BEGIN` will fail if SQLite really is still in a transaction, but that
+        // is a crank away and this is where the evidence is.
+        logger?.error(
+          'failed to discard transaction after release',
+          abortError,
+        );
+      }
+      throw error;
+    }
     db._spStack.splice(idx);
     if (db._spStack.length === 0) {
       commitIfNeeded();

packages/kernel-test/src/crank-rollback.test.ts

@@ -138,6 +138,42 @@ describe('crank rollback against a real database', () => {
     expect(kdb.kernelKVStore.get('second')).toBe('yes');
   });
 
+  // An aborted crank still owes work after the rollback — terminating the vat,
+  // collecting garbage — whose writes have to survive it.
+  it('keeps the writes a crank makes after rolling its delivery back', async () => {
+    const { kernelStore, kdb } = await makeStore();
+
+    kernelStore.startCrank();
+    kernelStore.createCrankSavepoint('crank');
+    kernelStore.createCrankSavepoint('delivery');
+    kdb.kernelKVStore.set('delivered', 'yes');
+
+    kernelStore.rollbackCrank('delivery');
+    kdb.kernelKVStore.set('terminated', 'yes');
+    kernelStore.endCrank();
+
+    expect(kdb.kernelKVStore.get('delivered')).toBeUndefined();
+    expect(kdb.kernelKVStore.get('terminated')).toBe('yes');
+  });
+
+  // And survive it *inside the crank's transaction*, not as autocommitted
+  // statements. Rolling back `crank` is the only way to observe that from here;
+  // the run loop never does it.
+  it('holds those writes in the transaction rather than autocommitting them', async () => {
+    const { kernelStore, kdb } = await makeStore();
+
+    kernelStore.startCrank();
+    kernelStore.createCrankSavepoint('crank');
+    kernelStore.createCrankSavepoint('delivery');
+    kernelStore.rollbackCrank('delivery');
+    kdb.kernelKVStore.set('terminated', 'yes');
+
+    kernelStore.rollbackCrank('crank');
+    kernelStore.endCrank();
+
+    expect(kdb.kernelKVStore.get('terminated')).toBeUndefined();
+  });
+
   // `createCrankSavepoint` records the name only once the database has the
   // savepoint. Asking to roll back one that was never created must therefore say
   // so, rather than releasing someone else's savepoint.

packages/kernel-test/src/garbage-collection.test.ts

@@ -105,6 +105,29 @@ describe('Garbage Collection', () => {
     expect(parseReplyBody(useResult.body)).toBe(objectId);
   });
 
+  /**
+   * Reap the importer vat until the kernel's bookkeeping catches up with the
+   * vat's own garbage collection, or the attempts run out.
+   *
+   * `bringOutYourDead` reports an import as dropped only once the engine has
+   * collected the vat's presence and run its finalizer, which `gcAndFinalize`
+   * does not guarantee on the first attempt. Each attempt needs its own reap —
+   * `nextReapAction` shifts the one scheduled entry off, so cranking again finds
+   * nothing to do — plus a message to wake the run loop and consume it.
+   *
+   * Gives up after five attempts; the caller's assertion reports the failure.
+   *
+   * @param settled - Whether the state under test has arrived yet.
+   */
+  async function reapImporterUntil(settled: () => boolean): Promise<void> {
+    const isImporter = (vatId: VatId): boolean => vatId === importerVatId;
+    for (let attempt = 0; attempt < 5 && !settled(); attempt += 1) {
+      kernel.reapVats(isImporter);
+      await kernel.queueMessage(importerKRef, 'noop', []);
+      await waitUntilQuiescent(500);
+    }
+  }
+
   it('should trigger GC syscalls through bringOutYourDead', async () => {
     // Create an object in the exporter vat with a known ID
     const objectId = 'test-object';
@@ -149,14 +172,10 @@ describe('Garbage Collection', () => {
     await kernel.queueMessage(importerKRef, 'makeWeak', [objectId]);
     await waitUntilQuiescent();
 
-    // Schedule reap to trigger bringOutYourDead on next crank
-    kernel.reapVats((vatId) => vatId === importerVatId);
-
-    // Run 3 cranks to allow bringOutYourDead to be processed
-    for (let i = 0; i < 3; i++) {
-      await kernel.queueMessage(importerKRef, 'noop', []);
-      await waitUntilQuiescent(500);
-    }
+    // Reap until the importer reports the drop
+    await reapImporterUntil(
+      () => kernelStore.getObjectRefCount(createObjectRef).reachable === 1,
+    );
 
     // Check reference counts after dropImports
     const afterWeakRefCounts = kernelStore.getObjectRefCount(createObjectRef);
@@ -168,13 +187,10 @@ describe('Garbage Collection', () => {
     await kernel.queueMessage(importerKRef, 'forgetImport', []);
     await waitUntilQuiescent();
 
-    // Schedule another reap
-    kernel.reapVats((vatId) => vatId === importerVatId);
-
-    for (let i = 0; i < 3; i++) {
-      await kernel.queueMessage(importerKRef, 'noop', []);
-      await waitUntilQuiescent(500);
-    }
+    // Reap until the importer reports the retirement
+    await reapImporterUntil(
+      () => kernelStore.getObjectRefCount(createObjectRef).recognizable === 1,
+    );
 
     // Check reference counts after retireImports
     const afterForgetRefCounts = kernelStore.getObjectRefCount(createObjectRef);

packages/ocap-kernel/CHANGELOG.md

@@ -48,6 +48,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Roll back the crank the run loop died in instead of committing it, so a restart resumes from a consistent boundary ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005))
   - Because the killing item is no longer consumed, a restart re-dequeues it; an item that reliably kills a crank needs `clearState`/`reset` rather than a restart
   - Store state only — a crank that had already flushed its buffer settled JS-side subscriptions irreversibly
+- Keep a crank's store work inside one transaction ([#1012](https://github.com/MetaMask/ocap-kernel/pull/1012))
+  - A crank now takes two savepoints, `crank` and `delivery`, and rolls back only `delivery`. Rolling back the outermost one ends the transaction, so terminating the vat and collecting garbage — which follow the rollback and must survive it — were autocommitting a statement at a time
+  - Buffered vat outputs are flushed after that work rather than before it, so a later failure can no longer roll the crank back underneath an answer already given. Termination still settles the dying vat's own promises immediately
+- Forget a crank's savepoints when the store call that ends them fails, in both `rollbackCrank` and `endCrank` ([#1012](https://github.com/MetaMask/ocap-kernel/pull/1012))
+  - A failed rollback or release discards the whole transaction, savepoints included. Keeping them listed had `endCrank` throw `No such savepoint: t0` from the run loop's `finally`, over whatever really killed the kernel
 - Refuse inbound remote deliveries once the run loop is dead, rolling back without acknowledging them, so the peer retries and gives up instead of waiting on a kernel that will never deliver ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005))
   - Covers `bringOutYourDead` as well as `message` and `notify`: a reap is queue work too, consumed only by the run loop. The remaining GC arms need no guard, since they only touch refcounts
 - Refuse `launchSubcluster` once the run loop is dead ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005))