diff --git a/packages/extension/test/e2e/control-panel.test.ts b/packages/extension/test/e2e/control-panel.test.ts index 167429e76..754a4017b 100644 --- a/packages/extension/test/e2e/control-panel.test.ts +++ b/packages/extension/test/e2e/control-panel.test.ts @@ -252,7 +252,6 @@ test.describe('Control Panel', () => { `{"key":"v3.c.o+0","value":"${v3Root}"}`, `{"key":"v3.c.${v3Promise}","value":"R p-1"}`, `{"key":"v3.c.p-1","value":"${v3Promise}"}`, - `{"key":"${v3Root}.refCount","value":"1,1"}`, `{"key":"${v3Promise}.refCount","value":"2"}`, ]; // Derived too: v1 imports the two roots as the bootstrap's calls are diff --git a/packages/kernel-test/src/garbage-collection.test.ts b/packages/kernel-test/src/garbage-collection.test.ts index 268ed7920..67055414f 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,116 @@ 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. + * + * @param vatId - The vat to reap. + * @param rootKRef - That vat's root, to poke with cranks afterwards. + */ + async function reapAndSettle(vatId: VatId, rootKRef: KRef): Promise { + kernel.reapVats((id) => id === vatId); + for (let i = 0; i < 3; i++) { + await kernel.queueMessage(rootKRef, 'noop', []); + await waitUntilQuiescent(500); + } + } + + 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 e8b6e2781..b1c91f3f4 100644 --- a/packages/kernel-test/src/persistence.test.ts +++ b/packages/kernel-test/src/persistence.test.ts @@ -176,7 +176,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/utils.ts b/packages/kernel-test/src/utils.ts index c255347b8..7d867f2c2 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 8af7998ec..c48411a0f 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -32,10 +32,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Log a warning when a vat requests an unknown global - Export `OcapURLIssuerService` and `OcapURLRedemptionService` types so vats can type the corresponding kernel-service endowments ([#952](https://github.com/MetaMask/ocap-kernel/pull/952)) - Reference-marker sigil (`@@NAME`) at the `queueMessage` RPC boundary lets JSON-RPC callers name a live kernel object as a call argument ([#984](https://github.com/MetaMask/ocap-kernel/pull/984)) + - 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: `auditRefCounts`, `recomputeRefCounts`, `formatRefCountViolations`, `assertRefCountsIfAuditing`, and `setRefCountAuditing` on the kernel store, plus a `Kernel.make` option `auditRefCounts` that verifies every kref's counts against the references the kernel actually holds at the end of each crank ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - Reports drift in both directions: counts too low (a live capability can be collected) and counts too high (a leak) + - Only references visible in the kernel's own state are checkable, so a holder that keeps a kref outside them has to take a pin to be counted at all + - `recomputeRefCounts` is a repair tool for a drifted store, offered to embedders and never run automatically: opening an existing store does not migrate it + - Exports the `RefCountViolation` type +- Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- Add `getOcapURLObjects` and `retainForOcapURL` to the kernel store, and `VatManager.releaseVatRootPin` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + ### Changed - **BREAKING:** `Kernel.make`'s `ioChannelFactory` option is now `ioListenerFactory`, and the exported `IOChannelFactory` type is replaced by `IOListener` and `IOListenerFactory`. A cluster config's `io` entries now create listeners; vats call `accept()` to obtain a channel instead of reading and writing the endowment directly ([#1007](https://github.com/MetaMask/ocap-kernel/pull/1007)) @@ -49,7 +58,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - A message delivered to a kernel-owned kref with no registered service now rejects the caller with `ENDPOINT_UNREACHABLE` instead of throwing, which escaped the crank and killed the run loop — turning one unreachable reference into a dead kernel ([#1007](https://github.com/MetaMask/ocap-kernel/pull/1007)) - - Reachable without any kernel bug: an anonymous kernel object hosts something that cannot outlive the process, such as an accepted socket connection, so a vat holding one across a restart or a message to one still queued from the previous incarnation lands here. Kernel objects are born with a `(1, 1)` refcount, so the init sweep cannot delete such an object and its `kernel` owner survives (see [#1006](https://github.com/MetaMask/ocap-kernel/issues/1006)) + - Reachable without any kernel bug: an anonymous kernel object hosts something that cannot outlive the process, such as an accepted socket connection, so a vat holding one across a restart or a message to one still queued from the previous incarnation lands here. That surviving reference is exactly what stops the init sweep deleting the object, so its `kernel` owner survives with it - Matches what `KernelRouter` already does for a delivery whose endpoint has vanished. A message sent with no result promise has nobody to report to, so it is logged instead - The kernel run queue no longer strands messages, going quiet with no error, no log, and no crash. `runQueueLengthCache` uses a negative value to mean "unknown, re-read from the database", but `enqueueRun`/`dequeueRun` adjusted it arithmetically without materializing it first — so an enqueue while the cache held `-1` produced `0` for a queue that actually held an item, and because `0` is not negative it was never re-read again. The run loop then saw an empty queue, went to sleep, and stranded everything behind it. Two paths reach that `-1`: kernel startup, and `rollbackCrank`, which invalidates the cache because a rollback may have restored dequeued items. The rollback path is the more likely of the two in practice, since a rollback is normally followed immediately by enqueueing an error or termination message. The run loop is also now woken by any non-empty queue rather than only by the empty-to-one transition, so a drifted count cannot silently lose the wakeup either ([#1007](https://github.com/MetaMask/ocap-kernel/pull/1007)) - Stop reporting a healthy kernel after the run loop dies ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) @@ -65,6 +74,31 @@ 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 ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - `initKernelObject` now births objects at `(0, 0)` instead of `(1, 1)`. The old constant made the arithmetic come out right for exactly one importer, masking the missing increment; with two importers a live capability could be dropped and retired out from under a holder + - Removes the owner-side baseline decrements in `cleanupTerminatedVat` and `forgetEndpointImports`, which double-claimed the same unit an importer's drop also spent — the source of `"koNN" underflow -1,0` escaping mid-cleanup and leaving a vat half-cleaned + - `translateRefKtoE` now re-establishes reachability, so a vat handed an object it previously dropped is counted as holding it live again + - Renames `krefsToExistingErefs` to `krefsToErefs`, which now throws on an unmapped kref instead of silently dropping it +- Pin vat root objects for the lifetime of their vat, and release the pin on termination ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - A root is addressable while its vat lives whether or not anyone imports it; the old `(1, 1)` birth baseline was standing in for this +- Garbage-collection action delivery now moves the kernel's own c-list: `dropExports` clears the owner's reachable flag and `retireExports`/`retireImports` tear the entry down ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - Previously the owner's flag never cleared, so the same action could be re-derived, and retired entries outlived the objects they named +- Charge a delivered message's target reference against the run-queue item's own target rather than the routed target, so a message routed through a resolved promise no longer decrements an object nobody charged while leaking the promise ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- Release a queued notification's reference before the paths that decide there is nothing to deliver, and stop decrementing references on promises retired alongside it that nobody had taken ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- Transfer, rather than duplicate, the references a message carries when it is queued on an unresolved promise and later re-enqueued on resolution ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- Fix the stale `cle.`/`clk.` key prefixes in `getPromisesByDecider` and `deleteEndpoint`, which no longer matched the `${endpointId}.c.` c-list layout ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - `getPromisesByDecider` matched nothing, so promises a terminating vat or restarting peer was deciding were never rejected +- Issuing an ocap URL now retains its target, so the URL stays redeemable ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - A URL carries its kref inside an encrypted bearer token and nothing else, so the kernel cannot see from its own state that a holder exists. Under the old `(1, 1)` birth baseline nothing exported was ever collectable and this went unnoticed; at `(0, 0)` the target is collected as soon as the message that carried it to the issuer is delivered, and the URL names a dead capability + - One pin per kref however many URLs name it, and no release: the token is persistent and unexpiring, so `revoke` is the way to kill the capability + - Issuing a URL for a kref the kernel has already deleted is now refused rather than resurrecting its counts + - The retention is taken before the token is minted, since minting awaits and a collection crank can run in that window; it is released again if minting fails, and only when that call was the one that took it +- Refuse to import a kref the kernel has deleted into an endpoint's c-list ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - `getObjectRefCount` reads a missing entry as `(0, 0)`, so the new entry's own increment wrote it back and resurrected a live-looking object with no owner — deliverable to by nobody, and endorsed by the audit, since the entry is a legitimate holder for exactly the count it finds +- Release a vat's root pin when a subcluster is deleted without its vats having run ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + + - `deleteSubcluster` bypasses `stopVat`, so nothing released the pin `launchVat` took in the incarnation that did run them, leaving the root's count permanently above zero and `pinnedObjects` naming a vat that no longer exists + - 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.ts b/packages/ocap-kernel/src/Kernel.ts index a312e60e6..c041d060b 100644 --- a/packages/ocap-kernel/src/Kernel.ts +++ b/packages/ocap-kernel/src/Kernel.ts @@ -109,6 +109,10 @@ 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. Intended for tests and debugging; the + * audit walks the whole store. */ // eslint-disable-next-line no-restricted-syntax private constructor( @@ -122,6 +126,7 @@ export class Kernel { ioListenerFactory?: IOListenerFactory; allowedGlobalNames?: AllowedGlobalName[]; onRunLoopFailure?: OnRunLoopFailure; + auditRefCounts?: boolean; } = {}, ) { this.#platformServices = platformServices; @@ -129,6 +134,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(); } @@ -168,6 +176,7 @@ export class Kernel { this.#ocapURLManager = new OcapURLManager({ remoteManager: this.#remoteManager, + kernelStore: this.#kernelStore, }); this.#kernelServiceManager = new KernelServiceManager({ @@ -249,6 +258,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 +274,7 @@ export class Kernel { systemSubclusters?: SystemSubclusterConfig[]; allowedGlobalNames?: AllowedGlobalName[]; onRunLoopFailure?: OnRunLoopFailure; + auditRefCounts?: boolean; } = {}, ): Promise { const kernel = new Kernel(platformServices, kernelDatabase, options); diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index d7beb0e2f..1b3bd4a35 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', diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index 3465e93cd..afda8139c 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(); } /** @@ -504,7 +505,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 ea833293a..11aa8922c 100644 --- a/packages/ocap-kernel/src/KernelRouter.test.ts +++ b/packages/ocap-kernel/src/KernelRouter.test.ts @@ -59,9 +59,12 @@ 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(), createCrankSavepoint: vi.fn(), } as unknown as KernelStore; @@ -283,8 +286,35 @@ 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('splats message when promise resolves to a non-object', async () => { @@ -649,6 +679,42 @@ 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'], + ]); + }, + ); }); describe('bringOutYourDead', () => { diff --git a/packages/ocap-kernel/src/KernelRouter.ts b/packages/ocap-kernel/src/KernelRouter.ts index 5cfb8335d..6bd080e7c 100644 --- a/packages/ocap-kernel/src/KernelRouter.ts +++ b/packages/ocap-kernel/src/KernelRouter.ts @@ -11,6 +11,7 @@ import { isPromiseRef } from './store/utils/promise-ref.ts'; import type { EndpointId, EndpointHandle, + ERef, KRef, KernelMessage, RunQueueItem, @@ -255,7 +256,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 +318,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,6 +378,9 @@ 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 }; @@ -385,16 +404,13 @@ 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(#1006 follow-up): SwingSet also tears down the c-list entry for each + // promise in the batch here, since the endpoint can never refer to a + // settled promise by that eref again. Left alone for now because the + // debug UI discovers exported ocap URLs by scanning these entries. 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; + return await endpoint.deliverNotify(resolutions); } /** @@ -409,7 +425,21 @@ export class KernelRouter { `@@@@ deliver ${endpointId} ${type} ${JSON.stringify(krefs)}`, ); const endpoint = this.#getEndpoint(endpointId); - const erefs = this.#kernelStore.krefsToExistingErefs(endpointId, krefs); + const erefs = this.#kernelStore.krefsToErefs(endpointId, krefs); + // Telling an endpoint to let go is also the kernel letting go. Otherwise a + // dropped export stays flagged reachable, so the same action gets derived + // again, and retired entries outlive the objects they name. + krefs.forEach((kref, index) => { + if (type === 'dropExports') { + this.#kernelStore.clearReachableFlag(endpointId, kref); + } else { + this.#kernelStore.deleteCListEntry( + endpointId, + kref, + erefs[index] as ERef, + ); + } + }); const method = `deliver${(type[0] as string).toUpperCase()}${type.slice(1)}` as | 'deliverDropExports' diff --git a/packages/ocap-kernel/src/KernelServiceManager.ts b/packages/ocap-kernel/src/KernelServiceManager.ts index bc236e611..6744c98c7 100644 --- a/packages/ocap-kernel/src/KernelServiceManager.ts +++ b/packages/ocap-kernel/src/KernelServiceManager.ts @@ -162,11 +162,10 @@ export class KernelServiceManager { * 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. + * once nothing references it, so a survivor that a vat import or a still + * queued message holds 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. @@ -196,8 +195,7 @@ export class KernelServiceManager { * * The kernel object itself is deleted here once nothing references it, * rather than being left to `collectGarbage`, which skips kernel-owned - * objects. With the current refcount baseline this branch does not fire; - * it is the correct place for the deletion once that changes (see #1006). + * objects. * * @param kref - The kref of the object to release. */ diff --git a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts index b97b79369..5bba9896a 100644 --- a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts @@ -7,6 +7,8 @@ import type { RemoteManager } from './RemoteManager.ts'; import { createMockRemotesFactory } from '../../../test/remotes-mocks.ts'; import type { SlotValue } from '../../liveslots/kernel-marshal.ts'; import { kslot } from '../../liveslots/kernel-marshal.ts'; +import type { KernelStore } from '../../store/index.ts'; +import type { KRef } from '../../types.ts'; import type { RemoteComms } from '../types.ts'; type RedeemService = { @@ -23,6 +25,12 @@ describe('OcapURLManager', () => { let mockRemoteComms: RemoteComms; let mockRemoteHandle: RemoteHandle; let mockFactory: ReturnType; + let mockKernelStore: KernelStore; + // Issuing retains the target, which requires it to exist, so mint real + // entities rather than naming krefs the kernel never had. + let objectKRef: KRef; + let otherObjectKRef: KRef; + let promiseKRef: KRef; beforeEach(() => { mockFactory = createMockRemotesFactory({ @@ -33,6 +41,10 @@ describe('OcapURLManager', () => { const mocks = mockFactory.makeOcapURLManagerMocks(); mockRemoteComms = mocks.remoteComms; mockRemoteHandle = mocks.remoteHandle; + mockKernelStore = mocks.kernelStore; + objectKRef = mockKernelStore.initKernelObject('kernel'); + otherObjectKRef = mockKernelStore.initKernelObject('kernel'); + [promiseKRef] = mockKernelStore.initKernelPromise(); mockRemoteManager = mocks.remoteManager as unknown as RemoteManager; // Override specific mock behaviors for this test @@ -48,6 +60,7 @@ describe('OcapURLManager', () => { ocapURLManager = new OcapURLManager({ remoteManager: mockRemoteManager, + kernelStore: mockKernelStore, }); }); @@ -92,8 +105,75 @@ describe('OcapURLManager', () => { }); describe('issueOcapURL', () => { + it('retains the target, so garbage collection cannot take it', async () => { + // The URL is the only holder, and it lives outside the store's reference + // graph, so without the pin the target collects and the URL goes dead. + mockKernelStore.incrementRefCount(objectKRef, 'queue|slot'); + await ocapURLManager.issueOcapURL(objectKRef); + mockKernelStore.decrementRefCount(objectKRef, 'deliver|send|slot'); + + expect(mockKernelStore.isObjectPinned(objectKRef)).toBe(true); + expect(mockKernelStore.getOcapURLObjects()).toStrictEqual([objectKRef]); + mockKernelStore.collectGarbage(); + expect([...mockKernelStore.getGCActions()]).toStrictEqual([]); + expect(mockKernelStore.kernelRefExists(objectKRef)).toBe(true); + }); + + it('retains a target named by several URLs only once', async () => { + await ocapURLManager.issueOcapURL(objectKRef); + await ocapURLManager.issueOcapURL(objectKRef); + + expect(mockKernelStore.getPinnedObjects()).toStrictEqual([objectKRef]); + expect(mockKernelStore.getObjectRefCount(objectKRef)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }); + + it('releases the retention when minting the URL fails', async () => { + // No URL exists to depend on the pin, and nothing else would ever release + // it: the rejection is reported to the caller, not thrown out of a crank. + vi.spyOn(mockRemoteComms, 'issueOcapURL').mockRejectedValue( + new Error('Issue failed'), + ); + + await expect(ocapURLManager.issueOcapURL(objectKRef)).rejects.toThrow( + 'Issue failed', + ); + + expect(mockKernelStore.isObjectPinned(objectKRef)).toBe(false); + expect(mockKernelStore.getOcapURLObjects()).toStrictEqual([]); + expect(mockKernelStore.getObjectRefCount(objectKRef)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + }); + + it('keeps the retention a URL already issued depends on', async () => { + await ocapURLManager.issueOcapURL(objectKRef); + vi.spyOn(mockRemoteComms, 'issueOcapURL').mockRejectedValue( + new Error('Issue failed'), + ); + + await expect(ocapURLManager.issueOcapURL(objectKRef)).rejects.toThrow( + 'Issue failed', + ); + + expect(mockKernelStore.isObjectPinned(objectKRef)).toBe(true); + expect(mockKernelStore.getOcapURLObjects()).toStrictEqual([objectKRef]); + }); + + it('refuses to issue a URL for a kref the kernel has deleted', async () => { + mockKernelStore.deleteKernelObject(objectKRef); + + await expect(ocapURLManager.issueOcapURL(objectKRef)).rejects.toThrow( + `cannot issue an ocap URL for deleted kref "${objectKRef}"`, + ); + expect(mockRemoteComms.issueOcapURL).not.toHaveBeenCalled(); + }); + it('issues OCAP URL for a kref', async () => { - const kref = 'ko123'; + const kref = objectKRef; const url = await ocapURLManager.issueOcapURL(kref); expect(url).toBe('ocap:abc123@local-peer-id'); @@ -183,7 +263,7 @@ describe('OcapURLManager', () => { describe('issuer service', () => { it('issues URL through issuer service with valid remotable', async () => { // Create a valid remotable object that krefOf can extract a kref from - const kref = 'ko777'; + const kref = objectKRef; const remotableObj = kslot(kref, 'TestInterface'); vi.spyOn(mockRemoteComms, 'issueOcapURL').mockResolvedValue( @@ -201,7 +281,7 @@ describe('OcapURLManager', () => { it('issues URL through issuer service with promise kref', async () => { // Create a promise-type kref (starts with 'p', 'kp', or 'rp') - const promiseKref = 'kp888'; + const promiseKref = promiseKRef; const promiseObj = kslot(promiseKref); vi.spyOn(mockRemoteComms, 'issueOcapURL').mockResolvedValue( @@ -223,7 +303,7 @@ describe('OcapURLManager', () => { // The issuer service is already tested implicitly through other tests. // Test that issueOcapURL is called correctly directly - const kref = 'ko777'; + const kref = objectKRef; vi.spyOn(mockRemoteComms, 'issueOcapURL').mockResolvedValue( `ocap:issued@local-peer-id`, ); @@ -291,7 +371,7 @@ describe('OcapURLManager', () => { describe('integration scenarios', () => { it('handles round-trip issue and redeem', async () => { // Issue a URL - const kref = 'ko789'; + const kref = objectKRef; vi.spyOn(mockRemoteComms, 'issueOcapURL').mockResolvedValue( 'ocap:xyz789@local-peer-id', ); @@ -306,8 +386,8 @@ describe('OcapURLManager', () => { it('handles multiple simultaneous operations', async () => { const promises = [ - ocapURLManager.issueOcapURL('ko1'), - ocapURLManager.issueOcapURL('ko2'), + ocapURLManager.issueOcapURL(objectKRef), + ocapURLManager.issueOcapURL(otherObjectKRef), ocapURLManager.redeemOcapURL('ocap:abc@local-peer-id'), ocapURLManager.redeemOcapURL('ocap:def@remote-peer-id'), ]; @@ -327,7 +407,7 @@ describe('OcapURLManager', () => { new Error('Issue failed'), ); - await expect(ocapURLManager.issueOcapURL('ko123')).rejects.toThrow( + await expect(ocapURLManager.issueOcapURL(objectKRef)).rejects.toThrow( 'Issue failed', ); }); diff --git a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts index eec66cae1..aee9b1710 100644 --- a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts +++ b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts @@ -4,6 +4,7 @@ import { parseOcapURL } from './remote-comms.ts'; import type { RemoteManager } from './RemoteManager.ts'; import { kslot, krefOf } from '../../liveslots/kernel-marshal.ts'; import type { SlotValue } from '../../liveslots/kernel-marshal.ts'; +import type { KernelStore } from '../../store/index.ts'; import type { KRef } from '../../types.ts'; /** @@ -25,6 +26,7 @@ export type OcapURLRedemptionService = { type OcapURLManagerConstructorProps = { remoteManager: RemoteManager; + kernelStore: KernelStore; }; /** @@ -34,6 +36,9 @@ export class OcapURLManager { /** Remote manager for handling remote connections */ readonly #remoteManager: RemoteManager; + /** The kernel's store, for retaining the objects issued URLs name */ + readonly #kernelStore: KernelStore; + /** OCAP URL issuer service object */ readonly #ocapURLIssuerService: object; @@ -45,9 +50,11 @@ export class OcapURLManager { * * @param options - Constructor options. * @param options.remoteManager - The remote manager for handling cross-kernel communications. + * @param options.kernelStore - The kernel's store. */ - constructor({ remoteManager }: OcapURLManagerConstructorProps) { + constructor({ remoteManager, kernelStore }: OcapURLManagerConstructorProps) { this.#remoteManager = remoteManager; + this.#kernelStore = kernelStore; // Create the OCAP URL issuer service this.#ocapURLIssuerService = Far('ocapURLIssuerService', { @@ -119,7 +126,23 @@ export class OcapURLManager { */ async issueOcapURL(kref: KRef): Promise { const identity = this.#remoteManager.getRemoteIdentity(); - return identity.issueOcapURL(kref); + // Before minting the token, not after: the URL is unretractable once it + // exists, so the target must already be retained. Minting also awaits, and + // a collection crank can run in that window. See `retainForOcapURL`. + const retained = this.#kernelStore.retainForOcapURL(kref); + try { + return await identity.issueOcapURL(kref); + } catch (error) { + // Nothing else undoes this. A rejected kernel-service call is reported to + // the caller rather than thrown out of the crank, so the crank commits + // and the pin outlives the kernel that took it, naming a URL that never + // existed. Only the pin this call took: a kref some live URL already + // names keeps the pin that URL depends on. + if (retained) { + this.#kernelStore.undoOcapURLRetention(kref); + } + throw error; + } } /** diff --git a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts index c14539bc6..4f53572c0 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) @@ -660,7 +660,12 @@ describe('RemoteHandle', () => { const remote = makeRemote(); const mockOcapURL = 'as if it was a URL'; const mockReplyKey = 'replyKey'; - const replyKRef = 'ko100'; + // A URL only ever names an object the kernel still has, so redeem one that + // exists: importing a deleted kref is refused outright. + const replyKRef = mockKernelStore.initKernelObject('kernel'); + vi.spyOn(mockRemoteComms, 'redeemLocalOcapURL').mockResolvedValue( + replyKRef, + ); const replyRRef = 'ro+1'; // Include seq for incoming message const request = JSON.stringify({ diff --git a/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts index 34e9e9150..29ca0b2da 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 58fefc80c..af06ace09 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', @@ -100,6 +103,7 @@ describe('kernel store', () => { 'getNextRemoteId', 'getNextVatId', 'getObjectRefCount', + 'getOcapURLObjects', 'getOwner', 'getPeerIncarnation', 'getPendingMessage', @@ -140,7 +144,7 @@ describe('kernel store', () => { 'isVatTerminated', 'kernelRefExists', 'krefToEref', - 'krefsToExistingErefs', + 'krefsToErefs', 'makeVatStore', 'markInitialized', 'markVatAsTerminated', @@ -148,6 +152,7 @@ describe('kernel store', () => { 'nextTerminatedVatCleanup', 'pinObject', 'provideIncarnationId', + 'recomputeRefCounts', 'recordLastActiveTime', 'releaseAllSavepoints', 'releaseSavepoint', @@ -155,6 +160,7 @@ describe('kernel store', () => { 'removeVatFromSubcluster', 'reset', 'resolveKernelPromise', + 'retainForOcapURL', 'retireKernelObjects', 'revoke', 'rollbackCrank', @@ -167,6 +173,8 @@ describe('kernel store', () => { 'setPeerIncarnation', 'setPendingMessage', 'setPromiseDecider', + 'setReachableFlag', + 'setRefCountAuditing', 'setRelayEntries', 'setRemoteHighestReceivedSeq', 'setRemoteIdentityValue', @@ -184,6 +192,7 @@ describe('kernel store', () => { 'translateRefEtoK', 'translateRefKtoE', 'translateSyscallVtoK', + 'undoOcapURLRetention', 'unpinObject', 'waitForCrank', ]); @@ -206,31 +215,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'); @@ -358,6 +367,62 @@ describe('kernel store', () => { }); }); + describe('ocap URL retention', () => { + it('pins the target and records it', () => { + const ks = makeKernelStore(mockKernelDatabase); + const kref = ks.initKernelObject('v1'); + + expect(ks.retainForOcapURL(kref)).toBe(true); + expect(ks.isObjectPinned(kref)).toBe(true); + expect(ks.getOcapURLObjects()).toStrictEqual([kref]); + }); + + it('reports that a second URL for the same target took no pin', () => { + const ks = makeKernelStore(mockKernelDatabase); + const kref = ks.initKernelObject('v1'); + ks.retainForOcapURL(kref); + + expect(ks.retainForOcapURL(kref)).toBe(false); + expect(ks.getPinnedObjects()).toStrictEqual([kref]); + }); + + it('refuses to retain a kref the kernel has deleted', () => { + const ks = makeKernelStore(mockKernelDatabase); + + expect(() => ks.retainForOcapURL('ko99')).toThrow( + 'cannot issue an ocap URL for deleted kref "ko99"', + ); + }); + + it('undoing the last retention clears the record entirely', () => { + const ks = makeKernelStore(mockKernelDatabase); + const kref = ks.initKernelObject('v1'); + const otherKref = ks.initKernelObject('v1'); + ks.retainForOcapURL(kref); + ks.retainForOcapURL(otherKref); + + ks.undoOcapURLRetention(kref); + expect(ks.isObjectPinned(kref)).toBe(false); + expect(ks.getOcapURLObjects()).toStrictEqual([otherKref]); + + ks.undoOcapURLRetention(otherKref); + expect(ks.getOcapURLObjects()).toStrictEqual([]); + expect( + mockKernelDatabase.kernelKVStore.get('ocapURLObjects'), + ).toBeUndefined(); + }); + + it('undoing a retention that was never taken leaves the pin alone', () => { + const ks = makeKernelStore(mockKernelDatabase); + const kref = ks.initKernelObject('v1'); + ks.pinObject(kref); + + ks.undoOcapURLRetention(kref); + + expect(ks.isObjectPinned(kref)).toBe(true); + }); + }); + describe('reset', () => { it('clears store and resets counters', () => { const ks = makeKernelStore(mockKernelDatabase); diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 5c8f49fc3..6ba7c5177 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 @@ -64,6 +65,7 @@ * kernelService.${serviceName} = ${koid} // kref of kernel service object ${serviceName} */ +import { Fail } from '@endo/errors'; import type { KernelDatabase, KVStore, VatStore } from '@metamask/kernel-store'; import { Logger } from '@metamask/logger'; @@ -79,6 +81,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'; @@ -152,12 +155,14 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { subclusters: provideCachedStoredValue('subclusters', '[]'), nextSubclusterId: provideCachedStoredValue('nextSubclusterId', '1'), vatToSubclusterMap: provideCachedStoredValue('vatToSubclusterMap', '{}'), + auditRefCounts: 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); @@ -291,6 +296,7 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { ...id, ...queue, ...refCount, + ...refCountAudit, ...object, ...promise, ...revocation, @@ -363,8 +369,53 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { kv.set('anonymousKernelObjects', [...krefs].sort().join(',')); } }, + + // Objects named by an issued ocap URL + // + // An ocap URL is a durable bearer token: it carries an encrypted kref and + // nothing else, so the kernel cannot discover from its own state that a + // holder exists. Retaining the target is therefore the only thing keeping + // the URL redeemable, and it has to outlive every other reference — the + // token is persistent, unexpiring, and may be redeemed by a peer that was + // not running when it was issued. `revoke` is the way to kill the + // capability; `undoOcapURLRetention` is not a release, only an unwind of a + // retention whose URL was never minted. + getOcapURLObjects(): KRef[] { + const raw = kv.get('ocapURLObjects'); + return raw ? (raw.split(',') as KRef[]) : []; + }, + retainForOcapURL(kref: KRef): boolean { + // Refuse to mint a token for something already collected: the pin would + // resurrect a `(1, 1)` row for an object with no owner, and the URL would + // name a capability that can never be delivered to. + this.kernelRefExists(kref) || + Fail`cannot issue an ocap URL for deleted kref ${kref}`; + const krefs = new Set(this.getOcapURLObjects()); + // One pin per kref, however many URLs name it: pins are a multiset, and + // a second pin here would be one nothing could ever release. + if (krefs.has(kref)) { + return false; + } + krefs.add(kref); + kv.set('ocapURLObjects', [...krefs].sort().join(',')); + this.pinObject(kref); + return true; + }, + undoOcapURLRetention(kref: KRef): void { + const krefs = new Set(this.getOcapURLObjects()); + if (!krefs.delete(kref)) { + return; + } + if (krefs.size === 0) { + kv.delete('ocapURLObjects'); + } else { + kv.set('ocapURLObjects', [...krefs].sort().join(',')); + } + this.unpinObject(kref); + }, }); } 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 bd09b457a..2ccd17f1f 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 000000000..c7d1cf22d --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts @@ -0,0 +1,298 @@ +import { describe, it, expect, beforeEach } from 'vitest'; + +import { makeMapKernelDatabase } from '../../../test/storage.ts'; +import type { VatConfig, VatId } from '../../types.ts'; +import { makeKernelStore } from '../index.ts'; + +/** + * Regressions for the asymmetry described in + * https://github.com/MetaMask/ocap-kernel/issues/1006: creating an import + * c-list entry changed no refcount while tearing one down decremented both, + * and `initKernelObject` compensated by minting every object at (1, 1). That + * 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('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('tolerates an importer entry that outlives the object it names', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v3', kref, true); + // v3 has let go of the object but can still recognize the name + kernelStore.clearReachableFlag('v3', kref); + kernelStore.markVatAsTerminated('v1'); + kernelStore.cleanupTerminatedVat('v1'); + + kernelStore.collectGarbage(); + + // The collector deletes the object and queues the retirement together, so + // v3's entry names a kref the kernel has already dropped until that action + // is delivered. Counting it as a holder fails the end-of-crank audit on a + // state the collector itself just created, which kills the run loop. + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v3 retireImport ${kref}`, + ]); + 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([]); + }); + }); +}); diff --git a/packages/ocap-kernel/src/store/methods/clist.test.ts b/packages/ocap-kernel/src/store/methods/clist.test.ts index e43d6e428..4df8841bf 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 425f4a129..d079100ae 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,31 @@ 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. + * * @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 +143,24 @@ 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. Garbage collection is the only caller + * and has already established that each kref has an entry, so a missing one + * means the two disagree — worth hearing about rather than silently dropping + * the notification. * * @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/gc.test.ts b/packages/ocap-kernel/src/store/methods/gc.test.ts index a294920e9..e91fb29bb 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 04f3c0cc0..31b21294c 100644 --- a/packages/ocap-kernel/src/store/methods/gc.ts +++ b/packages/ocap-kernel/src/store/methods/gc.ts @@ -165,8 +165,10 @@ export function getGCMethods(ctx: StoreContext) { 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) { diff --git a/packages/ocap-kernel/src/store/methods/object.test.ts b/packages/ocap-kernel/src/store/methods/object.test.ts index def40adae..26d4fb9b1 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 9e596f5e8..57b7aaf88 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 baeba6c2e..d85e291ef 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 c3aedbf6c..40fa73b04 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 fdaab477e..1ec32bdb9 100644 --- a/packages/ocap-kernel/src/store/methods/reachable.test.ts +++ b/packages/ocap-kernel/src/store/methods/reachable.test.ts @@ -15,13 +15,65 @@ 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, + }); + }); + + it.each(['setReachableFlag', 'clearReachableFlag'] as const)( + 'is idempotent: %s', + (method) => { + const ko1 = kernelStore.initKernelObject('v1'); + kernelStore.addCListEntry('v1', ko1, 'o-1'); + kernelStore.setReachableFlag('v1', ko1); + + const before = kernelStore.getObjectRefCount(ko1); + kernelStore[method]('v1', ko1); + kernelStore[method]('v1', ko1); + const after = kernelStore.getObjectRefCount(ko1); + + expect(after).toStrictEqual( + method === 'setReachableFlag' + ? before + : { reachable: 0, recognizable: 1 }, + ); + }, + ); - const refCounts = kernelStore.getObjectRefCount(ko1); - expect(refCounts.reachable).toBe(0); + 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 4caa2d096..05e4c5f98 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 000000000..70af7d407 --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts @@ -0,0 +1,269 @@ +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; + + /** + * Register and initialize an endpoint so it can hold c-list entries. + * + * @param vatIds - The vats to bring into existence. + */ + function givenVats(...vatIds: VatId[]): void { + for (const vatId of vatIds) { + kernelStore.setVatConfig(vatId, { sourceSpec: 'x' } as VatConfig); + kernelStore.initEndpoint(vatId); + } + } + + beforeEach(() => { + kernelStore = makeKernelStore(makeMapKernelDatabase()); + kernelStore.markInitialized(); + givenVats('v1', 'v2', '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 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([ + { + 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([ + { 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([ + { + kref, + stored: '(deleted)', + 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([]); + }); + }); + + 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([ + { + 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('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 000000000..b4178e5c6 --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -0,0 +1,376 @@ +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 = { + kref: KRef; + /** + * The counts as stored, in the store's own encoding: `"reachable,recognizable"` + * for objects, a single number for promises, or `"(deleted)"` if the kref has + * no refcount entry at all. + */ + stored: string; + /** The counts implied by `holders`, in the same encoding as `stored`. */ + expected: string; + /** One entry per reference found, so a mismatch can be traced to its source. */ + holders: string[]; +}; + +/** + * 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. + * + * Only references the kernel can see in its own state are checkable, so a + * holder that keeps a kref outside them is invisible here and the audit will + * pronounce its target unreferenced. Anything of that shape has to take a pin + * to be counted at all — see `retainForOcapURL`, where an issued URL's + * encrypted kref does exactly that. + * + * @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. + * + * @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({ + kref, + stored: '(deleted)', + expected: expectedText, + holders: tally.holders, + }); + } + continue; + } + const storedText = isPromiseRef(kref) + ? raw + : renderCounts(kref, getObjectRefCount(kref)); + if (storedText !== expectedText) { + violations.push({ + kref, + stored: storedText, + expected: expectedText, + holders: tally.holders, + }); + } + } + return violations; + } + + /** + * Overwrite stored reference counts with the counts implied by ground truth. + * + * A repair tool for a store whose counts have drifted, offered to embedders + * and never run automatically: nothing calls it, and opening an existing + * store does not migrate it. Krefs that are referenced but have already been + * deleted cannot be repaired this way and are reported instead. + * + * Ground truth here means the references the kernel can see in its own + * state. A holder the store cannot see — an issued ocap URL names its target + * only inside an encrypted bearer token — is not among them, which is why + * such a target is pinned when the URL is issued rather than left for this to + * infer. + * + * @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.stored === '(deleted)') { + 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 multi-line description, one paragraph per violation. + */ + function formatRefCountViolations(violations: RefCountViolation[]): string { + return violations + .map(({ kref, stored, expected, holders }) => { + 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.auditRefCounts) { + return; + } + const violations = auditRefCounts(); + if (violations.length > 0) { + throw Error( + `reference count invariant violated:\n${formatRefCountViolations(violations)}`, + ); + } + } + + /** + * Turn the per-crank reference count audit on or off. + * + * @param enabled - Whether to audit after every crank. + */ + function setRefCountAuditing(enabled: boolean): void { + ctx.auditRefCounts = enabled; + } + + return { + auditRefCounts, + recomputeRefCounts, + formatRefCountViolations, + assertRefCountsIfAuditing, + setRefCountAuditing, + }; +} diff --git a/packages/ocap-kernel/src/store/methods/refcount.ts b/packages/ocap-kernel/src/store/methods/refcount.ts index b87d13c25..6cea54268 100644 --- a/packages/ocap-kernel/src/store/methods/refcount.ts +++ b/packages/ocap-kernel/src/store/methods/refcount.ts @@ -71,6 +71,11 @@ export function getRefCountMethods(ctx: StoreContext) { * have only a "reachable" count, whereas objects track both "reachable" * and "recognizable" counts. * + * Every rule below has a mirror in `computeExpectedRefCounts` + * (`refcount-audit.ts`), which recomputes these counts from the references + * themselves; the two have to change together or the audit starts reporting + * violations against correct accounting. + * * @param kref - The kernel slot whose refcount is to be incremented. * @param tag - The tag of the kernel slot. * @param options - Options for the increment. diff --git a/packages/ocap-kernel/src/store/methods/translators.test.ts b/packages/ocap-kernel/src/store/methods/translators.test.ts index 0ac95eafb..b50f21b4a 100644 --- a/packages/ocap-kernel/src/store/methods/translators.test.ts +++ b/packages/ocap-kernel/src/store/methods/translators.test.ts @@ -14,6 +14,8 @@ import type { } from '../../types.ts'; import type { StoreContext } from '../types.ts'; import * as clistModule from './clist.ts'; +import * as reachableModule from './reachable.ts'; +import * as refCountModule from './refcount.ts'; import { getTranslators } from './translators.ts'; import * as vatModule from './vat.ts'; @@ -22,10 +24,13 @@ describe('getTranslators', () => { const mockErefToKref = vi.fn(); const mockAllocateErefForKref = vi.fn(); const mockExportFromEndpoint = vi.fn(); + const mockSetReachableFlag = vi.fn(); + const mockKernelRefExists = vi.fn(); const mockCtx = {} as StoreContext; beforeEach(() => { vi.clearAllMocks(); + mockKernelRefExists.mockReturnValue(true); vi.spyOn(clistModule, 'getCListMethods').mockReturnValue({ krefToEref: mockKrefToEref, @@ -33,6 +38,14 @@ describe('getTranslators', () => { allocateErefForKref: mockAllocateErefForKref, } as unknown as ReturnType); + vi.spyOn(reachableModule, 'getReachableMethods').mockReturnValue({ + setReachableFlag: mockSetReachableFlag, + } as unknown as ReturnType); + + vi.spyOn(refCountModule, 'getRefCountMethods').mockReturnValue({ + kernelRefExists: mockKernelRefExists, + } as unknown as ReturnType); + vi.spyOn(vatModule, 'getVatMethods').mockReturnValue({ exportFromEndpoint: mockExportFromEndpoint, } as unknown as ReturnType); @@ -64,6 +77,18 @@ describe('getTranslators', () => { expect(result).toStrictEqual(expectedEref); }); + it('refuses to import a kref the kernel has deleted', () => { + const vatId: VatId = 'v1'; + const kref: KRef = 'ko1' as KRef; + mockKrefToEref.mockReturnValue(null); + mockKernelRefExists.mockReturnValue(false); + const { translateRefKtoE } = getTranslators(mockCtx); + expect(() => translateRefKtoE(vatId, kref, true)).toThrow( + `cannot import deleted kref "${kref}" into "${vatId}"`, + ); + expect(mockAllocateErefForKref).not.toHaveBeenCalled(); + }); + it('throws error when not found and importIfNeeded is false', () => { const vatId: VatId = 'v1'; const kref: KRef = 'ko1' as KRef; diff --git a/packages/ocap-kernel/src/store/methods/translators.ts b/packages/ocap-kernel/src/store/methods/translators.ts index c5948fedf..3ba551d28 100644 --- a/packages/ocap-kernel/src/store/methods/translators.ts +++ b/packages/ocap-kernel/src/store/methods/translators.ts @@ -21,6 +21,8 @@ import type { } from '../../types.ts'; import type { StoreContext } from '../types.ts'; import { getCListMethods } from './clist.ts'; +import { getReachableMethods } from './reachable.ts'; +import { getRefCountMethods } from './refcount.ts'; import { getVatMethods } from './vat.ts'; import { Fail, assert } from '../../utils/assert.ts'; @@ -35,6 +37,8 @@ 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 { kernelRefExists } = getRefCountMethods(ctx); const { exportFromEndpoint } = getVatMethods(ctx); /** @@ -54,6 +58,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; @@ -69,11 +78,21 @@ export function getTranslators(ctx: StoreContext) { let eref = krefToEref(endpointId, kref); if (!eref) { if (importIfNeeded) { + // A kref the kernel has already deleted must not acquire a new c-list + // entry. `getObjectRefCount` reads a missing row as `(0, 0)`, so the + // entry's own increment would write it back and resurrect a + // live-looking object that nobody owns — one the audit then endorses, + // since the entry is a legitimate holder for exactly the count it + // finds. Reached by redeeming an ocap URL issued for an object that + // has since been collected. + kernelRefExists(kref) || + Fail`cannot import deleted kref ${kref} into ${endpointId}`; eref = allocateErefForKref(endpointId, kref); } else { 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 d04c60989..fd1f05402 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,30 @@ 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(); + // Queued for GC even though the new owner keeps the kref: tearing our + // pair down may have been what took its last reference, and the new + // owner's accounting decides the outcome. + 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 48d431dc1..29e2a85df 100644 --- a/packages/ocap-kernel/src/store/methods/vat.ts +++ b/packages/ocap-kernel/src/store/methods/vat.ts @@ -5,7 +5,6 @@ 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 type { EndpointId, KRef, @@ -35,18 +34,14 @@ 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 { initKernelObject, getObjectRefCount } = getObjectMethods(ctx); + const { initKernelPromise, setPromiseDecider, addPromiseSubscriber } = + getPromiseMethods(ctx); + const { initKernelObject } = getObjectMethods(ctx); const { addCListEntry } = getCListMethods(ctx); - const { incrementRefCount, decrementRefCount } = getRefCountMethods(ctx); /** * Delete all persistent state associated with an endpoint. @@ -54,10 +49,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}`); @@ -261,11 +253,8 @@ export function getVatMethods(ctx: StoreContext) { const { vatSlot } = getReachableAndVatSlot(vatID, kref); ctx.kv.delete(getSlotKey(vatID, kref)); ctx.kv.delete(getSlotKey(vatID, vatSlot)); - // Skip baseline decrement if GC already zeroed reachable via dropImports. - const { reachable } = getObjectRefCount(kref); - if (reachable > 0) { - 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; } @@ -282,20 +271,15 @@ 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 rejected the orphan promises via getPromisesByDecider() before + // calling us, which is what released each promise's unsettled reference, + // but their kpids are still in the dead vat's c-list. Clean those up now. 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; } @@ -377,49 +361,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); } } @@ -444,11 +407,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 1ea54d19c..3bf54862f 100644 --- a/packages/ocap-kernel/src/store/types.ts +++ b/packages/ocap-kernel/src/store/types.ts @@ -27,6 +27,7 @@ export type StoreContext = { subclusters: StoredValue; // Holds Subcluster[] nextSubclusterId: StoredValue; // Holds string vatToSubclusterMap: StoredValue; // Holds Record + auditRefCounts: boolean; // If set, verify refcounts against ground truth every crank logger?: Logger | undefined; }; diff --git a/packages/ocap-kernel/src/vats/SubclusterManager.test.ts b/packages/ocap-kernel/src/vats/SubclusterManager.test.ts index bea731738..23af0e040 100644 --- a/packages/ocap-kernel/src/vats/SubclusterManager.test.ts +++ b/packages/ocap-kernel/src/vats/SubclusterManager.test.ts @@ -83,6 +83,7 @@ describe('SubclusterManager', () => { collectGarbage: vi.fn(), terminateAllVats: vi.fn().mockResolvedValue(undefined), hasVat: vi.fn().mockReturnValue(false), + releaseVatRootPin: vi.fn(), } as unknown as Mocked; mockGetKernelService = vi.fn().mockReturnValue(undefined) as unknown as ( @@ -768,6 +769,11 @@ describe('SubclusterManager', () => { expect( mockKernelStore.deleteSystemSubclusterMapping, ).toHaveBeenCalledWith('orphan'); + // These vats never ran here, so nothing else releases the root pin the + // incarnation that did run them took. + for (const vatId of Object.values(subcluster.vats)) { + expect(mockVatManager.releaseVatRootPin).toHaveBeenCalledWith(vatId); + } }); it('restores valid persisted system subclusters', () => { diff --git a/packages/ocap-kernel/src/vats/SubclusterManager.ts b/packages/ocap-kernel/src/vats/SubclusterManager.ts index 2686fda5f..1b21b6c26 100644 --- a/packages/ocap-kernel/src/vats/SubclusterManager.ts +++ b/packages/ocap-kernel/src/vats/SubclusterManager.ts @@ -295,6 +295,11 @@ export class SubclusterManager { // Delete vat configs and mark vats as terminated so their data will be cleaned up for (const vatId of Object.values(subcluster.vats)) { + // These vats are not running, so `stopVat` never gets to release the pin + // its `launchVat` took in the incarnation that did run them. Without this + // the root's count never reaches zero and `pinnedObjects` keeps naming a + // vat that no longer exists. + this.#vatManager.releaseVatRootPin(vatId); this.#kernelStore.deleteVatConfig(vatId); this.#kernelStore.markVatAsTerminated(vatId); } diff --git a/packages/ocap-kernel/src/vats/VatManager.test.ts b/packages/ocap-kernel/src/vats/VatManager.test.ts index d5e92b1aa..21361f942 100644 --- a/packages/ocap-kernel/src/vats/VatManager.test.ts +++ b/packages/ocap-kernel/src/vats/VatManager.test.ts @@ -160,6 +160,14 @@ describe('VatManager', () => { expect(kref).toBe('ko1'); }); + it('pins the root for the vat lifetime', async () => { + // A root is addressable while its vat lives whether or not anyone + // imports it, so without this GC retires it as the last importer lets go. + await vatManager.launchVat(createMockVatConfig(), 'test'); + + expect(mockKernelStore.pinObject).toHaveBeenCalledWith('ko1'); + }); + it('launches a new vat with subcluster', async () => { const config = createMockVatConfig(); const kref = await vatManager.launchVat(config, 'test', 's1'); @@ -236,6 +244,24 @@ describe('VatManager', () => { expect(vatManager.hasVat('v1')).toBe(false); }); + it('keeps the root pin across a restart', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + + await vatManager.stopVat('v1', false); + + // The same root comes back, so releasing the pin would let GC retire it + // in the window where the vat has no handle. + expect(mockKernelStore.unpinObject).not.toHaveBeenCalled(); + }); + + it('releases the root pin on termination', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + + await vatManager.stopVat('v1', true); + + expect(mockKernelStore.unpinObject).toHaveBeenCalledWith('ko1'); + }); + it('stops a vat for termination with reason', async () => { const config = createMockVatConfig(); await vatManager.runVat('v1', config); @@ -429,6 +455,25 @@ describe('VatManager', () => { }); }); + describe('releaseVatRootPin', () => { + it('releases the pin on a vat root', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + + vatManager.releaseVatRootPin('v1'); + + expect(mockKernelStore.unpinObject).toHaveBeenCalledWith('ko1'); + }); + + it('does nothing for a vat with no root', () => { + // Teardown can outlive the kernel's knowledge of the vat, and there is + // no pin to release in that case. + mockKernelStore.getRootObject.mockReturnValue(undefined); + + expect(() => vatManager.releaseVatRootPin('v1')).not.toThrow(); + expect(mockKernelStore.unpinObject).not.toHaveBeenCalled(); + }); + }); + describe('pinVatRoot', () => { it('pins vat root successfully', async () => { const config = createMockVatConfig(); diff --git a/packages/ocap-kernel/src/vats/VatManager.ts b/packages/ocap-kernel/src/vats/VatManager.ts index b90f6f30c..5da2bebe2 100644 --- a/packages/ocap-kernel/src/vats/VatManager.ts +++ b/packages/ocap-kernel/src/vats/VatManager.ts @@ -128,6 +128,11 @@ export class VatManager { 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; } @@ -186,6 +191,10 @@ export class VatManager { } else if (terminating) { terminationError = new VatDeletedError(vatId); } + if (terminating) { + // A restart keeps the pin: the same root comes back. + this.releaseVatRootPin(vatId); + } await this.#platformServices .terminate(vatId, terminationError) .catch(this.#logger.error); @@ -286,7 +295,26 @@ export class VatManager { } /** - * Pin a vat root. + * Release the pin `launchVat` took on a vat's root, so the root can be + * collected once its importers let go. + * + * For paths that end a vat's life. Tolerant of a root that is already gone, + * since a vat can be torn down after the kernel has lost track of it. + * + * @param vatId - The ID of the vat whose life is ending. + */ + releaseVatRootPin(vatId: VatId): void { + const rootRef = this.#kernelStore.getRootObject(vatId); + if (rootRef) { + this.#kernelStore.unpinObject(rootRef); + } + } + + /** + * Pin a vat root, on behalf of an embedder that wants to keep it addressable. + * + * Pins are counted, and `launchVat` already holds one for the vat's lifetime, + * so this adds to that rather than replacing it. * * @param vatId - The ID of the vat. * @returns The KRef of the vat root. @@ -301,7 +329,10 @@ export class VatManager { } /** - * Unpin a vat root. + * Release one embedder pin on a vat root. + * + * Removes a single pin, so a root still pinned for its vat's lifetime stays + * addressable: this does not make it collectable while the vat lives. * * @param vatId - The ID of the vat. */ diff --git a/packages/ocap-kernel/test/remotes-mocks.ts b/packages/ocap-kernel/test/remotes-mocks.ts index 0e37b4a77..3d3e332e1 100644 --- a/packages/ocap-kernel/test/remotes-mocks.ts +++ b/packages/ocap-kernel/test/remotes-mocks.ts @@ -164,6 +164,7 @@ export class MockRemotesFactory { }, remoteComms, remoteHandle, + kernelStore: this.config.kernelStore as KernelStore, }; }