fix(ocap-kernel): keep an absent endpoint from killing the run loop

packages/kernel-test/src/remote-comms.test.ts

@@ -3,7 +3,7 @@ import { peerIdFromPrivateKey } from '@libp2p/peer-id';
 import { NodejsPlatformServices } from '@metamask/kernel-node-runtime';
 import type { KernelDatabase } from '@metamask/kernel-store';
 import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs';
-import { fromHex } from '@metamask/kernel-utils';
+import { fromHex, waitUntilQuiescent } from '@metamask/kernel-utils';
 import { makeKernelStore, kunser, Kernel } from '@metamask/ocap-kernel';
 import type {
   KernelStore,
@@ -494,6 +494,95 @@ describe('Remote Communications (Integration Tests)', () => {
       await rm(tempDir, { recursive: true, force: true });
     }
   });
+
+  it('is not bricked by a peer asking it to bring out its dead', async () => {
+    // `bringOutYourDead` is an ordinary arm of the remote protocol: any peer can
+    // send one, unsolicited. The kernel answers by scheduling a reap against the
+    // remote it came from, in the persisted reap queue.
+    //
+    // `scheduleReap` does not wake a parked run loop, so an idle kernel holds
+    // that reap indefinitely — and carries it into its next incarnation, which
+    // starts its run loop inside `Kernel.make`, before an embedder can call
+    // `initRemoteComms` to restore any remote to deliver it to. One message from
+    // a peer is therefore enough to stop a kernel ever booting again, given only
+    // that it restarts at some point.
+    const tempDir = await mkdtemp(join(tmpdir(), 'kernel-test-rc-reap-'));
+    const dbFile = join(tempDir, 'victim.db');
+    try {
+      // Only the victim needs to survive a restart, so only it needs a file.
+      await kernel1.stop();
+      const victimStore = makeKernelStore(
+        await makeSQLKernelDatabase({ dbFilename: dbFile }),
+      );
+      let victim = await makeTestKernel(
+        'victim',
+        await makeSQLKernelDatabase({ dbFilename: dbFile }),
+        directNetwork,
+        true,
+        'kernel1-peer',
+        '01',
+      );
+
+      // One exchange, so each kernel holds a remote for the other.
+      await runTestVats(victim, makeSenderSubclusterConfig('Sender'));
+      const receiver = (await runTestVats(
+        kernel2,
+        makeReceiverSubclusterConfig('Receiver'),
+      )) as BootstrapResult;
+      await victim.queueMessage(
+        victimStore.getRootObject('v1') as KRef,
+        'sendMessage',
+        [receiver.ocapURL, 'hello', ['once']],
+      );
+
+      // The attack, in one message. The peer is given local work purely so its
+      // own loop cranks and sends the request; nothing touches the victim
+      // afterwards, so the victim's loop stays parked and never delivers the
+      // reap it just queued.
+      kernel2.reapRemotes();
+      await kernel2.queueMessage(
+        makeKernelStore(kernelDatabase2).getRootObject('v1') as KRef,
+        'hello',
+        ['probe'],
+      );
+      await waitUntilQuiescent();
+      await victim.stop();
+
+      // Asserted, not assumed: if the victim had cranked it would have eaten its
+      // own reap while the remote still existed, and the rest would prove nothing.
+      const armed = await makeSQLKernelDatabase({ dbFilename: dbFile });
+      expect(
+        JSON.parse(armed.kernelKVStore.get('reapQueue') ?? '[]'),
+      ).not.toStrictEqual([]);
+
+      // Twice, because it is unrecoverable rather than merely fatal: the crank
+      // that dies is rolled back, which puts the reap back on the queue for the
+      // boot after this one.
+      let database = armed;
+      const bootStates = [];
+      for (const boot of [1, 2]) {
+        victim = await makeTestKernel(
+          `victim-boot${boot}`,
+          database,
+          directNetwork,
+          false,
+          'kernel1-peer',
+          '01',
+        );
+        bootStates.push((await victim.getStatus()).runLoop);
+        await victim.stop();
+        database = await makeSQLKernelDatabase({ dbFilename: dbFile });
+      }
+      // Asserted together rather than per boot, so a failure reports both: the
+      // point is that the second is no better than the first.
+      expect(bootStates).toStrictEqual([
+        { state: 'running' },
+        { state: 'running' },
+      ]);
+    } finally {
+      await rm(tempDir, { recursive: true, force: true });
+    }
+  });
 });
 
 /**

packages/ocap-kernel/CHANGELOG.md

@@ -61,6 +61,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- A `notify`, GC action, or `bringOutYourDead` addressed to an endpoint that is not running is skipped instead of killing the run loop ([#1029](https://github.com/MetaMask/ocap-kernel/pull/1029))
+  - These deliveries looked up their endpoint unguarded, so an endpoint named by persisted state but absent from the running kernel threw `VatNotFoundError` from inside the crank, which killed the run loop permanently. Because the crank was rolled back the item was restored to the queue, so the next boot dequeued it and died too
+  - Reached by a peer's routine remote GC. A kernel answers a peer's `bringOutYourDead` by scheduling a reap against the remote it came from, and the reap queue is persisted. `scheduleReap` does not wake a parked run loop, so on an otherwise idle kernel that reap sits in the queue until something else gives the loop work — and a kernel shut down in the meantime carries it into the next incarnation. That incarnation starts its run loop inside `Kernel.make`, before an embedder can call `initRemoteComms`, which is what restores remote handles; reaps are taken ahead of the run queue, so the loop's first act is to deliver one addressed to a remote that does not exist yet. The kernel is dead before `Kernel.make` returns, and stays dead on every boot after that
+  - A reap is the delivery that reaches this most easily, because nothing filters it: a GC action is dropped by `shouldProcessAction` once the endpoint has no c-list entry, and a `notify` short-circuits on the same check, but a reap carries no kref and is handed back with no liveness check at all. Nothing purges the reap queue when its endpoint goes away
+  - Also reachable while a terminated vat awaits cleanup, which happens one vat per crank, since `deleteVat` takes its config and subcluster membership but leaves its c-lists and reachable flags in place. Not via `terminateSubcluster`, which drains every pending cleanup after each vat it terminates
+  - Unlike a `send`, none of these has a caller to reject; `send` already tolerated a vanished endpoint by rejecting the caller with `ENDPOINT_UNREACHABLE`
+  - A skipped GC action still performs the kernel's own half — clearing the reachable flag, or tearing the c-list entry down — since that does not depend on the endpoint being there to be told. Skipping it would leave a dropped export flagged reachable, so the same action would be derived again on every sweep
+  - Unless the c-list entries have gone in the meantime: an action is selected only while they exist, but the run loop cleans one terminated vat between that selection and the delivery, and cleaning a vat takes its whole c-list. The cleanup has done the kernel's half in that case, and translating the krefs anyway would report an unmapped kref by throwing — out of the crank, killing the run loop exactly as the unguarded lookup did
+  - A skipped `notify` no longer translates the resolution first. Those translations import if needed, which would mint c-list entries and take references in an endpoint that will never be told and so can never release them
+  - An endpoint id that names neither a vat nor a remote still throws, since that is corrupt state rather than an endpoint that has gone away
 - 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. 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

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

@@ -1,3 +1,4 @@
+import { Logger } from '@metamask/logger';
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import type { MockInstance } from 'vitest';
 
@@ -63,6 +64,7 @@ describe('KernelRouter', () => {
       krefsToErefs: vi.fn((_endpointId: string, krefs: string[]) =>
         krefs.map((kref: string) => `translated-${kref}`),
       ) as unknown as MockInstance,
+      hasCListEntry: vi.fn().mockReturnValue(true),
       clearReachableFlag: vi.fn(),
       deleteCListEntry: vi.fn(),
       forgetKref: vi.fn(),
@@ -808,6 +810,207 @@ describe('KernelRouter', () => {
       });
     });
 
+    describe('an endpoint named by persisted state that is not running', () => {
+      // A vat's ownership entries outlive it. `deleteVat` takes its config and
+      // subcluster membership at termination, but its c-lists and reachable
+      // flags stay until `cleanupTerminatedVat` gets to it — and that runs one
+      // vat per crank, so terminating a subcluster of N leaves a window N
+      // cranks wide in which the kernel still addresses a vat with no handle.
+      //
+      // Unlike a send, none of these deliveries has a caller to reject.
+      const endpointId = 'v2';
+
+      beforeEach(() => {
+        (getEndpoint as unknown as MockInstance).mockImplementation(
+          (requested: EndpointId) => {
+            if (requested === endpointId) {
+              throw new Error(`Vat not found: ${requested}`);
+            }
+            return endpointHandle;
+          },
+        );
+      });
+
+      /**
+       * Set up a notify whose promise is resolved and still in the endpoint's
+       * c-list, so delivery is reached rather than short-circuited.
+       *
+       * @returns The notify item to deliver.
+       */
+      const makeLiveNotify = (): RunQueueItemNotify => {
+        const kpid = 'kp123';
+        (
+          kernelStore.getKernelPromise as unknown as MockInstance
+        ).mockReturnValue({
+          state: 'fulfilled',
+          value: { body: JSON.stringify({ value: 'v' }), slots: [] },
+        });
+        (kernelStore.krefToEref as unknown as MockInstance).mockReturnValue(
+          'p+123',
+        );
+        (
+          kernelStore.getKpidsToRetire as unknown as MockInstance
+        ).mockReturnValue([kpid]);
+        return { type: 'notify', endpointId, kpid };
+      };
+
+      it.each([
+        [
+          'notify',
+          (): RunQueueItem => makeLiveNotify(),
+          'deliverNotify' as const,
+        ],
+        [
+          'dropExports',
+          (): RunQueueItem => ({
+            type: 'dropExports' as GCRunQueueType,
+            endpointId,
+            krefs: ['ko1'],
+          }),
+          'deliverDropExports' as const,
+        ],
+        [
+          'bringOutYourDead',
+          (): RunQueueItem => ({ type: 'bringOutYourDead', endpointId }),
+          'deliverBringOutYourDead' as const,
+        ],
+      ])(
+        'skips a %s addressed to it instead of throwing out of the crank',
+        async (_what, makeItem, deliverMethod) => {
+          // Throwing here escapes the crank and kills the run loop for good —
+          // and because the crank is rolled back, the same item is re-dequeued
+          // on the next boot and kills that one too.
+          const result = await kernelRouter.deliver(makeItem());
+
+          expect(result).toStrictEqual({ didDelivery: endpointId });
+          expect(
+            endpointHandle[deliverMethod as keyof EndpointHandle],
+          ).not.toHaveBeenCalled();
+        },
+      );
+
+      it('still releases the kernel side of a skipped dropExports', async () => {
+        await kernelRouter.deliver({
+          type: 'dropExports',
+          endpointId,
+          krefs: ['ko1'],
+        });
+
+        // Telling an endpoint to let go is also the kernel letting go, and that
+        // half does not depend on the endpoint being there to be told. Skip it
+        // and the export stays flagged reachable, so the same action is derived
+        // again on the next sweep, forever.
+        expect(kernelStore.clearReachableFlag).toHaveBeenCalledWith(
+          endpointId,
+          'ko1',
+        );
+      });
+
+      it.each(['retireExports', 'retireImports'] as const)(
+        'still tears down the c-list entry of a skipped %s',
+        async (type) => {
+          await kernelRouter.deliver({ type, endpointId, krefs: ['ko1'] });
+
+          expect(kernelStore.deleteCListEntry).toHaveBeenCalledWith(
+            endpointId,
+            'ko1',
+            'translated-ko1',
+          );
+        },
+      );
+
+      it('skips a GC action whose c-list entries went in the same crank', async () => {
+        // `processGCActionSet` selects an action only while the endpoint still
+        // has a c-list entry for its krefs, but the run loop then calls
+        // `nextTerminatedVatCleanup` before delivering it — and that takes the
+        // whole c-list of the vat it cleans. So the entries can be gone by the
+        // time this runs, and `krefsToErefs` reports an unmapped kref by
+        // throwing, which would leave the crank and kill the run loop just as
+        // the unguarded lookup used to.
+        (kernelStore.hasCListEntry as unknown as MockInstance).mockReturnValue(
+          false,
+        );
+        (
+          kernelStore.krefsToErefs as unknown as MockInstance
+        ).mockImplementation(() => {
+          throw new Error(`unmapped kref ko1 in ${endpointId} c-list`);
+        });
+
+        const result = await kernelRouter.deliver({
+          type: 'dropExports',
+          endpointId,
+          krefs: ['ko1'],
+        });
+
+        expect(result).toStrictEqual({ didDelivery: endpointId });
+        // The cleanup performed the kernel's half already; there is nothing
+        // left for this delivery to release.
+        expect(kernelStore.clearReachableFlag).not.toHaveBeenCalled();
+        expect(kernelStore.deleteCListEntry).not.toHaveBeenCalled();
+      });
+
+      it('allocates nothing in the c-list of an endpoint it is skipping', async () => {
+        await kernelRouter.deliver(makeLiveNotify());
+
+        // Both translations import if needed, minting a c-list entry and taking
+        // a reference on every slot. Doing that for an endpoint nobody will
+        // tell writes rows only that endpoint could release, and it cannot.
+        // While the lookup threw, the rollback undid them; once it is skipped
+        // the crank commits.
+        expect(kernelStore.translateRefKtoE).not.toHaveBeenCalled();
+        expect(kernelStore.translateCapDataKtoE).not.toHaveBeenCalled();
+      });
+
+      it('throws for an endpoint id that is neither a vat nor a remote', async () => {
+        (getEndpoint as unknown as MockInstance).mockImplementation(
+          (requested: EndpointId) => {
+            throw new Error(`invalid endpoint ID ${requested}`);
+          },
+        );
+
+        // A missing vat and a missing remote are ordinary; an id that is
+        // neither is corrupt state or a kernel bug, and GC actions are parsed
+        // through `insistEndpointId` before they are ever queued.
+        await expect(
+          kernelRouter.deliver({
+            type: 'bringOutYourDead',
+            endpointId: 'bogus' as EndpointId,
+          }),
+        ).rejects.toThrow('invalid endpoint ID bogus');
+      });
+
+      it('reports the skip above the per-delivery trace level', async () => {
+        const logger = new Logger('test');
+        const warnSpy = vi.spyOn(logger, 'warn');
+        const router = new KernelRouter(
+          kernelStore,
+          kernelQueue,
+          getEndpoint,
+          vi.fn(),
+          logger,
+        );
+
+        await router.deliver({ type: 'bringOutYourDead', endpointId });
+
+        // A delivery dropped on the floor is not routine traffic, and it is the
+        // only trace of a vat that has quietly stopped doing anything.
+        expect(warnSpy).toHaveBeenCalledWith(
+          expect.stringContaining(endpointId),
+          expect.anything(),
+        );
+      });
+
+      it('still delivers to endpoints that are running', async () => {
+        const result = await kernelRouter.deliver({
+          type: 'bringOutYourDead',
+          endpointId: 'v1',
+        });
+
+        expect(endpointHandle.deliverBringOutYourDead).toHaveBeenCalled();
+        expect(result).toStrictEqual({ didDelivery: 'v1' });
+      });
+    });
+
     it('throws on unknown run queue item type', async () => {
       // @ts-expect-error - deliberately using an invalid type
       const invalidItem: RunQueueItem = { type: 'invalid' };