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 = [
@@ -190,6 +189,13 @@ test.describe('Control Panel', () => {
         popupPage.locator('[data-testid="message-output"]'),
       ).toContainText(value);
     }
+    // Asserted per checkpoint rather than from `v3Values`, which is used as a
+    // negative below: v3's root keeps a count for as long as v1 imports it, so
+    // it is not one of the keys that vanish with the vat. The value is the root
+    // pin plus that import.
+    await expect(
+      popupPage.locator('[data-testid="message-output"]'),
+    ).toContainText('{"key":"ko6.refCount","value":"2,2"}');
     await popupPage.click('button:text("Control Panel")');
     await popupPage.locator('[data-testid="accordion-header"]').first().click();
     await popupPage
@@ -215,6 +221,11 @@ test.describe('Control Panel', () => {
         popupPage.locator('[data-testid="message-output"]'),
       ).toContainText(value);
     }
+    // Terminating v3 released the pin its root was held by, leaving v1's import
+    // as the only holder.
+    await expect(
+      popupPage.locator('[data-testid="message-output"]'),
+    ).toContainText('{"key":"ko6.refCount","value":"1,1"}');
     await popupPage.click('button:text("Control Panel")');
 
     await popupPage.click('button:text("Collect Garbage")');
@@ -242,6 +253,11 @@ test.describe('Control Panel', () => {
     await expect(
       popupPage.locator('[data-testid="message-output"]'),
     ).toContainText('{"key":"kp4.refCount","value":"1"}');
+    // v3's cleanup took its own c-list, not v1's import, so the root survives
+    // its owner at the one count that import justifies.
+    await expect(
+      popupPage.locator('[data-testid="message-output"]'),
+    ).toContainText('{"key":"ko6.refCount","value":"1,1"}');
     await popupPage.click('button:text("Control Panel")');
     await popupPage.locator('[data-testid="accordion-header"]').first().click();
     // delete v1

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

@@ -138,6 +138,94 @@ 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();
+  });
+
+  // The set is not per-crank: only `collectGarbage` empties it, and that runs at
+  // the end of a crank that had an item. So a candidate created while the run
+  // loop was idle — `terminateVat` unpinning a root is the real path — is still
+  // owed a collection, and an unrelated crank's rollback must not cancel it.
+  it('keeps GC candidates that predate the crank it rolled back', async () => {
+    const { kernelStore } = await makeStore();
+    const idle = kernelStore.initKernelPromise()[0];
+    kernelStore.decrementRefCount(idle, 'test');
+
+    kernelStore.startCrank();
+    kernelStore.createCrankSavepoint('start');
+    const abandoned = kernelStore.initKernelPromise()[0];
+    kernelStore.decrementRefCount(abandoned, 'test');
+    kernelStore.rollbackCrank('start');
+    kernelStore.endCrank();
+
+    kernelStore.startCrank();
+    kernelStore.createCrankSavepoint('start');
+    kernelStore.collectGarbage();
+    kernelStore.endCrank();
+
+    // Collected, because it was owed before the abandoned crank began.
+    expect(() => kernelStore.getKernelPromise(idle)).toThrow(
+      'unknown kernel promise',
+    );
+  });
+
   // `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');