fix(ocap-kernel): free retired exports and harden GC delivery

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

@@ -254,17 +254,30 @@ describe('Garbage Collection', () => {
     });
 
     /**
-     * Give an importer a chance to notice a dropped object and tell the kernel.
+     * 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);
-      for (let i = 0; i < 3; i++) {
+      // 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 () => {

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/ocap-kernel/CHANGELOG.md

@@ -38,12 +38,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - 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)
+  - Reports drift in both directions: counts too low (a live capability can be collected) and counts too high with no holder (an orphaned count). It compares counts against the holders it finds, so a holder that should have been torn down but wasn't justifies its own count and is not detectable this way
   - 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
+  - Exports the `RefCountViolation` type, a union over `kind: 'mismatch' | 'dangling'`
 - 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))
+- Add `orphanKernelObject` to the kernel store, which drops an object's owner mapping and hands it to the collector ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022))
 
 ### Changed
 
@@ -96,6 +97,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - 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
+- 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 ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022))
+  - They leaked, and the next collection to visit such a kref read a c-list entry that was no longer there and killed the run loop. Reproduces on `main`, so it predates this stack
+- 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 ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022))
+  - Nothing upstream of `performExportCleanup` checked that the vref it was handed is even an export, and the audit could not see the damage, because an export entry carries no count
+- 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 ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022))
+  - It releases only where the endpoint is genuinely gone: a terminated vat, whose cleanup tears the whole c-list down anyway, or a remote, which reconciles on its next incarnation. A vat that is absent yet not terminated is one `restartVat` has taken out of the kernel's reach while keeping its c-list, so the crank fails there instead of committing a release the returning incarnation would disagree with
+- A failed garbage-collection delivery to a remote is logged and survived rather than escaping the crank and stopping the run loop ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022))
+- Tear down and mark for cleanup a vat whose worker launched but whose kernel-side registration then failed ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022))
 - 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))

packages/ocap-kernel/src/Kernel.ts

@@ -111,8 +111,12 @@ export class Kernel {
    * @param options.onRunLoopFailure - Optional handler called if the run loop dies.
    * @param options.auditRefCounts - If true, verify every kref's reference
    * counts against the references the kernel actually holds at the end of each
-   * crank, and throw on any mismatch. Intended for tests and debugging; the
-   * audit walks the whole store.
+   * crank, and throw on any mismatch. This is the check standing in for the
+   * accounting invariant `collectGarbage` still cannot assert (see the comment
+   * on its `retireExport` branch), so it is not optional
+   * instrumentation: it is off by default only because it walks the whole store
+   * every crank. Any kernel whose accounting is under test wants it on, and
+   * every kernel `kernel-test` builds enables it.
    */
   // eslint-disable-next-line no-restricted-syntax
   private constructor(

packages/ocap-kernel/src/KernelRouter.test.ts

@@ -65,6 +65,9 @@ describe('KernelRouter', () => {
       clearReachableFlag: vi.fn(),
       deleteCListEntry: vi.fn(),
       forgetKref: vi.fn(),
+      orphanKernelObject: vi.fn(),
+      hasCListEntry: vi.fn().mockReturnValue(true),
+      isVatTerminated: vi.fn().mockReturnValue(false),
       createCrankSavepoint: vi.fn(),
     } as unknown as KernelStore;
 
@@ -317,6 +320,38 @@ describe('KernelRouter', () => {
         ]);
       });
 
