diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 47264fc7fa..118cd5bb84 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/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 diff --git a/packages/ocap-kernel/src/KernelRouter.result-promise.test.ts b/packages/ocap-kernel/src/KernelRouter.result-promise.test.ts new file mode 100644 index 0000000000..222d7e19b0 --- /dev/null +++ b/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): Promise<{ + kernelStore: ReturnType; + kernelQueue: KernelQueue; + logger: Logger; + runCrank: () => Promise; + }> { + 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 => { + 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, + 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}`), + ); + }, + ); +}); diff --git a/packages/ocap-kernel/src/KernelRouter.test.ts b/packages/ocap-kernel/src/KernelRouter.test.ts index eddf28a6a4..b541ee55a2 100644 --- a/packages/ocap-kernel/src/KernelRouter.test.ts +++ b/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', diff --git a/packages/ocap-kernel/src/KernelRouter.ts b/packages/ocap-kernel/src/KernelRouter.ts index 6bd080e7c3..0975c354a6 100644 --- a/packages/ocap-kernel/src/KernelRouter.ts +++ b/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, + ): 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 }