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/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,55 @@
+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);
+
+    await expect(
+      kernel.queueMessage(exporterKRef, 'createObject', ['y']),
+    ).rejects.toThrow(/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;
 }

packages/ocap-kernel/CHANGELOG.md

@@ -23,10 +23,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Log a warning when a vat requests an unknown global
 - Export `OcapURLIssuerService` and `OcapURLRedemptionService` types so vats can type the corresponding kernel-service endowments ([#952](https://github.com/MetaMask/ocap-kernel/pull/952))
 - Reference-marker sigil (`@@NAME`) at the `queueMessage` RPC boundary lets JSON-RPC callers name a live kernel object as a call argument ([#984](https://github.com/MetaMask/ocap-kernel/pull/984))
+
   - Anywhere in the args tree, a string of the form `@@NAME` (NAME is one or more alphanumeric characters, currently a well-formed kref) is expanded to a `kslot` standin so `kser` encodes it as a real CapData slot in the dispatched message
   - Purely an RPC-boundary concern: internal callers of `Kernel.queueMessage` are unaffected
   - Caveat: a legitimate string argument that begins with `@@` followed by alphanumerics will be misinterpreted as a marker; wrap such literals inside an object
 
+- Reference-count auditing: `auditRefCounts`, `recomputeRefCounts`, `formatRefCountViolations`, `assertRefCountsIfAuditing`, and `setRefCountAuditing` on the kernel store, plus a `Kernel.make` option `auditRefCounts` that verifies every kref's counts against the references the kernel actually holds at the end of each crank ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+  - Reports drift in both directions: counts too low (a live capability can be collected) and counts too high with no holder (a leak)
+  - Exports the `RefCountViolation` type, a union over `kind: 'mismatch' | 'dangling'`
+- Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+- Add `orphanKernelObject` to the kernel store, which drops an object's owner mapping and hands it to the collector ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+
 ### Changed
 
 - Attribute a failed subcluster vat launch to the specific vat by kernel id and `ClusterConfig` name (e.g. `Failed to launch vat v3 (bob)`), preserving the original error as the `cause` ([#975](https://github.com/MetaMask/ocap-kernel/pull/975))
@@ -38,6 +45,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **BREAKING:** Make c-list reference accounting symmetric: creating an import c-list entry now takes a reference, as tearing one down has always released one ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+  - `initKernelObject` now births objects at `(0, 0)` instead of `(1, 1)`. The old constant made the arithmetic come out right for exactly one importer, masking the missing increment; with two importers a live capability could be dropped and retired out from under a holder
+  - Removes the owner-side baseline decrements in `cleanupTerminatedVat` and `forgetEndpointImports`, which double-claimed the same unit an importer's drop also spent — the source of `"koNN" underflow -1,0` escaping mid-cleanup and leaving a vat half-cleaned
+  - `translateRefKtoE` now re-establishes reachability, so a vat handed an object it previously dropped is counted as holding it live again
+  - Renames `krefsToExistingErefs` to `krefsToErefs`, which now throws on an unmapped kref instead of silently dropping it
+- Pin vat root objects for the lifetime of their vat, and release the pin on termination ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+  - A root is addressable while its vat lives whether or not anyone imports it; the old `(1, 1)` birth baseline was standing in for this
+- Garbage-collection action delivery now moves the kernel's own c-list: `dropExports` clears the owner's reachable flag and `retireExports`/`retireImports` tear the entry down ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+  - Previously the owner's flag never cleared, so the same action could be re-derived, and retired entries outlived the objects they named
+- Orphan a kernel object when its owner stops naming it (a delivered `retireExport`, or a `retireExports`/`abandonExports` syscall), so its `owner` and `refCount` records no longer outlive the c-list entry they were reachable through — they leaked, and the next collection to visit such a kref killed the run loop ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+- `queueMessage` now rejects with the error that stopped the run loop, and messages the kernel itself is awaiting are rejected rather than left pending forever ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+- Reject a `retireExports`/`abandonExports` syscall for an object the calling endpoint does not own, instead of letting it erase another endpoint's claim to an object it is still exporting ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+- Garbage-collection action delivery releases the kernel's side even when the endpoint has vanished, and rolls back rather than committing a release the endpoint was never told about ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+- Tear down and mark for cleanup a vat whose worker launched but whose kernel-side registration then failed ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+- Charge a delivered message's target reference against the run-queue item's own target rather than the routed target, so a message routed through a resolved promise no longer decrements an object nobody charged while leaking the promise ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+- Release a queued notification's reference before the paths that decide there is nothing to deliver, and stop decrementing references on promises retired alongside it that nobody had taken ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+- Transfer, rather than duplicate, the references a message carries when it is queued on an unresolved promise and later re-enqueued on resolution ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+- Fix the stale `cle.`/`clk.` key prefixes in `getPromisesByDecider` and `deleteEndpoint`, which no longer matched the `${endpointId}.c.` c-list layout ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
+
+  - `getPromisesByDecider` matched nothing, so promises a terminating vat or restarting peer was deciding were never rejected
+
 - Deserialize CapData rejections in `Kernel.queueMessage` so vat errors surface as plain `Error` objects to all callers ([#928](https://github.com/MetaMask/ocap-kernel/pull/928))
 - Detect peer restart across receiver state loss so the receiving kernel no longer silently drops a restarted peer's `seq=1` messages ([#948](https://github.com/MetaMask/ocap-kernel/pull/948))
   - Persist the peer's last-observed incarnation and compare it on every successful handshake; on a detected restart, clear the peer's c-list contributions and reject the promises it was deciding before the new incarnation reuses any erefs