diff --git a/packages/kernel-test/src/garbage-collection.test.ts b/packages/kernel-test/src/garbage-collection.test.ts index 2fc1dc1605..44345bfcdd 100644 --- a/packages/kernel-test/src/garbage-collection.test.ts +++ b/packages/kernel-test/src/garbage-collection.test.ts @@ -254,17 +254,30 @@ describe('Garbage Collection', () => { }); /** - * Give an importer a chance to notice a dropped object and tell the kernel. + * Give an importer a chance to notice a dropped object and tell the kernel, + * then keep cranking until the resulting GC actions have all been consumed. * * @param vatId - The vat to reap. * @param rootKRef - That vat's root, to poke with cranks afterwards. */ async function reapAndSettle(vatId: VatId, rootKRef: KRef): Promise { kernel.reapVats((id) => id === vatId); - for (let i = 0; i < 3; i++) { + // BOYD has to reach the vat, the vat has to answer, and the kernel has to + // act on the answer — but a round can queue more work, so loop until the + // queue is actually empty rather than guessing at a crank count. + const maxRounds = 10; + for (let round = 0; round < maxRounds; round++) { await kernel.queueMessage(rootKRef, 'noop', []); await waitUntilQuiescent(500); + if ([...kernelStore.getGCActions()].length === 0) { + return; + } } + throw Error( + `GC actions still pending after ${maxRounds} rounds: ${[ + ...kernelStore.getGCActions(), + ].join(', ')}`, + ); } it('survives until both importers let go', async () => { 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..3ab09fdba6 --- /dev/null +++ b/packages/kernel-test/src/refcount-audit.test.ts @@ -0,0 +1,63 @@ +import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; +import { makeKernelStore } from '@metamask/ocap-kernel'; +import type { KRef, VatId } from '@metamask/ocap-kernel'; +import { expect, describe, it } from 'vitest'; + +import { + getBundleSpec, + makeKernel, + makeMockLogger, + runTestVats, +} from './utils.ts'; + +/** + * The per-crank audit throws from inside the run loop, which nothing restarts. + * Unless that failure is reported to whoever is waiting on the kernel, the only + * symptom is a test that hangs until its timeout, with no mention of reference + * counts anywhere — which would make the audit worthless as a build gate. + */ +describe('reference count audit', () => { + it('reports a violation to kernel callers rather than hanging', async () => { + const kernelDatabase = await makeSQLKernelDatabase({ + dbFilename: ':memory:', + }); + const kernelStore = makeKernelStore(kernelDatabase); + const kernel = await makeKernel(kernelDatabase, true, makeMockLogger()); + await runTestVats(kernel, { + bootstrap: 'exporter', + forceReset: true, + vats: { + exporter: { + bundleSpec: getBundleSpec('exporter-vat'), + parameters: { name: 'Exporter' }, + }, + }, + }); + + const exporterVatId = kernel.getVats()[0]?.id as VatId; + const exporterKRef = kernelStore.getRootObject(exporterVatId) as KRef; + + kernelStore.setObjectRefCount(exporterKRef, { + reachable: 7, + recognizable: 9, + }); + + // The crank carrying this message settles its result before the + // end-of-crank audit runs, so this one may still succeed. + await kernel + .queueMessage(exporterKRef, 'createObject', ['x']) + .catch(() => undefined); + + // What a caller is told directly is that the run loop is gone; the audit + // failure that killed it rides along as the `cause`. That chain is the part + // that has to survive, since "run loop died" on its own names nothing. + const failure = (await kernel + .queueMessage(exporterKRef, 'createObject', ['y']) + .catch((error) => error)) as Error; + + expect(failure.message).toMatch(/Kernel run loop died/u); + expect(String(failure.cause)).toMatch( + /reference count invariant violated/u, + ); + }, 30000); +}); diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 4c363fd234..86794f9103 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -38,12 +38,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Caveat: a legitimate string argument that begins with `@@` followed by alphanumerics will be misinterpreted as a marker; wrap such literals inside an object - Reference-count auditing: `auditRefCounts`, `recomputeRefCounts`, `formatRefCountViolations`, `assertRefCountsIfAuditing`, and `setRefCountAuditing` on the kernel store, plus a `Kernel.make` option `auditRefCounts` that verifies every kref's counts against the references the kernel actually holds at the end of each crank ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) - - Reports drift in both directions: counts too low (a live capability can be collected) and counts too high (a leak) + - Reports drift in both directions: counts too low (a live capability can be collected) and counts too high with no holder (an orphaned count). It compares counts against the holders it finds, so a holder that should have been torn down but wasn't justifies its own count and is not detectable this way - Only references visible in the kernel's own state are checkable, so a holder that keeps a kref outside them has to take a pin to be counted at all - `recomputeRefCounts` is a repair tool for a drifted store, offered to embedders and never run automatically: opening an existing store does not migrate it - - Exports the `RefCountViolation` type + - Exports the `RefCountViolation` type, a union over `kind: 'mismatch' | 'dangling'` - Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) - Add `getOcapURLObjects` and `retainForOcapURL` to the kernel store, and `VatManager.releaseVatRootPin` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- Add `orphanKernelObject` to the kernel store, which drops an object's owner mapping and hands it to the collector ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) ### Changed @@ -96,6 +97,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - A root is addressable while its vat lives whether or not anyone imports it; the old `(1, 1)` birth baseline was standing in for this - Garbage-collection action delivery now moves the kernel's own c-list: `dropExports` clears the owner's reachable flag and `retireExports`/`retireImports` tear the entry down ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) - Previously the owner's flag never cleared, so the same action could be re-derived, and retired entries outlived the objects they named +- Orphan a kernel object when its owner stops naming it (a delivered `retireExport`, or a `retireExports`/`abandonExports` syscall), so its `owner` and `refCount` records no longer outlive the c-list entry they were reachable through ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) + - They leaked, and the next collection to visit such a kref read a c-list entry that was no longer there and killed the run loop. Reproduces on `main`, so it predates this stack +- Reject a `retireExports`/`abandonExports` syscall for an object the calling endpoint does not own, instead of letting it erase another endpoint's claim to an object it is still exporting ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) + - Nothing upstream of `performExportCleanup` checked that the vref it was handed is even an export, and the audit could not see the damage, because an export entry carries no count +- Garbage-collection action delivery releases the kernel's side even when the endpoint has vanished, and rolls back rather than committing a release the endpoint was never told about ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) + - It releases only where the endpoint is genuinely gone: a terminated vat, whose cleanup tears the whole c-list down anyway, or a remote, which reconciles on its next incarnation. A vat that is absent yet not terminated is one `restartVat` has taken out of the kernel's reach while keeping its c-list, so the crank fails there instead of committing a release the returning incarnation would disagree with +- A failed garbage-collection delivery to a remote is logged and survived rather than escaping the crank and stopping the run loop ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) +- Tear down and mark for cleanup a vat whose worker launched but whose kernel-side registration then failed ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) - Charge a delivered message's target reference against the run-queue item's own target rather than the routed target, so a message routed through a resolved promise no longer decrements an object nobody charged while leaking the promise ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) - Release a queued notification's reference before the paths that decide there is nothing to deliver, and stop decrementing references on promises retired alongside it that nobody had taken ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) - Transfer, rather than duplicate, the references a message carries when it is queued on an unresolved promise and later re-enqueued on resolution ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) diff --git a/packages/ocap-kernel/src/Kernel.ts b/packages/ocap-kernel/src/Kernel.ts index c041d060be..f615ea6cbf 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 11aa8922c1..e88c54b52c 100644 --- a/packages/ocap-kernel/src/KernelRouter.test.ts +++ b/packages/ocap-kernel/src/KernelRouter.test.ts @@ -65,6 +65,9 @@ describe('KernelRouter', () => { clearReachableFlag: vi.fn(), deleteCListEntry: vi.fn(), forgetKref: vi.fn(), + orphanKernelObject: vi.fn(), + hasCListEntry: vi.fn().mockReturnValue(true), + isVatTerminated: vi.fn().mockReturnValue(false), createCrankSavepoint: vi.fn(), } as unknown as KernelStore; @@ -317,6 +320,38 @@ describe('KernelRouter', () => { ]); }); + it('charges the promise, not the object it resolved to', async () => { + const promiseId = 'kp123'; + const resolvedObject = 'ko456'; + ( + kernelStore.getKernelPromise as unknown as MockInstance + ).mockReturnValueOnce({ + state: 'fulfilled', + value: { body: '#"$0"', slots: [resolvedObject] }, + }); + (kernelStore.getOwner as unknown as MockInstance).mockReturnValue('v1'); + + await kernelRouter.deliver({ + type: 'send', + target: promiseId, + message: { + methargs: { body: 'method args', slots: [] }, + result: null, + }, + }); + + // The run queue item was charged against the promise it named, so that + // is what has to be released — not whatever routing resolved it to. + expect(kernelStore.decrementRefCount).toHaveBeenCalledWith( + promiseId, + 'deliver|send|target', + ); + expect(kernelStore.decrementRefCount).not.toHaveBeenCalledWith( + resolvedObject, + 'deliver|send|target', + ); + }); + it('splats message when promise resolves to a non-object', async () => { // Setup a fulfilled promise that doesn't resolve to an object const promiseId = 'kp123'; @@ -580,6 +615,12 @@ describe('KernelRouter', () => { // Verify no notification was delivered to the vat expect(endpointHandle.deliverNotify).not.toHaveBeenCalled(); expect(result).toStrictEqual({ didDelivery: endpointId }); + // Nothing was delivered, but the queued notification is gone either + // way, so its reference has to be released on this path too. + expect(kernelStore.decrementRefCount).toHaveBeenCalledWith( + kpid, + 'deliver|notify', + ); }); it('returns didDelivery when no kpids to retire', async () => { @@ -618,6 +659,10 @@ describe('KernelRouter', () => { // Verify no notification was delivered to the vat expect(endpointHandle.deliverNotify).not.toHaveBeenCalled(); expect(result).toStrictEqual({ didDelivery: endpointId }); + expect(kernelStore.decrementRefCount).toHaveBeenCalledWith( + kpid, + 'deliver|notify', + ); }); it('throws if notification is for an unresolved promise', async () => { @@ -715,6 +760,168 @@ describe('KernelRouter', () => { ]); }, ); + + it('orphans the object when delivering retireExports', async () => { + await kernelRouter.deliver({ + type: 'retireExports', + endpointId: 'v1', + krefs: ['ko1', 'ko2'], + }); + + // The owner has given up the last name for the object, so the kernel's + // record of who owns it must go too or it outlives every reference. + expect( + (kernelStore.orphanKernelObject as unknown as MockInstance).mock + .calls, + ).toStrictEqual([ + ['ko1', 'v1'], + ['ko2', 'v1'], + ]); + }); + + it('leaves ownership alone when delivering retireImports', async () => { + await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'v1', + krefs: ['ko1'], + }); + + expect(kernelStore.orphanKernelObject).not.toHaveBeenCalled(); + }); + + it('still releases the kernel side when a terminated vat has vanished', async () => { + getEndpoint.mockImplementationOnce(() => { + throw Error('vat v1 not found'); + }); + ( + kernelStore.isVatTerminated as unknown as MockInstance + ).mockReturnValue(true); + + const result = await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'v1', + krefs: ['ko1'], + }); + + expect(result).toStrictEqual({ didDelivery: 'v1' }); + // The action has already been consumed, so skipping the teardown would + // lose it and leave the entry behind for good + expect(kernelStore.deleteCListEntry).toHaveBeenCalledWith( + 'v1', + 'ko1', + 'translated-ko1', + ); + }); + + it('still releases the kernel side when a remote has vanished', async () => { + getEndpoint.mockImplementationOnce(() => { + throw Error('remote r1 not found'); + }); + + const result = await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'r1', + krefs: ['ko1'], + }); + + expect(result).toStrictEqual({ didDelivery: 'r1' }); + expect(kernelStore.deleteCListEntry).toHaveBeenCalledWith( + 'r1', + 'ko1', + 'translated-ko1', + ); + }); + + it.each(['dropExports', 'retireExports', 'retireImports'] as const)( + 'refuses to release %s for a vat that is absent but not terminated', + async (actionType) => { + // A vat between incarnations still holds every one of these krefs, so + // committing the kernel's release would leave the two disagreeing. + getEndpoint.mockImplementationOnce(() => { + throw Error('vat v1 not found'); + }); + + await expect( + kernelRouter.deliver({ + type: actionType, + endpointId: 'v1', + krefs: ['ko1'], + }), + ).rejects.toThrow('vat v1 not found'); + + expect(kernelStore.clearReachableFlag).not.toHaveBeenCalled(); + expect(kernelStore.deleteCListEntry).not.toHaveBeenCalled(); + expect(kernelStore.orphanKernelObject).not.toHaveBeenCalled(); + }, + ); + + it('skips krefs already cleaned up before delivery', async () => { + ( + kernelStore.hasCListEntry as unknown as MockInstance + ).mockImplementation( + (_endpointId: string, kref: string) => kref === 'ko1', + ); + + await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'v1', + krefs: ['ko1', 'ko2'], + }); + + expect( + (kernelStore.deleteCListEntry as unknown as MockInstance).mock.calls, + ).toStrictEqual([['v1', 'ko1', 'translated-ko1']]); + }); + + it('does nothing when every kref is already gone', async () => { + (kernelStore.hasCListEntry as unknown as MockInstance).mockReturnValue( + false, + ); + + const result = await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'v1', + krefs: ['ko1'], + }); + + expect(result).toStrictEqual({ didDelivery: 'v1' }); + expect(kernelStore.deleteCListEntry).not.toHaveBeenCalled(); + expect(endpointHandle.deliverRetireImports).not.toHaveBeenCalled(); + }); + + it('rolls back and terminates the vat when delivery fails', async () => { + ( + endpointHandle.deliverRetireImports as unknown as MockInstance + ).mockRejectedValueOnce(Error('endpoint went away mid-delivery')); + + const result = await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'v1', + krefs: ['ko1'], + }); + + // Committing the release while v1 still holds the eref would leave the + // two disagreeing, and v1 would mint a fresh kref for the same object + expect(result?.abort).toBe(true); + expect(result?.terminate?.vatId).toBe('v1'); + }); + + it('does not retry a remote that refuses the delivery', async () => { + ( + endpointHandle.deliverRetireImports as unknown as MockInstance + ).mockRejectedValueOnce(Error('remote queue full')); + + const result = await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'r1', + krefs: ['ko1'], + }); + + // Aborting would restore the action, and GC actions are selected ahead + // of all other work, so a remote that keeps refusing would be handed + // this same item every crank and nothing else would ever run + expect(result).toStrictEqual({ didDelivery: 'r1' }); + }); }); describe('bringOutYourDead', () => { diff --git a/packages/ocap-kernel/src/KernelRouter.ts b/packages/ocap-kernel/src/KernelRouter.ts index 6bd080e7c3..5117905f36 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,123 @@ 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. Fail the crank rather than commit that. + // Nothing here can make the restart safe: the action is already spent from + // the durable set, and a crank that neither delivers nor releases would + // simply be handed the same action again on the next one. + if ( + isVatId(endpointId) && + !this.#kernelStore.isVatTerminated(endpointId) + ) { + throw error; + } + // 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 — it + // reconciles on the next incarnation change. 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. + 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/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 0344da12d4..d2fb46efe3 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -150,6 +150,7 @@ describe('kernel store', () => { 'markVatAsTerminated', 'nextReapAction', 'nextTerminatedVatCleanup', + 'orphanKernelObject', 'pinObject', 'provideIncarnationId', 'recomputeRefCounts', 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 c7d1cf22d1..5ce0dc12b1 100644 --- a/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts +++ b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts @@ -5,10 +5,9 @@ import type { VatConfig, VatId } from '../../types.ts'; import { makeKernelStore } from '../index.ts'; /** - * Regressions for the asymmetry described in - * https://github.com/MetaMask/ocap-kernel/issues/1006: creating an import - * c-list entry changed no refcount while tearing one down decremented both, - * and `initKernelObject` compensated by minting every object at (1, 1). That + * Regressions for an asymmetry in c-list accounting: creating an import c-list + * entry changed no refcount while tearing one down decremented both, and + * `initKernelObject` compensated by minting every object at (1, 1). That * constant came out right for exactly one importer, which is why nothing * noticed. */ @@ -159,6 +158,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 d079100ae4..d7e3a52af0 100644 --- a/packages/ocap-kernel/src/store/methods/clist.ts +++ b/packages/ocap-kernel/src/store/methods/clist.ts @@ -33,7 +33,8 @@ export function getCListMethods(ctx: StoreContext) { * {@link deleteCListEntry}. An import is born recognizing but not reaching: * reachability is `setReachableFlag`'s job, when the reference is handed * over. An export takes no count for an object — the owner is not one of its - * own referrers — and is born flagged. + * own referrers — and is born flagged. For a promise both directions count; + * only objects exempt the owner. * * @param endpointId - The endpoint whose c-list is to be added to. * @param kref - The KRef. @@ -146,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.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 70af7d4075..868034e55c 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts @@ -1,3 +1,4 @@ +import type { KernelDatabase } from '@metamask/kernel-store'; import { describe, it, expect, beforeEach } from 'vitest'; import { makeMapKernelDatabase } from '../../../test/storage.ts'; @@ -6,6 +7,7 @@ import { makeKernelStore } from '../index.ts'; describe('reference count audit', () => { let kernelStore: ReturnType; + let kdb: KernelDatabase; /** * Register and initialize an endpoint so it can hold c-list entries. @@ -19,8 +21,35 @@ 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 { + kdb.kernelKVStore.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(() => { - kernelStore = makeKernelStore(makeMapKernelDatabase()); + kdb = makeMapKernelDatabase(); + kernelStore = makeKernelStore(kdb); kernelStore.markInitialized(); givenVats('v1', 'v2', 'v3'); }); @@ -93,6 +122,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); @@ -120,6 +157,7 @@ describe('reference count audit', () => { expect(kernelStore.auditRefCounts()).toStrictEqual([ { + kind: 'mismatch', kref, stored: '0,0', expected: '1,1', @@ -133,7 +171,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: [] }, ]); }); @@ -144,8 +182,8 @@ describe('reference count audit', () => { expect(kernelStore.auditRefCounts()).toStrictEqual([ { + kind: 'dangling', kref, - stored: '(deleted)', expected: '1,1', holders: ['v2 c-list import o-1'], }, @@ -163,6 +201,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'); @@ -204,6 +402,7 @@ describe('reference count audit', () => { expect(corrected).toStrictEqual([ { + kind: 'mismatch', kref, stored: '1,1', expected: '2,2', @@ -230,6 +429,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 b4178e5c6f..27a6d912f4 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -13,19 +13,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 @@ -247,6 +262,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[] { @@ -267,8 +291,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, }); @@ -280,6 +304,7 @@ export function getRefCountAuditMethods(ctx: StoreContext) { : renderCounts(kref, getObjectRefCount(kref)); if (storedText !== expectedText) { violations.push({ + kind: 'mismatch', kref, stored: storedText, expected: expectedText, @@ -313,7 +338,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; } @@ -330,11 +355,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})`; }) @@ -351,9 +379,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/vat.ts b/packages/ocap-kernel/src/store/methods/vat.ts index 29e2a85dff..c1875d8581 100644 --- a/packages/ocap-kernel/src/store/methods/vat.ts +++ b/packages/ocap-kernel/src/store/methods/vat.ts @@ -271,9 +271,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..e437d042b9 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', () => { diff --git a/packages/ocap-kernel/src/vats/VatManager.ts b/packages/ocap-kernel/src/vats/VatManager.ts index 5da2bebe2d..0870a9dacf 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 }, + ); + } } /** 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),