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

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

@@ -252,7 +252,6 @@ test.describe('Control Panel', () => {
       `{"key":"v3.c.o+0","value":"${v3Root}"}`,
       `{"key":"v3.c.${v3Promise}","value":"R p-1"}`,
       `{"key":"v3.c.p-1","value":"${v3Promise}"}`,
-      `{"key":"${v3Root}.refCount","value":"1,1"}`,
       `{"key":"${v3Promise}.refCount","value":"2"}`,
     ];
     // Derived too: v1 imports the two roots as the bootstrap's calls are

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,116 @@ 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.
+     *
+     * @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);
+      for (let i = 0; i < 3; i++) {
+        await kernel.queueMessage(rootKRef, 'noop', []);
+        await waitUntilQuiescent(500);
+      }
+    }
+
+    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

@@ -176,7 +176,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/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

@@ -32,10 +32,19 @@ 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 ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+  - Reports drift in both directions: counts too low (a live capability can be collected) and counts too high (a leak)
+  - Only references visible in the kernel's own state are checkable, so a holder that keeps a kref outside them has to take a pin to be counted at all
+  - `recomputeRefCounts` is a repair tool for a drifted store, offered to embedders and never run automatically: opening an existing store does not migrate it
+  - Exports the `RefCountViolation` type
+- Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+- Add `getOcapURLObjects` and `retainForOcapURL` to the kernel store, and `VatManager.releaseVatRootPin` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+
 ### Changed
 
 - **BREAKING:** `Kernel.make`'s `ioChannelFactory` option is now `ioListenerFactory`, and the exported `IOChannelFactory` type is replaced by `IOListener` and `IOListenerFactory`. A cluster config's `io` entries now create listeners; vats call `accept()` to obtain a channel instead of reading and writing the endowment directly ([#1007](https://github.com/MetaMask/ocap-kernel/pull/1007))
@@ -49,7 +58,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - A message delivered to a kernel-owned kref with no registered service now rejects the caller with `ENDPOINT_UNREACHABLE` instead of throwing, which escaped the crank and killed the run loop — turning one unreachable reference into a dead kernel ([#1007](https://github.com/MetaMask/ocap-kernel/pull/1007))
-  - Reachable without any kernel bug: an anonymous kernel object hosts something that cannot outlive the process, such as an accepted socket connection, so a vat holding one across a restart or a message to one still queued from the previous incarnation lands here. Kernel objects are born with a `(1, 1)` refcount, so the init sweep cannot delete such an object and its `kernel` owner survives (see [#1006](https://github.com/MetaMask/ocap-kernel/issues/1006))
+  - Reachable without any kernel bug: an anonymous kernel object hosts something that cannot outlive the process, such as an accepted socket connection, so a vat holding one across a restart or a message to one still queued from the previous incarnation lands here. That surviving reference is exactly what stops the init sweep deleting the object, so its `kernel` owner survives with it
   - Matches what `KernelRouter` already does for a delivery whose endpoint has vanished. A message sent with no result promise has nobody to report to, so it is logged instead
 - The kernel run queue no longer strands messages, going quiet with no error, no log, and no crash. `runQueueLengthCache` uses a negative value to mean "unknown, re-read from the database", but `enqueueRun`/`dequeueRun` adjusted it arithmetically without materializing it first — so an enqueue while the cache held `-1` produced `0` for a queue that actually held an item, and because `0` is not negative it was never re-read again. The run loop then saw an empty queue, went to sleep, and stranded everything behind it. Two paths reach that `-1`: kernel startup, and `rollbackCrank`, which invalidates the cache because a rollback may have restored dequeued items. The rollback path is the more likely of the two in practice, since a rollback is normally followed immediately by enqueueing an error or termination message. The run loop is also now woken by any non-empty queue rather than only by the empty-to-one transition, so a drifted count cannot silently lose the wakeup either ([#1007](https://github.com/MetaMask/ocap-kernel/pull/1007))
 - Stop reporting a healthy kernel after the run loop dies ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005))
@@ -65,6 +74,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Report the database error when an aborted crank cannot be rolled back, instead of a spurious "no such savepoint" ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005))
   - The abort path recorded the rollback only after it succeeded, so a throwing rollback had the run loop try again against the savepoint `rollbackCrank` had already discarded. The second attempt's "no such savepoint" then became the reported cause of death — and since only `error.message` crosses the wire, the real failure reached neither `getStatus` nor the daemon log
 - Reject the run loop's promise with the same `Error` its status reports, rather than re-throwing a non-`Error` for the embedder to normalize a second time ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005))
+- **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 ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+  - `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 ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+  - 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 ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+  - Previously the owner's flag never cleared, so the same action could be re-derived, and retired entries outlived the objects they named
+- 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 ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+- 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 ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+- Transfer, rather than duplicate, the references a message carries when it is queued on an unresolved promise and later re-enqueued on resolution ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+- Fix the stale `cle.`/`clk.` key prefixes in `getPromisesByDecider` and `deleteEndpoint`, which no longer matched the `${endpointId}.c.` c-list layout ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+  - `getPromisesByDecider` matched nothing, so promises a terminating vat or restarting peer was deciding were never rejected
+- Issuing an ocap URL now retains its target, so the URL stays redeemable ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+  - A URL carries its kref inside an encrypted bearer token and nothing else, so the kernel cannot see from its own state that a holder exists. Under the old `(1, 1)` birth baseline nothing exported was ever collectable and this went unnoticed; at `(0, 0)` the target is collected as soon as the message that carried it to the issuer is delivered, and the URL names a dead capability
+  - One pin per kref however many URLs name it, and no release: the token is persistent and unexpiring, so `revoke` is the way to kill the capability
+  - Issuing a URL for a kref the kernel has already deleted is now refused rather than resurrecting its counts
+  - The retention is taken before the token is minted, since minting awaits and a collection crank can run in that window; it is released again if minting fails, and only when that call was the one that took it
+- Refuse to import a kref the kernel has deleted into an endpoint's c-list ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+  - `getObjectRefCount` reads a missing entry as `(0, 0)`, so the new entry's own increment wrote it back and resurrected a live-looking object with no owner — deliverable to by nobody, and endorsed by the audit, since the entry is a legitimate holder for exactly the count it finds
+- Release a vat's root pin when a subcluster is deleted without its vats having run ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
+
+  - `deleteSubcluster` bypasses `stopVat`, so nothing released the pin `launchVat` took in the incarnation that did run them, leaving the root's count permanently above zero and `pinnedObjects` naming a vat that no longer exists
+
 - 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