+      it('charges the promise, not the object it resolved to', async () => {
+        const promiseId = 'kp123';
+        const resolvedObject = 'ko456';
+        (
+          kernelStore.getKernelPromise as unknown as MockInstance
+        ).mockReturnValueOnce({
+          state: 'fulfilled',
+          value: { body: '#"$0"', slots: [resolvedObject] },
+        });
+        (kernelStore.getOwner as unknown as MockInstance).mockReturnValue('v1');
+
+        await kernelRouter.deliver({
+          type: 'send',
+          target: promiseId,
+          message: {
+            methargs: { body: 'method args', slots: [] },
+            result: null,
+          },
+        });
+
+        // The run queue item was charged against the promise it named, so that
+        // is what has to be released — not whatever routing resolved it to.
+        expect(kernelStore.decrementRefCount).toHaveBeenCalledWith(
+          promiseId,
+          'deliver|send|target',
+        );
+        expect(kernelStore.decrementRefCount).not.toHaveBeenCalledWith(
+          resolvedObject,
+          'deliver|send|target',
+        );
+      });
+
       it('splats message when promise resolves to a non-object', async () => {
         // Setup a fulfilled promise that doesn't resolve to an object
         const promiseId = 'kp123';
@@ -580,6 +615,12 @@ describe('KernelRouter', () => {
         // Verify no notification was delivered to the vat
         expect(endpointHandle.deliverNotify).not.toHaveBeenCalled();
         expect(result).toStrictEqual({ didDelivery: endpointId });
+        // Nothing was delivered, but the queued notification is gone either
+        // way, so its reference has to be released on this path too.
+        expect(kernelStore.decrementRefCount).toHaveBeenCalledWith(
+          kpid,
+          'deliver|notify',
+        );
       });
 
       it('returns didDelivery when no kpids to retire', async () => {
@@ -618,6 +659,10 @@ describe('KernelRouter', () => {
         // Verify no notification was delivered to the vat
         expect(endpointHandle.deliverNotify).not.toHaveBeenCalled();
         expect(result).toStrictEqual({ didDelivery: endpointId });
+        expect(kernelStore.decrementRefCount).toHaveBeenCalledWith(
+          kpid,
+          'deliver|notify',
+        );
       });
 
       it('throws if notification is for an unresolved promise', async () => {
@@ -715,6 +760,168 @@ describe('KernelRouter', () => {
           ]);
         },
       );
+
+      it('orphans the object when delivering retireExports', async () => {
+        await kernelRouter.deliver({
+          type: 'retireExports',
+          endpointId: 'v1',
+          krefs: ['ko1', 'ko2'],
+        });
+
+        // The owner has given up the last name for the object, so the kernel's
+        // record of who owns it must go too or it outlives every reference.
+        expect(
+          (kernelStore.orphanKernelObject as unknown as MockInstance).mock
+            .calls,
+        ).toStrictEqual([
+          ['ko1', 'v1'],
+          ['ko2', 'v1'],
+        ]);
+      });
+
+      it('leaves ownership alone when delivering retireImports', async () => {
+        await kernelRouter.deliver({
+          type: 'retireImports',
+          endpointId: 'v1',
+          krefs: ['ko1'],
+        });
+
+        expect(kernelStore.orphanKernelObject).not.toHaveBeenCalled();
+      });
+
+      it('still releases the kernel side when a terminated vat has vanished', async () => {
+        getEndpoint.mockImplementationOnce(() => {
+          throw Error('vat v1 not found');
+        });
+        (
+          kernelStore.isVatTerminated as unknown as MockInstance
+        ).mockReturnValue(true);
+
+        const result = await kernelRouter.deliver({
+          type: 'retireImports',
+          endpointId: 'v1',
+          krefs: ['ko1'],
+        });
+
+        expect(result).toStrictEqual({ didDelivery: 'v1' });
+        // The action has already been consumed, so skipping the teardown would
+        // lose it and leave the entry behind for good
+        expect(kernelStore.deleteCListEntry).toHaveBeenCalledWith(
+          'v1',
+          'ko1',
+          'translated-ko1',
+        );
+      });
+
+      it('still releases the kernel side when a remote has vanished', async () => {
+        getEndpoint.mockImplementationOnce(() => {
+          throw Error('remote r1 not found');
+        });
+
+        const result = await kernelRouter.deliver({
+          type: 'retireImports',
+          endpointId: 'r1',
+          krefs: ['ko1'],
+        });
+
+        expect(result).toStrictEqual({ didDelivery: 'r1' });
+        expect(kernelStore.deleteCListEntry).toHaveBeenCalledWith(
+          'r1',
+          'ko1',
+          'translated-ko1',
+        );
+      });
+
+      it.each(['dropExports', 'retireExports', 'retireImports'] as const)(
+        'refuses to release %s for a vat that is absent but not terminated',
+        async (actionType) => {
+          // A vat between incarnations still holds every one of these krefs, so
+          // committing the kernel's release would leave the two disagreeing.
+          getEndpoint.mockImplementationOnce(() => {
+            throw Error('vat v1 not found');
+          });
+
+          await expect(
+            kernelRouter.deliver({
+              type: actionType,
+              endpointId: 'v1',
+              krefs: ['ko1'],
+            }),
+          ).rejects.toThrow('vat v1 not found');
+
+          expect(kernelStore.clearReachableFlag).not.toHaveBeenCalled();
+          expect(kernelStore.deleteCListEntry).not.toHaveBeenCalled();
+          expect(kernelStore.orphanKernelObject).not.toHaveBeenCalled();
+        },
+      );
+
+      it('skips krefs already cleaned up before delivery', async () => {
+        (
+          kernelStore.hasCListEntry as unknown as MockInstance
+        ).mockImplementation(
+          (_endpointId: string, kref: string) => kref === 'ko1',
+        );
+
+        await kernelRouter.deliver({
+          type: 'retireImports',
+          endpointId: 'v1',
+          krefs: ['ko1', 'ko2'],
+        });
+
+        expect(
+          (kernelStore.deleteCListEntry as unknown as MockInstance).mock.calls,
+        ).toStrictEqual([['v1', 'ko1', 'translated-ko1']]);
+      });
+
+      it('does nothing when every kref is already gone', async () => {
+        (kernelStore.hasCListEntry as unknown as MockInstance).mockReturnValue(
+          false,
+        );
+
+        const result = await kernelRouter.deliver({
+          type: 'retireImports',
+          endpointId: 'v1',
+          krefs: ['ko1'],
+        });
+
+        expect(result).toStrictEqual({ didDelivery: 'v1' });
+        expect(kernelStore.deleteCListEntry).not.toHaveBeenCalled();
+        expect(endpointHandle.deliverRetireImports).not.toHaveBeenCalled();
+      });
+
+      it('rolls back and terminates the vat when delivery fails', async () => {
+        (
+          endpointHandle.deliverRetireImports as unknown as MockInstance
+        ).mockRejectedValueOnce(Error('endpoint went away mid-delivery'));
+
+        const result = await kernelRouter.deliver({
+          type: 'retireImports',
+          endpointId: 'v1',
+          krefs: ['ko1'],
+        });
+
+        // Committing the release while v1 still holds the eref would leave the
+        // two disagreeing, and v1 would mint a fresh kref for the same object
+        expect(result?.abort).toBe(true);
+        expect(result?.terminate?.vatId).toBe('v1');
+      });
+
+      it('does not retry a remote that refuses the delivery', async () => {
+        (
+          endpointHandle.deliverRetireImports as unknown as MockInstance
+        ).mockRejectedValueOnce(Error('remote queue full'));
+
+        const result = await kernelRouter.deliver({
+          type: 'retireImports',
+          endpointId: 'r1',
+          krefs: ['ko1'],
+        });
+
+        // Aborting would restore the action, and GC actions are selected ahead
+        // of all other work, so a remote that keeps refusing would be handed
+        // this same item every crank and nothing else would ever run
+        expect(result).toStrictEqual({ didDelivery: 'r1' });
+      });
     });
 
     describe('bringOutYourDead', () => {