diff --git a/packages/kernel-test/src/refcount-audit.test.ts b/packages/kernel-test/src/refcount-audit.test.ts new file mode 100644 index 0000000000..351203e3d4 --- /dev/null +++ b/packages/kernel-test/src/refcount-audit.test.ts @@ -0,0 +1,70 @@ +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, + takeRunLoopFailure, +} 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, + ); + + // This test kills the loop deliberately, so the death is its result and not + // a stray one for the shared hooks to report against it. + expect(String(takeRunLoopFailure())).toMatch( + /reference count invariant violated/u, + ); + }, 30000); +}); diff --git a/packages/kernel-test/src/utils.ts b/packages/kernel-test/src/utils.ts index b840fa2e30..9997041154 100644 --- a/packages/kernel-test/src/utils.ts +++ b/packages/kernel-test/src/utils.ts @@ -41,6 +41,18 @@ function assertRunLoopAlive(): void { afterEach(assertRunLoopAlive); afterAll(assertRunLoopAlive); +/** + * Claim a run loop death the running test caused on purpose, so the hooks above + * do not report it as the test's own failure. + * + * @returns The failure, if the loop has died since the last check. + */ +export function takeRunLoopFailure(): Error | undefined { + const failure = runLoopFailure; + runLoopFailure = undefined; + return failure; +} + /** * Kernel options under which reference count drift fails the test run. * diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 138e014ebd..83cb0bbcd1 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -39,11 +39,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Reports drift in both directions: counts too low, which lets a live capability be collected, and counts too high, which keeps a dead one alive. A holder that should have been torn down but wasn't is not detectable this way, since it justifies its own count - 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. Reach it by calling `makeKernelStore` over the kernel's own database - - 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`, `getOcapURLIssuanceCount`, `retainForOcapURL`, `undoOcapURLRetention` and `releaseOcapURLRetentions` to the kernel store, and `VatManager.releaseVatRootPin` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) - `undoOcapURLRetention` unwinds one issuance whose URL was never minted; `releaseOcapURLRetentions` drops a target's whole retention, for disavowing every URL naming it at once - Add `getPinCount` to the kernel store, which reports how many pins are held on an object ([#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/Consensys-Incorporated/ocap-kernel/pull/1022)) ### Changed @@ -107,6 +108,21 @@ 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/Consensys-Incorporated/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/Consensys-Incorporated/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/Consensys-Incorporated/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 that crank aborts instead of committing a release the returning incarnation would disagree with, and the returning incarnation is handed the action. Until it returns the action is reselected every crank; see [#1061](https://github.com/Consensys-Incorporated/ocap-kernel/issues/1061) +- 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/Consensys-Incorporated/ocap-kernel/pull/1022)) + - The next incarnation change reconciles a dropped or retired export but not a `retireImports`, whose entries `forgetEndpointImports` leaves in place +- Two garbage-collection actions of one type for one endpoint no longer stop the run loop; one vat dropping two exports in a crank was enough ([#1022](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1022)) + - The krefs were hardened before being sorted, and `sort` writes back into the array even when it is already in order. Reproduces on `main`, so it predates this stack +- `getImporters` reports remote importers as well as vat ones, so retiring an object tells every endpoint holding it ([#1022](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1022)) + - The object is deleted once the importers have been told, so a remote left out kept a c-list entry naming a kref that no longer existed — which the reference count audit reports as dangling, killing the run loop +- Tear down and mark for cleanup a vat whose worker launched but whose kernel-side registration then failed ([#1022](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1022)) +- `stopVat` forgets the vat whether or not its teardown succeeds ([#1022](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1022)) + - A throw from unpinning the root or from the handle's own `terminate` left the handle on the books while the store went on to wipe the vat's state, so `hasVat` and `getVatIds` reported a vat with no c-list and the next `bringOutYourDead` for it killed the run loop - 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)) diff --git a/packages/ocap-kernel/src/Kernel.ts b/packages/ocap-kernel/src/Kernel.ts index d44467b88f..a366eed955 100644 --- a/packages/ocap-kernel/src/Kernel.ts +++ b/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( diff --git a/packages/ocap-kernel/src/KernelRouter.test.ts b/packages/ocap-kernel/src/KernelRouter.test.ts index eddf28a6a4..d7dfd935f5 100644 --- a/packages/ocap-kernel/src/KernelRouter.test.ts +++ b/packages/ocap-kernel/src/KernelRouter.test.ts @@ -66,6 +66,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; @@ -782,6 +785,167 @@ 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'); + }); + + const result = await kernelRouter.deliver({ + type: actionType, + endpointId: 'v1', + krefs: ['ko1'], + }); + + expect(result).toStrictEqual({ abort: true }); + 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', () => { diff --git a/packages/ocap-kernel/src/KernelRouter.ts b/packages/ocap-kernel/src/KernelRouter.ts index 6bd080e7c3..9d4c393e28 100644 --- a/packages/ocap-kernel/src/KernelRouter.ts +++ b/packages/ocap-kernel/src/KernelRouter.ts @@ -3,7 +3,10 @@ import type { CapData } from '@endo/marshal'; import { Logger } from '@metamask/logger'; import { KernelQueue } from './KernelQueue.ts'; -import { makeKernelError } from './liveslots/kernel-marshal.ts'; +import { + makeFatalKernelError, + makeKernelError, +} from './liveslots/kernel-marshal.ts'; import type { KernelStore } from './store/index.ts'; import { extractSingleRef } from './store/utils/extract-ref.ts'; import { parseRef } from './store/utils/parse-ref.ts'; @@ -21,6 +24,7 @@ import type { RunQueueItemGCAction, CrankResult, } from './types.ts'; +import { isVatId } from './types.ts'; import { assert, Fail } from './utils/assert.ts'; type MessageRoute = { @@ -405,10 +409,12 @@ export class KernelRouter { this.#kernelStore.translateCapDataKtoE(endpointId, tPromise.value), ]); } - // TODO(#1006 follow-up): SwingSet also tears down the c-list entry for each - // promise in the batch here, since the endpoint can never refer to a - // settled promise by that eref again. Left alone for now because the - // debug UI discovers exported ocap URLs by scanning these entries. + // TODO: SwingSet also tears down the c-list entry for each promise in the + // batch here, since the endpoint can never refer to a settled promise by + // that eref again. Left alone for now because the debug UI discovers + // exported ocap URLs by scanning these entries. The cost of keeping them is + // that a settled promise reached this way holds a count forever, so it is + // never collected and its resolution slots are never released. const endpoint = this.#getEndpoint(endpointId); return await endpoint.deliverNotify(resolutions); } @@ -424,29 +430,136 @@ export class KernelRouter { this.#logger?.log( `@@@@ deliver ${endpointId} ${type} ${JSON.stringify(krefs)}`, ); - const endpoint = this.#getEndpoint(endpointId); - const erefs = this.#kernelStore.krefsToErefs(endpointId, krefs); + // This action was selected while the endpoint's c-list held every one of + // these krefs, but `nextTerminatedVatCleanup` runs between selection and + // here and can take the entries — and the endpoint — with it. Whatever + // survives still has to be released on the kernel's side: the action has + // already been consumed from the durable set, so skipping the teardown + // would lose it and leave the entry behind for good. + const live = krefs.filter((kref) => + this.#kernelStore.hasCListEntry(endpointId, kref), + ); + if (live.length < krefs.length) { + this.#logger?.error( + `${type} for ${endpointId}: ${krefs.length - live.length} of ${krefs.length} kref(s) were cleaned up before delivery`, + ); + } + if (live.length === 0) { + return { didDelivery: endpointId }; + } + // Resolved before anything is torn down, so a lookup that fails has nothing + // to undo, and so the two outcomes below are decided rather than discovered + // halfway through. + let endpoint: EndpointHandle | undefined; + try { + endpoint = this.#getEndpoint(endpointId); + } catch (error) { + // A vat absent from the kernel's vat table but not marked terminated is a + // vat between incarnations, and its c-list is whole: every kref here is one + // the returning incarnation still has in its own tables. `restartVat` + // takes a vat out of that table for as long as launching a worker and + // negotiating with it takes, so this is reachable, and releasing the + // kernel's side would commit exactly the disagreement the failed delivery + // below rolls back to avoid — the vat would mint fresh krefs for objects + // the kernel thinks it let go of. + // + // Abort rather than throw. A throw here leaves `deliver` by the run loop's + // catch and kills the run loop for good over a vat that is about to come + // back. The rollback restores the c-list and the action, so the returning + // incarnation is handed it instead. Until it returns the action is + // reselected every crank, since GC actions are chosen ahead of all other + // work — a spin, where this used to be a death. See #1061. + if ( + isVatId(endpointId) && + !this.#kernelStore.isVatTerminated(endpointId) + ) { + this.#logger?.error( + `Endpoint ${endpointId} is between incarnations; deferring ${type} of ${JSON.stringify(live)}:`, + error, + ); + return { abort: true }; + } + // A terminated vat's cleanup tears its c-list down wholesale, and a remote + // reconciles on its next incarnation, so for those the release below is + // safe to commit — and has to be, since the action is already spent from + // the durable set. + this.#logger?.error( + `Endpoint ${endpointId} vanished before ${type} of ${JSON.stringify(live)}; releasing the kernel's side anyway:`, + error, + ); + } + const erefs = this.#kernelStore.krefsToErefs(endpointId, live); // Telling an endpoint to let go is also the kernel letting go. Otherwise a // dropped export stays flagged reachable, so the same action gets derived // again, and retired entries outlive the objects they name. - krefs.forEach((kref, index) => { + live.forEach((kref, index) => { if (type === 'dropExports') { this.#kernelStore.clearReachableFlag(endpointId, kref); - } else { - this.#kernelStore.deleteCListEntry( - endpointId, - kref, - erefs[index] as ERef, - ); + return; + } + // `erefs` is parallel to `live`: krefsToErefs throws rather than + // returning a short array, so every index is populated. + this.#kernelStore.deleteCListEntry( + endpointId, + kref, + erefs[index] as ERef, + ); + if (type === 'retireExports') { + // Retiring an export is the owner giving up the last name for the + // object, so the kernel's record of who owns it goes too. + this.#kernelStore.orphanKernelObject(kref, endpointId); } }); + if (!endpoint) { + return { didDelivery: endpointId }; + } const method = `deliver${(type[0] as string).toUpperCase()}${type.slice(1)}` as | 'deliverDropExports' | 'deliverRetireExports' | 'deliverRetireImports'; - const crankResult = await endpoint[method](erefs); - return crankResult; + try { + return await endpoint[method](erefs); + } catch (error) { + if (!isVatId(endpointId)) { + // A remote is a separate kernel across a link that can drop messages, + // so its protocol already has to tolerate one going missing. Retrying + // instead would starve the kernel: GC actions are selected ahead of all + // other work, so a remote that keeps refusing (a full send queue, say) + // would be handed the same item every crank and nothing else would ever + // run. + // + // The next incarnation change reconciles a dropped or retired export, + // but not a `retireImports`: `forgetEndpointImports` keeps only entries + // whose direction is `export`. Those stay on the peer's side until it + // drops them itself. + this.#logger?.error( + `Delivery of ${type} to remote ${endpointId} failed; the kernel has released ${JSON.stringify(live)} regardless:`, + error, + ); + return { didDelivery: endpointId }; + } + // A vat is local and reliable, so a refusal means it is broken. Undo the + // teardown rather than commit it: leaving the two disagreeing would have + // the vat mint fresh krefs for objects the kernel thinks it let go of. + // Aborting restores the entries and the action; terminating the vat is + // what stops that restored action from being retried forever. + this.#logger?.error( + `Delivery of ${type} to ${endpointId} failed; rolling back the kernel's release of ${JSON.stringify(live)} and terminating it:`, + error, + ); + return { + abort: true, + terminate: { + vatId: endpointId, + reject: true, + info: makeFatalKernelError( + 'INTERNAL_ERROR', + `failed to accept ${type}: ${error instanceof Error ? error.message : String(error)}`, + ), + }, + }; + } } /** diff --git a/packages/ocap-kernel/src/garbage-collection/garbage-collection.test.ts b/packages/ocap-kernel/src/garbage-collection/garbage-collection.test.ts index bca1752652..18fd4a0849 100644 --- a/packages/ocap-kernel/src/garbage-collection/garbage-collection.test.ts +++ b/packages/ocap-kernel/src/garbage-collection/garbage-collection.test.ts @@ -84,6 +84,25 @@ describe('garbage-collection', () => { expect(kernelStore.getGCActions().size).toBe(0); }); + it('groups two actions of one type for one vat into a single item', () => { + const ko1 = kernelStore.initKernelObject('v1'); + const ko2 = kernelStore.initKernelObject('v1'); + kernelStore.addCListEntry('v1', ko1, 'o+1'); + kernelStore.addCListEntry('v1', ko2, 'o+2'); + kernelStore.setObjectRefCount(ko1, { reachable: 0, recognizable: 1 }); + kernelStore.setObjectRefCount(ko2, { reachable: 0, recognizable: 1 }); + kernelStore.addGCActions([ + `v1 dropExport ${ko2}`, + `v1 dropExport ${ko1}`, + ]); + + expect(processGCActionSet(kernelStore)).toStrictEqual({ + type: 'dropExports', + endpointId: 'v1', + krefs: [ko1, ko2], + }); + }); + it('processes actions in priority order', () => { // Setup: Create objects and add multiple GC actions const ko1 = kernelStore.initKernelObject('v1'); diff --git a/packages/ocap-kernel/src/garbage-collection/garbage-collection.ts b/packages/ocap-kernel/src/garbage-collection/garbage-collection.ts index 5bd107793c..393c5cf08d 100644 --- a/packages/ocap-kernel/src/garbage-collection/garbage-collection.ts +++ b/packages/ocap-kernel/src/garbage-collection/garbage-collection.ts @@ -106,6 +106,11 @@ function filterActionsForProcessing( actionSetUpdated = true; } + // Sorted before hardening, not by the caller afterwards: `harden` freezes the + // array, and `sort` writes back into it even when it is already in order, so + // any endpoint with two actions of one type threw out of here and killed the + // run loop. One vat dropping two exports in a crank is enough. + krefs.sort(); return harden({ krefs, actionSetUpdated }); } @@ -174,9 +179,6 @@ export function processGCActionSet( actionSetUpdated = actionSetUpdated || updated; if (krefs.length > 0) { - // We found actions to process - krefs.sort(); - // Update the durable set before returning storage.setGCActions(allActionsSet); diff --git a/packages/ocap-kernel/src/garbage-collection/gc-delivery.test.ts b/packages/ocap-kernel/src/garbage-collection/gc-delivery.test.ts new file mode 100644 index 0000000000..dbb9eceb00 --- /dev/null +++ b/packages/ocap-kernel/src/garbage-collection/gc-delivery.test.ts @@ -0,0 +1,227 @@ +import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; +import { describe, it, expect, vi } from 'vitest'; + +import { processGCActionSet } from './garbage-collection.ts'; +import { KernelQueue } from '../KernelQueue.ts'; +import { KernelRouter } from '../KernelRouter.ts'; +import { makeKernelStore } from '../store/index.ts'; +import type { + CrankResult, + EndpointHandle, + EndpointId, + ERef, + RunQueueItemGCAction, +} from '../types.ts'; + +/** + * Kernel-issued GC deliveries end to end, against a real store. + * + * Selection has its own real-store coverage next door and delivery has unit + * coverage over a mocked store, but the two halves have never met: the mock + * answers `hasCListEntry` and `krefsToErefs` from `vi.fn()`s, so nothing there + * pins what the kernel actually releases, and nothing pins that a crank the + * delivery aborts gives the action back. + * + * The crank is driven here rather than by `KernelQueue.run`, which never + * resolves and, for the abort below, would reselect the same action every crank + * (see #1061). `runCrank` is the shape `#runLoop` gives a delivery. + */ + +type Delivered = { method: string; erefs: ERef[] }; + +/** + * Build a store over a fresh in-memory database, wired to a router whose + * endpoints are whatever the test registered. + * + * @returns The store, the router, what the endpoints received, and the + * endpoint table to register into. + */ +async function makeFixture(): Promise<{ + kernelStore: ReturnType; + runCrank: ( + beforeDeliver?: (item: RunQueueItemGCAction) => void, + ) => Promise; + delivered: Delivered[]; + endpoints: Map; +}> { + const kdb = await makeSQLKernelDatabase({ dbFilename: ':memory:' }); + const kernelStore = makeKernelStore(kdb); + const kernelQueue = new KernelQueue(kernelStore, async () => undefined); + const endpoints = new Map(); + const delivered: Delivered[] = []; + const kernelRouter = new KernelRouter( + kernelStore, + kernelQueue, + (endpointId) => { + const endpoint = endpoints.get(endpointId); + if (!endpoint) { + throw new Error(`vat ${endpointId} not found`); + } + return endpoint; + }, + () => undefined, + ); + + const runCrank = async ( + beforeDeliver?: (item: RunQueueItemGCAction) => void, + ): Promise => { + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('crank'); + kernelStore.createCrankSavepoint('delivery'); + try { + const item = processGCActionSet(kernelStore); + if (!item) { + return undefined; + } + beforeDeliver?.(item); + const result = await kernelRouter.deliver(item); + if (result?.abort) { + kernelStore.rollbackCrank('delivery'); + } + kernelStore.collectGarbage(); + return result; + } finally { + kernelStore.endCrank(); + } + }; + + return { kernelStore, runCrank, delivered, endpoints }; +} + +/** + * Register an endpoint that records what it is told to let go of. + * + * @param endpoints - The router's endpoint table. + * @param delivered - Where to record the deliveries. + * @param endpointId - The endpoint to register. + * @returns The registered handle. + */ +function registerEndpoint( + endpoints: Map, + delivered: Delivered[], + endpointId: EndpointId, +): EndpointHandle { + const record = + (method: string) => + async (erefs: ERef[]): Promise => { + delivered.push({ method, erefs }); + return { didDelivery: endpointId }; + }; + const endpoint = { + deliverMessage: vi.fn(), + deliverNotify: vi.fn(), + deliverDropExports: vi.fn(record('dropExports')), + deliverRetireExports: vi.fn(record('retireExports')), + deliverRetireImports: vi.fn(record('retireImports')), + deliverBringOutYourDead: vi.fn(), + } as unknown as EndpointHandle; + endpoints.set(endpointId, endpoint); + return endpoint; +} + +describe('a GC action the kernel issues', () => { + it('tells the exporter to drop, and stops treating the object as reachable', async () => { + const { kernelStore, runCrank, delivered, endpoints } = await makeFixture(); + registerEndpoint(endpoints, delivered, 'v1'); + const kref = kernelStore.initKernelObject('v1'); + kernelStore.addCListEntry('v1', kref, 'o+1'); + kernelStore.setObjectRefCount(kref, { reachable: 0, recognizable: 1 }); + kernelStore.addGCActions([`v1 dropExport ${kref}`]); + + await runCrank(); + + expect(delivered).toStrictEqual([ + { method: 'dropExports', erefs: ['o+1'] }, + ]); + expect(kernelStore.getReachableFlag('v1', kref)).toBe(false); + // A drop is not a retire: the object is still recognizable, so the entry + // that names it has to survive. + expect(kernelStore.hasCListEntry('v1', kref)).toBe(true); + expect(kernelStore.getOwner(kref)).toBe('v1'); + }); + + it('tells the exporter to retire, and gives up the object', async () => { + const { kernelStore, runCrank, delivered, endpoints } = await makeFixture(); + registerEndpoint(endpoints, delivered, 'v1'); + const kref = kernelStore.initKernelObject('v1'); + kernelStore.addCListEntry('v1', kref, 'o+1'); + kernelStore.setObjectRefCount(kref, { reachable: 0, recognizable: 0 }); + kernelStore.addGCActions([`v1 retireExport ${kref}`]); + + await runCrank(); + + expect(delivered).toStrictEqual([ + { method: 'retireExports', erefs: ['o+1'] }, + ]); + expect(kernelStore.hasCListEntry('v1', kref)).toBe(false); + expect(kernelStore.getOwner(kref)).toBeUndefined(); + }); + + it('tells the importer to retire, and leaves ownership alone', async () => { + const { kernelStore, runCrank, delivered, endpoints } = await makeFixture(); + registerEndpoint(endpoints, delivered, 'v2'); + const kref = kernelStore.initKernelObject('v1'); + // The exporter's own entry, so the object outlives the import being + // retired and `getOwner` still has something to answer. + kernelStore.addCListEntry('v1', kref, 'o+1'); + kernelStore.addCListEntry('v2', kref, 'o-1'); + kernelStore.addGCActions([`v2 retireImport ${kref}`]); + + await runCrank(); + + expect(delivered).toStrictEqual([ + { method: 'retireImports', erefs: ['o-1'] }, + ]); + expect(kernelStore.hasCListEntry('v2', kref)).toBe(false); + // Only the exporter giving up its last name for an object orphans it. + expect(kernelStore.hasCListEntry('v1', kref)).toBe(true); + expect(kernelStore.getOwner(kref)).toBe('v1'); + }); + + // `nextTerminatedVatCleanup` runs between selection and delivery and can take + // a c-list entry with it, so the erefs have to come from the krefs that + // survived rather than the ones the action named. With the survivor at index + // 0 the two agree, which is why this drops the first. + it('names the surviving kref when an earlier one was cleaned up first', async () => { + const { kernelStore, runCrank, delivered, endpoints } = await makeFixture(); + registerEndpoint(endpoints, delivered, 'v1'); + const gone = kernelStore.initKernelObject('v1'); + const kept = kernelStore.initKernelObject('v1'); + kernelStore.addCListEntry('v1', gone, 'o+1'); + kernelStore.addCListEntry('v1', kept, 'o+2'); + kernelStore.setObjectRefCount(gone, { reachable: 0, recognizable: 1 }); + kernelStore.setObjectRefCount(kept, { reachable: 0, recognizable: 1 }); + kernelStore.addGCActions([ + `v1 dropExport ${gone}`, + `v1 dropExport ${kept}`, + ]); + + await runCrank((item) => { + expect(item.krefs).toStrictEqual([gone, kept]); + kernelStore.deleteCListEntry('v1', gone, 'o+1'); + }); + + expect(delivered).toStrictEqual([ + { method: 'dropExports', erefs: ['o+2'] }, + ]); + }); + + // A vat between incarnations still holds these krefs. Releasing the kernel's + // side would leave the two disagreeing; throwing killed the run loop. + it('gives the action back when the vat is between incarnations', async () => { + const { kernelStore, runCrank } = await makeFixture(); + const kref = kernelStore.initKernelObject('v1'); + kernelStore.addCListEntry('v1', kref, 'o+1'); + kernelStore.setObjectRefCount(kref, { reachable: 0, recognizable: 1 }); + kernelStore.addGCActions([`v1 dropExport ${kref}`]); + + const result = await runCrank(); + + expect(result).toStrictEqual({ abort: true }); + expect(kernelStore.getReachableFlag('v1', kref)).toBe(true); + expect(kernelStore.hasCListEntry('v1', kref)).toBe(true); + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v1 dropExport ${kref}`, + ]); + }); +}); diff --git a/packages/ocap-kernel/src/garbage-collection/gc-handlers.test.ts b/packages/ocap-kernel/src/garbage-collection/gc-handlers.test.ts new file mode 100644 index 0000000000..8b5231eec9 --- /dev/null +++ b/packages/ocap-kernel/src/garbage-collection/gc-handlers.test.ts @@ -0,0 +1,115 @@ +import { describe, it, expect, beforeEach } from 'vitest'; + +import { makeMapKernelDatabase } from '../../test/storage.ts'; +import { makeKernelStore } from '../store/index.ts'; +import type { VatConfig, VatId } from '../types.ts'; +import { performExportCleanup } from './gc-handlers.ts'; + +describe('performExportCleanup', () => { + let kernelStore: ReturnType; + + /** + * Register and initialize an endpoint so it can hold c-list entries. + * + * @param vatIds - The vats to bring into existence. + */ + function givenVats(...vatIds: VatId[]): void { + for (const vatId of vatIds) { + kernelStore.setVatConfig(vatId, { sourceSpec: 'x' } as VatConfig); + kernelStore.initEndpoint(vatId); + } + } + + beforeEach(() => { + kernelStore = makeKernelStore(makeMapKernelDatabase()); + kernelStore.markInitialized(); + givenVats('v1', 'v2'); + }); + + // `checkReachable` is what separates a retire from an abandon; the ownership + // check precedes it, so both syscalls have to be covered. + const actions = [ + { name: 'retireExports', checkReachable: true }, + { name: 'abandonExports', checkReachable: false }, + ] as const; + + it.each(actions)( + 'lets an owner give up its own export via $name', + ({ checkReachable }) => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.clearReachableFlag('v1', kref); + + performExportCleanup([kref], checkReachable, 'v1', kernelStore); + + expect(kernelStore.getOwner(kref)).toBeUndefined(); + expect(kernelStore.hasCListEntry('v1', kref)).toBe(false); + }, + ); + + it.each(actions)( + 'refuses $name for an object owned by another endpoint', + ({ name, checkReachable }) => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + // v2 holds it as an import, which is what makes the kref nameable in a + // syscall from v2 at all. + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + + expect(() => + performExportCleanup([kref], checkReachable, 'v2', kernelStore), + ).toThrow(`endpoint v2 issued ${name} for ${kref}, which is owned by v1`); + + // v1's claim survives intact, entry and ownership both. + expect(kernelStore.getOwner(kref)).toBe('v1'); + expect(kernelStore.hasCListEntry('v1', kref)).toBe(true); + expect(kernelStore.hasCListEntry('v2', kref)).toBe(true); + }, + ); + + it.each(actions)( + 'allows $name for an already-orphaned object', + ({ checkReachable }) => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + kernelStore.clearReachableFlag('v1', kref); + kernelStore.forgetKref('v1', kref); + kernelStore.orphanKernelObject(kref, 'v1'); + + // No claim is left to erase, so there is nothing for the guard to protect. + expect(() => + performExportCleanup([kref], checkReachable, 'v2', kernelStore), + ).not.toThrow(); + expect(kernelStore.hasCListEntry('v2', kref)).toBe(false); + }, + ); + + it('refuses retireExports for an object the owner still reaches', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + + expect(() => performExportCleanup([kref], true, 'v1', kernelStore)).toThrow( + `retireExports but ${kref} is still reachable`, + ); + expect(kernelStore.getOwner(kref)).toBe('v1'); + }); + + it('abandons an export the owner still reaches', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + + performExportCleanup([kref], false, 'v1', kernelStore); + + expect(kernelStore.getOwner(kref)).toBeUndefined(); + }); + + it.each(actions)( + 'refuses $name for a promise', + ({ name, checkReachable }) => { + const kpid = kernelStore.initKernelPromise()[0]; + kernelStore.exportFromEndpoint('v1', 'p+1'); + + expect(() => + performExportCleanup([kpid], checkReachable, 'v1', kernelStore), + ).toThrow(`endpoint v1 issued invalid ${name} for ${kpid}`); + }, + ); +}); diff --git a/packages/ocap-kernel/src/garbage-collection/gc-handlers.ts b/packages/ocap-kernel/src/garbage-collection/gc-handlers.ts index c96b6e26c8..15e254eb3b 100644 --- a/packages/ocap-kernel/src/garbage-collection/gc-handlers.ts +++ b/packages/ocap-kernel/src/garbage-collection/gc-handlers.ts @@ -78,11 +78,26 @@ export function performExportCleanup( `endpoint ${endpointId} issued invalid ${action}Exports for ${kref}`, ); } + // Only an owner may give up an object. Nothing upstream of here checks that + // the vref is even an export — `translateSyscallVtoK` maps import and + // export directions alike — so without this a vat could disown an object + // belonging to a different, live vat. An already-orphaned object is fine: + // there is no claim left to erase. + const owner = kernelStore.getOwner(kref); + if (owner !== undefined && owner !== endpointId) { + throw Error( + `endpoint ${endpointId} issued ${action}Exports for ${kref}, which is owned by ${owner}`, + ); + } if (checkReachable) { if (kernelStore.getReachableFlag(endpointId, kref)) { throw Error(`${action}Exports but ${kref} is still reachable`); } } kernelStore.forgetKref(endpointId, kref); + // The owner no longer names the object, so nothing can reach it through + // this endpoint again. Drop the owner mapping too, or the kernel's record + // of the object outlives the only c-list entry it was reachable through. + kernelStore.orphanKernelObject(kref, endpointId); } } diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 4bebf0f540..e764a91fa2 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -116,6 +116,7 @@ describe('kernel store', () => { 'getReachableFlag', 'getRefCount', 'getRelayEntries', + 'getRemoteIDs', 'getRemoteIdentityValue', 'getRemoteIdentityValueRequired', 'getRemoteInfo', @@ -152,6 +153,7 @@ describe('kernel store', () => { 'markVatAsTerminated', 'nextReapAction', 'nextTerminatedVatCleanup', + 'orphanKernelObject', 'outOfCrankWorkPending', 'pinObject', 'provideIncarnationId', diff --git a/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts index 789413f8e7..34494f3059 100644 --- a/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts +++ b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts @@ -176,6 +176,83 @@ describe('c-list reference accounting', () => { expect(kernelStore.getImporters(kref)).toStrictEqual([]); }); + describe('an owner that gives up its own export', () => { + it('frees the object once the last importer lets go', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + kernelStore.collectGarbage(); + + // The owner is told to drop, which clears its flag, and it then retires + // the export itself — leaving nothing naming the object from its side. + kernelStore.clearReachableFlag('v1', kref); + kernelStore.forgetKref('v1', kref); + kernelStore.orphanKernelObject(kref, 'v1'); + + kernelStore.forgetKref('v2', kref); + kernelStore.collectGarbage(); + + expect(kernelStore.kernelRefExists(kref)).toBe(false); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('collects an orphan that no importer ever recognized', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + + kernelStore.forgetKref('v1', kref); + kernelStore.orphanKernelObject(kref, 'v1'); + kernelStore.collectGarbage(); + + expect(kernelStore.getOwner(kref)).toBeUndefined(); + expect(kernelStore.kernelRefExists(kref)).toBe(false); + }); + + it('retires stragglers that still recognize an orphaned object', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + + kernelStore.clearReachableFlag('v1', kref); + kernelStore.forgetKref('v1', kref); + kernelStore.orphanKernelObject(kref, 'v1'); + kernelStore.collectGarbage(); + + // v2 can still recognize it, so it has to be told the name is dead + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v2 retireImport ${kref}`, + ]); + // v2's entry outlives the object it names until that action is delivered. + // The audit has to tolerate that window, or the end-of-crank check throws + // on a state the collector itself just created. + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('rejects an endpoint disowning an object it does not own', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + + expect(() => kernelStore.orphanKernelObject(kref, 'v2')).toThrow( + 'owned by "v1"', + ); + expect(kernelStore.getOwner(kref)).toBe('v1'); + }); + + it('survives an owner mapping left behind without a c-list entry', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + kernelStore.clearReachableFlag('v1', kref); + // Tear the owner's side down but leave the ownership record, the shape + // that used to make the next collection read a key that wasn't there. + kernelStore.forgetKref('v1', kref); + kernelStore.forgetKref('v2', kref); + + expect(() => kernelStore.collectGarbage()).not.toThrow(); + expect(kernelStore.getOwner(kref)).toBeUndefined(); + expect(kernelStore.kernelRefExists(kref)).toBe(false); + }); + }); + describe('cleanupTerminatedVat', () => { it('does nothing for a vat that is not terminated', () => { expect(kernelStore.cleanupTerminatedVat('v1')).toStrictEqual({ diff --git a/packages/ocap-kernel/src/store/methods/clist.ts b/packages/ocap-kernel/src/store/methods/clist.ts index b4e104dcc9..928f8a403c 100644 --- a/packages/ocap-kernel/src/store/methods/clist.ts +++ b/packages/ocap-kernel/src/store/methods/clist.ts @@ -147,10 +147,9 @@ export function getCListMethods(ctx: StoreContext) { * Look up the ERefs that an endpoint's c-list maps a list of KRefs to, * without allocating entries or disturbing reachability. * - * Every kref must already be mapped. Garbage collection is the only caller - * and has already established that each kref has an entry, so a missing one - * means the two disagree — worth hearing about rather than silently dropping - * the notification. + * Every kref must already be mapped: a missing entry means the caller's list + * of krefs and the c-list disagree, which is worth hearing about rather than + * silently dropping the one that got away. * * @param endpointId - The endpoint in question. * @param krefs - The KRefs to look up. diff --git a/packages/ocap-kernel/src/store/methods/gc.test.ts b/packages/ocap-kernel/src/store/methods/gc.test.ts index e91fb29bb9..88abdc5277 100644 --- a/packages/ocap-kernel/src/store/methods/gc.test.ts +++ b/packages/ocap-kernel/src/store/methods/gc.test.ts @@ -201,6 +201,25 @@ describe('GC methods', () => { expect(kernelStore.getObjectRefCount(ko2)).toBeDefined(); }); + // The object is deleted once the importers have been told, so a remote + // left out keeps a c-list entry naming a kref that no longer exists — which + // the audit reports as dangling, killing the run loop. + it('tells a remote importer too', () => { + kernelStore.setVatConfig('v1', { bundleName: 'vat1' }); + kernelStore.setRemoteInfo('r1', { + peerId: 'peer-1', + } as unknown as Parameters[1]); + const kref = kernelStore.initKernelObject('v1'); + kernelStore.addCListEntry('r1', kref, 'ro-1'); + kernelStore.setGCActions(new Set()); + + kernelStore.retireKernelObjects([kref]); + + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `r1 retireImport ${kref}`, + ]); + }); + it('throws for non-array input', () => { expect(() => { kernelStore.retireKernelObjects('not-an-array' as unknown as KRef[]); diff --git a/packages/ocap-kernel/src/store/methods/gc.ts b/packages/ocap-kernel/src/store/methods/gc.ts index 31b21294c8..92a093c968 100644 --- a/packages/ocap-kernel/src/store/methods/gc.ts +++ b/packages/ocap-kernel/src/store/methods/gc.ts @@ -1,6 +1,7 @@ import { Fail } from '@endo/errors'; import { getBaseMethods } from './base.ts'; +import { getCListMethods } from './clist.ts'; import { getObjectMethods } from './object.ts'; import { getPromiseMethods } from './promise.ts'; import { getReachableMethods } from './reachable.ts'; @@ -33,6 +34,37 @@ export function getGCMethods(ctx: StoreContext) { const { getImporters, isVatTerminated } = getVatMethods(ctx); const { getReachableFlag, getReachableAndVatSlot } = getReachableMethods(ctx); const { clearEmptySubclusters } = getSubclusterMethods(ctx); + const { hasCListEntry } = getCListMethods(ctx); + + /** + * Give up the kernel's record of who owns an object. The object survives only + * as long as something still names it; the collector disposes of it from + * there, retiring any stragglers that still recognize it. + * + * Called when an owner stops naming its own export — it retired or abandoned + * it, or a GC `retireExport` was delivered. Without this the owner mapping + * outlives the c-list entry it was reachable through, which both leaks the + * object record and leaves `collectGarbage` reading a c-list entry that is no + * longer there. + * + * Disowning an object is only ever the owner's own doing, so `expectedOwner` + * is required: taking it on trust would let one endpoint erase another's claim + * to an object it is still exporting. An object that is already orphaned is + * left alone — the caller and the kernel agree it has no owner. + * + * @param kref - The object whose owner mapping is to be dropped. + * @param expectedOwner - The endpoint the caller believes owns `kref`. + */ + function orphanKernelObject(kref: KRef, expectedOwner: EndpointId): void { + const owner = getOwner(kref); + if (owner === undefined) { + return; + } + owner === expectedOwner || + Fail`cannot orphan ${kref} for ${expectedOwner}: owned by ${owner}`; + ctx.kv.delete(getOwnerKey(kref)); + ctx.maybeFreeKrefs.add(kref); + } /** * Get the set of GC actions to perform. @@ -158,7 +190,18 @@ export function getGCMethods(ctx: StoreContext) { // might still alive, or might be terminated and in the // process of being deleted. These two clauses are // mutually exclusive. - if (ownerVatID && !terminated) { + if (ownerVatID && !terminated && !hasCListEntry(ownerVatID, kref)) { + // Should be unreachable: every path that tears down an owner's + // export entry orphans the object with it. Repair it so the + // collector can keep going, but say so — absorbing this in silence + // would hide whatever upstream broke the pairing. + ctx.logger?.error( + `${kref} is owned by live endpoint ${ownerVatID} which has no ` + + `c-list entry for it; treating it as orphaned`, + ); + orphanKernelObject(kref, ownerVatID); + ownerVatID = undefined; + } else if (ownerVatID && !terminated) { const vatConsidersReachable = getReachableFlag(ownerVatID, kref); if (vatConsidersReachable) { // the reachable count is zero, but the vat doesn't realize it @@ -221,6 +264,7 @@ export function getGCMethods(ctx: StoreContext) { scheduleReap, nextReapAction, retireKernelObjects, + orphanKernelObject, collectGarbage, }; } diff --git a/packages/ocap-kernel/src/store/methods/reachable.test.ts b/packages/ocap-kernel/src/store/methods/reachable.test.ts index 1ec32bdb9f..2464339b68 100644 --- a/packages/ocap-kernel/src/store/methods/reachable.test.ts +++ b/packages/ocap-kernel/src/store/methods/reachable.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { makeMapKernelDatabase } from '../../../test/storage.ts'; +import type { KRef } from '../../types.ts'; import { makeKernelStore } from '../index.ts'; describe('GC methods', () => { @@ -37,25 +38,42 @@ describe('GC methods', () => { }); }); - it.each(['setReachableFlag', 'clearReachableFlag'] as const)( - 'is idempotent: %s', - (method) => { - const ko1 = kernelStore.initKernelObject('v1'); - kernelStore.addCListEntry('v1', ko1, 'o-1'); - kernelStore.setReachableFlag('v1', ko1); - - const before = kernelStore.getObjectRefCount(ko1); - kernelStore[method]('v1', ko1); - kernelStore[method]('v1', ko1); - const after = kernelStore.getObjectRefCount(ko1); - - expect(after).toStrictEqual( - method === 'setReachableFlag' - ? before - : { reachable: 0, recognizable: 1 }, - ); - }, - ); + /** + * Give v1 an import entry it reaches, the state both idempotence tests + * start from. + * + * @returns The kref of the reached import. + */ + function givenReachedImport(): KRef { + const ko1 = kernelStore.initKernelObject('v1'); + kernelStore.addCListEntry('v1', ko1, 'o-1'); + kernelStore.setReachableFlag('v1', ko1); + return ko1; + } + + it('setReachableFlag is idempotent', () => { + const ko1 = givenReachedImport(); + + kernelStore.setReachableFlag('v1', ko1); + kernelStore.setReachableFlag('v1', ko1); + + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }); + + it('clearReachableFlag is idempotent', () => { + const ko1 = givenReachedImport(); + + kernelStore.clearReachableFlag('v1', ko1); + kernelStore.clearReachableFlag('v1', ko1); + + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 0, + recognizable: 1, + }); + }); it('leaves an export entry alone: it carries no reachable count', () => { const ko1 = kernelStore.initKernelObject('v1'); diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts index 03368852bb..461ee24e71 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts @@ -42,6 +42,32 @@ describe('reference count audit', () => { } } + /** + * Overwrite a kref's stored count, going around the store's own arithmetic so + * that drift can be introduced in either direction regardless of what the + * current count happens to be. + * + * @param kref - The kref whose count to overwrite. + * @param counts - The count text, in the store's encoding. + */ + function setStoredCount(kref: KRef, counts: string): void { + kv().set(`${kref}.refCount`, counts); + } + + /** + * Shift every component of a count by the same amount. + * + * @param counts - The count text, in the store's encoding. + * @param delta - How far to shift each component. + * @returns The shifted count text. + */ + function shift(counts: string, delta: number): string { + return counts + .split(',') + .map((part) => `${Number(part) + delta}`) + .join(','); + } + beforeEach(() => { kernelDatabase = makeMapKernelDatabase(); kernelStore = makeKernelStore(kernelDatabase); @@ -117,6 +143,14 @@ describe('reference count audit', () => { expect(kernelStore.auditRefCounts()).toStrictEqual([]); }); + it('holds for a queued notification', () => { + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.enqueueRun({ type: 'notify', endpointId: 'v2', kpid }); + kernelStore.incrementRefCount(kpid, 'notify'); + + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + it('holds for an unsettled promise with importers', () => { const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); kernelStore.translateRefKtoE('v2', kpid, true); @@ -174,6 +208,7 @@ describe('reference count audit', () => { expect(kernelStore.auditRefCounts()).toStrictEqual([ { + kind: 'mismatch', kref: koid, stored: '1,1', expected: '0,0', @@ -190,6 +225,7 @@ describe('reference count audit', () => { expect(kernelStore.auditRefCounts()).toStrictEqual([ { + kind: 'mismatch', kref, stored: '0,0', expected: '1,1', @@ -203,7 +239,7 @@ describe('reference count audit', () => { kernelStore.setObjectRefCount(kref, { reachable: 1, recognizable: 1 }); expect(kernelStore.auditRefCounts()).toStrictEqual([ - { kref, stored: '1,1', expected: '0,0', holders: [] }, + { kind: 'mismatch', kref, stored: '1,1', expected: '0,0', holders: [] }, ]); }); @@ -214,8 +250,8 @@ describe('reference count audit', () => { expect(kernelStore.auditRefCounts()).toStrictEqual([ { + kind: 'dangling', kref, - stored: '(deleted)', expected: '1,1', holders: ['v2 c-list import o-1'], }, @@ -233,10 +269,11 @@ describe('reference count audit', () => { // kernel would arrive: `getObjectRefCount` throws on all three, so // reading the row through it would take the whole sweep down with the // one violation it exists to report. - kernelDatabase.kernelKVStore.set(`${kref}.refCount`, row); + setStoredCount(kref, row); expect(kernelStore.auditRefCounts()).toStrictEqual([ { + kind: 'mismatch', kref, stored: row, expected: '1,1', @@ -256,6 +293,166 @@ describe('reference count audit', () => { }); }); + // The clean-audit cases above prove each rule agrees with whatever the store + // did, which stays true if a rule and the code it mirrors are wrong by the + // same constant. These pin each credit source to a literal count and holder + // label, and check drift in both directions: too low collects a live + // capability, too high leaks it. + describe('each credit source, on its own', () => { + const sources: { + what: string; + hold: () => KRef; + expected: string; + holders: string[]; + }[] = [ + { + what: 'an object import a vat still reaches', + hold: () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + return kref; + }, + expected: '1,1', + holders: ['v2 c-list import o-1'], + }, + { + what: 'an object import a vat has dropped but not retired', + hold: () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + return kref; + }, + expected: '0,1', + holders: ['v2 c-list import o-1'], + }, + { + what: 'a pinned object', + hold: () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.pinObject(kref); + return kref; + }, + expected: '1,1', + holders: ['pin'], + }, + { + what: "a run-queue send's target and slot", + hold: () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.enqueueRun({ + type: 'send', + target: kref, + message: { methargs: { body: '#[]', slots: [kref] }, result: null }, + }); + kernelStore.incrementRefCount(kref, 'queue|target'); + kernelStore.incrementRefCount(kref, 'queue|slot'); + return kref; + }, + expected: '2,2', + holders: ['run queue #1 send target', 'run queue #1 send slot'], + }, + { + what: "a run-queue send's result promise", + hold: () => { + const target = kernelStore.exportFromEndpoint('v1', 'o+1'); + const kpid = kernelStore.initKernelPromise()[0]; + kernelStore.enqueueRun({ + type: 'send', + target, + message: { methargs: { body: '#[]', slots: [] }, result: kpid }, + }); + kernelStore.incrementRefCount(target, 'queue|target'); + kernelStore.incrementRefCount(kpid, 'queue|result'); + return kpid; + }, + expected: '2', + holders: ['unsettled promise', 'run queue #1 send result'], + }, + { + what: 'a queued notification', + hold: () => { + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.enqueueRun({ type: 'notify', endpointId: 'v2', kpid }); + kernelStore.incrementRefCount(kpid, 'notify'); + return kpid; + }, + expected: '3', + holders: [ + 'unsettled promise', + 'run queue #1 notify', + 'v1 c-list export p+1', + ], + }, + { + // `enqueuePromiseMessage` takes the references itself, which is the + // point of the transfer-don't-duplicate fix; incrementing here too + // would be the double-count it exists to prevent. + what: 'a message parked on an unresolved promise', + hold: () => { + const target = kernelStore.exportFromEndpoint('v1', 'o+1'); + const kpid = kernelStore.initKernelPromise()[0]; + kernelStore.enqueuePromiseMessage(kpid, { + methargs: { body: '#[]', slots: [target] }, + result: null, + }); + return kpid; + }, + expected: '2', + holders: ['unsettled promise', 'kp1 queue #1 target'], + }, + { + what: 'a promise nobody has settled yet', + hold: () => kernelStore.initKernelPromise()[0], + expected: '1', + holders: ['unsettled promise'], + }, + { + what: "a settled promise's resolution slot", + hold: () => { + const koid = kernelStore.exportFromEndpoint('v1', 'o+1'); + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.incrementRefCount(koid, 'resolve|slot'); + kernelStore.resolveKernelPromise(kpid, false, { + body: '#"$0"', + slots: [koid], + }); + return koid; + }, + expected: '1,1', + holders: ['kp1 resolution slot'], + }, + { + what: "a promise's own c-list entries", + hold: () => { + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.translateRefKtoE('v2', kpid, true); + return kpid; + }, + expected: '3', + holders: [ + 'unsettled promise', + 'v1 c-list export p+1', + 'v2 c-list import p-1', + ], + }, + ]; + + it.each(sources)('credits $what exactly', ({ hold, expected, holders }) => { + const kref = hold(); + + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + + for (const delta of [1, -1]) { + const stored = shift(expected, delta); + setStoredCount(kref, stored); + expect(kernelStore.auditRefCounts()).toStrictEqual([ + { kind: 'mismatch', kref, stored, expected, holders }, + ]); + } + }); + }); + describe('assertRefCountsIfAuditing', () => { it('does nothing while auditing is off', () => { const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); @@ -297,6 +494,7 @@ describe('reference count audit', () => { expect(corrected).toStrictEqual([ { + kind: 'mismatch', kref, stored: '1,1', expected: '2,2', @@ -323,6 +521,34 @@ describe('reference count audit', () => { expect(unfixable[0]?.kref).toBe(kref); }); + it('rebuilds a promise count', () => { + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.translateRefKtoE('v2', kpid, true); + // A promise has one undifferentiated count, so its repair goes down a + // different path from an object's pair. + kernelStore.incrementRefCount(kpid, 'phantom'); + kernelStore.incrementRefCount(kpid, 'phantom'); + + const { corrected, unfixable } = kernelStore.recomputeRefCounts(); + + expect(corrected).toStrictEqual([ + { + kind: 'mismatch', + kref: kpid, + stored: '5', + expected: '3', + holders: [ + 'unsettled promise', + 'v1 c-list export p+1', + 'v2 c-list import p-1', + ], + }, + ]); + expect(unfixable).toStrictEqual([]); + expect(kernelStore.getRefCount(kpid)).toBe(3); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + it('queues krefs it zeroes for collection', () => { const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); kernelStore.setObjectRefCount(kref, { reachable: 1, recognizable: 1 }); diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.ts index 5c88376bf7..5dc5b51cf0 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -12,19 +12,34 @@ import { parseReachableAndVatSlot } from '../utils/reachable.ts'; * A kref whose stored reference counts disagree with the counts implied by the * references the kernel can actually be seen to hold. */ -export type RefCountViolation = { - kref: KRef; - /** - * The counts as stored, in the store's own encoding: `"reachable,recognizable"` - * for objects, a single number for promises, or `"(deleted)"` if the kref has - * no refcount entry at all. - */ - stored: string; - /** The counts implied by `holders`, in the same encoding as `stored`. */ - expected: string; - /** One entry per reference found, so a mismatch can be traced to its source. */ - holders: string[]; -}; +export type RefCountViolation = + | { + /** The kref is still counted, just by the wrong amount. */ + kind: 'mismatch'; + kref: KRef; + /** + * The counts as stored, in the store's own encoding: + * `"reachable,recognizable"` for objects, a single number for promises. + */ + stored: string; + /** The counts implied by `holders`, in the same encoding as `stored`. */ + expected: string; + /** One entry per reference found, so a mismatch can be traced to its source. */ + holders: string[]; + } + | { + /** + * The kref has no refcount entry, so each entry in `holders` names + * something the kernel has already deleted. Rewriting a count cannot + * repair this. + */ + kind: 'dangling'; + kref: KRef; + /** The counts `holders` imply, which there is nothing left to credit. */ + expected: string; + /** One entry per dangling reference found. */ + holders: string[]; + }; /** * The running total of references found for one kref. For a promise, which has @@ -283,6 +298,15 @@ export function getRefCountAuditMethods(ctx: StoreContext) { * Compare every kref's stored reference counts against the references the * kernel can be seen to hold. * + * What this can and cannot find is worth being precise about, because the + * ground truth here *is* the holder set. A count that disagrees with its + * holders is caught in either direction: too low, and a live capability can be + * collected; too high with no holder left, and the count itself is orphaned. + * But a holder that should have been torn down and wasn't justifies its own + * count — at any value — so a leaked *reference* is invisible to this by + * construction. A c-list entry that outlives what it names is the case that + * matters: see the settled-promise TODO in `KernelRouter`. + * * @returns The krefs whose counts disagree with ground truth, in kref order. */ function auditRefCounts(): RefCountViolation[] { @@ -303,8 +327,8 @@ export function getRefCountAuditMethods(ctx: StoreContext) { // pointing at it is a dangling reference. if (tally.holders.length > 0) { violations.push({ + kind: 'dangling', kref, - stored: '(deleted)', expected: expectedText, holders: tally.holders, }); @@ -319,6 +343,7 @@ export function getRefCountAuditMethods(ctx: StoreContext) { const storedText = raw; if (storedText !== expectedText) { violations.push({ + kind: 'mismatch', kref, stored: storedText, expected: expectedText, @@ -352,7 +377,7 @@ export function getRefCountAuditMethods(ctx: StoreContext) { const corrected: RefCountViolation[] = []; const unfixable: RefCountViolation[] = []; for (const violation of auditRefCounts()) { - if (violation.stored === '(deleted)') { + if (violation.kind === 'dangling') { unfixable.push(violation); continue; } @@ -369,11 +394,14 @@ export function getRefCountAuditMethods(ctx: StoreContext) { * Render violations as a human-readable report. * * @param violations - The violations to describe. - * @returns A multi-line description, one paragraph per violation. + * @returns A newline-separated report, one line per violation. */ function formatRefCountViolations(violations: RefCountViolation[]): string { return violations - .map(({ kref, stored, expected, holders }) => { + .map((violation) => { + const { kref, expected, holders } = violation; + const stored = + violation.kind === 'dangling' ? '(deleted)' : violation.stored; const held = holders.length > 0 ? holders.join(', ') : 'nothing'; return `${kref}: stored ${stored}, expected ${expected} (held by: ${held})`; }) @@ -390,9 +418,11 @@ export function getRefCountAuditMethods(ctx: StoreContext) { } const violations = auditRefCounts(); if (violations.length > 0) { - throw Error( - `reference count invariant violated:\n${formatRefCountViolations(violations)}`, - ); + const report = formatRefCountViolations(violations); + // Logged as well as thrown: if this is the last crank before the kernel + // goes idle, nobody sends another message and the log is the only record. + ctx.logger?.error(`reference count invariant violated:\n${report}`); + throw Error(`reference count invariant violated:\n${report}`); } } diff --git a/packages/ocap-kernel/src/store/methods/remote.ts b/packages/ocap-kernel/src/store/methods/remote.ts index 23ae73aec5..16a99cc993 100644 --- a/packages/ocap-kernel/src/store/methods/remote.ts +++ b/packages/ocap-kernel/src/store/methods/remote.ts @@ -47,6 +47,17 @@ export function getRemoteMethods(ctx: StoreContext) { } } + /** + * Get the IDs of all active remotes. + * + * @returns The remote IDs. + */ + function getRemoteIDs(): RemoteId[] { + return Array.from(getPrefixedKeys(REMOTE_INFO_BASE)).map( + (remoteKey) => remoteKey.slice(REMOTE_INFO_BASE_LEN) as RemoteId, + ); + } + /** * Fetch the stored info about a remote. * @@ -299,6 +310,7 @@ export function getRemoteMethods(ctx: StoreContext) { return { getAllRemoteRecords, + getRemoteIDs, getRemoteInfo, setRemoteInfo, deleteRemoteInfo, diff --git a/packages/ocap-kernel/src/store/methods/vat.ts b/packages/ocap-kernel/src/store/methods/vat.ts index e603ce5248..083ff9bfac 100644 --- a/packages/ocap-kernel/src/store/methods/vat.ts +++ b/packages/ocap-kernel/src/store/methods/vat.ts @@ -5,6 +5,7 @@ import { getCListMethods } from './clist.ts'; import { getObjectMethods } from './object.ts'; import { getPromiseMethods } from './promise.ts'; import { getReachableMethods } from './reachable.ts'; +import { getRemoteMethods } from './remote.ts'; import type { EndpointId, KRef, @@ -42,6 +43,7 @@ export function getVatMethods(ctx: StoreContext) { getPromiseMethods(ctx); const { initKernelObject } = getObjectMethods(ctx); const { addCListEntry } = getCListMethods(ctx); + const { getRemoteIDs } = getRemoteMethods(ctx); /** * Delete all persistent state associated with an endpoint. @@ -143,14 +145,17 @@ export function getVatMethods(ctx: StoreContext) { } /** - * Checks if a vat imports the specified kernel slot. + * Checks if an endpoint imports the specified kernel slot. * - * @param vatID - The ID of the vat to check. + * @param endpointId - The ID of the endpoint to check. * @param kernelSlot - The kernel slot reference. - * @returns True if the vat imports the kernel slot, false otherwise. + * @returns True if the endpoint imports the kernel slot, false otherwise. */ - function importsKernelSlot(vatID: VatId, kernelSlot: KRef): boolean { - const data = ctx.kv.get(getSlotKey(vatID, kernelSlot)); + function importsKernelSlot( + endpointId: EndpointId, + kernelSlot: KRef, + ): boolean { + const data = ctx.kv.get(getSlotKey(endpointId, kernelSlot)); if (data) { const { vatSlot } = parseReachableAndVatSlot(data); const { direction } = parseRef(vatSlot); @@ -162,15 +167,19 @@ export function getVatMethods(ctx: StoreContext) { } /** - * Gets all vats that import a specific kernel object. + * Gets all endpoints that import a specific kernel object. + * + * Remotes as well as vats: `retireKernelObjects` deletes the object once it + * has told the importers, so one missed here keeps a c-list entry naming a + * kref that no longer exists — which the reference count audit reports as + * dangling, killing the run loop. * * @param koid - The kernel object ID. - * @returns An array of vat IDs that import the kernel object. + * @returns An array of endpoint IDs that import the kernel object. */ - function getImporters(koid: KRef): VatId[] { - const importers = []; - importers.push( - ...getVatIDs().filter((vatID) => importsKernelSlot(vatID, koid)), + function getImporters(koid: KRef): EndpointId[] { + const importers: EndpointId[] = [...getVatIDs(), ...getRemoteIDs()].filter( + (endpointId) => importsKernelSlot(endpointId, koid), ); importers.sort(); return importers; @@ -291,9 +300,10 @@ export function getVatMethods(ctx: StoreContext) { work.imports += 1; } - // The caller rejected the orphan promises via getPromisesByDecider() before - // calling us, which is what released each promise's unsettled reference, - // but their kpids are still in the dead vat's c-list. Clean those up now. + // The caller looked the orphan promises up with getPromisesByDecider() and + // rejected them before calling us; that rejection is what released each + // promise's unsettled reference. Their kpids are still in the dead vat's + // c-list, so clean those up now. for (const key of getPrefixedKeys(promisePrefix)) { const krefStr = ctx.kv.get(key) ?? Fail`getNextKey ensures get`; assert(key.startsWith(clistPrefix), key); diff --git a/packages/ocap-kernel/src/vats/VatManager.test.ts b/packages/ocap-kernel/src/vats/VatManager.test.ts index 21361f942c..90cec52c32 100644 --- a/packages/ocap-kernel/src/vats/VatManager.test.ts +++ b/packages/ocap-kernel/src/vats/VatManager.test.ts @@ -207,6 +207,55 @@ describe('VatManager', () => { expect((error as Error).cause).toBe(cause); }); + + it('tears the worker down when kernel-side registration fails', async () => { + const config = createMockVatConfig(); + const cause = new Error('initEndpoint threw'); + mockKernelStore.initEndpoint.mockImplementationOnce(() => { + throw cause; + }); + + const error = await vatManager + .launchVat(config, 'bob', 's1') + .catch((reason: unknown) => reason); + + expect((error as Error).message).toBe('Failed to launch vat v1 (bob)'); + expect((error as Error).cause).toBe(cause); + // The worker is already running by this point, so it has to be stopped, + // and the vat marked so the terminated-vat cleanup reclaims what the + // partial launch wrote. + expect(mockPlatformServices.terminate).toHaveBeenCalledWith( + 'v1', + expect.any(Error), + ); + expect(vatHandles[0]?.terminate).toHaveBeenCalled(); + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + expect(vatManager.hasVat('v1')).toBe(false); + }); + + it('still marks the vat terminated when the cleanup itself fails', async () => { + const config = createMockVatConfig(); + const cause = new Error('setVatConfig threw'); + mockKernelStore.setVatConfig.mockImplementationOnce(() => { + throw cause; + }); + // `stopVat` unpins the root it was launched with, which is the first + // thing in the teardown that can fail. + mockKernelStore.unpinObject.mockImplementationOnce(() => { + throw new Error('worker will not die'); + }); + + const error = await vatManager + .launchVat(config, 'bob', 's1') + .catch((reason: unknown) => reason); + + expect((error as Error).message).toBe( + 'Failed to launch vat v1 (bob) (cleanup also failed)', + ); + // The launch failure, not the cleanup failure, is what the caller needs. + expect((error as Error).cause).toBe(cause); + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + }); }); describe('runVat', () => { @@ -262,6 +311,33 @@ describe('VatManager', () => { expect(mockKernelStore.unpinObject).toHaveBeenCalledWith('ko1'); }); + it.each([ + { + step: 'unpinning the root', + arrange: () => { + mockKernelStore.unpinObject.mockImplementationOnce(() => { + throw new Error('unpin failed'); + }); + }, + }, + { + step: 'terminating the handle', + arrange: () => { + vatHandles[0]?.terminate.mockRejectedValueOnce( + new Error('terminate failed'), + ); + }, + }, + ])('forgets the vat when $step throws', async ({ arrange }) => { + await vatManager.runVat('v1', createMockVatConfig()); + arrange(); + + await expect(vatManager.stopVat('v1', true)).rejects.toThrow('failed'); + + expect(vatManager.hasVat('v1')).toBe(false); + expect(vatManager.getVatIds()).toStrictEqual([]); + }); + it('stops a vat for termination with reason', async () => { const config = createMockVatConfig(); await vatManager.runVat('v1', config); diff --git a/packages/ocap-kernel/src/vats/VatManager.ts b/packages/ocap-kernel/src/vats/VatManager.ts index f080dbf177..5339409434 100644 --- a/packages/ocap-kernel/src/vats/VatManager.ts +++ b/packages/ocap-kernel/src/vats/VatManager.ts @@ -123,18 +123,42 @@ export class VatManager { cause: error, }); } - this.#kernelStore.initEndpoint(vatId); - const rootRef = this.#kernelStore.exportFromEndpoint( - vatId, - ROOT_OBJECT_VREF, - ); - // A root is addressable for as long as its vat lives, whether or not - // anyone currently imports it: the kernel's own API hands out root krefs - // and `getRootObject` resolves them through this c-list entry. Without a - // pin, GC would retire the entry the moment the last importer let go. - this.#kernelStore.pinObject(rootRef); - this.#kernelStore.setVatConfig(vatId, vatConfig); - return rootRef; + try { + this.#kernelStore.initEndpoint(vatId); + const rootRef = this.#kernelStore.exportFromEndpoint( + vatId, + ROOT_OBJECT_VREF, + ); + // A root is addressable for as long as its vat lives, whether or not + // anyone currently imports it: the kernel's own API hands out root krefs + // and `getRootObject` resolves them through this c-list entry. Without a + // pin, GC would retire the entry the moment the last importer let go. + this.#kernelStore.pinObject(rootRef); + this.#kernelStore.setVatConfig(vatId, vatConfig); + return rootRef; + } catch (error) { + // The worker is already running, so leaving it would strand a vat the + // kernel has no record of. Tear it down before reporting the failure. + let stopFailure: unknown; + try { + await this.stopVat(vatId, true); + } catch (caught) { + stopFailure = caught; + this.#logger.error( + `Failed to stop vat ${vatId} after incomplete launch; its worker may still be running:`, + caught, + ); + } + // `stopVat` only tears down the worker. Whatever store records the + // partial launch did write — the endpoint counters, the root's c-list + // pair, its owner entry — are reclaimed by the terminated-vat cleanup, + // which never runs unless the vat is marked. + this.#kernelStore.markVatAsTerminated(vatId); + throw new Error( + `Failed to launch vat ${vatId} (${vatName})${stopFailure ? ' (cleanup also failed)' : ''}`, + { cause: error }, + ); + } } /** @@ -191,15 +215,22 @@ export class VatManager { } else if (terminating) { terminationError = new VatDeletedError(vatId); } - if (terminating) { - // A restart keeps the pin: the same root comes back. - this.releaseVatRootPin(vatId); + try { + if (terminating) { + // A restart keeps the pin: the same root comes back. + this.releaseVatRootPin(vatId); + } + await this.#platformServices + .terminate(vatId, terminationError) + .catch(this.#logger.error); + await vat.terminate(terminating, terminationError); + } finally { + // A handle left behind outlives the store state the terminated-vat + // cleanup wipes, so `hasVat`, `getVatIds` and `#getEndpoint` go on + // reporting a vat with no c-list, and the next `bringOutYourDead` + // selected for it kills the run loop. + this.#vats.delete(vatId); } - await this.#platformServices - .terminate(vatId, terminationError) - .catch(this.#logger.error); - await vat.terminate(terminating, terminationError); - this.#vats.delete(vatId); } /** diff --git a/packages/ocap-kernel/src/vats/VatSyscall.test.ts b/packages/ocap-kernel/src/vats/VatSyscall.test.ts index 04a0e8bf42..bf425bc23b 100644 --- a/packages/ocap-kernel/src/vats/VatSyscall.test.ts +++ b/packages/ocap-kernel/src/vats/VatSyscall.test.ts @@ -30,6 +30,9 @@ describe('VatSyscall', () => { clearReachableFlag: vi.fn(), getReachableFlag: vi.fn(), forgetKref: vi.fn(), + // Only an owner may disown an object, so the cleanup syscalls check first + getOwner: vi.fn().mockReturnValue('v1'), + orphanKernelObject: vi.fn(), getVatConfig: vi.fn(() => ({})), isVatActive: vi.fn(() => true), isInCrank: vi.fn(() => true),