fix(ocap-kernel): leave a settled result promise alone on delivery failure

packages/ocap-kernel/CHANGELOG.md

@@ -61,6 +61,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- A delivery failure no longer re-rejects a result promise the endpoint had already settled, which threw from the delivery's own catch and killed the run loop ([#1084](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1084))
 - Two pending garbage-collection actions of one type for one endpoint no longer kill the run loop: `processGCActionSet` sorted the kref list after hardening it, and sorting a frozen array throws ([#1082](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1082))
 - 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

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

@@ -0,0 +1,133 @@
+import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs';
+import type { Logger } from '@metamask/logger';
+import { describe, it, expect, vi } from 'vitest';
+
+import { KernelQueue } from './KernelQueue.ts';
+import { KernelRouter } from './KernelRouter.ts';
+import { kser } from './liveslots/kernel-marshal.ts';
+import { makeKernelStore } from './store/index.ts';
+import type { EndpointHandle, KRef, RunQueueItem } from './types.ts';
+
+/**
+ * `KernelRouter`'s own tests mock `resolvePromises`, so nothing there can see a
+ * second resolution of one promise. These run against a real store and queue.
+ */
+describe('a result promise whose delivery fails', () => {
+  /**
+   * A router over a real store, delivering to one endpoint the test supplies.
+   *
+   * @param deliverMessage - What the endpoint does with a message.
+   * @returns The store, queue and a `runCrank` that delivers one queued item.
+   */
+  async function makeFixture(deliverMessage: () => Promise<never>): Promise<{
+    kernelStore: ReturnType<typeof makeKernelStore>;
+    kernelQueue: KernelQueue;
+    logger: Logger;
+    runCrank: () => Promise<void>;
+  }> {
+    const kdb = await makeSQLKernelDatabase({ dbFilename: ':memory:' });
+    const kernelStore = makeKernelStore(kdb);
+    kernelStore.setRefCountAuditing(true);
+    const kernelQueue = new KernelQueue(kernelStore, async () => undefined);
+    const endpoint = {
+      deliverMessage: vi.fn(deliverMessage),
+      deliverNotify: vi.fn(),
+      deliverDropExports: vi.fn(),
+      deliverRetireExports: vi.fn(),
+      deliverRetireImports: vi.fn(),
+      deliverBringOutYourDead: vi.fn(),
+    } as unknown as EndpointHandle;
+    const logger = { error: vi.fn(), log: vi.fn() } as unknown as Logger;
+    const kernelRouter = new KernelRouter(
+      kernelStore,
+      kernelQueue,
+      () => endpoint,
+      () => undefined,
+      logger,
+    );
+
+    // The run loop's crank, minus the run loop.
+    const runCrank = async (): Promise<void> => {
+      kernelStore.startCrank();
+      kernelStore.createCrankSavepoint('start');
+      try {
+        const item = kernelStore.dequeueRun() as RunQueueItem;
+        await kernelRouter.deliver(item);
+        kernelStore.collectGarbage();
+        kernelStore.assertRefCountsIfAuditing();
+      } finally {
+        kernelStore.endCrank();
+      }
+    };
+
+    return { kernelStore, kernelQueue, logger, runCrank };
+  }
+
+  /**
+   * Queue a message to v1's root with a result promise.
+   *
+   * @param kernelStore - The store to set up.
+   * @param kernelQueue - The queue to send through.
+   * @returns The result promise's kref.
+   */
+  function queueSendWithResult(
+    kernelStore: ReturnType<typeof makeKernelStore>,
+    kernelQueue: KernelQueue,
+  ): KRef {
+    kernelStore.setVatConfig('v1', { bundleName: 'vat1' });
+    kernelStore.initEndpoint('v1');
+    const target = kernelStore.exportFromEndpoint('v1', 'o+1');
+    const [result] = kernelStore.initKernelPromise();
+    kernelQueue.enqueueSend(target, {
+      methargs: kser(['ping', []]),
+      result,
+    });
+    return result;
+  }
+
+  it('rejects it once the delivery reports failure', async () => {
+    const { kernelStore, kernelQueue, runCrank } = await makeFixture(
+      async () => {
+        throw new Error('stream closed');
+      },
+    );
+    const result = queueSendWithResult(kernelStore, kernelQueue);
+
+    await runCrank();
+
+    expect(kernelStore.getKernelPromise(result).state).toBe('rejected');
+  });
+
+  it.each([
+    { how: 'fulfilled', rejected: false, state: 'fulfilled' },
+    { how: 'rejected', rejected: true, state: 'rejected' },
+  ])(
+    'leaves a result the endpoint already $how alone',
+    async ({ rejected, state }) => {
+      // Held in an object so the endpoint can reach it before the queue that
+      // settles the promise exists.
+      const endpointDoes = { settleTheResult: (): void => undefined };
+      const { kernelStore, kernelQueue, logger, runCrank } = await makeFixture(
+        async () => {
+          endpointDoes.settleTheResult();
+          throw new Error('stream closed');
+        },
+      );
+      const result = queueSendWithResult(kernelStore, kernelQueue);
+      endpointDoes.settleTheResult = () =>
+        kernelQueue.resolvePromises('v1', [
+          [result, rejected, kser('settled by the endpoint')],
+        ]);
+
+      await runCrank();
+
+      const settled = kernelStore.getKernelPromise(result);
+      expect(settled.state).toBe(state);
+      // The endpoint's own settlement, not the delivery's `DELIVERY_FAILED`.
+      expect(settled.value?.body).toContain('settled by the endpoint');
+      expect(logger.error).toHaveBeenCalledWith(
+        expect.stringContaining(`already ${state}`),
+      );
+    },
+  );
+});

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

@@ -46,7 +46,9 @@ describe('KernelRouter', () => {
     kernelStore = {
       getOwner: vi.fn(),
       isRevoked: vi.fn(),
-      getKernelPromise: vi.fn(),
+      // Unresolved is the default so that reading a promise's state is never
+      // a `TypeError` in a test that does not care about it.
+      getKernelPromise: vi.fn().mockReturnValue({ state: 'unresolved' }),
       decrementRefCount: vi.fn(),
       setPromiseDecider: vi.fn(),
       translateRefKtoE: vi.fn(
@@ -490,7 +492,6 @@ describe('KernelRouter', () => {
         (
           endpointHandle.deliverMessage as unknown as MockInstance
         ).mockRejectedValueOnce(new Error('queue full'));
-
         const message: Message = {
           methargs: { body: 'method args', slots: [] },
           result: 'kp1',

packages/ocap-kernel/src/KernelRouter.ts

@@ -194,6 +194,34 @@ export class KernelRouter {
     }
   }
 
+  /**
+   * Reject a message's result promise, unless it is already settled.
+   *
+   * A failed delivery may have settled the result on its way down: the vat
+   * resolved it and then lost its stream. Resolving a settled promise is a
+   * `Fail`, thrown from inside the catch that is handling the failure.
+   *
+   * @param endpointId - The endpoint that was to have decided it.
+   * @param kpid - The result promise.
+   * @param failure - Why the message could not be delivered.
+   */
+  #rejectResultIfPending(
+    endpointId: EndpointId,
+    kpid: KRef,
+    failure: CapData<KRef>,
+  ): void {
+    const { state } = this.#kernelStore.getKernelPromise(kpid);
+    if (state !== 'unresolved') {
+      // The caller gets the endpoint's answer, not the delivery's failure, and
+      // the `error` above is the only other trace of either.
+      this.#logger?.error(
+        `Result ${kpid} of the failed delivery to ${endpointId} is already ${state}; leaving it alone`,
+      );
+      return;
+    }
+    this.#kernelQueue.resolvePromises(endpointId, [[kpid, true, failure]]);
+  }
+
   /**
    * Deliver a 'send' run queue item.
    *
@@ -303,13 +331,11 @@ export class KernelRouter {
           if (message.result) {
             const detail =
               error instanceof Error ? error.message : String(error);
-            this.#kernelQueue.resolvePromises(endpointId, [
-              [
-                message.result,
-                true,
-                makeKernelError('DELIVERY_FAILED', detail),
-              ],
-            ]);
+            this.#rejectResultIfPending(
+              eid,
+              message.result,
+              makeKernelError('DELIVERY_FAILED', detail),
+            );
           }
           // Continue processing other messages - don't let one failure crash the queue
         }