diff --git a/packages/extension/test/e2e/control-panel.test.ts b/packages/extension/test/e2e/control-panel.test.ts index eaa19ed27e..65f62158e8 100644 --- a/packages/extension/test/e2e/control-panel.test.ts +++ b/packages/extension/test/e2e/control-panel.test.ts @@ -165,7 +165,6 @@ test.describe('Control Panel', () => { '{"key":"v3.c.o+0","value":"ko6"}', '{"key":"v3.c.kp4","value":"R p-1"}', '{"key":"v3.c.p-1","value":"kp4"}', - '{"key":"ko6.refCount","value":"1,1"}', '{"key":"kp4.refCount","value":"2"}', ]; const v1koValues = [ @@ -190,6 +189,13 @@ test.describe('Control Panel', () => { popupPage.locator('[data-testid="message-output"]'), ).toContainText(value); } + // Asserted per checkpoint rather than from `v3Values`, which is used as a + // negative below: v3's root keeps a count for as long as v1 imports it, so + // it is not one of the keys that vanish with the vat. The value is the root + // pin plus that import. + await expect( + popupPage.locator('[data-testid="message-output"]'), + ).toContainText('{"key":"ko6.refCount","value":"2,2"}'); await popupPage.click('button:text("Control Panel")'); await popupPage.locator('[data-testid="accordion-header"]').first().click(); await popupPage @@ -215,6 +221,11 @@ test.describe('Control Panel', () => { popupPage.locator('[data-testid="message-output"]'), ).toContainText(value); } + // Terminating v3 released the pin its root was held by, leaving v1's import + // as the only holder. + await expect( + popupPage.locator('[data-testid="message-output"]'), + ).toContainText('{"key":"ko6.refCount","value":"1,1"}'); await popupPage.click('button:text("Control Panel")'); await popupPage.click('button:text("Collect Garbage")'); @@ -242,6 +253,11 @@ test.describe('Control Panel', () => { await expect( popupPage.locator('[data-testid="message-output"]'), ).toContainText('{"key":"kp4.refCount","value":"1"}'); + // v3's cleanup took its own c-list, not v1's import, so the root survives + // its owner at the one count that import justifies. + await expect( + popupPage.locator('[data-testid="message-output"]'), + ).toContainText('{"key":"ko6.refCount","value":"1,1"}'); await popupPage.click('button:text("Control Panel")'); await popupPage.locator('[data-testid="accordion-header"]').first().click(); // delete v1 diff --git a/packages/kernel-test/src/crank-rollback.test.ts b/packages/kernel-test/src/crank-rollback.test.ts index cd2a708aa0..919f8ed585 100644 --- a/packages/kernel-test/src/crank-rollback.test.ts +++ b/packages/kernel-test/src/crank-rollback.test.ts @@ -138,6 +138,94 @@ describe('crank rollback against a real database', () => { expect(kdb.kernelKVStore.get('second')).toBe('yes'); }); + // Every `provideCachedStoredValue` keeps its value in a closure and writes + // through to kv, so a rollback that only reverts the database leaves the cache + // holding the abandoned crank's value — and the next `set` persists it. The GC + // action set is the case that matters: `processGCActionSet` consumes an action + // before delivering it, so losing the rollback loses the action outright. + it('restores the GC action set consumed by a rolled-back crank', async () => { + const { kernelStore } = await makeStore(); + kernelStore.addGCActions(['v1 dropExport ko1']); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('start'); + // Consume the action the way `processGCActionSet` does. + kernelStore.setGCActions(new Set()); + + kernelStore.rollbackCrank('start'); + kernelStore.endCrank(); + + expect([...kernelStore.getGCActions()]).toStrictEqual([ + 'v1 dropExport ko1', + ]); + }); + + // Same closure, same failure: a reap scheduled and then consumed by a crank + // that rolls back must still be pending afterwards. + it('restores the reap queue consumed by a rolled-back crank', async () => { + const { kernelStore } = await makeStore(); + kernelStore.scheduleReap('v1'); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('start'); + expect(kernelStore.nextReapAction()).toBeDefined(); + + kernelStore.rollbackCrank('start'); + kernelStore.endCrank(); + + expect(kernelStore.nextReapAction()).toBeDefined(); + }); + + // `maybeFreeKrefs` is RAM-only, so nothing rolls it back. Left populated, the + // next crank's `collectGarbage` visits krefs whose decrements were undone — + // and `getKernelPromise` throws outright for one the rollback deleted, which + // kills the run loop. + it('discards GC candidates accumulated by a rolled-back crank', async () => { + const { kernelStore } = await makeStore(); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('start'); + // Born at 1, so this drops it to 0 and leaves `kpid` in `maybeFreeKrefs` + // while the rollback removes the promise record it names. + const kpid = kernelStore.initKernelPromise()[0]; + kernelStore.decrementRefCount(kpid, 'test'); + + kernelStore.rollbackCrank('start'); + kernelStore.endCrank(); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('start'); + expect(() => kernelStore.collectGarbage()).not.toThrow(); + kernelStore.endCrank(); + }); + + // The set is not per-crank: only `collectGarbage` empties it, and that runs at + // the end of a crank that had an item. So a candidate created while the run + // loop was idle — `terminateVat` unpinning a root is the real path — is still + // owed a collection, and an unrelated crank's rollback must not cancel it. + it('keeps GC candidates that predate the crank it rolled back', async () => { + const { kernelStore } = await makeStore(); + const idle = kernelStore.initKernelPromise()[0]; + kernelStore.decrementRefCount(idle, 'test'); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('start'); + const abandoned = kernelStore.initKernelPromise()[0]; + kernelStore.decrementRefCount(abandoned, 'test'); + kernelStore.rollbackCrank('start'); + kernelStore.endCrank(); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('start'); + kernelStore.collectGarbage(); + kernelStore.endCrank(); + + // Collected, because it was owed before the abandoned crank began. + expect(() => kernelStore.getKernelPromise(idle)).toThrow( + 'unknown kernel promise', + ); + }); + // `createCrankSavepoint` records the name only once the database has the // savepoint. Asking to roll back one that was never created must therefore say // so, rather than releasing someone else's savepoint. diff --git a/packages/kernel-test/src/garbage-collection.test.ts b/packages/kernel-test/src/garbage-collection.test.ts index 268ed7920e..84b120947b 100644 --- a/packages/kernel-test/src/garbage-collection.test.ts +++ b/packages/kernel-test/src/garbage-collection.test.ts @@ -21,9 +21,11 @@ import { /** * Make a test subcluster with vats for GC testing * + * @param extraImporters - Names of additional importer vats to include, for + * topologies where more than one vat shares the same exported object. * @returns The test subcluster */ -function makeTestSubcluster(): ClusterConfig { +function makeTestSubcluster(extraImporters: string[] = []): ClusterConfig { return { bootstrap: 'exporter', forceReset: true, @@ -40,6 +42,15 @@ function makeTestSubcluster(): ClusterConfig { name: 'Importer', }, }, + ...Object.fromEntries( + extraImporters.map((name) => [ + name, + { + bundleSpec: getBundleSpec('importer-vat'), + parameters: { name }, + }, + ]), + ), }, }; } @@ -81,10 +92,11 @@ describe('Garbage Collection', () => { [objectId], ); const createObjectRef = createObjectData.slots[0] as KRef; - // Verify initial reference counts from database - const initialRefCounts = kernelStore.getObjectRefCount(createObjectRef); - expect(initialRefCounts.reachable).toBe(2); - expect(initialRefCounts.recognizable).toBe(2); + // Held only by the resolved promise's value, which still carries the slot + expect(kernelStore.getObjectRefCount(createObjectRef)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); // Send the object to the importer vat const objectRef = kunser(createObjectData); await kernel.queueMessage(importerKRef, 'storeImport', [objectRef]); @@ -116,10 +128,10 @@ describe('Garbage Collection', () => { await waitUntilQuiescent(); const createObjectRef = createObjectData.slots[0] as KRef; - // Store initial reference count information - const initialRefCounts = kernelStore.getObjectRefCount(createObjectRef); - expect(initialRefCounts.reachable).toBe(2); - expect(initialRefCounts.recognizable).toBe(2); + expect(kernelStore.getObjectRefCount(createObjectRef)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); // Store the reference in the importer vat const objectRef = kunser(createObjectData); @@ -201,4 +213,129 @@ describe('Garbage Collection', () => { ); expect(parseReplyBody(exporterFinalCheck.body)).toBe(false); }, 40000); + + describe('an object shared by two importers', () => { + let secondImporterKRef: KRef; + let secondImporterVatId: VatId; + + beforeEach(async () => { + kernelDatabase = await makeSQLKernelDatabase({ dbFilename: ':memory:' }); + kernelStore = makeKernelStore(kernelDatabase); + kernel = await makeKernel(kernelDatabase, true, makeMockLogger()); + await runTestVats(kernel, makeTestSubcluster(['Importer2'])); + + const vats = kernel.getVats(); + const idOf = (name: string): VatId => + vats.find((row) => row.config.parameters?.name === name)?.id as VatId; + exporterVatId = idOf('Exporter'); + importerVatId = idOf('Importer'); + secondImporterVatId = idOf('Importer2'); + exporterKRef = kernelStore.getRootObject(exporterVatId) as KRef; + importerKRef = kernelStore.getRootObject(importerVatId) as KRef; + secondImporterKRef = kernelStore.getRootObject( + secondImporterVatId, + ) as KRef; + }); + + /** + * 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); + // 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 () => { + const objectId = 'shared-object'; + const createObjectData = await kernel.queueMessage( + exporterKRef, + 'createObject', + [objectId], + ); + const sharedKRef = createObjectData.slots[0] as KRef; + const objectRef = kunser(createObjectData); + + for (const importer of [importerKRef, secondImporterKRef]) { + await kernel.queueMessage(importer, 'storeImport', [ + objectRef, + objectId, + ]); + } + await waitUntilQuiescent(); + + expect(kernelStore.getImporters(sharedKRef)).toStrictEqual( + [importerVatId, secondImporterVatId].sort(), + ); + // Two importers, plus the resolved createObject promise whose value + // still carries the slot + expect(kernelStore.getObjectRefCount(sharedKRef)).toStrictEqual({ + reachable: 3, + recognizable: 3, + }); + + await kernel.queueMessage(importerKRef, 'makeWeak', [objectId]); + await kernel.queueMessage(importerKRef, 'forgetImport', []); + await waitUntilQuiescent(); + await reapAndSettle(importerVatId, importerKRef); + + // The exporter must not have been told to drop it: the second importer + // legitimately still holds it + expect(kernelStore.getReachableFlag(exporterVatId, sharedKRef)).toBe( + true, + ); + expect(kernelStore.getImporters(sharedKRef)).toStrictEqual([ + secondImporterVatId, + ]); + expect( + parseReplyBody( + ( + await kernel.queueMessage(exporterKRef, 'isObjectPresent', [ + objectId, + ]) + ).body, + ), + ).toBe(true); + + expect( + parseReplyBody( + ( + await kernel.queueMessage(secondImporterKRef, 'useImport', [ + objectId, + ]) + ).body, + ), + ).toBe(objectId); + + await kernel.queueMessage(secondImporterKRef, 'makeWeak', [objectId]); + await kernel.queueMessage(secondImporterKRef, 'forgetImport', []); + await waitUntilQuiescent(); + await reapAndSettle(secondImporterVatId, secondImporterKRef); + + expect(kernelStore.getImporters(sharedKRef)).toStrictEqual([]); + // Only the createObject result's stored value still names it + expect(kernelStore.getObjectRefCount(sharedKRef)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }, 60000); + }); }); diff --git a/packages/kernel-test/src/persistence.test.ts b/packages/kernel-test/src/persistence.test.ts index c4dd6ec814..0a97a58fff 100644 --- a/packages/kernel-test/src/persistence.test.ts +++ b/packages/kernel-test/src/persistence.test.ts @@ -170,7 +170,8 @@ describe('persistent storage', { timeout: 20_000 }, () => { // Enqueue a send message into the database kv1.set('queue.run.head', '4'); kv1.set('nextPromiseId', '4'); - kv1.set(`${v1Root}.refCount`, '3,3'); + // The root's pin, plus the send being injected below. + kv1.set(`${v1Root}.refCount`, '2,2'); kv1.set('queue.kp3.head', '1'); kv1.set('queue.kp3.tail', '1'); kv1.set('kp3.state', 'unresolved'); 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/kernel-test/src/utils.ts b/packages/kernel-test/src/utils.ts index c255347b89..7d867f2c28 100644 --- a/packages/kernel-test/src/utils.ts +++ b/packages/kernel-test/src/utils.ts @@ -93,6 +93,9 @@ export async function makeKernel( resetStorage, logger, keySeed, + // Refcount drift is invisible to ordinary assertions until something gets + // collected out from under a live holder, so check it every crank. + auditRefCounts: true, }); return kernel; } diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 588c44df69..5dea4b3c49 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -34,6 +34,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Anywhere in the args tree, a string of the form `@@NAME` (NAME is one or more alphanumeric characters, currently a well-formed kref) is expanded to a `kslot` standin so `kser` encodes it as a real CapData slot in the dispatched message - Purely an RPC-boundary concern: internal callers of `Kernel.queueMessage` are unaffected - 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 on the kernel store — `auditRefCounts`, `recomputeRefCounts`, `formatRefCountViolations`, `assertRefCountsIfAuditing`, `setRefCountAuditing` — plus a `Kernel.make` option `auditRefCounts` that checks every kref's counts against the references the kernel actually holds at the end of each crank, and exports the `RefCountViolation` type ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) + - Reports drift in both directions, but its ground truth is the holder set, so a holder that should have been torn down and wasn't justifies its own count and is invisible to it +- Add `setReachableFlag` and `orphanKernelObject` to the kernel store ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) ### Changed @@ -64,6 +67,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Report the database error when an aborted crank cannot be rolled back, instead of a spurious "no such savepoint" ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) - The abort path recorded the rollback only after it succeeded, so a throwing rollback had the run loop try again against the savepoint `rollbackCrank` had already discarded. The second attempt's "no such savepoint" then became the reported cause of death — and since only `error.message` crosses the wire, the real failure reached neither `getStatus` nor the daemon log - Reject the run loop's promise with the same `Error` its status reports, rather than re-throwing a non-`Error` for the embedder to normalize a second time ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) +- **BREAKING:** Make c-list reference accounting symmetric: creating an import c-list entry now takes a reference, as tearing one down has always released one ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) + - `initKernelObject` births objects at `(0, 0)` instead of `(1, 1)`, and the owner-side baseline decrements in `cleanupTerminatedVat` and `forgetEndpointImports` are gone; the old constant only balanced for a single importer + - Renames `krefsToExistingErefs` to `krefsToErefs`, which throws on an unmapped kref instead of silently dropping it + - A store written under the old accounting carries baselines the new scheme never balances. Call `recomputeRefCounts` once against such a store to rebuild its counts from the references themselves +- Pin vat root objects for the lifetime of their vat, releasing the pin on termination — including when a relaunch fails, which vat cleanup does not do ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Garbage-collection action delivery now moves the kernel's own c-list and orphans an object whose owner has retired it. The action is spent from the durable set before delivery, so the release is made for every kref the endpoint still holds even when the endpoint has gone; a live vat that refuses the delivery has it rolled back and is terminated ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- The run loop carries out a vat restart itself, as a queued request, so a vat is never out of the kernel's reach while cranks run; a crank that landed in that window read a live vat as a dead one ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) + - Adds the `restartVat` run-queue item and `KernelQueue.enqueueRestartVat()`. `Kernel.restartVat` settles when the crank has done it, and rejects outright if the run loop is dead +- A delivery whose endpoint has vanished is dropped only where the endpoint is gone for good — a terminated vat or a remote. `notify` and `bringOutYourDead` no longer take the run loop down when it has, and a `send` no longer reports a live endpoint as unreachable and discards a deliverable message ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) + - Adds `VatManager.provideVat()`, which waits out a vat being torn down before answering, and makes the kernel's endpoint lookup asynchronous +- Reject a `retireExports`/`abandonExports` syscall for an object the calling endpoint does not own ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- `rollbackCrank` now reverts the two pieces of state a database rollback cannot reach: cached stored values, and the garbage-collection candidate set, which is restored to its state at the savepoint rather than emptied — candidates accrued while the run loop was idle are owed a collection an unrelated crank's rollback must not cancel ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Tear down and mark for cleanup a vat whose worker launched but whose kernel-side registration then failed ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Charge a delivered message's target reference against the run-queue item's own target rather than the routed target ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Release a queued notification's reference before the paths that decide there is nothing to deliver ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Transfer, rather than duplicate, the references a message carries when it is queued on an unresolved promise and later re-enqueued on resolution ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Fix the stale `cle.`/`clk.` key prefixes in `getPromisesByDecider` and `deleteEndpoint`, which no longer matched the `${endpointId}.c.` c-list layout. The former matched nothing, so a terminating vat never rejected the promises it was deciding and their subscribers waited forever ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- A restart that cannot relaunch its vat now terminates it and reports the failure to the caller, instead of killing the run loop — which rolled the crank back, undoing the termination records and returning the request to the queue, so every subsequent process start replayed the same failing restart ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Terminating a vat with a restart still queued for it no longer kills the run loop when the crank reaches that request ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Work outliving a vat that has already been cleaned up — a `bringOutYourDead` scheduled before it died, say — is dropped rather than taken as a live vat the kernel has lost track of, which killed the run loop. Cleanup unmarks the vat it finishes, so "terminated" alone could not identify one ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- `getImporters` now counts remotes, so retiring an object queues a `retireImport` for a remote importer rather than deleting the object and leaving the remote's c-list entry naming nothing ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) - Deserialize CapData rejections in `Kernel.queueMessage` so vat errors surface as plain `Error` objects to all callers ([#928](https://github.com/MetaMask/ocap-kernel/pull/928)) - Detect peer restart across receiver state loss so the receiving kernel no longer silently drops a restarted peer's `seq=1` messages ([#948](https://github.com/MetaMask/ocap-kernel/pull/948)) - Persist the peer's last-observed incarnation and compare it on every successful handshake; on a detected restart, clear the peer's c-list contributions and reject the promises it was deciding before the new incarnation reuses any erefs diff --git a/packages/ocap-kernel/src/Kernel.test.ts b/packages/ocap-kernel/src/Kernel.test.ts index 670da38c04..a815725b2a 100644 --- a/packages/ocap-kernel/src/Kernel.test.ts +++ b/packages/ocap-kernel/src/Kernel.test.ts @@ -31,13 +31,23 @@ const mocks = vi.hoisted(() => { #rejectRunLoop: ((error: Error) => void) | undefined; + #deliver: ((item: unknown) => Promise) | undefined; + // Like the real run loop, this settles only if the kernel dies. - run = vi.fn( - async () => - new Promise((_resolve, reject) => { - this.#rejectRunLoop = reject; - }), - ); + run = vi.fn(async (deliver: (item: unknown) => Promise) => { + this.#deliver = deliver; + return new Promise((_resolve, reject) => { + this.#rejectRunLoop = reject; + }); + }); + + // A restart is the run loop's work, so stand in for it reaching the request + // on its next crank. The failure is absorbed here rather than dropped: the + // real run loop would die of it, and the caller hears about it from the + // waiter `restartVat` registered, not from this call. + enqueueRestartVat = vi.fn((vatId: string) => { + this.#deliver?.({ type: 'restartVat', vatId }).catch(() => undefined); + }); /** * Fail the run loop, in the order the real `KernelQueue.run` does: the diff --git a/packages/ocap-kernel/src/Kernel.ts b/packages/ocap-kernel/src/Kernel.ts index a312e60e65..5253f4d4d6 100644 --- a/packages/ocap-kernel/src/Kernel.ts +++ b/packages/ocap-kernel/src/Kernel.ts @@ -109,6 +109,14 @@ export class Kernel { * @param options.ioListenerFactory - Optional factory for creating IO listeners. * @param options.allowedGlobalNames - Optional list of allowed global names for vat endowments. * @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. Not optional instrumentation: it is what + * establishes that the accounting is right, and 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. Note + * that it checks counts against their holders, which is a different invariant + * from the one `collectGarbage`'s `retireExport` branch still cannot assert. */ // eslint-disable-next-line no-restricted-syntax private constructor( @@ -122,6 +130,7 @@ export class Kernel { ioListenerFactory?: IOListenerFactory; allowedGlobalNames?: AllowedGlobalName[]; onRunLoopFailure?: OnRunLoopFailure; + auditRefCounts?: boolean; } = {}, ) { this.#platformServices = platformServices; @@ -129,6 +138,9 @@ export class Kernel { this.#onRunLoopFailure = options.onRunLoopFailure; this.#logger = options.logger ?? new Logger('ocap-kernel'); this.#kernelStore = makeKernelStore(kernelDatabase, this.#logger); + if (options.auditRefCounts) { + this.#kernelStore.setRefCountAuditing(true); + } if (!this.#kernelStore.isInitialized()) { this.#kernelStore.markInitialized(); } @@ -217,6 +229,7 @@ export class Kernel { this.#kernelServiceManager.invokeKernelService.bind( this.#kernelServiceManager, ), + this.#vatManager.performVatRestart.bind(this.#vatManager), this.#logger, ); @@ -249,6 +262,8 @@ export class Kernel { * @param options.systemSubclusters - Optional array of system subcluster configurations. * @param options.allowedGlobalNames - Optional list of allowed global names for vat endowments. When set, only these names from the `VatSupervisor`'s configured endowments (see `createDefaultEndowments`) are available to vats. * @param options.onRunLoopFailure - Optional handler called if the run loop dies. The kernel must be restarted after that, so an embedder that outlives it (e.g. a daemon) should use this to terminate or restart. + * @param options.auditRefCounts - If true, verify reference counts against + * ground truth at the end of each crank and throw on any mismatch. * @returns A promise for the new kernel instance. */ static async make( @@ -263,6 +278,7 @@ export class Kernel { systemSubclusters?: SystemSubclusterConfig[]; allowedGlobalNames?: AllowedGlobalName[]; onRunLoopFailure?: OnRunLoopFailure; + auditRefCounts?: boolean; } = {}, ): Promise { const kernel = new Kernel(platformServices, kernelDatabase, options); @@ -635,13 +651,19 @@ export class Kernel { /** * Gets an endpoint by its ID. * + * Asynchronous because a vat may be mid-teardown: `provideVat` waits that out + * rather than answering from a vat table the store has not caught up with, so + * by the time a caller is told the vat is gone the store says so too — which + * is what lets `#resolveEndpoint` tell a terminated vat from a missing one. A + * restart needs no such window, being carried out by the run loop itself. + * * @param endpointId - The ID of the endpoint to retrieve. - * @returns The endpoint handle for the given ID. + * @returns A promise for the endpoint handle for the given ID. * @throws If the endpoint ID is invalid (neither a vat ID nor a remote ID). */ - #getEndpoint(endpointId: EndpointId): EndpointHandle { + async #getEndpoint(endpointId: EndpointId): Promise { if (isVatId(endpointId)) { - return this.#vatManager.getVat(endpointId); + return await this.#vatManager.provideVat(endpointId); } if (isRemoteId(endpointId)) { return this.#remoteManager.getRemote(endpointId); diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index d7beb0e2f1..97adcb0ac4 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -44,6 +44,7 @@ describe('KernelQueue', () => { kernelStore = { nextTerminatedVatCleanup: vi.fn(), collectGarbage: vi.fn(), + assertRefCountsIfAuditing: vi.fn(), runQueueLength: vi.fn(), dequeueRun: vi.fn(), enqueueRun: vi.fn(), @@ -652,10 +653,6 @@ describe('KernelQueue', () => { reject: rejectHandler, }); kernelQueue.resolvePromises(endpointId, [resolution], false); - expect(kernelStore.incrementRefCount).toHaveBeenCalledWith( - kpid, - 'resolve|kpid', - ); expect(kernelStore.incrementRefCount).toHaveBeenCalledWith( 'ko1', 'resolve|slot', @@ -709,10 +706,6 @@ describe('KernelQueue', () => { const insistEndpointIdSpy = vi.spyOn(types, 'insistEndpointId'); kernelQueue.resolvePromises(undefined, [resolution], false); expect(insistEndpointIdSpy).not.toHaveBeenCalled(); - expect(kernelStore.incrementRefCount).toHaveBeenCalledWith( - kpid, - 'resolve|kpid', - ); expect(kernelStore.incrementRefCount).toHaveBeenCalledWith( 'ko1', 'resolve|slot', @@ -1002,6 +995,27 @@ describe('KernelQueue', () => { }); }); + describe('enqueueRestartVat', () => { + it('enqueues the request for the run loop to carry out', () => { + kernelQueue.enqueueRestartVat('v1'); + + expect(kernelStore.enqueueRun).toHaveBeenCalledWith({ + type: 'restartVat', + vatId: 'v1', + }); + }); + + it('refuses once the run loop has died', async () => { + await killRunLoop(new Error('boom')); + + // The restart is the loop's work, so a dead loop will never do it and the + // caller would wait forever. + expect(() => kernelQueue.enqueueRestartVat('v1')).toThrow( + 'Kernel run loop died; cannot restart a vat', + ); + }); + }); + describe('waitForCrank', () => { it('handles when waitForCrank returns a delayed promise', async () => { let resolvePromise: ((value: void) => void) | undefined; diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index 3465e93cde..e3a328ff50 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -344,6 +344,7 @@ export class KernelQueue { await this.#terminateVat(vatId, info); } this.#kernelStore.collectGarbage(); + this.#kernelStore.assertRefCountsIfAuditing(); } /** @@ -457,6 +458,23 @@ export class KernelQueue { } } + /** + * Enqueue a request to replace a vat's worker. + * + * The work itself belongs to the run loop, which is the point: a restart done + * where it is asked for takes the vat out of the kernel's reach while cranks + * continue, and a crank that lands in that window reads a live vat as a dead + * one. Queued, the restart happens in a crank of its own. + * + * @param vatId - The vat whose worker is to be replaced. + */ + enqueueRestartVat(vatId: VatId): void { + // The restart is the run loop's work now, so a dead loop will never do it, + // and a caller awaiting it would wait forever. + this.assertRunLoopAlive('restart a vat'); + this.#enqueueRun({ type: 'restartVat', vatId }); + } + /** * Enqueue a notification of promise resolution to an endpoint. * @@ -504,7 +522,6 @@ export class KernelQueue { for (const resolution of resolutions) { const [kpid, rejected, data] = resolution; - this.#kernelStore.incrementRefCount(kpid, 'resolve|kpid'); for (const slot of data.slots || []) { this.#kernelStore.incrementRefCount(slot, 'resolve|slot'); } diff --git a/packages/ocap-kernel/src/KernelRouter.test.ts b/packages/ocap-kernel/src/KernelRouter.test.ts index ea833293a6..0847f0da58 100644 --- a/packages/ocap-kernel/src/KernelRouter.test.ts +++ b/packages/ocap-kernel/src/KernelRouter.test.ts @@ -12,6 +12,7 @@ import type { RunQueueItemGCAction, RunQueueItemBringOutYourDead, EndpointId, + VatId, GCRunQueueType, CrankResult, EndpointHandle, @@ -21,8 +22,11 @@ describe('KernelRouter', () => { // Mock dependencies let kernelStore: KernelStore; let kernelQueue: KernelQueue; - let getEndpoint: (endpointId: EndpointId) => EndpointHandle; + let getEndpoint: ( + endpointId: EndpointId, + ) => EndpointHandle | Promise; let endpointHandle: EndpointHandle; + let restartVat: MockInstance<(vatId: VatId) => Promise>; let kernelRouter: KernelRouter; beforeEach(() => { @@ -59,9 +63,16 @@ describe('KernelRouter', () => { krefToEref: vi.fn() as unknown as MockInstance, getKpidsToRetire: vi.fn().mockReturnValue([]), translateCapDataKtoE: vi.fn(), - krefsToExistingErefs: vi.fn((_endpointId: string, krefs: string[]) => + krefsToErefs: vi.fn((_endpointId: string, krefs: string[]) => krefs.map((kref: string) => `translated-${kref}`), ) as unknown as MockInstance, + clearReachableFlag: vi.fn(), + deleteCListEntry: vi.fn(), + forgetKref: vi.fn(), + orphanKernelObject: vi.fn(), + hasCListEntry: vi.fn().mockReturnValue(true), + isVatTerminated: vi.fn().mockReturnValue(false), + isVatActive: vi.fn().mockReturnValue(true), createCrankSavepoint: vi.fn(), } as unknown as KernelStore; @@ -71,6 +82,7 @@ describe('KernelRouter', () => { } as unknown as KernelQueue; const mockInvokeKernelService = vi.fn(); + restartVat = vi.fn().mockResolvedValue(undefined); // Create the router to test kernelRouter = new KernelRouter( @@ -78,6 +90,7 @@ describe('KernelRouter', () => { kernelQueue, getEndpoint, mockInvokeKernelService, + restartVat, ); }); @@ -283,8 +296,109 @@ describe('KernelRouter', () => { expect(endpointHandle.deliverMessage).not.toHaveBeenCalled(); expect(result).toBeUndefined(); - // Verify that no refcount decrementation happened since we're requeuing - expect(kernelStore.decrementRefCount).not.toHaveBeenCalled(); + expect(kernelStore.decrementRefCount).toHaveBeenCalledWith( + target, + 'requeue|target', + ); + }); + + it('hands over every reference a requeued message carries', async () => { + const target = 'kp123'; + ( + kernelStore.getKernelPromise as unknown as MockInstance + ).mockReturnValueOnce({ state: 'unresolved' }); + const message: KernelMessage = { + methargs: { body: 'method args', slots: ['ko1', 'ko2'] }, + result: 'kp9', + }; + await kernelRouter.deliver({ type: 'send', target, message }); + + expect(kernelStore.enqueuePromiseMessage).toHaveBeenCalledWith( + target, + message, + ); + expect( + (kernelStore.decrementRefCount as unknown as MockInstance).mock.calls, + ).toStrictEqual([ + [target, 'requeue|target'], + ['kp9', 'requeue|result'], + ['ko1', 'requeue|slot'], + ['ko2', 'requeue|slot'], + ]); + }); + + 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', + ); + }); + + // The same distinction, on the path that discovers the endpoint is gone + // only after routing has already succeeded. Every other test of this + // branch aims at a plain object, where the item's target and the routed + // target are the same kref and the two spellings are indistinguishable. + it('charges the promise, not the object it resolved to, when the endpoint is gone', 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'); + ( + kernelStore.isVatTerminated as unknown as MockInstance + ).mockReturnValue(true); + (getEndpoint as unknown as MockInstance).mockRejectedValueOnce( + new Error('vat v1 not found'), + ); + + await kernelRouter.deliver({ + type: 'send', + target: promiseId, + message: { + methargs: { body: 'method args', slots: [] }, + result: null, + }, + }); + + expect(kernelStore.decrementRefCount).toHaveBeenCalledWith( + promiseId, + 'deliver|splat|target', + ); + // Charging this instead leaks the promise and collects an object that + // nobody released. + expect(kernelStore.decrementRefCount).not.toHaveBeenCalledWith( + resolvedObject, + 'deliver|splat|target', + ); }); it('splats message when promise resolves to a non-object', async () => { @@ -378,13 +492,41 @@ describe('KernelRouter', () => { ); }); + it('propagates a lookup failure for a vat that is absent but not terminated', async () => { + // Not a splat: reporting a live endpoint as unreachable would discard a + // deliverable message and reject its result for no reason. + (kernelStore.getOwner as unknown as MockInstance).mockReturnValueOnce( + 'v1', + ); + (getEndpoint as unknown as MockInstance).mockImplementationOnce(() => { + throw new Error('vat v1 not found'); + }); + + await expect( + kernelRouter.deliver({ + type: 'send', + target: 'ko123', + message: { + methargs: { body: 'method args', slots: [] }, + result: 'kp1', + } as unknown as SwingsetMessage, + }), + ).rejects.toThrow('vat v1 not found'); + + expect(kernelQueue.resolvePromises).not.toHaveBeenCalled(); + }); + it('splats message with ENDPOINT_UNREACHABLE when endpoint vanishes', async () => { const endpointId = 'v1'; const target = 'ko123'; (kernelStore.getOwner as unknown as MockInstance).mockReturnValueOnce( endpointId, ); - // getEndpoint throws (endpoint gone) + // The endpoint is gone for good, which is what makes it a splat rather + // than an error worth propagating. + ( + kernelStore.isVatTerminated as unknown as MockInstance + ).mockReturnValue(true); (getEndpoint as unknown as MockInstance).mockImplementationOnce(() => { throw new Error('vat not found'); }); @@ -457,6 +599,39 @@ describe('KernelRouter', () => { }); describe('notify', () => { + it('drops a notify whose endpoint is gone for good', async () => { + // Reachable while a vat is being torn down: `provideVat` waits for the + // teardown, then reports the vat gone. Without this the rejection escapes + // the crank and kills the run loop. + ( + kernelStore.getKernelPromise as unknown as MockInstance + ).mockReturnValueOnce({ + state: 'fulfilled', + value: { body: JSON.stringify({ value: 'v' }), slots: [] }, + }); + (kernelStore.krefToEref as unknown as MockInstance).mockReturnValueOnce( + 'p+123', + ); + ( + kernelStore.isVatTerminated as unknown as MockInstance + ).mockReturnValue(true); + (getEndpoint as unknown as MockInstance).mockRejectedValueOnce( + new Error('vat v1 not found'), + ); + + const result = await kernelRouter.deliver({ + type: 'notify', + endpointId: 'v1', + kpid: 'kp123', + }); + + expect(result).toStrictEqual({ didDelivery: 'v1' }); + expect(endpointHandle.deliverNotify).not.toHaveBeenCalled(); + // Resolved before the translation, which would otherwise mint c-list + // entries for an endpoint that cannot be told about them. + expect(kernelStore.translateRefKtoE).not.toHaveBeenCalled(); + }); + it('delivers a notify to a vat and returns crank results', async () => { const endpointId = 'v1'; const kpid = 'kp123'; @@ -550,6 +725,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 () => { @@ -588,6 +769,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 () => { @@ -649,9 +834,282 @@ describe('KernelRouter', () => { expect(result).toStrictEqual(mockCrankResult); }, ); + + it('clears the reachable flag when delivering dropExports', async () => { + await kernelRouter.deliver({ + type: 'dropExports', + endpointId: 'v1', + krefs: ['ko1', 'ko2'], + }); + + expect( + (kernelStore.clearReachableFlag as unknown as MockInstance).mock + .calls, + ).toStrictEqual([ + ['v1', 'ko1'], + ['v1', 'ko2'], + ]); + expect(kernelStore.deleteCListEntry).not.toHaveBeenCalled(); + }); + + it.each(['retireExports', 'retireImports'] as const)( + 'tears down the c-list entry when delivering %s', + async (actionType) => { + await kernelRouter.deliver({ + type: actionType, + endpointId: 'v1', + krefs: ['ko1', 'ko2'], + }); + + expect( + (kernelStore.deleteCListEntry as unknown as MockInstance).mock + .calls, + ).toStrictEqual([ + ['v1', 'ko1', 'translated-ko1'], + ['v1', 'ko2', 'translated-ko2'], + ]); + }, + ); + + 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('waits for a vat that is coming back, then delivers to it', async () => { + // The restart window: `provideVat` answers once the new incarnation is + // up, so the crank waits instead of resolving a live vat as a dead one. + let finishRestart!: (handle: EndpointHandle) => void; + (getEndpoint as unknown as MockInstance).mockReturnValueOnce( + new Promise((resolve) => { + finishRestart = resolve; + }), + ); + + const delivered = kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'v1', + krefs: ['ko1'], + }); + + // Nothing is released ahead of knowing where the action is going. + expect(kernelStore.deleteCListEntry).not.toHaveBeenCalled(); + + finishRestart(endpointHandle); + await delivered; + + expect(endpointHandle.deliverRetireImports).toHaveBeenCalledWith([ + 'translated-ko1', + ]); + expect(kernelStore.deleteCListEntry).toHaveBeenCalledWith( + 'v1', + 'ko1', + 'translated-ko1', + ); + }); + + 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', () => { + it('skips a reap whose endpoint is gone for good', async () => { + ( + kernelStore.isVatTerminated as unknown as MockInstance + ).mockReturnValue(true); + (getEndpoint as unknown as MockInstance).mockRejectedValueOnce( + new Error('vat v1 not found'), + ); + + const result = await kernelRouter.deliver({ + type: 'bringOutYourDead', + endpointId: 'v1', + }); + + // A reap only asks an endpoint to tidy up, so one that is gone has + // nothing left to ask — and nothing was delivered. + expect(result).toBeUndefined(); + expect(endpointHandle.deliverBringOutYourDead).not.toHaveBeenCalled(); + }); + + // Nothing purges the reap queue when a vat dies, and cleanup ends by + // *unmarking* the vat it finished — so a reap scheduled before the vat + // died arrives at an endpoint that is neither present nor terminated. + // Read as a disagreement, that throw kills the run loop. + it('skips a reap for a vat that has already been cleaned up', async () => { + ( + kernelStore.isVatTerminated as unknown as MockInstance + ).mockReturnValue(false); + (kernelStore.isVatActive as unknown as MockInstance).mockReturnValue( + false, + ); + (getEndpoint as unknown as MockInstance).mockRejectedValueOnce( + new Error('vat v1 not found'), + ); + + const result = await kernelRouter.deliver({ + type: 'bringOutYourDead', + endpointId: 'v1', + }); + + expect(result).toBeUndefined(); + expect(endpointHandle.deliverBringOutYourDead).not.toHaveBeenCalled(); + }); + it('delivers bringOutYourDead to a vat and returns crank results', async () => { const endpointId = 'v1'; const bringOutYourDeadItem: RunQueueItemBringOutYourDead = { @@ -675,6 +1133,30 @@ describe('KernelRouter', () => { }); }); + describe('restartVat', () => { + it('carries out a queued restart and reports no delivery', async () => { + // Not a delivery: nothing was handed to the vat, and the incarnation that + // comes back has taken none yet. + const result = await kernelRouter.deliver({ + type: 'restartVat', + vatId: 'v1', + }); + + expect(restartVat).toHaveBeenCalledWith('v1'); + expect(result).toBeUndefined(); + }); + + it('lets a failed restart take the crank down', async () => { + // Aborting would undo the terminated mark that makes the half-restarted + // vat's c-list reclaimable. + restartVat.mockRejectedValueOnce(new Error('worker died')); + + await expect( + kernelRouter.deliver({ type: 'restartVat', vatId: 'v1' }), + ).rejects.toThrow('worker died'); + }); + }); + it('throws on unknown run queue item type', async () => { // @ts-expect-error - deliberately using an invalid type const invalidItem: RunQueueItem = { type: 'invalid' }; diff --git a/packages/ocap-kernel/src/KernelRouter.ts b/packages/ocap-kernel/src/KernelRouter.ts index 5cfb8335d4..5822dd811d 100644 --- a/packages/ocap-kernel/src/KernelRouter.ts +++ b/packages/ocap-kernel/src/KernelRouter.ts @@ -3,14 +3,19 @@ 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'; import { isPromiseRef } from './store/utils/promise-ref.ts'; import type { + VatId, EndpointId, EndpointHandle, + ERef, KRef, KernelMessage, RunQueueItem, @@ -18,8 +23,10 @@ import type { RunQueueItemBringOutYourDead, RunQueueItemNotify, RunQueueItemGCAction, + RunQueueItemRestartVat, CrankResult, } from './types.ts'; +import { isVatId } from './types.ts'; import { assert, Fail } from './utils/assert.ts'; type MessageRoute = { @@ -41,11 +48,17 @@ export class KernelRouter { readonly #kernelQueue: KernelQueue; /** A function that returns an endpoint handle for a given endpoint id. */ - readonly #getEndpoint: (endpointId: EndpointId) => EndpointHandle; + readonly #getEndpoint: (endpointId: EndpointId) => Promise; /** A function that invokes a method on a kernel service. */ readonly #invokeKernelService: (target: KRef, message: KernelMessage) => void; + /** + * A function that replaces a vat's worker, for the crank that carries out a + * queued restart request. + */ + readonly #restartVat: (vatId: VatId) => Promise; + /** The logger, if any. */ readonly #logger: Logger | undefined; @@ -56,19 +69,22 @@ export class KernelRouter { * @param kernelQueue - The kernel's queue. * @param getEndpoint - A function that returns an endpoint handle for a given endpoint id. * @param invokeKernelService - A function that calls a method on a kernel service object. + * @param restartVat - A function that replaces a vat's worker. * @param logger - The logger. If not provided, no logging will be done. */ constructor( kernelStore: KernelStore, kernelQueue: KernelQueue, - getEndpoint: (endpointId: EndpointId) => EndpointHandle, + getEndpoint: (endpointId: EndpointId) => Promise, invokeKernelService: (target: KRef, message: KernelMessage) => void, + restartVat: (vatId: VatId) => Promise, logger?: Logger, ) { this.#kernelStore = kernelStore; this.#kernelQueue = kernelQueue; this.#getEndpoint = getEndpoint; this.#invokeKernelService = invokeKernelService; + this.#restartVat = restartVat; this.#logger = logger; } @@ -102,6 +118,8 @@ export class KernelRouter { return await this.#deliverGCAction(item); case 'bringOutYourDead': return await this.#deliverBringOutYourDead(item); + case 'restartVat': + return await this.#restartVatWorker(item); default: // @ts-expect-error Runtime does not respect "never". Fail`unsupported or unknown run queue item type ${item.type}`; @@ -230,14 +248,15 @@ export class KernelRouter { const isKernelServiceMessage = endpointId === 'kernel'; let endpoint: EndpointHandle | null = null; if (!isKernelServiceMessage) { - try { - endpoint = this.#getEndpoint(endpointId); - } catch { - // TODO: Narrow this catch to the expected error type (e.g., - // VatNotFoundError) so that unexpected errors are not silently - // swallowed and deliverable messages are not incorrectly discarded. - // Endpoint vanished (e.g., vat terminated but ownership entries not - // yet cleaned up). Treat the same as a splat. + // An endpoint that is gone for good — a terminated vat whose ownership + // entries are not cleaned up yet, or a disconnected remote — has nothing + // to deliver to, so the message goes splat. Anything else `resolveEndpoint` + // propagates, rather than reporting a live endpoint as unreachable and + // discarding a deliverable message. + endpoint = + (await this.#resolveEndpoint(endpointId, `send of ${target}`)) ?? + null; + if (!endpoint) { if (message.result) { const promise = this.#kernelStore.getKernelPromise(message.result); this.#kernelQueue.resolvePromises(promise.decider, [ @@ -255,7 +274,10 @@ export class KernelRouter { 'deliver|splat|result', ); } - this.#kernelStore.decrementRefCount(target, 'deliver|splat|target'); + this.#kernelStore.decrementRefCount( + item.target, + 'deliver|splat|target', + ); for (const slot of message.methargs.slots) { this.#kernelStore.decrementRefCount(slot, 'deliver|splat|slot'); } @@ -314,12 +336,24 @@ export class KernelRouter { } else { Fail`no owner for kernel object ${target}`; } - this.#kernelStore.decrementRefCount(target, 'deliver|send|target'); + // `item.target`, not the routed `target`: a message aimed at a promise + // is charged against the promise, and routing may have resolved it to a + // different object. + this.#kernelStore.decrementRefCount(item.target, 'deliver|send|target'); for (const slot of message.methargs.slots) { this.#kernelStore.decrementRefCount(slot, 'deliver|send|slot'); } } else { + // The references move from this run queue item to the promise's queue + // entry. New holder first, so nothing transiently looks unreferenced. this.#kernelStore.enqueuePromiseMessage(target, message); + this.#kernelStore.decrementRefCount(item.target, 'requeue|target'); + if (message.result) { + this.#kernelStore.decrementRefCount(message.result, 'requeue|result'); + } + for (const slot of message.methargs.slots) { + this.#kernelStore.decrementRefCount(slot, 'requeue|slot'); + } } return crankResult; @@ -362,10 +396,22 @@ export class KernelRouter { if (state === 'unresolved') { Fail`notification on unresolved promise ${kpid}`; } + // Release the queued notification's reference up front, so the paths that + // decide there is nothing to deliver don't leak it. + this.#kernelStore.decrementRefCount(kpid, 'deliver|notify'); if (!this.#kernelStore.krefToEref(endpointId, kpid)) { // no c-list entry, already done return { didDelivery: endpointId }; } + // Ahead of the translation below, which would otherwise mint c-list entries + // for an endpoint with no way to hear about them. + const endpoint = await this.#resolveEndpoint( + endpointId, + `notify of ${kpid}`, + ); + if (!endpoint) { + return { didDelivery: endpointId }; + } const targets = this.#kernelStore.getKpidsToRetire(kpid, value); if (targets.length === 0) { // no kpids to retire, already done @@ -385,16 +431,58 @@ export class KernelRouter { tPromise.state === 'rejected', this.#kernelStore.translateCapDataKtoE(endpointId, tPromise.value), ]); - // decrement refcount for the promise being notified - if (toResolve !== kpid) { - this.#kernelStore.decrementRefCount(toResolve, 'deliver|notify|slot'); + } + // 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. + return await endpoint.deliverNotify(resolutions); + } + + /** + * The handle for an endpoint, or `undefined` if the endpoint is gone for good + * and the work addressed to it can be dropped. + * + * Gone for good means a vat the store has no live record of — marked + * terminated, and so awaiting a cleanup that takes its whole c-list with it, + * or already cleaned up — or a remote, which reconciles on its next + * incarnation. Both halves are needed: cleanup ends with `forgetTerminatedVat`, + * so a vat that is long gone is no longer *marked* terminated either, and work + * outliving it (a `bringOutYourDead` scheduled before it died, say) would + * otherwise be read as a disagreement. + * + * A vat the store still calls active but the kernel has no handle for is that + * disagreement: `restartVat` is carried out by the run loop and `terminateVat` + * records the vat as in flux, so neither leaves a vat in that state, and the + * caller is better served by the error than by an answer that says "gone" + * about a vat that isn't. + * + * @param endpointId - The endpoint to resolve. + * @param what - What was being delivered, for the log. + * @returns The endpoint handle, or undefined if it will not be back. + */ + async #resolveEndpoint( + endpointId: EndpointId, + what: string, + ): Promise { + try { + return await this.#getEndpoint(endpointId); + } catch (error) { + if ( + isVatId(endpointId) && + this.#kernelStore.isVatActive(endpointId) && + !this.#kernelStore.isVatTerminated(endpointId) + ) { + throw error; } + this.#logger?.error( + `Endpoint ${endpointId} vanished before ${what}:`, + error, + ); + return undefined; } - const endpoint = this.#getEndpoint(endpointId); - const crankResult = await endpoint.deliverNotify(resolutions); - // Decrement reference count for processed 'notify' item - this.#kernelStore.decrementRefCount(kpid, 'deliver|notify'); - return crankResult; } /** @@ -408,15 +496,120 @@ export class KernelRouter { this.#logger?.log( `@@@@ deliver ${endpointId} ${type} ${JSON.stringify(krefs)}`, ); - const endpoint = this.#getEndpoint(endpointId); - const erefs = this.#kernelStore.krefsToExistingErefs(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 stillHeld = (): KRef[] => + krefs.filter((kref) => this.#kernelStore.hasCListEntry(endpointId, kref)); + if (stillHeld().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. An endpoint that is gone for good still gets the release: + // the action is already spent from the durable set, and for a terminated vat + // cleanup would take the entries anyway. + // + // The throw `#resolveEndpoint` reserves for a vat that is absent without + // being terminated is, here, the least bad of three. Committing the release + // corrupts silently — the vat's own tables still name every one of these + // krefs, which is the disagreement the failed delivery below rolls back to + // avoid. Aborting spins: it does keep the action, since `rollbackCrank` + // restores the cached GC set, but nothing about the vat changes between + // cranks, so the same action is re-selected and re-aborted with no delivery + // to wait on — a run loop that is dead without saying so. + const endpoint = await this.#resolveEndpoint( + endpointId, + `${type}; releasing the kernel's side anyway`, + ); + // Re-read after the await, not before it: resolving an endpoint yields to + // other work, and a remote's incarnation change tears its c-list down + // without waiting for the crank. Reusing the earlier answer would hand + // `krefsToErefs` a kref whose entry has since gone, and it throws rather + // than returning short — killing the run loop over an entry that is + // already, correctly, released. + const live = stillHeld(); + 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 }; + } + 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. + live.forEach((kref, index) => { + if (type === 'dropExports') { + this.#kernelStore.clearReachableFlag(endpointId, kref); + 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 c-list entries, and the action too — but only + // because `rollbackCrank` re-provides the cached GC action set, not as a + // property of the database rollback. Terminating the vat is what then + // drains the restored action: `shouldProcessAction` keeps it only while + // the vat has a c-list entry for the kref, which cleanup removes. + 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)}`, + ), + }, + }; + } } /** @@ -430,8 +623,37 @@ export class KernelRouter { ): Promise { const { endpointId } = item; this.#logger?.log(`@@@@ deliver ${endpointId} bringOutYourDead`); - const endpoint = this.#getEndpoint(endpointId); - const crankResult = await endpoint.deliverBringOutYourDead(); - return crankResult; + const endpoint = await this.#resolveEndpoint( + endpointId, + 'bringOutYourDead', + ); + if (!endpoint) { + // A reap only asks an endpoint to tidy up, so one that is gone has nothing + // left to ask. No `didDelivery`, since nothing was delivered. + return undefined; + } + return await endpoint.deliverBringOutYourDead(); + } + + /** + * Carry out a queued request to replace a vat's worker. + * + * Not a delivery, so no `didDelivery`: nothing was handed to the vat, and the + * incarnation that comes back has taken no deliveries yet. + * + * `performVatRestart` reports a failed restart by terminating the vat rather + * than by throwing, so this commits either way. Neither ending a crank is open + * to it: aborting and throwing both roll the crank back, which would undo the + * termination records *and* put this request back on the run queue, leaving + * the same failing restart to be replayed for the life of the store. + * + * @param item - The restart request. + * @returns Nothing; the crank has no outcome to report. + */ + async #restartVatWorker( + item: RunQueueItemRestartVat, + ): Promise { + await this.#restartVat(item.vatId); + return undefined; } } diff --git a/packages/ocap-kernel/src/KernelServiceManager.ts b/packages/ocap-kernel/src/KernelServiceManager.ts index bc236e6113..39ca40883c 100644 --- a/packages/ocap-kernel/src/KernelServiceManager.ts +++ b/packages/ocap-kernel/src/KernelServiceManager.ts @@ -161,12 +161,11 @@ export class KernelServiceManager { * socket connection, say — so any that survived a restart are garbage, and * harmful if left: still pinned, so they accumulate with every restart. * - * Note what this does *not* guarantee. The kernel object is deleted only - * once nothing references it, which with the current `(1, 1)` refcount - * baseline (see #1006) is never; a survivor therefore keeps its `'kernel'` - * owner, and a delivery to it still routes to `invokeKernelService`. That - * case is made survivable there, by rejecting the caller's promise rather - * than throwing, and not here. + * Note what this does *not* guarantee. `collectGarbage` skips objects the + * kernel itself owns, so unpinning one does not delete it; a survivor keeps + * its `'kernel'` owner, and a delivery to it still routes to + * `invokeKernelService`. That case is made survivable there, by rejecting the + * caller's promise rather than throwing, and not here. * * Runs before the run queue starts, so the unpinning is complete before * anything can address one of these krefs. 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/remotes/kernel/RemoteHandle.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts index c14539bc6c..009834a65b 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts @@ -536,7 +536,8 @@ describe('RemoteHandle', () => { for (const kref of krefs) { const { isPromise } = parseRef(kref); if (isPromise) { - expect(mockKernelStore.getRefCount(kref)).toBe(1); + // 1 for the unsettled promise, 1 for the remote's c-list entry + expect(mockKernelStore.getRefCount(kref)).toBe(2); } else { expect(mockKernelStore.getObjectRefCount(kref)).toStrictEqual({ reachable: 1, @@ -557,7 +558,7 @@ describe('RemoteHandle', () => { for (const kref of krefs) { const { isPromise } = parseRef(kref); if (isPromise) { - expect(mockKernelStore.getRefCount(kref)).toBe(1); + expect(mockKernelStore.getRefCount(kref)).toBe(2); } else { expect(mockKernelStore.getObjectRefCount(kref)).toStrictEqual({ reachable: 0, @@ -623,7 +624,6 @@ describe('RemoteHandle', () => { // As if we're no longer using it (which, in fact, we weren't), which is a // prequisite for a valid 'retireImports' delivery - mockKernelStore.decrementRefCount(koref, 'test'); mockKernelStore.clearReachableFlag(remote.remoteId, koref); // Now have the "other end" retire the import (include seq for incoming message) diff --git a/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts index 34e9e91502..29ca0b2da7 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts @@ -23,16 +23,13 @@ describe('RemoteManager', () => { let remoteManager: RemoteManager; let mockPlatformServices: PlatformServices; let kernelStore: ReturnType; - let kernelKVStore: ReturnType['kernelKVStore']; let mockKernelQueue: KernelQueue; let logger: Logger; let mockRemoteComms: RemoteComms; let mockFactory: ReturnType; beforeEach(() => { - const kernelDatabase = makeMapKernelDatabase(); - kernelKVStore = kernelDatabase.kernelKVStore; - kernelStore = makeKernelStore(kernelDatabase); + kernelStore = makeKernelStore(makeMapKernelDatabase()); logger = new Logger('test'); mockFactory = createMockRemotesFactory({ @@ -776,7 +773,7 @@ describe('RemoteManager', () => { // Set up a promise where the remote is the decider const [kpid] = kernelStore.initKernelPromise(); kernelStore.setPromiseDecider(kpid, remoteId); - kernelKVStore.set(`cle.${remoteId}.p+1`, kpid); + kernelStore.addCListEntry(remoteId, kpid, 'rp+1'); const resolvePromisesSpy = vi.spyOn(mockKernelQueue, 'resolvePromises'); @@ -882,7 +879,7 @@ describe('RemoteManager', () => { const [kpid] = kernelStore.initKernelPromise(); kernelStore.setPromiseDecider(kpid, remoteId); - kernelKVStore.set(`cle.${remoteId}.p+1`, kpid); + kernelStore.addCListEntry(remoteId, kpid, 'rp+1'); kernelStore.setPeerIncarnation(peerId, 'incarnation-A'); const resolvePromisesSpy = vi.spyOn(mockKernelQueue, 'resolvePromises'); diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 58fefc80c3..6f2dff9ead 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -47,6 +47,8 @@ describe('kernel store', () => { 'addSubcluster', 'addSubclusterVat', 'allocateErefForKref', + 'assertRefCountsIfAuditing', + 'auditRefCounts', 'bufferCrankOutput', 'cleanupOrphanMessages', 'cleanupTerminatedVat', @@ -84,6 +86,7 @@ describe('kernel store', () => { 'forgetEref', 'forgetKref', 'forgetTerminatedVat', + 'formatRefCountViolations', 'getAllRemoteRecords', 'getAllSystemSubclusterMappings', 'getAllVatRecords', @@ -112,6 +115,7 @@ describe('kernel store', () => { 'getRelayEntries', 'getRemoteIdentityValue', 'getRemoteIdentityValueRequired', + 'getRemoteIds', 'getRemoteInfo', 'getRemoteSeqState', 'getRootObject', @@ -140,14 +144,16 @@ describe('kernel store', () => { 'isVatTerminated', 'kernelRefExists', 'krefToEref', - 'krefsToExistingErefs', + 'krefsToErefs', 'makeVatStore', 'markInitialized', 'markVatAsTerminated', 'nextReapAction', 'nextTerminatedVatCleanup', + 'orphanKernelObject', 'pinObject', 'provideIncarnationId', + 'recomputeRefCounts', 'recordLastActiveTime', 'releaseAllSavepoints', 'releaseSavepoint', @@ -167,6 +173,8 @@ describe('kernel store', () => { 'setPeerIncarnation', 'setPendingMessage', 'setPromiseDecider', + 'setReachableFlag', + 'setRefCountAuditing', 'setRelayEntries', 'setRemoteHighestReceivedSeq', 'setRemoteIdentityValue', @@ -206,31 +214,31 @@ describe('kernel store', () => { const ko2Owner = 'r23'; expect(ks.initKernelObject(ko1Owner)).toBe('ko1'); - // Check that the object is initialized with reachable=1, recognizable=1 - const refCounts = ks.getObjectRefCount('ko1'); - expect(refCounts.reachable).toBe(1); - expect(refCounts.recognizable).toBe(1); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); // Increment the reference count ks.incrementRefCount('ko1', 'test'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(2); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(2); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); // Increment again ks.incrementRefCount('ko1', 'test'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(3); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(3); - - // Decrement - ks.decrementRefCount('ko1', 'tess'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(2); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(2); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 2, + recognizable: 2, + }); - // Decrement twice more to reach 0 ks.decrementRefCount('ko1', 'test'); ks.decrementRefCount('ko1', 'test'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(0); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(0); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); // Create another object expect(ks.initKernelObject(ko2Owner)).toBe('ko2'); diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 5c8f49fc3d..15d59ee50f 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -38,9 +38,10 @@ * ${kpid}.decider = ${endid} // who decides on settlement * ${kpid}.value = JSON(CAPDATA) // value settled to, if settled * - * C-lists - * cle.${endid}.${eref} = ${kref} // ERef->KRef mapping - * clk.${endid}.${kref} = ${eref} // KRef->ERef mapping + * C-lists (both directions share one prefix; see `getCListPrefix`) + * ${endid}.c.${eref} = ${kref} // ERef->KRef mapping + * ${endid}.c.${kref} = R|_ ${eref} // KRef->ERef mapping, plus the + * // endpoint's reachable flag * * Vat bookkeeping * e.nextObjectId.${endid} = NN // allocation counter for imported object ERefs @@ -79,6 +80,7 @@ import { getPinMethods } from './methods/pinned.ts'; import { getPromiseMethods } from './methods/promise.ts'; import { getQueueMethods } from './methods/queue.ts'; import { getReachableMethods } from './methods/reachable.ts'; +import { getRefCountAuditMethods } from './methods/refcount-audit.ts'; import { getRefCountMethods } from './methods/refcount.ts'; import { getRelayMethods } from './methods/relay.ts'; import { getRemoteMethods } from './methods/remote.ts'; @@ -86,7 +88,7 @@ import { getRevocationMethods } from './methods/revocation.ts'; import { getSubclusterMethods } from './methods/subclusters.ts'; import { getTranslators } from './methods/translators.ts'; import { getVatMethods } from './methods/vat.ts'; -import type { StoreContext } from './types.ts'; +import type { StoreContext, StoredValue } from './types.ts'; /** * Create a new KernelStore object wrapped around a raw kernel database. The @@ -112,6 +114,48 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { const { provideCachedStoredValue, provideStoredQueue } = getBaseMethods(kv); + /** + * Every cached stored value the context holds, as `field: [key, initial]`. + * Declared once so that initialization and `refreshCachedValues` cannot + * disagree about which values exist: adding one here does both. + */ + const CACHED_VALUES = { + /** Counter for allocating kernel object IDs */ + nextObjectId: ['nextObjectId', '1'], + /** Counter for allocating kernel promise IDs */ + nextPromiseId: ['nextPromiseId', '1'], + /** Counter for allocating VatIDs */ + nextVatId: ['nextVatId', '1'], + /** Counter for allocating RemoteIDs */ + nextRemoteId: ['nextRemoteId', '1'], + // Garbage collection + gcActions: ['gcActions', '[]'], + reapQueue: ['reapQueue', '[]'], + terminatedVats: ['vats.terminated', '[]'], + // Subclusters + subclusters: ['subclusters', '[]'], + nextSubclusterId: ['nextSubclusterId', '1'], + vatToSubclusterMap: ['vatToSubclusterMap', '{}'], + } as const satisfies Record; + + /** + * Provide a fresh stored value for each of {@link CACHED_VALUES}, reading its + * current setting out of the database. + * + * @returns The stored values, keyed by the context field that holds each. + */ + function provideCachedValues(): Record< + keyof typeof CACHED_VALUES, + StoredValue + > { + return Object.fromEntries( + Object.entries(CACHED_VALUES).map(([field, [key, init]]) => [ + field, + provideCachedStoredValue(key, init), + ]), + ) as Record; + } + const context: StoreContext = { kv, /** The kernel's run queue. */ @@ -122,14 +166,16 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { refreshRunQueue: () => { context.runQueue = provideStoredQueue('run', true); }, - /** Counter for allocating kernel object IDs */ - nextObjectId: provideCachedStoredValue('nextObjectId', '1'), - /** Counter for allocating kernel promise IDs */ - nextPromiseId: provideCachedStoredValue('nextPromiseId', '1'), - /** Counter for allocating VatIDs */ - nextVatId: provideCachedStoredValue('nextVatId', '1'), - /** Counter for allocating RemoteIDs */ - nextRemoteId: provideCachedStoredValue('nextRemoteId', '1'), + ...provideCachedValues(), + /** + * Re-read every cached stored value from the database. Each one closes over + * the last value written through it (see `provideCachedStoredValue`), so + * reverting the database alone is not enough: the closure would still hold + * the abandoned value and the next `set` would write it straight back. + */ + refreshCachedValues: () => { + Object.assign(context, provideCachedValues()); + }, // As refcounts are decremented, we accumulate a set of krefs for which // action might need to be taken: // * promises which are now resolved and unreferenced can be deleted @@ -141,23 +187,17 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { // the change, else removals might be lost (not performed during the next // replay). maybeFreeKrefs: new Set(), - // Garbage collection - gcActions: provideCachedStoredValue('gcActions', '[]'), - reapQueue: provideCachedStoredValue('reapQueue', '[]'), - terminatedVats: provideCachedStoredValue('vats.terminated', '[]'), inCrank: false, savepoints: [], crankBuffer: [], - // Subclusters - subclusters: provideCachedStoredValue('subclusters', '[]'), - nextSubclusterId: provideCachedStoredValue('nextSubclusterId', '1'), - vatToSubclusterMap: provideCachedStoredValue('vatToSubclusterMap', '{}'), + refCountAuditingEnabled: false, // Logging logger: logger?.subLogger({ tags: ['kernel-store'] }), }; const id = getIdMethods(context); const refCount = getRefCountMethods(context); + const refCountAudit = getRefCountAuditMethods(context); const object = getObjectMethods(context); const promise = getPromiseMethods(context); const revocation = getRevocationMethods(context); @@ -209,23 +249,8 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { })); kdb.clear(); context.maybeFreeKrefs.clear(); - context.runQueue = provideStoredQueue('run', true); - context.gcActions = provideCachedStoredValue('gcActions', '[]'); - context.reapQueue = provideCachedStoredValue('reapQueue', '[]'); - context.terminatedVats = provideCachedStoredValue('vats.terminated', '[]'); - context.nextObjectId = provideCachedStoredValue('nextObjectId', '1'); - context.nextPromiseId = provideCachedStoredValue('nextPromiseId', '1'); - context.nextVatId = provideCachedStoredValue('nextVatId', '1'); - context.nextRemoteId = provideCachedStoredValue('nextRemoteId', '1'); - context.subclusters = provideCachedStoredValue('subclusters', '[]'); - context.nextSubclusterId = provideCachedStoredValue( - 'nextSubclusterId', - '1', - ); - context.vatToSubclusterMap = provideCachedStoredValue( - 'vatToSubclusterMap', - '{}', - ); + context.refreshRunQueue(); + context.refreshCachedValues(); crank.releaseAllSavepoints(); context.crankBuffer.length = 0; preservedState?.forEach(({ key, value }) => { @@ -291,6 +316,7 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { ...id, ...queue, ...refCount, + ...refCountAudit, ...object, ...promise, ...revocation, @@ -368,3 +394,4 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { export type KernelStore = ReturnType; export type { RelayEntry } from './types.ts'; +export type { RefCountViolation } from './methods/refcount-audit.ts'; diff --git a/packages/ocap-kernel/src/store/methods/base.ts b/packages/ocap-kernel/src/store/methods/base.ts index bd09b457a5..2ccd17f1f3 100644 --- a/packages/ocap-kernel/src/store/methods/base.ts +++ b/packages/ocap-kernel/src/store/methods/base.ts @@ -19,7 +19,18 @@ export function getBaseMethods(kv: KVStore) { * @returns The key for the reachable flag and vatSlot. */ function getSlotKey(endpointId: EndpointId, ref: Ref): string { - return `${endpointId}.c.${ref}`; + return `${getCListPrefix(endpointId)}${ref}`; + } + + /** + * Get the prefix shared by both directions of every entry in an endpoint's + * c-list, for iterating over the whole thing. + * + * @param endpointId - The endpoint whose c-list is of interest. + * @returns The prefix that all of that endpoint's c-list keys begin with. + */ + function getCListPrefix(endpointId: EndpointId): string { + return `${endpointId}.c.`; } /** @@ -206,6 +217,7 @@ export function getBaseMethods(kv: KVStore) { return { getSlotKey, + getCListPrefix, refCountKey, getOwnerKey, getRevokedKey, diff --git a/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts new file mode 100644 index 0000000000..7b47365896 --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts @@ -0,0 +1,393 @@ +import { describe, it, expect, beforeEach } from 'vitest'; + +import { makeMapKernelDatabase } from '../../../test/storage.ts'; +import type { RemoteInfo } from '../../remotes/types.ts'; +import type { VatConfig, VatId } from '../../types.ts'; +import { makeKernelStore } from '../index.ts'; + +/** + * 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. + */ +describe('c-list reference accounting', () => { + 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(); + kernelStore.setRefCountAuditing(true); + givenVats('v1', 'v2', 'v3'); + }); + + it('counts each importer separately', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + + kernelStore.translateRefKtoE('v2', kref, true); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + + kernelStore.translateRefKtoE('v3', kref, true); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 2, + recognizable: 2, + }); + }); + + it('keeps an object alive for a second importer after the first lets go', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.translateRefKtoE('v3', kref, true); + + kernelStore.clearReachableFlag('v2', kref); + kernelStore.forgetKref('v2', kref); + kernelStore.collectGarbage(); + + // v3 still holds it, so the owner must not be told to drop or retire + expect([...kernelStore.getGCActions()]).toStrictEqual([]); + expect(kernelStore.getReachableFlag('v3', kref)).toBe(true); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('drops an object once the last of several importers lets go', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + for (const vatId of ['v2', 'v3'] as VatId[]) { + kernelStore.translateRefKtoE(vatId, kref, true); + kernelStore.clearReachableFlag(vatId, kref); + kernelStore.forgetKref(vatId, kref); + } + kernelStore.collectGarbage(); + + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v1 dropExport ${kref}`, + `v1 retireExport ${kref}`, + ]); + }); + + it('cleans up a terminated owner whose importer had already dropped', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + kernelStore.markVatAsTerminated('v1'); + + // Previously the owner's baseline decrement drove this below zero and threw + // out of the middle of the export loop, leaving the vat half-cleaned + expect(kernelStore.cleanupTerminatedVat('v1')).toStrictEqual({ + exports: 1, + imports: 0, + promises: 0, + kv: 0, + }); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 1, + }); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('restores reachability when a dropped import is handed over again', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + const eref = kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + + expect(kernelStore.getReachableFlag('v2', kref)).toBe(false); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 1, + }); + + expect(kernelStore.translateRefKtoE('v2', kref, true)).toBe(eref); + expect(kernelStore.getReachableFlag('v2', kref)).toBe(true); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }); + + it('does not inflate the count when the same import is translated twice', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.translateRefKtoE('v2', kref, true); + + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }); + + it('collects an object whose only reference went splat', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.incrementRefCount(kref, 'queue|slot'); + kernelStore.decrementRefCount(kref, 'deliver|splat|slot'); + + // Previously this settled at (1,1) with no holder, forever + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + kernelStore.collectGarbage(); + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v1 dropExport ${kref}`, + `v1 retireExport ${kref}`, + ]); + 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({ + exports: 0, + imports: 0, + promises: 0, + kv: 0, + }); + }); + + it('orphans exports, releases imports, and forgets the vat', () => { + const mine = kernelStore.exportFromEndpoint('v1', 'o+1'); + const theirs = kernelStore.exportFromEndpoint('v2', 'o+1'); + kernelStore.translateRefKtoE('v1', theirs, true); + kernelStore.translateRefKtoE('v3', mine, true); + kernelStore.markVatAsTerminated('v1'); + + const work = kernelStore.cleanupTerminatedVat('v1'); + + expect(work).toMatchObject({ exports: 1, imports: 1, promises: 0 }); + // v1's export is orphaned but still recognized by v3 + expect(kernelStore.getOwner(mine)).toBeUndefined(); + expect(kernelStore.getObjectRefCount(mine)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + // v1's import of v2's object is released + expect(kernelStore.getObjectRefCount(theirs)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + expect(kernelStore.hasCListEntry('v1', mine)).toBe(false); + expect(kernelStore.hasCListEntry('v1', theirs)).toBe(false); + expect(kernelStore.isVatTerminated('v1')).toBe(false); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('releases the c-list entry of a promise the vat was deciding', () => { + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.setPromiseDecider(kpid, 'v1'); + kernelStore.translateRefKtoE('v2', kpid, true); + // The caller rejects the orphans first, which is what releases the + // unsettled-promise reference and clears the decider + expect([...kernelStore.getPromisesByDecider('v1')]).toStrictEqual([kpid]); + kernelStore.resolveKernelPromise(kpid, true, { + body: '#"gone"', + slots: [], + }); + kernelStore.markVatAsTerminated('v1'); + + const work = kernelStore.cleanupTerminatedVat('v1'); + + expect(work).toMatchObject({ exports: 0, imports: 0, promises: 1 }); + expect(kernelStore.getRefCount(kpid)).toBe(1); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('leaves a live vat that shares the object untouched', () => { + const kref = kernelStore.exportFromEndpoint('v2', 'o+1'); + kernelStore.translateRefKtoE('v1', kref, true); + kernelStore.translateRefKtoE('v3', kref, true); + kernelStore.markVatAsTerminated('v1'); + + kernelStore.cleanupTerminatedVat('v1'); + kernelStore.collectGarbage(); + + expect(kernelStore.getReachableFlag('v3', kref)).toBe(true); + expect([...kernelStore.getGCActions()]).toStrictEqual([]); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }); + }); + + describe('three endpoints sharing one object', () => { + it('accounts for every hand-off and release in turn', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + const importers = ['v2', 'v3'] as VatId[]; + + for (const vatId of importers) { + kernelStore.translateRefKtoE(vatId, kref, true); + } + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 2, + recognizable: 2, + }); + expect(kernelStore.getImporters(kref)).toStrictEqual(importers); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + + // v2 drops but still recognizes + kernelStore.clearReachableFlag('v2', kref); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 2, + }); + kernelStore.collectGarbage(); + expect([...kernelStore.getGCActions()]).toStrictEqual([]); + + // v2 retires; v3 keeps it alive + kernelStore.forgetKref('v2', kref); + kernelStore.collectGarbage(); + expect([...kernelStore.getGCActions()]).toStrictEqual([]); + expect(kernelStore.getImporters(kref)).toStrictEqual(['v3']); + + // v3 lets go too, and only now is the owner told + kernelStore.clearReachableFlag('v3', kref); + kernelStore.forgetKref('v3', kref); + kernelStore.collectGarbage(); + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v1 dropExport ${kref}`, + `v1 retireExport ${kref}`, + ]); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + }); + + describe('a remote importer', () => { + beforeEach(() => { + kernelStore.setRemoteInfo('r1', { peerId: 'peer-1' } as RemoteInfo); + kernelStore.initEndpoint('r1'); + }); + + it('counts towards an object the same as a vat does', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('r1', kref, true); + + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + expect(kernelStore.getImporters(kref)).toStrictEqual(['r1']); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + // `retireKernelObjects` deletes the object once it has told every importer, + // so an importer it never enumerated is left holding a c-list entry naming + // nothing — which nothing tears down, and which the audit reports as + // dangling, taking the run loop with it. + it('is told to retire an object the owner has abandoned', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('r1', kref, true); + // Dropped but still recognized, so collection retires rather than drops. + kernelStore.clearReachableFlag('r1', kref); + kernelStore.orphanKernelObject(kref, 'v1'); + + kernelStore.collectGarbage(); + + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `r1 retireImport ${kref}`, + ]); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + }); +}); diff --git a/packages/ocap-kernel/src/store/methods/clist.test.ts b/packages/ocap-kernel/src/store/methods/clist.test.ts index e43d6e428a..4df8841bf9 100644 --- a/packages/ocap-kernel/src/store/methods/clist.test.ts +++ b/packages/ocap-kernel/src/store/methods/clist.test.ts @@ -27,39 +27,58 @@ describe('clist-methods', () => { }); describe('addCListEntry', () => { - it('adds a bidirectional mapping between KRef and ERef', () => { - const endpointId: EndpointId = 'v1'; - const kref: KRef = 'ko1'; - const eref: ERef = 'o-1'; + it.each([ + { what: 'an object import', kref: 'ko1', eref: 'o-1', flag: '_' }, + { what: 'an object export', kref: 'ko1', eref: 'o+1', flag: 'R' }, + { what: 'a promise import', kref: 'kp1', eref: 'p-2', flag: '_' }, + { what: 'a promise export', kref: 'kp1', eref: 'p+2', flag: 'R' }, + ] as { what: string; kref: KRef; eref: ERef; flag: string }[])( + 'adds a bidirectional mapping for $what', + ({ kref, eref, flag }) => { + const endpointId: EndpointId = 'v1'; + + clistMethods.addCListEntry(endpointId, kref, eref); + + // Only an export is born reachable; an import earns reachability when + // the reference is actually handed over + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`${flag} ${eref}`); + expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); + }, + ); + + it('works with remote endpoints', () => { + const endpointId: EndpointId = 'r1'; + const kref: KRef = 'ko2'; + const eref: ERef = 'ro+3'; clistMethods.addCListEntry(endpointId, kref, eref); - // Check that both mappings are stored expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); }); - it('works with promise refs', () => { - const endpointId: EndpointId = 'v1'; - const kref: KRef = 'kp1'; - const eref: ERef = 'p+2'; + it('takes a recognizable reference for an object import', () => { + clistMethods.addCListEntry('v1', 'ko1', 'o-1'); - clistMethods.addCListEntry(endpointId, kref, eref); + expect(kv.get('ko1.refCount')).toBe('0,1'); + }); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); - expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); + it('takes no reference for an object export', () => { + clistMethods.addCListEntry('v1', 'ko1', 'o+1'); + + expect(kv.get('ko1.refCount')).toBeUndefined(); }); - it('works with remote endpoints', () => { - const endpointId: EndpointId = 'r1'; - const kref: KRef = 'ko2'; - const eref: ERef = 'ro+3'; + it.each(['p-1', 'p+1'] as ERef[])( + 'takes a reference for a promise entry (%s)', + (eref) => { + kv.set('kp1.refCount', '1'); - clistMethods.addCListEntry(endpointId, kref, eref); + clistMethods.addCListEntry('v1', 'kp1', eref); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); - expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); - }); + expect(kv.get('kp1.refCount')).toBe('2'); + }, + ); }); describe('hasCListEntry', () => { @@ -96,7 +115,7 @@ describe('clist-methods', () => { expect(kv.get(`e.nextObjectId.${endpointId}`)).toBe('2'); // Check that the mapping was added - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); }); @@ -113,7 +132,7 @@ describe('clist-methods', () => { expect(kv.get(`e.nextPromiseId.${endpointId}`)).toBe('2'); // Check that the mapping was added - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); }); @@ -151,7 +170,7 @@ describe('clist-methods', () => { }); }); - describe('krefsToExistingErefs', () => { + describe('krefsToErefs', () => { it('returns the ERefs for existing KRefs', () => { const endpointId: EndpointId = 'v1'; const kref1: KRef = 'ko1'; @@ -163,25 +182,18 @@ describe('clist-methods', () => { clistMethods.addCListEntry(endpointId, kref2, eref2); expect( - clistMethods.krefsToExistingErefs(endpointId, [kref1, kref2]), + clistMethods.krefsToErefs(endpointId, [kref1, kref2]), ).toStrictEqual([eref1, eref2]); }); - it('returns an empty array for non-existent KRefs', () => { - const endpointId: EndpointId = 'v1'; - const kref: KRef = 'ko1'; - - expect( - clistMethods.krefsToExistingErefs(endpointId, [kref]), - ).toStrictEqual([]); + it('throws for an unmapped KRef', () => { + expect(() => clistMethods.krefsToErefs('v1', ['ko1'])).toThrow( + 'unmapped kref "ko1" in "v1" c-list', + ); }); it('returns an empty array for empty KRef array', () => { - const endpointId: EndpointId = 'v1'; - - expect(clistMethods.krefsToExistingErefs(endpointId, [])).toStrictEqual( - [], - ); + expect(clistMethods.krefsToErefs('v1', [])).toStrictEqual([]); }); }); @@ -191,7 +203,7 @@ describe('clist-methods', () => { const kref: KRef = 'ko1'; const eref: ERef = 'o-1'; clistMethods.addCListEntry(endpointId, kref, eref); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); clistMethods.forgetEref(endpointId, eref); expect(kv.get(`${endpointId}.c.${kref}`)).toBeUndefined(); @@ -214,7 +226,7 @@ describe('clist-methods', () => { const kref: KRef = 'ko1'; const eref: ERef = 'o-1'; clistMethods.addCListEntry(endpointId, kref, eref); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); clistMethods.forgetKref(endpointId, kref); expect(kv.get(`${endpointId}.c.${kref}`)).toBeUndefined(); diff --git a/packages/ocap-kernel/src/store/methods/clist.ts b/packages/ocap-kernel/src/store/methods/clist.ts index 425f4a1295..d7e3a52af0 100644 --- a/packages/ocap-kernel/src/store/methods/clist.ts +++ b/packages/ocap-kernel/src/store/methods/clist.ts @@ -1,3 +1,5 @@ +import { Fail } from '@endo/errors'; + import { getBaseMethods } from './base.ts'; import { getReachableMethods } from './reachable.ts'; import { getRefCountMethods } from './refcount.ts'; @@ -20,23 +22,32 @@ import { export function getCListMethods(ctx: StoreContext) { const { getSlotKey } = getBaseMethods(ctx.kv); const { clearReachableFlag } = getReachableMethods(ctx); - const { decrementRefCount } = getRefCountMethods(ctx); + const { incrementRefCount, decrementRefCount } = getRefCountMethods(ctx); /** * Add an entry to an endpoint's c-list, creating a new bidirectional mapping * between an ERef belonging to the endpoint and a KRef belonging to the * kernel. * + * The entry is itself a reference, so creating one takes a count, mirroring + * {@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. 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. * @param eref - The ERef. */ function addCListEntry(endpointId: EndpointId, kref: KRef, eref: ERef): void { + const isExport = parseRef(eref).direction === 'export'; ctx.kv.set( getSlotKey(endpointId, kref), - buildReachableAndVatSlot(true, eref), + buildReachableAndVatSlot(isExport, eref), ); ctx.kv.set(getSlotKey(endpointId, eref), kref); + incrementRefCount(kref, 'add|kref', { isExport, onlyRecognizable: true }); } /** @@ -133,16 +144,23 @@ export function getCListMethods(ctx: StoreContext) { } /** - * Look up the ERefs that an endpoint's c-list maps aa list of KRefs to. + * 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: 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. - * @returns The given endpoint's ERefs corresponding to `krefs` + * @returns The given endpoint's ERefs corresponding to `krefs`. */ - function krefsToExistingErefs(endpointId: EndpointId, krefs: KRef[]): ERef[] { - return krefs - .map((kref) => krefToEref(endpointId, kref)) - .filter((eref): eref is ERef => Boolean(eref)); + function krefsToErefs(endpointId: EndpointId, krefs: KRef[]): ERef[] { + return krefs.map( + (kref) => + krefToEref(endpointId, kref) ?? + Fail`unmapped kref ${kref} in ${endpointId} c-list`, + ); } /** @@ -182,6 +200,6 @@ export function getCListMethods(ctx: StoreContext) { krefToEref, forgetEref, forgetKref, - krefsToExistingErefs, + krefsToErefs, }; } diff --git a/packages/ocap-kernel/src/store/methods/crank.test.ts b/packages/ocap-kernel/src/store/methods/crank.test.ts index db3de86450..a39c288dfe 100644 --- a/packages/ocap-kernel/src/store/methods/crank.test.ts +++ b/packages/ocap-kernel/src/store/methods/crank.test.ts @@ -2,7 +2,18 @@ import type { KernelDatabase } from '@metamask/kernel-store'; import { expect, describe, it, vi, beforeEach } from 'vitest'; import { getCrankMethods } from './crank.ts'; -import type { StoreContext } from '../types.ts'; +import type { KRef } from '../../types.ts'; +import type { Savepoint, StoreContext } from '../types.ts'; + +/** + * Build savepoint records holding no collection candidates, for tests that only + * care which savepoints are listed. + * + * @param names - The savepoint names, in order. + * @returns The savepoint records. + */ +const savepoints = (...names: string[]): Savepoint[] => + names.map((name) => ({ name, maybeFreeKrefs: new Set() })); describe('crank methods', () => { let context: StoreContext; @@ -10,6 +21,12 @@ describe('crank methods', () => { let crankMethods: ReturnType; let mockCrankBuffer: unknown[]; + /** + * @returns The names of the currently listed savepoints, in order. + */ + const savepointNames = (): string[] => + context.savepoints.map(({ name }) => name); + beforeEach(() => { mockCrankBuffer = []; context = { @@ -17,6 +34,8 @@ describe('crank methods', () => { savepoints: [], crankBuffer: mockCrankBuffer, refreshRunQueue: vi.fn(), + refreshCachedValues: vi.fn(), + maybeFreeKrefs: new Set(), } as unknown as StoreContext; kdb = { @@ -51,7 +70,7 @@ describe('crank methods', () => { context.inCrank = true; crankMethods.createCrankSavepoint('test'); - expect(context.savepoints).toStrictEqual(['test']); + expect(savepointNames()).toStrictEqual(['test']); expect(kdb.createSavepoint).toHaveBeenCalledWith('t0'); }); @@ -60,7 +79,7 @@ describe('crank methods', () => { crankMethods.createCrankSavepoint('first'); crankMethods.createCrankSavepoint('second'); - expect(context.savepoints).toStrictEqual(['first', 'second']); + expect(savepointNames()).toStrictEqual(['first', 'second']); expect(kdb.createSavepoint).toHaveBeenCalledWith('t0'); expect(kdb.createSavepoint).toHaveBeenCalledWith('t1'); }); @@ -92,7 +111,7 @@ describe('crank methods', () => { describe('rollbackCrank', () => { it('forgets the savepoint even if the database rollback fails', () => { context.inCrank = true; - context.savepoints = ['start']; + context.savepoints = savepoints('start'); vi.mocked(kdb.rollbackSavepoint).mockImplementationOnce(() => { throw new Error('database is gone'); }); @@ -110,17 +129,17 @@ describe('crank methods', () => { it('should rollback to specified savepoint', () => { context.inCrank = true; - context.savepoints = ['first', 'second', 'third']; + context.savepoints = savepoints('first', 'second', 'third'); crankMethods.rollbackCrank('second'); expect(kdb.rollbackSavepoint).toHaveBeenCalledWith('t1'); - expect(context.savepoints).toStrictEqual(['first']); + expect(savepointNames()).toStrictEqual(['first']); }); it('should throw when savepoint does not exist', () => { context.inCrank = true; - context.savepoints = ['first', 'second']; + context.savepoints = savepoints('first', 'second'); expect(() => crankMethods.rollbackCrank('nonexistent')).toThrow( 'no such savepoint as ""nonexistent""', @@ -141,12 +160,12 @@ describe('crank methods', () => { crankMethods.rollbackCrank('b'); crankMethods.createCrankSavepoint('b2'); expect(kdb.createSavepoint).toHaveBeenLastCalledWith('t1'); - expect(context.savepoints).toStrictEqual(['a', 'b2']); + expect(savepointNames()).toStrictEqual(['a', 'b2']); }); it('clears the crank buffer', () => { context.inCrank = true; - context.savepoints = ['start']; + context.savepoints = savepoints('start'); mockCrankBuffer.push({ type: 'send' }, { type: 'notify' }); crankMethods.rollbackCrank('start'); @@ -168,7 +187,7 @@ describe('crank methods', () => { it('should release savepoints if they exist', () => { context.inCrank = true; - context.savepoints = ['test']; + context.savepoints = savepoints('test'); crankMethods.endCrank(); expect(kdb.releaseSavepoint).toHaveBeenCalledWith('t0'); expect(context.savepoints).toStrictEqual([]); @@ -196,7 +215,7 @@ describe('crank methods', () => { it('settles the crank even if releasing savepoints fails', async () => { crankMethods.startCrank(); - context.savepoints = ['test']; + context.savepoints = savepoints('test'); const waiter = crankMethods.waitForCrank(); vi.mocked(kdb.releaseSavepoint).mockImplementationOnce(() => { throw new Error('database is gone'); @@ -211,7 +230,7 @@ describe('crank methods', () => { describe('releaseAllSavepoints', () => { it('should release all savepoints', () => { context.inCrank = true; - context.savepoints = ['test']; + context.savepoints = savepoints('test'); crankMethods.releaseAllSavepoints(); expect(kdb.releaseSavepoint).toHaveBeenCalledWith('t0'); expect(context.savepoints).toStrictEqual([]); diff --git a/packages/ocap-kernel/src/store/methods/crank.ts b/packages/ocap-kernel/src/store/methods/crank.ts index 87d2bc65b8..9e12b228d9 100644 --- a/packages/ocap-kernel/src/store/methods/crank.ts +++ b/packages/ocap-kernel/src/store/methods/crank.ts @@ -36,7 +36,12 @@ export function getCrankMethods(ctx: StoreContext, kdb: KernelDatabase) { // first would leave `endCrank` trying to release a savepoint that was never // created, and that error would replace whatever really went wrong. kdb.createSavepoint(`t${ordinal}`); - ctx.savepoints.push(name); + // Copied, not referenced: `maybeFreeKrefs` is mutated in place from here on, + // and this is the "before" a rollback restores. + ctx.savepoints.push({ + name, + maybeFreeKrefs: new Set(ctx.maybeFreeKrefs), + }); } /** @@ -48,7 +53,8 @@ export function getCrankMethods(ctx: StoreContext, kdb: KernelDatabase) { ctx.inCrank || Fail`rollbackCrank outside of crank`; ctx.crankBuffer.length = 0; // Discard buffered outputs for (const ordinal of ctx.savepoints.keys()) { - if (ctx.savepoints[ordinal] === savepoint) { + const restored = ctx.savepoints[ordinal]; + if (restored?.name === savepoint) { try { kdb.rollbackSavepoint(`t${ordinal}`); } finally { @@ -66,6 +72,24 @@ export function getCrankMethods(ctx: StoreContext, kdb: KernelDatabase) { // the database on next access, since the rollback may have restored // dequeued items. ctx.runQueueLengthCache = -1; + // Same staleness, worse consequence: a cached value reads from its + // closure and only writes through to kv, so one this crank consumed + // stays consumed and the next `set` persists that. `processGCActionSet` + // takes an action out of the set before delivering it, so an action not + // restored here is lost rather than retried. + ctx.refreshCachedValues(); + // Nothing rolls back RAM. Krefs this crank added are collection + // candidates only because of decrements that were just undone; left in + // place, `collectGarbage` throws on a later crank for any promise this + // one created, killing the run loop over work that no longer exists. + // Restored rather than cleared, because the set is not per-crank: it is + // emptied only by `collectGarbage`, so anything added while the run loop + // was idle — `terminateVat` unpinning a root, say — is still owed a + // collection and must survive an unrelated crank's rollback. + ctx.maybeFreeKrefs.clear(); + for (const kref of restored.maybeFreeKrefs) { + ctx.maybeFreeKrefs.add(kref); + } return; } } diff --git a/packages/ocap-kernel/src/store/methods/gc.test.ts b/packages/ocap-kernel/src/store/methods/gc.test.ts index a294920e9a..e91fb29bb9 100644 --- a/packages/ocap-kernel/src/store/methods/gc.test.ts +++ b/packages/ocap-kernel/src/store/methods/gc.test.ts @@ -76,21 +76,6 @@ describe('GC methods', () => { }); }); - describe('reachability tracking', () => { - it('manages reachable flags', () => { - const v1Object = kernelStore.initKernelObject('v1'); - kernelStore.addCListEntry('v1', v1Object, 'o-1'); - - expect(kernelStore.getReachableFlag('v1', v1Object)).toBe(true); - - kernelStore.clearReachableFlag('v1', v1Object); - expect(kernelStore.getReachableFlag('v1', v1Object)).toBe(false); - - const refCounts = kernelStore.getObjectRefCount(v1Object); - expect(refCounts.reachable).toBe(0); - }); - }); - describe('reaping', () => { it('processes reap queue in order', () => { const vatIds = ['v1', 'v2', 'v3']; diff --git a/packages/ocap-kernel/src/store/methods/gc.ts b/packages/ocap-kernel/src/store/methods/gc.ts index 04f3c0cc0c..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,15 +190,28 @@ 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 actions.add(makeGCAction(ownerVatID, 'dropExport', kref)); } if (recognizable === 0) { - // TODO: rethink this assert - // assert.equal(vatConsidersReachable, false, `${kref} is reachable but not recognizable`); + // No assertion that the owner has stopped considering this + // reachable: when the last holder both drops and retires before + // we run, we queue dropExport and retireExport together and the + // owner's flag is still set until the first of them is delivered. actions.add(makeGCAction(ownerVatID, 'retireExport', kref)); } } else if (ownerVatID && terminated) { @@ -219,6 +264,7 @@ export function getGCMethods(ctx: StoreContext) { scheduleReap, nextReapAction, retireKernelObjects, + orphanKernelObject, collectGarbage, }; } diff --git a/packages/ocap-kernel/src/store/methods/object.test.ts b/packages/ocap-kernel/src/store/methods/object.test.ts index def40adaef..26d4fb9b16 100644 --- a/packages/ocap-kernel/src/store/methods/object.test.ts +++ b/packages/ocap-kernel/src/store/methods/object.test.ts @@ -29,7 +29,7 @@ describe('object-methods', () => { }); describe('initKernelObject', () => { - it('creates a new kernel object with initial reference counts', () => { + it('creates a new kernel object, unreferenced', () => { const owner: EndpointId = 'v1'; const koId = objectStore.initKernelObject(owner); @@ -39,13 +39,13 @@ describe('object-methods', () => { // Check the owner is set correctly expect(kv.get(`${koId}.owner`)).toBe(owner); - // Check reference counts are initialized to 1,1 - expect(kv.get(`${koId}.refCount`)).toBe('1,1'); - - // Check via the API - const refCounts = objectStore.getObjectRefCount(koId); - expect(refCounts.reachable).toBe(1); - expect(refCounts.recognizable).toBe(1); + // A new object has no referrers yet; the owner's own export entry is + // not one of them + expect(kv.get(`${koId}.refCount`)).toBe('0,0'); + expect(objectStore.getObjectRefCount(koId)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); }); it('initializes the revoked flag to false', () => { @@ -171,8 +171,10 @@ describe('object-methods', () => { it('returns reference counts for existing objects', () => { const koId = objectStore.initKernelObject('v1'); + objectStore.setObjectRefCount(koId, { reachable: 1, recognizable: 2 }); + const refCounts = objectStore.getObjectRefCount(koId); - expect(refCounts).toStrictEqual({ reachable: 1, recognizable: 1 }); + expect(refCounts).toStrictEqual({ reachable: 1, recognizable: 2 }); }); it('returns zero counts for non-existent objects', () => { @@ -276,8 +278,8 @@ describe('object-methods', () => { // Check initial state expect(objectStore.getOwner(koId)).toBe('v1'); expect(objectStore.getObjectRefCount(koId)).toStrictEqual({ - reachable: 1, - recognizable: 1, + reachable: 0, + recognizable: 0, }); // Update reference counts diff --git a/packages/ocap-kernel/src/store/methods/object.ts b/packages/ocap-kernel/src/store/methods/object.ts index 9e596f5e8f..57b7aaf88e 100644 --- a/packages/ocap-kernel/src/store/methods/object.ts +++ b/packages/ocap-kernel/src/store/methods/object.ts @@ -19,10 +19,11 @@ export function getObjectMethods(ctx: StoreContext) { getBaseMethods(ctx.kv); /** - * Create a new kernel object. The new object will be born with reference and - * recognizability counts of 1, on the assumption that the new object - * corresponds to an object that has just been imported from somewhere. The - * object is initially unrevoked. + * Create a new kernel object, born unreferenced at `(0, 0)`. Every unit of + * an object's counts is owed to a reference someone else holds — an + * importer's c-list entry, a queued message, a promise's resolution value, a + * pin — and the owner's own export entry is not one of them. The object is + * initially unrevoked. * * @param owner - The endpoint or 'kernel' that is the owner of the new object. * @returns The new object's KRef. @@ -30,7 +31,7 @@ export function getObjectMethods(ctx: StoreContext) { function initKernelObject(owner: EndpointId | 'kernel'): KRef { const koId = getNextObjectId(); ctx.kv.set(getOwnerKey(koId), owner); - setObjectRefCount(koId, { reachable: 1, recognizable: 1 }); + setObjectRefCount(koId, { reachable: 0, recognizable: 0 }); return koId; } diff --git a/packages/ocap-kernel/src/store/methods/promise.test.ts b/packages/ocap-kernel/src/store/methods/promise.test.ts index baeba6c2e5..d85e291ef5 100644 --- a/packages/ocap-kernel/src/store/methods/promise.test.ts +++ b/packages/ocap-kernel/src/store/methods/promise.test.ts @@ -58,6 +58,7 @@ describe('promise store methods', () => { }; let context: StoreContext; let promiseMethods: ReturnType; + const mockIncrementRefCount = vi.fn(); const mockDecrementRefCount = vi.fn(); beforeEach(() => { @@ -78,6 +79,7 @@ describe('promise store methods', () => { incCounter: mockIncCounter, provideStoredQueue: mockProvideStoredQueue, getPrefixedKeys: mockGetPrefixedKeys, + getCListPrefix: (endpointId: string) => `${endpointId}.c.`, }); (getQueueMethods as ReturnType).mockReturnValue({ @@ -85,6 +87,7 @@ describe('promise store methods', () => { }); (getRefCountMethods as ReturnType).mockReturnValue({ + incrementRefCount: mockIncrementRefCount, decrementRefCount: mockDecrementRefCount, }); @@ -304,11 +307,12 @@ describe('promise store methods', () => { slots: ['o+1', 'o+2'], }; const message1: KernelMessage = { - method: 'method1', - } as unknown as KernelMessage; + methargs: { body: 'method1', slots: ['ko7'] }, + result: 'kp8', + }; const message2: KernelMessage = { - method: 'method2', - } as unknown as KernelMessage; + methargs: { body: 'method2', slots: [] }, + }; mockKV.set(`${kpid}.state`, 'unresolved'); mockKV.set(`${kpid}.decider`, 'v1'); @@ -338,7 +342,15 @@ describe('promise store methods', () => { expect(mockKV.has(`${kpid}.decider`)).toBe(false); expect(mockKV.has(`${kpid}.subscribers`)).toBe(false); expect(mockQueue.delete).toHaveBeenCalled(); - expect(mockDecrementRefCount).toHaveBeenCalledTimes(1); + // Each dequeued message releases what its queue entry held, then the + // promise releases the decision it was owed + expect(mockDecrementRefCount.mock.calls).toStrictEqual([ + [kpid, 'resolve|dequeue|target'], + ['kp8', 'resolve|dequeue|result'], + ['ko7', 'resolve|dequeue|slot'], + [kpid, 'resolve|dequeue|target'], + [kpid, 'resolve|decider'], + ]); }); it('rejects a promise and enqueues pending messages', () => { @@ -372,6 +384,23 @@ describe('promise store methods', () => { expect(mockProvideStoredQueue).toHaveBeenCalledWith(kpid, false); expect(mockQueue.enqueue).toHaveBeenCalledWith(message); }); + + it('takes a reference on everything the queued message carries', () => { + const kpid = 'kp123'; + const message: KernelMessage = { + methargs: { body: 'test', slots: ['ko1', 'kp2'] }, + result: 'kp3', + }; + + promiseMethods.enqueuePromiseMessage(kpid, message); + + expect(mockIncrementRefCount.mock.calls).toStrictEqual([ + [kpid, 'promiseQueue|target'], + ['kp3', 'promiseQueue|result'], + ['ko1', 'promiseQueue|slot'], + ['kp2', 'promiseQueue|slot'], + ]); + }); }); describe('getKernelPromiseMessageQueue', () => { @@ -410,69 +439,71 @@ describe('promise store methods', () => { }); describe('getPromisesByDecider', () => { - it('yields promises decided by a specific vat', () => { - const vatId = 'v1' as VatId; - const kpid1 = 'kp101'; - const kpid2 = 'kp102'; - const kpid3 = 'kp103'; - - // Set up mock data - mockGetPrefixedKeys.mockReturnValue([ - `cle.${vatId}.p1`, - `cle.${vatId}.p2`, - `cle.${vatId}.p3`, - ]); - - mockKV.set(`cle.${vatId}.p1`, kpid1); - mockKV.set(`cle.${vatId}.p2`, kpid2); - mockKV.set(`cle.${vatId}.p3`, kpid3); - - // kpid1 is decided by vatId - mockKV.set(`${kpid1}.state`, 'unresolved'); - mockKV.set(`${kpid1}.decider`, vatId); - mockKV.set(`${kpid1}.subscribers`, '[]'); - - // kpid2 is also decided by vatId - mockKV.set(`${kpid2}.state`, 'unresolved'); - mockKV.set(`${kpid2}.decider`, vatId); - mockKV.set(`${kpid2}.subscribers`, '[]'); + /** + * Populate a c-list and an unresolved promise record, using the real key + * layout so the scan is exercised rather than mocked around. + * + * @param endpointId - The endpoint whose c-list to add to. + * @param eref - The endpoint's ref for the promise. + * @param kpid - The kernel promise. + * @param decider - The promise's decider, if it has one. + * @param state - The promise's state. + */ + function givenCListPromise( + endpointId: string, + eref: string, + kpid: string, + decider: string | undefined, + state = 'unresolved', + ): void { + mockKV.set(`${endpointId}.c.${eref}`, kpid); + mockKV.set(`${endpointId}.c.${kpid}`, `R ${eref}`); + mockKV.set(`${kpid}.state`, state); + mockKV.set(`${kpid}.subscribers`, '[]'); + if (state === 'unresolved') { + if (decider) { + mockKV.set(`${kpid}.decider`, decider); + } + } else { + mockKV.set(`${kpid}.value`, '{"body":"value","slots":[]}'); + } + mockGetPrefixedKeys.mockImplementation((prefix: string) => + [...mockKV.keys()].filter((key) => key.startsWith(prefix)).sort(), + ); + } - // kpid3 is unresolved but decided by a different vat - mockKV.set(`${kpid3}.state`, 'unresolved'); - mockKV.set(`${kpid3}.decider`, 'v2'); - mockKV.set(`${kpid3}.subscribers`, '[]'); + it.each([ + { context: 'a vat', endpointId: 'v1', erefs: ['p+1', 'p-2'] }, + { context: 'a remote', endpointId: 'r1', erefs: ['rp+1', 'rp-2'] }, + ])('yields promises decided by $context', ({ endpointId, erefs }) => { + givenCListPromise(endpointId, erefs[0] as string, 'kp101', endpointId); + givenCListPromise(endpointId, erefs[1] as string, 'kp102', endpointId); + givenCListPromise(endpointId, 'p+3', 'kp103', 'v2'); - const result = Array.from(promiseMethods.getPromisesByDecider(vatId)); + const result = Array.from( + promiseMethods.getPromisesByDecider(endpointId as VatId), + ); - expect(result).toStrictEqual([kpid1, kpid2]); - expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`cle.${vatId}.p`); + expect(result).toStrictEqual(['kp101', 'kp102']); }); it('does not yield resolved promises', () => { - const vatId = 'v1' as VatId; - const kpid1 = 'kp101'; - const kpid2 = 'kp102'; + givenCListPromise('v1', 'p+1', 'kp101', undefined, 'fulfilled'); + givenCListPromise('v1', 'p+2', 'kp102', 'v1'); - mockGetPrefixedKeys.mockReturnValue([ - `cle.${vatId}.p1`, - `cle.${vatId}.p2`, - ]); + const result = Array.from(promiseMethods.getPromisesByDecider('v1')); - mockKV.set(`cle.${vatId}.p1`, kpid1); - mockKV.set(`cle.${vatId}.p2`, kpid2); - - // kpid1 is fulfilled - mockKV.set(`${kpid1}.state`, 'fulfilled'); - mockKV.set(`${kpid1}.value`, '{"body":"value","slots":[]}'); + expect(result).toStrictEqual(['kp102']); + }); - // kpid2 is unresolved and decided by vatId - mockKV.set(`${kpid2}.state`, 'unresolved'); - mockKV.set(`${kpid2}.decider`, vatId); - mockKV.set(`${kpid2}.subscribers`, '[]'); + it('ignores object entries in the same c-list', () => { + givenCListPromise('v1', 'p+1', 'kp101', 'v1'); + mockKV.set('v1.c.o+1', 'ko1'); + mockKV.set('v1.c.ko1', 'R o+1'); - const result = Array.from(promiseMethods.getPromisesByDecider(vatId)); + const result = Array.from(promiseMethods.getPromisesByDecider('v1')); - expect(result).toStrictEqual([kpid2]); + expect(result).toStrictEqual(['kp101']); }); it('yields nothing if no promises are decided by the vat', () => { diff --git a/packages/ocap-kernel/src/store/methods/promise.ts b/packages/ocap-kernel/src/store/methods/promise.ts index c3aedbf6c3..40fa73b049 100644 --- a/packages/ocap-kernel/src/store/methods/promise.ts +++ b/packages/ocap-kernel/src/store/methods/promise.ts @@ -16,6 +16,9 @@ import { makeKernelSlot } from '../utils/kernel-slots.ts'; import { parseRef } from '../utils/parse-ref.ts'; import { isPromiseRef } from '../utils/promise-ref.ts'; +/** Matches the promise erefs in a c-list: `p+NN`/`p-NN`, or `rp+NN`/`rp-NN` for a remote. */ +const PROMISE_EREF = /^r?p[-+]\d+$/u; + /** * Create a promise store object that provides functionality for managing kernel promises. * @@ -25,14 +28,20 @@ import { isPromiseRef } from '../utils/promise-ref.ts'; */ // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getPromiseMethods(ctx: StoreContext) { - const { incCounter, provideStoredQueue, getPrefixedKeys, refCountKey } = - getBaseMethods(ctx.kv); - const { decrementRefCount } = getRefCountMethods(ctx); + const { + incCounter, + provideStoredQueue, + getPrefixedKeys, + getCListPrefix, + refCountKey, + } = getBaseMethods(ctx.kv); + const { incrementRefCount, decrementRefCount } = getRefCountMethods(ctx); /** - * Create a new, unresolved kernel promise. The new promise will be born with - * a reference count of 1 on the assumption that the promise has just been - * imported from somewhere. + * Create a new, unresolved kernel promise, born with a reference count of 1: + * an unsettled promise is owed a decision, and that obligation is itself a + * reference. Released, exactly once, when the promise settles in + * {@link resolveKernelPromise}. * * @returns A tuple of the new promise's KRef and an object describing the * new promise itself. @@ -159,16 +168,18 @@ export function getPromiseMethods(ctx: StoreContext) { value: CapData, ): [KRef, KernelMessage][] { const queue = provideStoredQueue(kpid, false); - // Collect messages that were queued on this promise + // Releasing each queue entry's references as we go: the caller re-enqueues + // these on the run queue, which takes its own. const queuedMessages: [KRef, KernelMessage][] = []; for (const message of getKernelPromiseMessageQueue(kpid)) { queuedMessages.push([kpid, message]); + releaseQueuedMessageRefs(kpid, message, 'resolve|dequeue'); } ctx.kv.set(`${kpid}.state`, rejected ? 'rejected' : 'fulfilled'); ctx.kv.set(`${kpid}.value`, JSON.stringify(value)); ctx.kv.delete(`${kpid}.decider`); ctx.kv.delete(`${kpid}.subscribers`); - // Drop the baseline "decider" refcount now that the promise is settled. + // The promise has been decided, so it is no longer owed a decision. decrementRefCount(kpid, 'resolve|decider'); queue.delete(); return queuedMessages; @@ -177,13 +188,46 @@ export function getPromiseMethods(ctx: StoreContext) { /** * Append a message to a promise's message queue. * + * The queue entry becomes the message's holder, so it takes references on + * everything the message carries, just as the run queue does. + * * @param kpid - The KRef of the promise to enqueue on. * @param message - The message to enqueue. */ function enqueuePromiseMessage(kpid: KRef, message: KernelMessage): void { + incrementRefCount(kpid, 'promiseQueue|target'); + if (message.result) { + incrementRefCount(message.result, 'promiseQueue|result'); + } + for (const slot of message.methargs.slots) { + incrementRefCount(slot, 'promiseQueue|slot'); + } provideStoredQueue(kpid, false).enqueue(message); } + /** + * Release the references a promise-queue entry held on the message it + * carried. + * + * @param kpid - The promise whose queue the message was on, and hence the + * message's target. + * @param message - The message being taken off the queue. + * @param tag - Tag for refcount logging. + */ + function releaseQueuedMessageRefs( + kpid: KRef, + message: KernelMessage, + tag: string, + ): void { + decrementRefCount(kpid, `${tag}|target`); + if (message.result) { + decrementRefCount(message.result, `${tag}|result`); + } + for (const slot of message.methargs.slots) { + decrementRefCount(slot, `${tag}|slot`); + } + } + /** * Fetch the messages in a kernel promise's message queue. * @@ -211,8 +255,13 @@ export function getPromiseMethods(ctx: StoreContext) { * @yields the kpids of all the unresolved promises decided by `decider`. */ function* getPromisesByDecider(decider: EndpointId): Generator { - const basePrefix = `cle.${decider}.`; - for (const key of getPrefixedKeys(`${basePrefix}p`)) { + const prefix = getCListPrefix(decider); + for (const key of getPrefixedKeys(prefix)) { + // A c-list holds both directions of each pair. Iterate by eref, and only + // the promise ones: `p+NN`/`p-NN` for a vat, `rp+NN`/`rp-NN` for a remote. + if (!PROMISE_EREF.test(key.slice(prefix.length))) { + continue; + } const kpid = ctx.kv.getRequired(key); const kp = getKernelPromise(kpid); if (kp.state === 'unresolved' && kp.decider === decider) { diff --git a/packages/ocap-kernel/src/store/methods/reachable.test.ts b/packages/ocap-kernel/src/store/methods/reachable.test.ts index fdaab477e5..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', () => { @@ -15,13 +16,82 @@ describe('GC methods', () => { const ko1 = kernelStore.initKernelObject('v1'); kernelStore.addCListEntry('v1', ko1, 'o-1'); + // An import entry is born recognizing but not yet reaching + expect(kernelStore.getReachableFlag('v1', ko1)).toBe(false); + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 0, + recognizable: 1, + }); + + kernelStore.setReachableFlag('v1', ko1); expect(kernelStore.getReachableFlag('v1', ko1)).toBe(true); + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); kernelStore.clearReachableFlag('v1', ko1); expect(kernelStore.getReachableFlag('v1', ko1)).toBe(false); + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + 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(); - const refCounts = kernelStore.getObjectRefCount(ko1); - expect(refCounts.reachable).toBe(0); + 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'); + kernelStore.addCListEntry('v1', ko1, 'o+1'); + + expect(kernelStore.getReachableFlag('v1', ko1)).toBe(true); + kernelStore.setReachableFlag('v1', ko1); + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + + kernelStore.clearReachableFlag('v1', ko1); + expect(kernelStore.getReachableFlag('v1', ko1)).toBe(false); + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); }); }); }); diff --git a/packages/ocap-kernel/src/store/methods/reachable.ts b/packages/ocap-kernel/src/store/methods/reachable.ts index 4caa2d0960..05e4c5f986 100644 --- a/packages/ocap-kernel/src/store/methods/reachable.ts +++ b/packages/ocap-kernel/src/store/methods/reachable.ts @@ -54,6 +54,32 @@ export function getReachableMethods(ctx: StoreContext) { return parseReachableAndVatSlot(data); } + /** + * Set the reachable flag for a given endpoint and kref. + * + * The counterpart to {@link clearReachableFlag}: this is how an object + * regains reachability when a vat that dropped it is handed it again. + * Idempotent, so repeated translations don't inflate the count. + * + * @param endpointId - The endpoint for which the reachable flag is being set. + * @param kref - The kref. + */ + function setReachableFlag(endpointId: EndpointId, kref: KRef): void { + const key = getSlotKey(endpointId, kref); + const { isReachable, vatSlot } = getReachableAndVatSlot(endpointId, kref); + if (isReachable) { + return; + } + ctx.kv.set(key, buildReachableAndVatSlot(true, vatSlot)); + const { direction, isPromise } = parseRef(vatSlot); + // increment 'reachable' part of refcount, but only for object imports + if (!isPromise && direction === 'import' && kernelRefExists(kref)) { + const counts = getObjectRefCount(kref); + counts.reachable += 1; + setObjectRefCount(kref, counts); + } + } + /** * Clear the reachable flag for a given endpoint and kref. * @@ -84,6 +110,7 @@ export function getReachableMethods(ctx: StoreContext) { return { getReachableFlag, getReachableAndVatSlot, + setReachableFlag, clearReachableFlag, }; } diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts new file mode 100644 index 0000000000..868034e55c --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts @@ -0,0 +1,496 @@ +import type { KernelDatabase } from '@metamask/kernel-store'; +import { describe, it, expect, beforeEach } from 'vitest'; + +import { makeMapKernelDatabase } from '../../../test/storage.ts'; +import type { KRef, VatConfig, VatId } from '../../types.ts'; +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. + * + * @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); + } + } + + /** + * 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(() => { + kdb = makeMapKernelDatabase(); + kernelStore = makeKernelStore(kdb); + kernelStore.markInitialized(); + givenVats('v1', 'v2', 'v3'); + }); + + describe('auditRefCounts', () => { + it('finds nothing wrong in an empty store', () => { + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it.each([ + { + what: 'an export', + act: (kref: KRef) => kref, + }, + { + what: 'an export plus one importer', + act: (kref: KRef) => { + kernelStore.translateRefKtoE('v2', kref, true); + return kref; + }, + }, + { + what: 'an export plus two importers', + act: (kref: KRef) => { + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.translateRefKtoE('v3', kref, true); + return kref; + }, + }, + { + what: 'a dropped import', + act: (kref: KRef) => { + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + return kref; + }, + }, + { + what: 'a retired import', + act: (kref: KRef) => { + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + kernelStore.forgetKref('v2', kref); + return kref; + }, + }, + { + what: 'a pin', + act: (kref: KRef) => { + kernelStore.pinObject(kref); + return kref; + }, + }, + { + what: 'a queued message', + act: (kref: KRef) => { + 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; + }, + }, + ])('holds for $what', ({ act }) => { + act(kernelStore.exportFromEndpoint('v1', 'o+1')); + + 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); + + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + expect(kernelStore.getRefCount(kpid)).toBe(3); + }); + + it('holds for a settled promise whose value carries a slot', () => { + 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], + }); + + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('reports counts that are too low', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.setObjectRefCount(kref, { reachable: 0, recognizable: 0 }); + + expect(kernelStore.auditRefCounts()).toStrictEqual([ + { + kind: 'mismatch', + kref, + stored: '0,0', + expected: '1,1', + holders: ['v2 c-list import o-1'], + }, + ]); + }); + + it('reports counts that are too high even though nothing underflowed', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.setObjectRefCount(kref, { reachable: 1, recognizable: 1 }); + + expect(kernelStore.auditRefCounts()).toStrictEqual([ + { kind: 'mismatch', kref, stored: '1,1', expected: '0,0', holders: [] }, + ]); + }); + + it('reports a reference to a kref that has been deleted', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.deleteKernelObject(kref); + + expect(kernelStore.auditRefCounts()).toStrictEqual([ + { + kind: 'dangling', + kref, + expected: '1,1', + holders: ['v2 c-list import o-1'], + }, + ]); + }); + + it('does not mistake an owner for a referrer', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + }); + + // 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'); + kernelStore.setObjectRefCount(kref, { reachable: 9, recognizable: 9 }); + + expect(() => kernelStore.assertRefCountsIfAuditing()).not.toThrow(); + }); + + it('throws with the offending krefs once auditing is on', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.setObjectRefCount(kref, { reachable: 9, recognizable: 9 }); + kernelStore.setRefCountAuditing(true); + + expect(() => kernelStore.assertRefCountsIfAuditing()).toThrow( + `${kref}: stored 9,9, expected 0,0`, + ); + }); + + it('stays quiet when the counts agree', () => { + kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.setRefCountAuditing(true); + + expect(() => kernelStore.assertRefCountsIfAuditing()).not.toThrow(); + }); + }); + + describe('recomputeRefCounts', () => { + it('rebuilds counts written under the old accounting', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.translateRefKtoE('v3', kref, true); + // As the pre-fix kernel would have left it: born (1,1), with neither + // importer's c-list entry taking a reference. Two importers is the + // smallest topology where that disagrees with the truth — with one, the + // phantom baseline happens to come out to the right number. + kernelStore.setObjectRefCount(kref, { reachable: 1, recognizable: 1 }); + + const { corrected, unfixable } = kernelStore.recomputeRefCounts(); + + expect(corrected).toStrictEqual([ + { + kind: 'mismatch', + kref, + stored: '1,1', + expected: '2,2', + holders: ['v2 c-list import o-1', 'v3 c-list import o-1'], + }, + ]); + expect(unfixable).toStrictEqual([]); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 2, + recognizable: 2, + }); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('reports references it cannot repair', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.deleteKernelObject(kref); + + const { corrected, unfixable } = kernelStore.recomputeRefCounts(); + + expect(corrected).toStrictEqual([]); + expect(unfixable).toHaveLength(1); + 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 }); + + kernelStore.recomputeRefCounts(); + kernelStore.collectGarbage(); + + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v1 dropExport ${kref}`, + `v1 retireExport ${kref}`, + ]); + }); + }); + + describe('formatRefCountViolations', () => { + it('names the holders behind a mismatch', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.setObjectRefCount(kref, { reachable: 0, recognizable: 0 }); + + expect( + kernelStore.formatRefCountViolations(kernelStore.auditRefCounts()), + ).toBe( + `${kref}: stored 0,0, expected 1,1 (held by: v2 c-list import o-1)`, + ); + }); + + it('says so when a mismatch has no holders at all', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.setObjectRefCount(kref, { reachable: 1, recognizable: 1 }); + + expect( + kernelStore.formatRefCountViolations(kernelStore.auditRefCounts()), + ).toBe(`${kref}: stored 1,1, expected 0,0 (held by: nothing)`); + }); + }); +}); diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.ts new file mode 100644 index 0000000000..403dbcfc13 --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -0,0 +1,394 @@ +import type { CapData } from '@endo/marshal'; + +import { getBaseMethods } from './base.ts'; +import { getObjectMethods } from './object.ts'; +import { getPinMethods } from './pinned.ts'; +import type { KRef, KernelMessage, RunQueueItem } from '../../types.ts'; +import type { StoreContext } from '../types.ts'; +import { parseRef } from '../utils/parse-ref.ts'; +import { isPromiseRef } from '../utils/promise-ref.ts'; +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 = + | { + /** 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 + * only a single count, that count accumulates in `reachable`. + */ +type Tally = { + reachable: number; + recognizable: number; + holders: string[]; +}; + +/** Matches the kref-keyed half of a c-list entry, e.g. `v1.c.ko3`. */ +const CLIST_KREF_KEY = /^([vr]\d+)\.c\.(k[op]\d+)$/u; + +/** Matches a queue entry (but not the queue's `head`/`tail` bookkeeping). */ +const QUEUE_ENTRY_KEY = /^queue\.([^.]+)\.(\d+)$/u; + +/** Matches the state record that exists for every live kernel promise. */ +const PROMISE_STATE_KEY = /^(kp\d+)\.state$/u; + +/** Matches the refcount record that exists for every live kernel object or promise. */ +const REFCOUNT_KEY = /^(k[op]\d+)\.refCount$/u; + +/** + * Get the methods that audit reference counts against ground truth. + * + * The kernel's reference counts are a cache: every unit of every count is owed + * to some reference the kernel is holding somewhere else in the store — a + * c-list entry, a queued message, a promise's resolution value, a pin. This + * module recomputes those counts from the references themselves and reports + * where the cache has drifted, in either direction. Counts that are too low + * let a live capability be collected; counts that are too high leak it. + * + * @param ctx - The store context. + * @returns The reference count audit methods. + */ +// eslint-disable-next-line @typescript-eslint/explicit-function-return-type +export function getRefCountAuditMethods(ctx: StoreContext) { + const { getPrefixedKeys, refCountKey } = getBaseMethods(ctx.kv); + const { getObjectRefCount } = getObjectMethods(ctx); + const { getPinnedObjects } = getPinMethods(ctx); + + /** + * Render a tally the way the store encodes it, so expected and stored values + * can be compared and reported as like for like. + * + * @param kref - The kref the counts belong to. + * @param counts - The counts to render. + * @param counts.reachable - The reachable count (the only count, for a promise). + * @param counts.recognizable - The recognizable count (ignored for a promise). + * @returns The encoded counts. + */ + function renderCounts( + kref: KRef, + counts: { reachable: number; recognizable: number }, + ): string { + return isPromiseRef(kref) + ? `${counts.reachable}` + : `${counts.reachable},${counts.recognizable}`; + } + + /** + * Walk the whole store and total up, for every kref, the references the + * kernel is holding to it. + * + * The credits below mirror `incrementRefCount` case for case; when that + * function's rules change, these have to change with it. + * + * @returns A tally per kref that anything refers to. + */ + function computeExpectedRefCounts(): Map { + const tallies = new Map(); + // `retireKernelObjects` deletes an object and queues a `retireImport` for + // each importer in the same breath, so between then and the delivery an + // importer's c-list entry legitimately names a kref the kernel has already + // dropped. Those entries are scheduled for teardown and are not holders. + const retiring = new Set( + (JSON.parse(ctx.gcActions.get() ?? '[]') as string[]).filter((action) => + action.includes(' retireImport '), + ), + ); + + const credit = ( + kref: KRef, + holder: string, + { onlyRecognizable = false }: { onlyRecognizable?: boolean } = {}, + ): void => { + let tally = tallies.get(kref); + if (!tally) { + tally = { reachable: 0, recognizable: 0, holders: [] }; + tallies.set(kref, tally); + } + tally.holders.push(holder); + if (isPromiseRef(kref)) { + // Promises have a single count and no reachable/recognizable split. + tally.reachable += 1; + return; + } + if (!onlyRecognizable) { + tally.reachable += 1; + } + tally.recognizable += 1; + }; + + /** + * A queued message holds its result promise and every slot it carries. + * + * @param message - The queued message. + * @param holder - Description of the queue entry holding it. + */ + const creditMessage = (message: KernelMessage, holder: string): void => { + if (message.result) { + credit(message.result, `${holder} result`); + } + for (const slot of message.methargs.slots) { + credit(slot, `${holder} slot`); + } + }; + + for (const key of getPrefixedKeys('')) { + const clistMatch = CLIST_KREF_KEY.exec(key); + if (clistMatch) { + const [, endpointId, kref] = clistMatch as unknown as [ + string, + string, + KRef, + ]; + const { isReachable, vatSlot } = parseReachableAndVatSlot( + ctx.kv.getRequired(key), + ); + const { direction } = parseRef(vatSlot); + const holder = `${endpointId} c-list ${direction} ${vatSlot}`; + if (isPromiseRef(kref)) { + // Both directions count for a promise. + credit(kref, holder); + } else if ( + direction === 'import' && + !retiring.has(`${endpointId} retireImport ${kref}`) + ) { + // An object export is the owner's own entry and carries no count; + // an object import always recognizes and, while flagged, reaches. + credit(kref, holder, { onlyRecognizable: !isReachable }); + } + continue; + } + + const queueMatch = QUEUE_ENTRY_KEY.exec(key); + if (queueMatch) { + const [, queueName, seq] = queueMatch as unknown as [ + string, + string, + string, + ]; + const entry = ctx.kv.getRequired(key); + if (queueName === 'run') { + const item = JSON.parse(entry) as RunQueueItem; + if (item.type === 'send') { + credit(item.target, `run queue #${seq} send target`); + creditMessage(item.message, `run queue #${seq} send`); + } else if (item.type === 'notify') { + credit(item.kpid, `run queue #${seq} notify`); + } + } else { + const kpid = queueName as KRef; + const message = JSON.parse(entry) as KernelMessage; + credit(kpid, `${kpid} queue #${seq} target`); + creditMessage(message, `${kpid} queue #${seq}`); + } + continue; + } + + const promiseMatch = PROMISE_STATE_KEY.exec(key); + if (promiseMatch) { + const kpid = promiseMatch[1] as KRef; + if (ctx.kv.getRequired(key) === 'unresolved') { + // The unit `initKernelPromise` mints, released when the promise settles. + credit(kpid, 'unsettled promise'); + } else { + const value = JSON.parse( + ctx.kv.getRequired(`${kpid}.value`), + ) as CapData; + for (const slot of value.slots) { + credit(slot, `${kpid} resolution slot`); + } + } + } + } + + for (const kref of getPinnedObjects()) { + credit(kref, 'pin'); + } + + return tallies; + } + + /** + * Collect every kref the store has a refcount entry for. + * + * @returns The krefs with refcount entries. + */ + function getCountedKrefs(): KRef[] { + const krefs: KRef[] = []; + for (const key of getPrefixedKeys('')) { + const match = REFCOUNT_KEY.exec(key); + if (match) { + krefs.push(match[1] as KRef); + } + } + return krefs; + } + + /** + * 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[] { + const expected = computeExpectedRefCounts(); + const violations: RefCountViolation[] = []; + const krefs = new Set([...expected.keys(), ...getCountedKrefs()]); + + for (const kref of [...krefs].sort()) { + const tally = expected.get(kref) ?? { + reachable: 0, + recognizable: 0, + holders: [], + }; + const expectedText = renderCounts(kref, tally); + const raw = ctx.kv.get(refCountKey(kref)); + if (raw === undefined) { + // The kref has been deleted from the kernel, so anything still + // pointing at it is a dangling reference. + if (tally.holders.length > 0) { + violations.push({ + kind: 'dangling', + kref, + expected: expectedText, + holders: tally.holders, + }); + } + continue; + } + const storedText = isPromiseRef(kref) + ? raw + : renderCounts(kref, getObjectRefCount(kref)); + if (storedText !== expectedText) { + violations.push({ + kind: 'mismatch', + kref, + stored: storedText, + expected: expectedText, + holders: tally.holders, + }); + } + } + return violations; + } + + /** + * Overwrite stored reference counts with the counts implied by ground truth. + * + * This is how a store written under the pre-fix accounting is brought onto + * the current scheme: the references themselves are authoritative, so the + * counts can simply be rebuilt from them. Krefs that are referenced but have + * already been deleted cannot be repaired this way and are reported instead. + * + * @returns The violations that were corrected and those that could not be. + */ + function recomputeRefCounts(): { + corrected: RefCountViolation[]; + unfixable: RefCountViolation[]; + } { + const corrected: RefCountViolation[] = []; + const unfixable: RefCountViolation[] = []; + for (const violation of auditRefCounts()) { + if (violation.kind === 'dangling') { + unfixable.push(violation); + continue; + } + ctx.kv.set(refCountKey(violation.kref), violation.expected); + if (violation.expected.startsWith('0')) { + ctx.maybeFreeKrefs.add(violation.kref); + } + corrected.push(violation); + } + return { corrected, unfixable }; + } + + /** + * Render violations as a human-readable report. + * + * @param violations - The violations to describe. + * @returns A newline-separated report, one line per violation. + */ + function formatRefCountViolations(violations: RefCountViolation[]): string { + return violations + .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})`; + }) + .join('\n'); + } + + /** + * Audit reference counts and throw if any have drifted. Enabled per kernel + * via the `auditRefCounts` option, and run at the end of every crank. + */ + function assertRefCountsIfAuditing(): void { + if (!ctx.refCountAuditingEnabled) { + return; + } + const violations = auditRefCounts(); + if (violations.length > 0) { + 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}`); + } + } + + /** + * Turn the per-crank reference count audit on or off. + * + * @param enabled - Whether to audit after every crank. + */ + function setRefCountAuditing(enabled: boolean): void { + ctx.refCountAuditingEnabled = enabled; + } + + return { + auditRefCounts, + recomputeRefCounts, + formatRefCountViolations, + assertRefCountsIfAuditing, + setRefCountAuditing, + }; +} diff --git a/packages/ocap-kernel/src/store/methods/remote.ts b/packages/ocap-kernel/src/store/methods/remote.ts index 23ae73aec5..a32c3d0789 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) { } } + /** + * The IDs of every remote the kernel knows about, without reading their info. + * + * @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/translators.test.ts b/packages/ocap-kernel/src/store/methods/translators.test.ts index 0ac95eafb0..fd1b5e1df6 100644 --- a/packages/ocap-kernel/src/store/methods/translators.test.ts +++ b/packages/ocap-kernel/src/store/methods/translators.test.ts @@ -14,6 +14,7 @@ import type { } from '../../types.ts'; import type { StoreContext } from '../types.ts'; import * as clistModule from './clist.ts'; +import * as reachableModule from './reachable.ts'; import { getTranslators } from './translators.ts'; import * as vatModule from './vat.ts'; @@ -22,6 +23,7 @@ describe('getTranslators', () => { const mockErefToKref = vi.fn(); const mockAllocateErefForKref = vi.fn(); const mockExportFromEndpoint = vi.fn(); + const mockSetReachableFlag = vi.fn(); const mockCtx = {} as StoreContext; beforeEach(() => { @@ -33,6 +35,10 @@ describe('getTranslators', () => { allocateErefForKref: mockAllocateErefForKref, } as unknown as ReturnType); + vi.spyOn(reachableModule, 'getReachableMethods').mockReturnValue({ + setReachableFlag: mockSetReachableFlag, + } as unknown as ReturnType); + vi.spyOn(vatModule, 'getVatMethods').mockReturnValue({ exportFromEndpoint: mockExportFromEndpoint, } as unknown as ReturnType); diff --git a/packages/ocap-kernel/src/store/methods/translators.ts b/packages/ocap-kernel/src/store/methods/translators.ts index c5948fedff..b7d418a1c3 100644 --- a/packages/ocap-kernel/src/store/methods/translators.ts +++ b/packages/ocap-kernel/src/store/methods/translators.ts @@ -21,6 +21,7 @@ import type { } from '../../types.ts'; import type { StoreContext } from '../types.ts'; import { getCListMethods } from './clist.ts'; +import { getReachableMethods } from './reachable.ts'; import { getVatMethods } from './vat.ts'; import { Fail, assert } from '../../utils/assert.ts'; @@ -35,6 +36,7 @@ import { Fail, assert } from '../../utils/assert.ts'; // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getTranslators(ctx: StoreContext) { const { krefToEref, erefToKref, allocateErefForKref } = getCListMethods(ctx); + const { setReachableFlag } = getReachableMethods(ctx); const { exportFromEndpoint } = getVatMethods(ctx); /** @@ -54,6 +56,11 @@ export function getTranslators(ctx: StoreContext) { /** * Translate a reference from kernel space into endpoint space. * + * Translating is how the kernel hands an endpoint a reference, so it also + * re-establishes reachability: a vat given an object it previously dropped + * holds it live again. Garbage collection deliveries must not do this, and + * don't — they map through `krefsToErefs`, which never touches the flag. + * * @param endpointId - The endpoint for whom translation is desired. * @param kref - The KRef of the entity of interest. * @param importIfNeeded - If true, allocate a new clist entry if necessary; @@ -74,6 +81,7 @@ export function getTranslators(ctx: StoreContext) { throw Fail`unmapped kref ${kref} endpoint=${endpointId}`; } } + setReachableFlag(endpointId, kref); if (isRemoteId(endpointId)) { // The import/export relationship between a vat and the kernel is // asymmetric -- the vat always exports to the kernel and imports from the diff --git a/packages/ocap-kernel/src/store/methods/vat.test.ts b/packages/ocap-kernel/src/store/methods/vat.test.ts index d04c609896..029cf31786 100644 --- a/packages/ocap-kernel/src/store/methods/vat.test.ts +++ b/packages/ocap-kernel/src/store/methods/vat.test.ts @@ -118,6 +118,7 @@ describe('vat store methods', () => { (getBaseMethods as ReturnType).mockReturnValue({ getPrefixedKeys: mockGetPrefixedKeys, getSlotKey: mockGetSlotKey, + getCListPrefix: (endpointId: string) => `${endpointId}.c.`, getOwnerKey: mockGetOwnerKey, }); @@ -273,33 +274,26 @@ describe('vat store methods', () => { it('deletes all keys related to the endpoint', () => { const endpointId = 'e1'; - // Setup mock data - mockKV.set(`cle.${endpointId}.obj1`, 'data1'); - mockKV.set(`cle.${endpointId}.obj2`, 'data2'); - mockKV.set(`clk.${endpointId}.prom1`, 'data3'); + // The c-list holds both directions of each pair under one prefix + mockKV.set(`${endpointId}.c.o-1`, 'ko1'); + mockKV.set(`${endpointId}.c.ko1`, 'R o-1'); + mockKV.set(`${endpointId}.c.p+1`, 'kp1'); mockKV.set(`e.nextObjectId.${endpointId}`, '10'); mockKV.set(`e.nextPromiseId.${endpointId}`, '5'); - mockGetPrefixedKeys.mockImplementation((prefix: string) => { - if (prefix === `cle.${endpointId}.`) { - return [`cle.${endpointId}.obj1`, `cle.${endpointId}.obj2`]; - } - if (prefix === `clk.${endpointId}.`) { - return [`clk.${endpointId}.prom1`]; - } - return []; - }); + mockGetPrefixedKeys.mockImplementation((prefix: string) => + [...mockKV.keys()].filter((key) => key.startsWith(prefix)), + ); vatMethods.deleteEndpoint(endpointId); - expect(mockKV.has(`cle.${endpointId}.obj1`)).toBe(false); - expect(mockKV.has(`cle.${endpointId}.obj2`)).toBe(false); - expect(mockKV.has(`clk.${endpointId}.prom1`)).toBe(false); + expect(mockKV.has(`${endpointId}.c.o-1`)).toBe(false); + expect(mockKV.has(`${endpointId}.c.ko1`)).toBe(false); + expect(mockKV.has(`${endpointId}.c.p+1`)).toBe(false); expect(mockKV.has(`e.nextObjectId.${endpointId}`)).toBe(false); expect(mockKV.has(`e.nextPromiseId.${endpointId}`)).toBe(false); - expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`cle.${endpointId}.`); - expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`clk.${endpointId}.`); + expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`${endpointId}.c.`); }); it('does nothing if endpoint has no associated keys', () => { @@ -309,8 +303,7 @@ describe('vat store methods', () => { expect(() => vatMethods.deleteEndpoint(endpointId)).not.toThrow(); - expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`cle.${endpointId}.`); - expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`clk.${endpointId}.`); + expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`${endpointId}.c.`); }); }); @@ -470,11 +463,10 @@ describe('vat store methods', () => { expect(result).toBe('kp123'); expect(mockInitKernelPromise).toHaveBeenCalled(); expect(mockSetPromiseDecider).toHaveBeenCalledWith('kp123', vatId); + // addCListEntry takes the entry's reference; exportFromEndpoint no + // longer takes one of its own expect(mockAddCListEntry).toHaveBeenCalledWith(vatId, 'kp123', vref); - expect(mockIncrementRefCount).toHaveBeenCalledWith('kp123', 'export', { - isExport: true, - onlyRecognizable: true, - }); + expect(mockIncrementRefCount).not.toHaveBeenCalled(); }); it('creates a kernel object for an exported object', () => { @@ -486,10 +478,7 @@ describe('vat store methods', () => { expect(result).toBe('ko456'); expect(mockInitKernelObject).toHaveBeenCalledWith(vatId); expect(mockAddCListEntry).toHaveBeenCalledWith(vatId, 'ko456', vref); - expect(mockIncrementRefCount).toHaveBeenCalledWith('ko456', 'export', { - isExport: true, - onlyRecognizable: true, - }); + expect(mockIncrementRefCount).not.toHaveBeenCalled(); }); it('throws an error for non-export reference', () => { @@ -566,38 +555,22 @@ describe('vat store methods', () => { }); } - it("decrements the decider refcount for the peer's promise exports", () => { + it("releases the peer's promise exports through the c-list", () => { seedClist([['rp+1', 'kp123']]); - mockGetKernelPromise.mockReturnValue({ decider: endpointId }); vatMethods.forgetEndpointImports(endpointId); + // The caller rejected the promises the peer was deciding first, which + // released the unsettled-promise reference; the entry's own reference is + // all that is left, and deleteCListEntry releases it. expect(mockDeleteCListEntry).toHaveBeenCalledWith( endpointId, 'kp123', 'rp+1', ); - expect(mockDecrementRefCount).toHaveBeenCalledWith( - 'kp123', - 'cleanup|peerRestart|promise|decider', - ); }); - it('skips the decider decrement when the peer is no longer the decider', () => { - seedClist([['rp+1', 'kp123']]); - mockGetKernelPromise.mockReturnValue({ decider: 'someoneElse' }); - - vatMethods.forgetEndpointImports(endpointId); - - expect(mockDeleteCListEntry).toHaveBeenCalledWith( - endpointId, - 'kp123', - 'rp+1', - ); - expect(mockDecrementRefCount).not.toHaveBeenCalled(); - }); - - it("releases the peer's object exports: owner, c-list, baseline refcount, GC", () => { + it("releases the peer's object exports: owner, c-list, GC", () => { seedClist([['ro+7', 'ko42']]); mockKV.set(`owner.ko42`, endpointId); mockGetReachableAndVatSlot.mockReturnValue({ vatSlot: 'ro+7' }); @@ -607,33 +580,29 @@ describe('vat store methods', () => { expect(mockKV.has(`owner.ko42`)).toBe(false); expect(mockKV.has(`slot.${endpointId}.ko42`)).toBe(false); expect(mockKV.has(`slot.${endpointId}.ro+7`)).toBe(false); - expect(mockDecrementRefCount).toHaveBeenCalledWith( - 'ko42', - 'cleanup|peerRestart|export|baseline', - ); expect(mockMaybeFreeKrefs.add).toHaveBeenCalledWith('ko42'); - // Object-export tear-down handles the c-list pair directly; we don't - // also call deleteCListEntry (which uses the recognizable-only path - // and would corrupt the count). + // An export entry carries no reference, so tearing it down changes no + // count; the object is simply orphaned for GC to retire. + expect(mockDecrementRefCount).not.toHaveBeenCalled(); expect(mockDeleteCListEntry).not.toHaveBeenCalled(); }); - it('preserves baseline refcount when ownership has migrated', () => { + it('leaves the owner mapping alone when ownership has migrated', () => { seedClist([['ro+7', 'ko42']]); mockKV.set(`owner.ko42`, 'someoneElse'); mockGetReachableAndVatSlot.mockReturnValue({ vatSlot: 'ro+7' }); vatMethods.forgetEndpointImports(endpointId); - // Foreign owner survives — the baseline reference is theirs now. expect(mockKV.get(`owner.ko42`)).toBe('someoneElse'); - // Our c-list pair is still torn down (the peer can't reach the kref - // through us anymore), but the refcount stays untouched so we don't - // corrupt the new owner's accounting. + // Our c-list pair is still torn down: the peer can't reach the kref + // through us anymore. expect(mockKV.has(`slot.${endpointId}.ko42`)).toBe(false); expect(mockKV.has(`slot.${endpointId}.ro+7`)).toBe(false); expect(mockDecrementRefCount).not.toHaveBeenCalled(); - expect(mockMaybeFreeKrefs.add).not.toHaveBeenCalled(); + // We dropped a reference to it, so it still goes to the collector — the + // new owner's mapping is what decides its fate from there. + expect(mockMaybeFreeKrefs.add).toHaveBeenCalledWith('ko42'); }); it('preserves our exports to the peer (import-direction entries)', () => { diff --git a/packages/ocap-kernel/src/store/methods/vat.ts b/packages/ocap-kernel/src/store/methods/vat.ts index 84e065ee55..bd4497dc5c 100644 --- a/packages/ocap-kernel/src/store/methods/vat.ts +++ b/packages/ocap-kernel/src/store/methods/vat.ts @@ -5,7 +5,7 @@ import { getCListMethods } from './clist.ts'; import { getObjectMethods } from './object.ts'; import { getPromiseMethods } from './promise.ts'; import { getReachableMethods } from './reachable.ts'; -import { getRefCountMethods } from './refcount.ts'; +import { getRemoteMethods } from './remote.ts'; import type { EndpointId, KRef, @@ -35,18 +35,15 @@ const VAT_CONFIG_BASE_LEN = VAT_CONFIG_BASE.length; // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getVatMethods(ctx: StoreContext) { const { kv } = ctx; - const { getPrefixedKeys, getSlotKey, getOwnerKey } = getBaseMethods(ctx.kv); + const { getPrefixedKeys, getSlotKey, getCListPrefix, getOwnerKey } = + getBaseMethods(ctx.kv); const { deleteCListEntry } = getCListMethods(ctx); const { getReachableAndVatSlot } = getReachableMethods(ctx); - const { - initKernelPromise, - setPromiseDecider, - getKernelPromise, - addPromiseSubscriber, - } = getPromiseMethods(ctx); + const { initKernelPromise, setPromiseDecider, addPromiseSubscriber } = + getPromiseMethods(ctx); const { initKernelObject } = getObjectMethods(ctx); const { addCListEntry } = getCListMethods(ctx); - const { incrementRefCount, decrementRefCount } = getRefCountMethods(ctx); + const { getRemoteIds } = getRemoteMethods(ctx); /** * Delete all persistent state associated with an endpoint. @@ -54,10 +51,7 @@ export function getVatMethods(ctx: StoreContext) { * @param endpointId - The endpoint whose state is to be deleted. */ function deleteEndpoint(endpointId: EndpointId): void { - for (const key of getPrefixedKeys(`cle.${endpointId}.`)) { - kv.delete(key); - } - for (const key of getPrefixedKeys(`clk.${endpointId}.`)) { + for (const key of getPrefixedKeys(getCListPrefix(endpointId))) { kv.delete(key); } kv.delete(`e.nextObjectId.${endpointId}`); @@ -131,14 +125,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 vat or remote 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); @@ -150,15 +147,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 count. `retireKernelObjects` deletes the object once it has queued a + * `retireImport` for each importer, so an importer missing from this list + * keeps a c-list entry naming an object that no longer exists — which nothing + * ever tears down, and which the refcount audit reports as dangling. * * @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; @@ -261,8 +262,8 @@ export function getVatMethods(ctx: StoreContext) { const { vatSlot } = getReachableAndVatSlot(vatID, kref); ctx.kv.delete(getSlotKey(vatID, kref)); ctx.kv.delete(getSlotKey(vatID, vatSlot)); - // Decrease refcounts that belonged to the terminating vat - decrementRefCount(kref, 'cleanup|export|baseline'); + // An export entry holds no count, so there is nothing to release; the + // object is now orphaned, and GC retires it once importers let go. ctx.maybeFreeKrefs.add(kref); work.exports += 1; } @@ -279,20 +280,16 @@ export function getVatMethods(ctx: StoreContext) { work.imports += 1; } - // The caller used enumeratePromisesByDecider() before calling us, - // so they have already rejected the orphan promises, but those - // kpids are still present 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); const vref = key.slice(clistPrefix.length) as ERef; // the following will also delete both db keys deleteCListEntry(vatID, krefStr, vref); - // If the dead vat was still the decider, drop the decider’s refcount, too. - const kp = getKernelPromise(krefStr); - if (kp.decider === vatID) { - decrementRefCount(krefStr, 'cleanup|promise|decider'); - } work.promises += 1; } @@ -374,49 +371,28 @@ export function getVatMethods(ctx: StoreContext) { } const { isPromise } = parseRef(eref); if (isPromise) { - // deleteCListEntry decrements the promise refcount via the - // recognizable path. Additionally, if the endpoint was still - // recorded as decider, drop the decider's reference too. + // The caller already rejected the promises this endpoint was deciding, + // so only the c-list entry's own reference is left. deleteCListEntry(endpointId, kref, eref); - const kp = getKernelPromise(kref); - if (kp.decider === endpointId) { - decrementRefCount(kref, 'cleanup|peerRestart|promise|decider'); - } } else { // Object exports: drop the owner mapping if it still names the - // restarting endpoint, decrement the baseline refcount the kernel - // implicitly held while the endpoint owned the object, and queue - // it for GC. Then tear down the c-list pair. - // - // We deliberately do NOT call deleteCListEntry here: that path uses - // `onlyRecognizable: true`, which is the right semantics for an - // endpoint dropping its imports but the wrong semantics for - // releasing an export the endpoint owned. The baseline decrement - // below corresponds to the implicit reference exportFromEndpoint - // installed when the kernel object was first created. + // restarting endpoint, tear down the c-list pair, and queue the object + // for GC. An export entry holds no count, so this changes none. If + // ownership has migrated (e.g. a kernel-internal handoff), leave the + // new owner's mapping alone: the kref is theirs from here. const ownerKey = getOwnerKey(kref); const currentOwner = ctx.kv.get(ownerKey); - const stillOwned = currentOwner === endpointId; - if (stillOwned) { + if (currentOwner === endpointId) { ctx.kv.delete(ownerKey); } else if (currentOwner !== undefined) { - // Ownership has migrated (e.g. via a kernel-internal handoff). - // The baseline reference is now owed to the new owner; do not - // decrement against their accounting. Tear down our c-list pair - // and stop — the new owner is responsible for the kref's lifetime. ctx.logger?.warn( `forgetEndpointImports: kref ${kref} was exported by ${endpointId} ` + - `but is now owned by ${currentOwner}; preserving baseline refcount`, + `but is now owned by ${currentOwner}`, ); - const { vatSlot } = getReachableAndVatSlot(endpointId, kref); - ctx.kv.delete(getSlotKey(endpointId, kref)); - ctx.kv.delete(getSlotKey(endpointId, vatSlot)); - continue; } const { vatSlot } = getReachableAndVatSlot(endpointId, kref); ctx.kv.delete(getSlotKey(endpointId, kref)); ctx.kv.delete(getSlotKey(endpointId, vatSlot)); - decrementRefCount(kref, 'cleanup|peerRestart|export|baseline'); ctx.maybeFreeKrefs.add(kref); } } @@ -441,11 +417,9 @@ export function getVatMethods(ctx: StoreContext) { } else { kref = initKernelObject(endpointId); } + // addCListEntry takes the entry's reference: none for an object, since the + // owner is not one of its referrers, and one for a promise. addCListEntry(endpointId, kref, eref); - incrementRefCount(kref, 'export', { - isExport: true, - onlyRecognizable: true, - }); ctx.logger?.debug('exportFromEndpoint', endpointId, eref, kref); if (context === 'remote' && isPromise) { addPromiseSubscriber(endpointId, kref); diff --git a/packages/ocap-kernel/src/store/types.ts b/packages/ocap-kernel/src/store/types.ts index 1ea54d19cc..fcb8319009 100644 --- a/packages/ocap-kernel/src/store/types.ts +++ b/packages/ocap-kernel/src/store/types.ts @@ -11,6 +11,7 @@ export type StoreContext = { runQueue: StoredQueue; // Holds RunAction[] runQueueLengthCache: number; // Holds number refreshRunQueue: () => void; + refreshCachedValues: () => void; nextObjectId: StoredValue; // Holds string nextPromiseId: StoredValue; // Holds string nextVatId: StoredValue; // Holds string @@ -22,14 +23,26 @@ export type StoreContext = { inCrank: boolean; crankSettled?: Promise; resolveCrank?: (() => void) | undefined; - savepoints: string[]; + savepoints: Savepoint[]; crankBuffer: CrankBufferItem[]; // Buffer for sends and notifications during crank subclusters: StoredValue; // Holds Subcluster[] nextSubclusterId: StoredValue; // Holds string vatToSubclusterMap: StoredValue; // Holds Record + refCountAuditingEnabled: boolean; // If true, verify refcounts against ground truth every crank logger?: Logger | undefined; }; +/** + * A database savepoint, paired with the RAM state a database rollback cannot + * reach. `maybeFreeKrefs` is the collection-candidate set as it stood when the + * savepoint was taken, so a rollback can put back exactly what the abandoned + * work added and no more. + */ +export type Savepoint = { + name: string; + maybeFreeKrefs: Set; +}; + export type StoredValue = { get(): string | undefined; set(newValue: string): void; diff --git a/packages/ocap-kernel/src/types.ts b/packages/ocap-kernel/src/types.ts index aeefa76800..54e86d312c 100644 --- a/packages/ocap-kernel/src/types.ts +++ b/packages/ocap-kernel/src/types.ts @@ -376,11 +376,27 @@ export type RunQueueItemBringOutYourDead = Infer< typeof RunQueueItemBringOutYourDeadStruct >; +/** + * A request to replace a vat's worker, queued so the run loop performs it. + * + * Queued rather than done where it is asked for, because the run loop is then the + * only thing that takes a vat out of the kernel's reach: no crank can observe the + * vat mid-replacement, and the vat is idle when it happens, since the crank doing + * the work is the one that would otherwise be delivering to it. + */ +const RunQueueItemRestartVatStruct = object({ + type: literal('restartVat'), + vatId: VatIdStruct, +}); + +export type RunQueueItemRestartVat = Infer; + export const RunQueueItemStruct = union([ RunQueueItemSendStruct, RunQueueItemNotifyStruct, RunQueueItemGCActionStruct, RunQueueItemBringOutYourDeadStruct, + RunQueueItemRestartVatStruct, ]); export type RunQueueItem = Infer; diff --git a/packages/ocap-kernel/src/vats/VatManager.test.ts b/packages/ocap-kernel/src/vats/VatManager.test.ts index d5e92b1aac..9bae4e06f3 100644 --- a/packages/ocap-kernel/src/vats/VatManager.test.ts +++ b/packages/ocap-kernel/src/vats/VatManager.test.ts @@ -15,6 +15,17 @@ import type { VatId, VatConfig, PlatformServices } from '../types.ts'; import { VatHandle } from './VatHandle.ts'; import { VatManager } from './VatManager.ts'; +/** + * Let the pending microtasks run, so an operation under test gets as far as its + * first real await. + * + * @returns A promise that resolves once the microtask queue has drained. + */ +const drainMicrotasks = async (): Promise => + new Promise((resolve) => { + setTimeout(resolve, 0); + }); + describe('VatManager', () => { let mockPlatformServices: Mocked; let mockKernelStore: Mocked; @@ -70,6 +81,8 @@ describe('VatManager', () => { ), getVatSubcluster: vi.fn().mockReturnValue('s1'), markVatAsTerminated: vi.fn(), + deleteVat: vi.fn(), + getPromisesByDecider: vi.fn().mockReturnValue([]), getRootObject: vi.fn().mockReturnValue('ko1'), pinObject: vi.fn(), unpinObject: vi.fn(), @@ -80,6 +93,16 @@ describe('VatManager', () => { mockKernelQueue = { waitForCrank: vi.fn().mockResolvedValue(undefined), + resolvePromises: vi.fn(), + // A restart is the run loop's work, so stand in for it reaching the + // request on its next crank. Nothing is expected to come back out: + // `performVatRestart` reports a failure through the waiter `restartVat` + // registered, precisely so that it never takes the crank down. The catch + // is here so that a regression on that shows up as a failing assertion + // rather than an unhandled rejection. + enqueueRestartVat: vi.fn((vatId: VatId) => { + vatManager.performVatRestart(vatId).catch(() => undefined); + }), } as unknown as Mocked; mockLogger = new Logger('test'); @@ -319,6 +342,32 @@ describe('VatManager', () => { expect.objectContaining({ message: 'Vat termination: Custom reason' }), ); }); + + it('waits out the crank in flight before recording the vat as in flux', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + let finishCrank!: () => void; + ( + mockKernelQueue.waitForCrank as unknown as MockInstance + ).mockReturnValueOnce( + new Promise((resolve) => { + finishCrank = resolve; + }), + ); + + const terminated = vatManager.terminateVat('v1'); + await drainMicrotasks(); + + // Recording first and waiting after would deadlock, and this is the + // assertion that catches it: a crank already running reaches its endpoint + // lookup here, and would find a record whose teardown is waiting for that + // same crank to end. Reverse the order in `#trackFlux` and this hangs. + expect(await vatManager.provideVat('v1')).toBe(vatHandles[0]); + + finishCrank(); + await terminated; + + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + }); }); describe('restartVat', () => { @@ -329,7 +378,7 @@ describe('VatManager', () => { const result = await vatManager.restartVat('v1'); - expect(mockKernelQueue.waitForCrank).toHaveBeenCalled(); + expect(mockKernelQueue.enqueueRestartVat).toHaveBeenCalledWith('v1'); expect(originalHandle?.terminate).toHaveBeenCalledWith(false, undefined); expect(mockPlatformServices.launch).toHaveBeenCalledTimes(2); expect(makeVatHandleMock).toHaveBeenCalledTimes(2); @@ -343,6 +392,178 @@ describe('VatManager', () => { VatNotFoundError, ); }); + + it('marks a vat terminated when its relaunch fails', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + makeVatHandleMock.mockRejectedValueOnce(new Error('worker died')); + + await expect(vatManager.restartVat('v1')).rejects.toThrow('worker died'); + + // Nothing else reclaims a vat with no worker that the store still counts + // among the living. + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + await expect(vatManager.provideVat('v1')).rejects.toThrow( + VatNotFoundError, + ); + }); + + it('releases the root pin when its relaunch fails', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + makeVatHandleMock.mockRejectedValueOnce(new Error('worker died')); + + await expect(vatManager.restartVat('v1')).rejects.toThrow('worker died'); + + // The restart's `stopVat` was told the vat was coming back, so it kept the + // pin, and vat cleanup does not release pins. Without this the root's + // refcount is held for the life of the kernel. + expect(mockKernelStore.unpinObject).toHaveBeenCalledWith('ko1'); + }); + + // The crank has to commit for those records to survive. Thrown instead, the + // run loop's catch rolls the crank back — unmarking the vat, re-pinning its + // root, and returning this very request to the run queue, so the next + // process start dequeues it and fails the same way, forever. + it('reports a failed relaunch without taking the crank down', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + makeVatHandleMock.mockRejectedValueOnce(new Error('worker died')); + const restarted = vatManager.restartVat('v1'); + + await expect(restarted).rejects.toThrow('worker died'); + // The caller heard about it; the crank did not. + expect(await vatManager.performVatRestart('v1')).toBeUndefined(); + }); + + it('rejects the promises a vat was deciding when its relaunch fails', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + ( + mockKernelStore.getPromisesByDecider as unknown as MockInstance + ).mockReturnValue(['kp1']); + makeVatHandleMock.mockRejectedValueOnce(new Error('worker died')); + + await expect(vatManager.restartVat('v1')).rejects.toThrow('worker died'); + + // Nothing else will ever decide them: the incarnation that owed them is + // gone and cleanup only tears the c-list down. + expect(mockKernelQueue.resolvePromises).toHaveBeenCalledWith('v1', [ + ['kp1', true, expect.objectContaining({ body: expect.any(String) })], + ]); + }); + + // Both are exposed as RPCs, and `terminateVat` does not go through the run + // queue, so it lands in the window between the request and the crank. + it('drops a queued restart for a vat that was terminated first', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + ( + mockKernelQueue.enqueueRestartVat as unknown as MockInstance + ).mockImplementation(() => undefined); + const restarted = vatManager.restartVat('v1'); + + await vatManager.terminateVat('v1'); + + // The caller is told, rather than left waiting on a request nothing will + // carry out. + await expect(restarted).rejects.toThrow(VatDeletedError); + // And the request itself goes quietly when the run loop reaches it. A + // throw here is a dead kernel: `#restartVatWorker` does not catch. + expect(await vatManager.performVatRestart('v1')).toBeUndefined(); + }); + + it('does not strand a waiter when the request cannot be queued', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + ( + mockKernelQueue.enqueueRestartVat as unknown as MockInstance + ).mockImplementationOnce(() => { + throw new Error('run loop died'); + }); + + await expect(vatManager.restartVat('v1')).rejects.toThrow( + 'run loop died', + ); + + // Left registered, the next request would reject it as superseded — and + // nobody ever awaited it, so that rejection goes unhandled. + ( + mockKernelQueue.enqueueRestartVat as unknown as MockInstance + ).mockImplementation(() => undefined); + const second = vatManager.restartVat('v1'); + await vatManager.performVatRestart('v1'); + expect(await second).toBe(vatHandles[1]); + }); + + it('leaves the vat in place until the run loop takes the request', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + const originalHandle = vatHandles[0]; + // Queue the request without standing in for the run loop. + ( + mockKernelQueue.enqueueRestartVat as unknown as MockInstance + ).mockImplementation(() => undefined); + + const restarted = vatManager.restartVat('v1'); + await drainMicrotasks(); + + // The vat is only ever out of reach inside the crank that carries the + // request out, where no other crank can see it. + expect(vatManager.getVat('v1')).toBe(originalHandle); + expect(originalHandle?.terminate).not.toHaveBeenCalled(); + + await vatManager.performVatRestart('v1'); + + expect(await restarted).toBe(vatHandles[1]); + }); + + it('supersedes a caller waiting on an earlier request for the same vat', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + ( + mockKernelQueue.enqueueRestartVat as unknown as MockInstance + ).mockImplementation(() => undefined); + + const first = vatManager.restartVat('v1'); + const second = vatManager.restartVat('v1'); + + // One waiter per vat, so the earlier caller is told rather than left + // waiting on a restart the later one will consume. + await expect(first).rejects.toThrow('superseded'); + await vatManager.performVatRestart('v1'); + expect(await second).toBe(vatHandles[1]); + }); + }); + + describe('provideVat', () => { + it('returns the running handle when the vat is not in flux', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + + expect(await vatManager.provideVat('v1')).toBe(vatHandles[0]); + }); + + it('throws if vat not found', async () => { + await expect(vatManager.provideVat('v1')).rejects.toThrow( + VatNotFoundError, + ); + }); + + it('reports a vat gone only once its termination has been recorded', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + let finishStop!: () => void; + (vatHandles[0]?.terminate as unknown as MockInstance).mockImplementation( + async () => + new Promise((resolve) => { + finishStop = resolve; + }), + ); + + const terminated = vatManager.terminateVat('v1'); + await drainMicrotasks(); + const provided = vatManager.provideVat('v1'); + + finishStop(); + + await expect(provided).rejects.toThrow(VatNotFoundError); + // The store agrees by the time a waiter is told, so a caller acting on + // "gone" — releasing the kernel's side of a GC action, say — is acting on + // a vat the store also calls terminated. + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + await terminated; + }); }); describe('pingVat', () => { diff --git a/packages/ocap-kernel/src/vats/VatManager.ts b/packages/ocap-kernel/src/vats/VatManager.ts index b90f6f30c8..039ed7c940 100644 --- a/packages/ocap-kernel/src/vats/VatManager.ts +++ b/packages/ocap-kernel/src/vats/VatManager.ts @@ -1,4 +1,5 @@ import type { CapData } from '@endo/marshal'; +import { makePromiseKit } from '@endo/promise-kit'; import { VatAlreadyExistsError, VatDeletedError, @@ -8,6 +9,7 @@ import { stringify } from '@metamask/kernel-utils'; import { Logger, splitLoggerStream } from '@metamask/logger'; import type { KernelQueue } from '../KernelQueue.ts'; +import { makeKernelError } from '../liveslots/kernel-marshal.ts'; import type { KernelStore } from '../store/index.ts'; import type { VatId, @@ -36,6 +38,36 @@ export class VatManager { /** Currently running vats, by ID */ readonly #vats: Map; + /** + * Vats being torn down, by ID, each mapped to a promise for the teardown. + * {@link provideVat} waits on these, which is what keeps the kernel's answer + * about a dying vat in step with the store's: by the time a waiter is told the + * vat is gone, it is marked terminated, and callers that must tell "terminated" + * from "missing" — {@link KernelRouter}'s endpoint lookup above all — get the + * former rather than a disagreement to raise. + * + * Recorded rather than guarded against: the run loop is free to run cranks + * throughout, and a delivery that arrives mid-flux waits for the vat instead + * of the flux waiting for the run loop. Inverted the other way — a lock the + * operation holds while the loop stands still — the holder must never await + * anything the run loop has to deliver, which is a much sharper edge. + * + * Only termination goes through here. A restart is queued for the run loop + * (see {@link restartVat}), which leaves no window at all; termination cannot + * be, because it has to work on a kernel whose run loop has died. + */ + readonly #vatsInFlux: Map>; + + /** + * Callers waiting for the run loop to carry out a queued restart, by vat ID. + * In RAM only: a request that outlives the kernel that queued it is still in + * the run queue, and is carried out with nobody left to tell. + */ + readonly #restartWaiters: Map< + VatId, + { resolve: () => void; reject: (error: unknown) => void } + >; + /** Service to spawn workers (in iframes) for vats to run in */ readonly #platformServices: PlatformServices; @@ -69,6 +101,8 @@ export class VatManager { allowedGlobalNames, }: VatManagerOptions) { this.#vats = new Map(); + this.#vatsInFlux = new Map(); + this.#restartWaiters = new Map(); this.#platformServices = platformServices; this.#kernelStore = kernelStore; this.#kernelQueue = kernelQueue; @@ -123,13 +157,42 @@ export class VatManager { cause: error, }); } - this.#kernelStore.initEndpoint(vatId); - const rootRef = this.#kernelStore.exportFromEndpoint( - vatId, - ROOT_OBJECT_VREF, - ); - 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` tears down the worker and releases the root pin, but no more. + // 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 }, + ); + } } /** @@ -186,6 +249,10 @@ export class VatManager { } else if (terminating) { terminationError = new VatDeletedError(vatId); } + if (terminating) { + // A restart keeps the pin: the same root comes back. + this.#unpinVatRoot(vatId); + } await this.#platformServices .terminate(vatId, terminationError) .catch(this.#logger.error); @@ -200,24 +267,250 @@ export class VatManager { * @param reason - If the vat is being terminated, the reason for the termination. */ async terminateVat(vatId: VatId, reason?: CapData): Promise { - await this.#kernelQueue.waitForCrank(); - await this.stopVat(vatId, true, reason); - // Mark for deletion (which will happen later, in vat-cleanup events) - this.#kernelStore.markVatAsTerminated(vatId); + // A restart still queued for this vat is overtaken by the termination, and + // will be dropped when the run loop reaches it. Tell whoever asked for it + // now, rather than leaving them waiting on a request that can no longer be + // carried out. + const superseded = this.#restartWaiters.get(vatId); + this.#restartWaiters.delete(vatId); + superseded?.reject(new VatDeletedError(vatId)); + // Not queued for the run loop the way `restartVat` is: teardown has to work + // on a kernel whose run loop has died, which `reset` depends on. So this one + // closes its window with a flux record instead. + await this.#trackFlux(vatId, async () => this.#endVat(vatId, reason)); + } + + /** + * Take a vat's worker down and mark the vat for cleanup. + * + * @param vatId - The ID of the vat. + * @param reason - The reason for the termination, if there is one. + */ + async #endVat(vatId: VatId, reason?: CapData): Promise { + try { + await this.stopVat(vatId, true, reason); + } finally { + // Mark for deletion (which will happen later, in vat-cleanup events). Not + // marked before `stopVat`, even though that would close the same window + // this method's flux record closes: the mark makes the vat eligible for + // `nextTerminatedVatCleanup`, which would wipe the c-list from under a + // worker that is still being shut down. + // + // In a `finally`, because a teardown that throws leaves the worker just as + // dead — `stopVat` asks the platform to kill it before anything here can + // fail — while an unmarked vat is one nothing ever reclaims, and one that + // `#resolveEndpoint` would go on reading as a live vat the kernel has + // merely lost track of. + this.#vats.delete(vatId); + this.#kernelStore.markVatAsTerminated(vatId); + } } /** * Restarts a vat. * + * Asks the run loop to do it, rather than doing it here. A restart keeps the + * vat's c-list while taking the vat itself out of the kernel's reach for as + * long as launching a worker and negotiating with it takes, and doing that + * alongside a running run loop means a crank can land in the window and read a + * live vat as a dead one. In a crank of its own there is no window: the run + * loop is the only thing that delivers, and it is here instead. + * * @param vatId - The ID of the vat. * @returns A promise for the restarted vat. */ async restartVat(vatId: VatId): Promise { + // Rejects an unknown vat here rather than from inside a crank, where the + // caller could only be told by way of a dead run loop. + this.getVat(vatId); + const restarted = this.#awaitRestart(vatId); + try { + this.#kernelQueue.enqueueRestartVat(vatId); + } catch (error) { + // Nothing was queued, so nothing will ever settle the waiter just + // registered. Take it back out: left behind, the next request for this vat + // would reject it as superseded, and since this caller never got as far as + // awaiting it that rejection would go unhandled. + this.#restartWaiters.delete(vatId); + throw error; + } + await restarted; + return this.getVat(vatId); + } + + /** + * Replace a vat's worker. Called by the run loop, for a queued restart request. + * + * @param vatId - The ID of the vat. + */ + async performVatRestart(vatId: VatId): Promise { + const settle = this.#restartWaiters.get(vatId); + this.#restartWaiters.delete(vatId); + if (!this.#vats.has(vatId)) { + // The vat went away between the request and this crank. `terminateVat` + // does not go through the run queue, so it can land in that window, and a + // request for a vat that no longer exists has nothing to carry out and + // nothing to put right. Dropped rather than thrown: the alternative is a + // dead run loop over work that is merely obsolete. + const error = new VatNotFoundError(vatId); + this.#logger.error( + `Restart of vat ${vatId} dropped; the vat is gone:`, + error, + ); + settle?.reject(error); + return; + } + try { + // Read before the handle goes away, and from the handle rather than the + // store, so the incarnation that comes back is configured like the one + // that left. + const { config } = this.getVat(vatId); + await this.stopVat(vatId, false); + await this.runVat(vatId, config); + } catch (error) { + // The vat has no worker and is not coming back, so it is terminated in + // fact; record that so the rest of the kernel agrees. This must not throw + // out of the crank, and not only to keep the run loop alive: the run + // loop's catch rolls the crank back, which would undo the very records + // written here *and* restore this request to the run queue, so the next + // process start would replay the same failing restart forever. + this.#abandonVat(vatId, error); + this.#logger.error( + `Restart of vat ${vatId} failed; terminating it:`, + error, + ); + settle?.reject(error); + return; + } + settle?.resolve(); + } + + /** + * Give up on a vat whose worker is gone and which has no successor coming, + * leaving the store agreeing with that. + * + * Does what `VatHandle.terminate(true)` does for a vat that still has a handle + * to do it with: rejects the promises the vat was deciding, so subscribers are + * told rather than left waiting on a decider that no longer exists, and drops + * the records that would otherwise make the vat look live. Marking is what + * makes the c-list reclaimable — `cleanupTerminatedVat` only visits vats that + * are marked. + * + * @param vatId - The vat to give up on. + * @param error - Why it is being given up on. + */ + #abandonVat(vatId: VatId, error: unknown): void { + const failure = makeKernelError( + 'VAT_TERMINATED', + error instanceof Error ? error.message : String(error), + ); + for (const kpid of this.#kernelStore.getPromisesByDecider(vatId)) { + this.#kernelQueue.resolvePromises(vatId, [[kpid, true, failure]]); + } + this.#vats.delete(vatId); + // By hand, because `stopVat` drops the pin only when it is the one ending + // the vat and it was told this vat was coming back; vat cleanup does not + // touch pins at all. Left alone, it holds the root's refcount for the life + // of the kernel. + this.#unpinVatRoot(vatId); + this.#kernelStore.deleteVat(vatId); + this.#kernelStore.markVatAsTerminated(vatId); + } + + /** + * Release the pin `launchVat` took on a vat's root, so the root can be + * collected once its importers let go. + * + * @param vatId - The vat whose root is to be unpinned. + */ + #unpinVatRoot(vatId: VatId): void { + const rootRef = this.#kernelStore.getRootObject(vatId); + if (rootRef) { + this.#kernelStore.unpinObject(rootRef); + } + } + + /** + * Wait for the run loop to carry out this vat's queued restart. + * + * Registered before the request is enqueued, so a crank cannot complete the + * restart before there is anything to tell. A request that outlives the kernel + * that queued it has no waiter when the new one gets to it, which is why + * settling is optional. + * + * @param vatId - The vat being restarted. + * @returns A promise that settles when the restart does. + */ + async #awaitRestart(vatId: VatId): Promise { + const { promise, resolve, reject } = makePromiseKit(); + // One waiter per vat: a second request for a vat already awaiting one would + // otherwise strand the first caller forever. + this.#restartWaiters + .get(vatId) + ?.reject(new Error(`Restart of vat ${vatId} superseded by a later one`)); + this.#restartWaiters.set(vatId, { resolve, reject }); + return await promise; + } + + /** + * Run an operation that takes a vat out of the kernel's reach, recording the + * vat as mid-flux for its duration so a delivery arriving meanwhile waits for + * the outcome instead of reading the vat as gone. + * + * Both steps live here, in this order, because the order is the whole + * mechanism and reversing it deadlocks. See the comments inline; a caller + * cannot get it wrong because a caller does not sequence it. + * + * @param vatId - The vat being taken out of reach. + * @param start - Begins the operation. Called once, after the wait. + * @returns The operation's own result, failure included. + */ + async #trackFlux(vatId: VatId, start: () => Promise): Promise { + // First: wait out the crank in flight, so the operation does not pull a + // worker out from under a delivery. This has to happen *before* the record + // exists. A crank that is already running has not necessarily reached its + // endpoint lookup yet, so if the record were there it would find it and wait + // for this operation — which is waiting for that crank to end. await this.#kernelQueue.waitForCrank(); - const vat = this.getVat(vatId); - const { config } = vat; - await this.stopVat(vatId, false); - await this.runVat(vatId, config); + const flux = start(); + // Second: record, with neither `start()` nor this function having awaited + // since, so no crank can run between the operation's first step and the + // record. An await introduced between these two lines reopens the window the + // record exists to close. + // + // Waiters see a plain completion rather than a failure, because the vat ends + // up marked terminated either way — `#endVat` marks it in a `finally` — and + // "gone" is what they should act on. The caller still gets the failure, from + // `flux` itself. + this.#vatsInFlux.set( + vatId, + flux.catch(() => undefined), + ); + try { + return await flux; + } finally { + this.#vatsInFlux.delete(vatId); + } + } + + /** + * The handle for a vat, waiting first for any teardown in flight. The + * counterpart to {@link getVat} for callers that can afford to wait — a crank, + * above all, which would otherwise be told a vat is missing before the store + * records why. + * + * @param vatId - The ID of the vat. + * @returns A promise for the vat's handle. + * @throws If the vat does not exist, or stopped existing while being awaited. + */ + async provideVat(vatId: VatId): Promise { + const flux = this.#vatsInFlux.get(vatId); + if (flux) { + // Only a teardown is ever recorded, so waiting it out settles the vat's + // fate: it is gone, and the store now says so. + await flux; + throw new VatNotFoundError(vatId); + } return this.getVat(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),