fix(ocap-kernel): make c-list import accounting symmetric

packages/extension/test/e2e/control-panel.test.ts

@@ -165,7 +165,6 @@ test.describe('Control Panel', () => {
       '{"key":"v3.c.o+0","value":"ko6"}',
       '{"key":"v3.c.kp4","value":"R p-1"}',
       '{"key":"v3.c.p-1","value":"kp4"}',
-      '{"key":"ko6.refCount","value":"1,1"}',
       '{"key":"kp4.refCount","value":"2"}',
     ];
     const v1koValues = [

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

@@ -138,6 +138,67 @@ describe('crank rollback against a real database', () => {
     expect(kdb.kernelKVStore.get('second')).toBe('yes');
   });
 
+  // Every `provideCachedStoredValue` keeps its value in a closure and writes
+  // through to kv, so a rollback that only reverts the database leaves the cache
+  // holding the abandoned crank's value — and the next `set` persists it. The GC
+  // action set is the case that matters: `processGCActionSet` consumes an action
+  // before delivering it, so losing the rollback loses the action outright.
+  it('restores the GC action set consumed by a rolled-back crank', async () => {
+    const { kernelStore } = await makeStore();
+    kernelStore.addGCActions(['v1 dropExport ko1']);
+
+    kernelStore.startCrank();
+    kernelStore.createCrankSavepoint('start');
+    // Consume the action the way `processGCActionSet` does.
+    kernelStore.setGCActions(new Set());
+
+    kernelStore.rollbackCrank('start');
+    kernelStore.endCrank();
+
+    expect([...kernelStore.getGCActions()]).toStrictEqual([
+      'v1 dropExport ko1',
+    ]);
+  });
+
+  // Same closure, same failure: a reap scheduled and then consumed by a crank
+  // that rolls back must still be pending afterwards.
+  it('restores the reap queue consumed by a rolled-back crank', async () => {
+    const { kernelStore } = await makeStore();
+    kernelStore.scheduleReap('v1');
+
+    kernelStore.startCrank();
+    kernelStore.createCrankSavepoint('start');
+    expect(kernelStore.nextReapAction()).toBeDefined();
+
+    kernelStore.rollbackCrank('start');
+    kernelStore.endCrank();
+
+    expect(kernelStore.nextReapAction()).toBeDefined();
+  });
+
+  // `maybeFreeKrefs` is RAM-only, so nothing rolls it back. Left populated, the
+  // next crank's `collectGarbage` visits krefs whose decrements were undone —
+  // and `getKernelPromise` throws outright for one the rollback deleted, which
+  // kills the run loop.
+  it('discards GC candidates accumulated by a rolled-back crank', async () => {
+    const { kernelStore } = await makeStore();
+
+    kernelStore.startCrank();
+    kernelStore.createCrankSavepoint('start');
+    // Born at 1, so this drops it to 0 and leaves `kpid` in `maybeFreeKrefs`
+    // while the rollback removes the promise record it names.
+    const kpid = kernelStore.initKernelPromise()[0];
+    kernelStore.decrementRefCount(kpid, 'test');
+
+    kernelStore.rollbackCrank('start');
+    kernelStore.endCrank();
+
+    kernelStore.startCrank();
+    kernelStore.createCrankSavepoint('start');
+    expect(() => kernelStore.collectGarbage()).not.toThrow();
+    kernelStore.endCrank();
+  });
+
   // `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

@@ -21,9 +21,11 @@ import {
 /**
  * Make a test subcluster with vats for GC testing
  *
+ * @param extraImporters - Names of additional importer vats to include, for
+ * topologies where more than one vat shares the same exported object.
  * @returns The test subcluster
  */
-function makeTestSubcluster(): ClusterConfig {
+function makeTestSubcluster(extraImporters: string[] = []): ClusterConfig {
   return {
     bootstrap: 'exporter',
     forceReset: true,
@@ -40,6 +42,15 @@ function makeTestSubcluster(): ClusterConfig {
           name: 'Importer',
         },
       },
+      ...Object.fromEntries(
+        extraImporters.map((name) => [
+          name,
+          {
+            bundleSpec: getBundleSpec('importer-vat'),
+            parameters: { name },
+          },
+        ]),
+      ),
     },
   };
 }
@@ -81,10 +92,11 @@ describe('Garbage Collection', () => {
       [objectId],
     );
     const createObjectRef = createObjectData.slots[0] as KRef;
-    // Verify initial reference counts from database
-    const initialRefCounts = kernelStore.getObjectRefCount(createObjectRef);
-    expect(initialRefCounts.reachable).toBe(2);
-    expect(initialRefCounts.recognizable).toBe(2);
+    // Held only by the resolved promise's value, which still carries the slot
+    expect(kernelStore.getObjectRefCount(createObjectRef)).toStrictEqual({
+      reachable: 1,
+      recognizable: 1,
+    });
     // Send the object to the importer vat
     const objectRef = kunser(createObjectData);
     await kernel.queueMessage(importerKRef, 'storeImport', [objectRef]);
@@ -116,10 +128,10 @@ describe('Garbage Collection', () => {
     await waitUntilQuiescent();
     const createObjectRef = createObjectData.slots[0] as KRef;
 
-    // Store initial reference count information
-    const initialRefCounts = kernelStore.getObjectRefCount(createObjectRef);
-    expect(initialRefCounts.reachable).toBe(2);
-    expect(initialRefCounts.recognizable).toBe(2);
+    expect(kernelStore.getObjectRefCount(createObjectRef)).toStrictEqual({
+      reachable: 1,
+      recognizable: 1,
+    });
 
     // Store the reference in the importer vat
     const objectRef = kunser(createObjectData);
@@ -201,4 +213,129 @@ describe('Garbage Collection', () => {
     );
     expect(parseReplyBody(exporterFinalCheck.body)).toBe(false);
   }, 40000);
+
+  describe('an object shared by two importers', () => {
+    let secondImporterKRef: KRef;
+    let secondImporterVatId: VatId;
+
+    beforeEach(async () => {
+      kernelDatabase = await makeSQLKernelDatabase({ dbFilename: ':memory:' });
+      kernelStore = makeKernelStore(kernelDatabase);
+      kernel = await makeKernel(kernelDatabase, true, makeMockLogger());
+      await runTestVats(kernel, makeTestSubcluster(['Importer2']));
+
+      const vats = kernel.getVats();
+      const idOf = (name: string): VatId =>
+        vats.find((row) => row.config.parameters?.name === name)?.id as VatId;
+      exporterVatId = idOf('Exporter');
+      importerVatId = idOf('Importer');
+      secondImporterVatId = idOf('Importer2');
+      exporterKRef = kernelStore.getRootObject(exporterVatId) as KRef;
+      importerKRef = kernelStore.getRootObject(importerVatId) as KRef;
+      secondImporterKRef = kernelStore.getRootObject(
+        secondImporterVatId,
+      ) as KRef;
+    });
+
+    /**
+     * Give an importer a chance to notice a dropped object and tell the kernel,
+     * then keep cranking until the resulting GC actions have all been consumed.
+     *
+     * @param vatId - The vat to reap.
+     * @param rootKRef - That vat's root, to poke with cranks afterwards.
+     */
+    async function reapAndSettle(vatId: VatId, rootKRef: KRef): Promise<void> {
+      kernel.reapVats((id) => id === vatId);
+      // BOYD has to reach the vat, the vat has to answer, and the kernel has to
+      // act on the answer — but a round can queue more work, so loop until the
+      // queue is actually empty rather than guessing at a crank count.
+      const maxRounds = 10;
+      for (let round = 0; round < maxRounds; round++) {
+        await kernel.queueMessage(rootKRef, 'noop', []);
+        await waitUntilQuiescent(500);
+        if ([...kernelStore.getGCActions()].length === 0) {
+          return;
+        }
+      }
+      throw Error(
+        `GC actions still pending after ${maxRounds} rounds: ${[
+          ...kernelStore.getGCActions(),
+        ].join(', ')}`,
+      );
+    }
+
+    it('survives until both importers let go', async () => {
+      const objectId = 'shared-object';
+      const createObjectData = await kernel.queueMessage(
+        exporterKRef,
+        'createObject',
+        [objectId],
+      );
+      const sharedKRef = createObjectData.slots[0] as KRef;
+      const objectRef = kunser(createObjectData);
+
+      for (const importer of [importerKRef, secondImporterKRef]) {
+        await kernel.queueMessage(importer, 'storeImport', [
+          objectRef,
+          objectId,
+        ]);
+      }
+      await waitUntilQuiescent();
+
+      expect(kernelStore.getImporters(sharedKRef)).toStrictEqual(
+        [importerVatId, secondImporterVatId].sort(),
+      );
+      // Two importers, plus the resolved createObject promise whose value
+      // still carries the slot
+      expect(kernelStore.getObjectRefCount(sharedKRef)).toStrictEqual({
+        reachable: 3,
+        recognizable: 3,
+      });
+
+      await kernel.queueMessage(importerKRef, 'makeWeak', [objectId]);
+      await kernel.queueMessage(importerKRef, 'forgetImport', []);
+      await waitUntilQuiescent();
+      await reapAndSettle(importerVatId, importerKRef);
+
+      // The exporter must not have been told to drop it: the second importer
+      // legitimately still holds it
+      expect(kernelStore.getReachableFlag(exporterVatId, sharedKRef)).toBe(
+        true,
+      );
+      expect(kernelStore.getImporters(sharedKRef)).toStrictEqual([
+        secondImporterVatId,
+      ]);
+      expect(
+        parseReplyBody(
+          (
+            await kernel.queueMessage(exporterKRef, 'isObjectPresent', [
+              objectId,
+            ])
+          ).body,
+        ),
+      ).toBe(true);
+
+      expect(
+        parseReplyBody(
+          (
+            await kernel.queueMessage(secondImporterKRef, 'useImport', [
+              objectId,
+            ])
+          ).body,
+        ),
+      ).toBe(objectId);
+
+      await kernel.queueMessage(secondImporterKRef, 'makeWeak', [objectId]);
+      await kernel.queueMessage(secondImporterKRef, 'forgetImport', []);
+      await waitUntilQuiescent();
+      await reapAndSettle(secondImporterVatId, secondImporterKRef);
+
+      expect(kernelStore.getImporters(sharedKRef)).toStrictEqual([]);
+      // Only the createObject result's stored value still names it
+      expect(kernelStore.getObjectRefCount(sharedKRef)).toStrictEqual({
+        reachable: 1,
+        recognizable: 1,
+      });
+    }, 60000);
+  });
 });

packages/kernel-test/src/persistence.test.ts

@@ -170,7 +170,8 @@ describe('persistent storage', { timeout: 20_000 }, () => {
     // Enqueue a send message into the database
     kv1.set('queue.run.head', '4');
     kv1.set('nextPromiseId', '4');
-    kv1.set(`${v1Root}.refCount`, '3,3');
+    // The root's pin, plus the send being injected below.
+    kv1.set(`${v1Root}.refCount`, '2,2');
     kv1.set('queue.kp3.head', '1');
     kv1.set('queue.kp3.tail', '1');
     kv1.set('kp3.state', 'unresolved');

packages/kernel-test/src/refcount-audit.test.ts

@@ -0,0 +1,63 @@
+import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs';
+import { makeKernelStore } from '@metamask/ocap-kernel';
+import type { KRef, VatId } from '@metamask/ocap-kernel';
+import { expect, describe, it } from 'vitest';
+
+import {
+  getBundleSpec,
+  makeKernel,
+  makeMockLogger,
+  runTestVats,
+} from './utils.ts';
+
+/**
+ * The per-crank audit throws from inside the run loop, which nothing restarts.
+ * Unless that failure is reported to whoever is waiting on the kernel, the only
+ * symptom is a test that hangs until its timeout, with no mention of reference
+ * counts anywhere — which would make the audit worthless as a build gate.
+ */
+describe('reference count audit', () => {
+  it('reports a violation to kernel callers rather than hanging', async () => {
+    const kernelDatabase = await makeSQLKernelDatabase({
+      dbFilename: ':memory:',
+    });
+    const kernelStore = makeKernelStore(kernelDatabase);
+    const kernel = await makeKernel(kernelDatabase, true, makeMockLogger());
+    await runTestVats(kernel, {
+      bootstrap: 'exporter',
+      forceReset: true,
+      vats: {
+        exporter: {
+          bundleSpec: getBundleSpec('exporter-vat'),
+          parameters: { name: 'Exporter' },
+        },
+      },
+    });
+
+    const exporterVatId = kernel.getVats()[0]?.id as VatId;
+    const exporterKRef = kernelStore.getRootObject(exporterVatId) as KRef;
+
+    kernelStore.setObjectRefCount(exporterKRef, {
+      reachable: 7,
+      recognizable: 9,
+    });
+
+    // The crank carrying this message settles its result before the
+    // end-of-crank audit runs, so this one may still succeed.
+    await kernel
+      .queueMessage(exporterKRef, 'createObject', ['x'])
+      .catch(() => undefined);
+
+    // What a caller is told directly is that the run loop is gone; the audit
+    // failure that killed it rides along as the `cause`. That chain is the part
+    // that has to survive, since "run loop died" on its own names nothing.
+    const failure = (await kernel
+      .queueMessage(exporterKRef, 'createObject', ['y'])
+      .catch((error) => error)) as Error;
+
+    expect(failure.message).toMatch(/Kernel run loop died/u);
+    expect(String(failure.cause)).toMatch(
+      /reference count invariant violated/u,
+    );
+  }, 30000);
+});

packages/kernel-test/src/utils.ts

@@ -93,6 +93,9 @@ export async function makeKernel(
     resetStorage,
     logger,
     keySeed,
+    // Refcount drift is invisible to ordinary assertions until something gets
+    // collected out from under a live holder, so check it every crank.
+    auditRefCounts: true,
   });
   return kernel;
 }