From a310fe47cc8bdb681426864208321831df2d1c34 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 5 Aug 2026 15:29:18 +0200 Subject: [PATCH 01/17] fix(ocap-kernel): make c-list import accounting symmetric (#1006) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 is correct for exactly one importer, which is why nothing caught it: with two importers a live capability gets dropped and retired out from under a holder, and the same unit is claimed by both an importer's drop and the owner's termination, so cleanup underflows and leaves a vat half-cleaned. Restore the increment and rebase the baseline to (0, 0), matching SwingSet, so `collectGarbage` — already a faithful port — receives the inputs it was written for. Build the invariant checker first, since every existing compensation becomes a double-count the moment the increment lands. It recomputes each kref's counts from ground truth (c-list entries and their reachable flags, run-queue and promise-queue messages, promise resolution values, pins) and reports drift in both directions: too low collects a live capability, too high leaks it. Enabled via `Kernel.make`'s `auditRefCounts` and run after every crank; on in kernel-test. The audit found four more unbalanced paths that the phantom baseline had been absorbing, each fixed here: a delivered message charged its target against the routed kref rather than the run-queue item's own, so a message routed through a resolved promise decremented an object nobody charged and leaked the promise; a notification leaked its reference on both early-return paths and decremented promises retired alongside it that nobody had taken; a message queued on an unresolved promise duplicated every reference it carried on re-enqueue; and `resolve|kpid` incremented with no matching release. Two things the baseline was silently standing in for, now explicit: vat roots are pinned for the lifetime of their vat (a root is addressable whether or not anyone imports it), and GC action delivery moves the kernel's own c-list so a dropped export's flag clears and retired entries don't outlive their objects. Also fixes the stale `cle.`/`clk.` key prefixes in `getPromisesByDecider` and `deleteEndpoint`, which stopped matching the `${endpointId}.c.` layout. `getPromisesByDecider` matched nothing, so promises a terminating vat was deciding were never rejected — load bearing here, because releasing a promise's unsettled reference is what makes the cleanup path's accounting add up. Refcounts are persisted, so counts written under the old scheme are recomputed from ground truth on first open, keyed off a new `refCountScheme` entry. Co-Authored-By: Claude Opus 5 (1M context) --- .../extension/test/e2e/control-panel.test.ts | 1 - .../src/garbage-collection.test.ts | 142 ++++++- packages/kernel-test/src/persistence.test.ts | 3 +- packages/kernel-test/src/utils.ts | 3 + packages/ocap-kernel/CHANGELOG.md | 21 ++ packages/ocap-kernel/src/Kernel.ts | 11 + packages/ocap-kernel/src/KernelQueue.test.ts | 9 +- packages/ocap-kernel/src/KernelQueue.ts | 2 +- packages/ocap-kernel/src/KernelRouter.test.ts | 72 +++- packages/ocap-kernel/src/KernelRouter.ts | 52 ++- .../src/remotes/kernel/RemoteHandle.test.ts | 6 +- .../src/remotes/kernel/RemoteManager.test.ts | 9 +- packages/ocap-kernel/src/store/index.test.ts | 40 +- packages/ocap-kernel/src/store/index.ts | 12 +- .../ocap-kernel/src/store/methods/base.ts | 14 +- .../store/methods/clist-accounting.test.ts | 278 ++++++++++++++ .../src/store/methods/clist.test.ts | 88 +++-- .../ocap-kernel/src/store/methods/clist.ts | 36 +- .../ocap-kernel/src/store/methods/gc.test.ts | 15 - packages/ocap-kernel/src/store/methods/gc.ts | 6 +- .../src/store/methods/object.test.ts | 24 +- .../ocap-kernel/src/store/methods/object.ts | 11 +- .../src/store/methods/promise.test.ts | 145 +++++--- .../ocap-kernel/src/store/methods/promise.ts | 69 +++- .../src/store/methods/reachable.test.ts | 56 ++- .../src/store/methods/reachable.ts | 27 ++ .../src/store/methods/refcount-audit.test.ts | 269 +++++++++++++ .../src/store/methods/refcount-audit.ts | 352 ++++++++++++++++++ .../src/store/methods/translators.test.ts | 6 + .../src/store/methods/translators.ts | 8 + .../ocap-kernel/src/store/methods/vat.test.ts | 90 ++--- packages/ocap-kernel/src/store/methods/vat.ts | 81 ++-- packages/ocap-kernel/src/store/types.ts | 1 + packages/ocap-kernel/src/vats/VatManager.ts | 13 + 34 files changed, 1637 insertions(+), 335 deletions(-) create mode 100644 packages/ocap-kernel/src/store/methods/clist-accounting.test.ts create mode 100644 packages/ocap-kernel/src/store/methods/refcount-audit.test.ts create mode 100644 packages/ocap-kernel/src/store/methods/refcount-audit.ts diff --git a/packages/extension/test/e2e/control-panel.test.ts b/packages/extension/test/e2e/control-panel.test.ts index 167429e76e..754a4017b5 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 268ed7920e..67055414f1 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 e8b6e27815..b1c91f3f45 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 c255347b89..7d867f2c28 100644 --- a/packages/kernel-test/src/utils.ts +++ b/packages/kernel-test/src/utils.ts @@ -93,6 +93,9 @@ export async function makeKernel( resetStorage, logger, keySeed, + // Refcount drift is invisible to ordinary assertions until something gets + // collected out from under a live holder, so check it every crank. + auditRefCounts: true, }); return kernel; } diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 8af7998ec7..0b9328172a 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -32,10 +32,16 @@ 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 ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) + - Reports drift in both directions: counts too low (a live capability can be collected) and counts too high with no holder (a leak) + - Exports the `RefCountViolation` type +- Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) + ### 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)) @@ -65,6 +71,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Report the database error when an aborted crank cannot be rolled back, instead of a spurious "no such savepoint" ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) - The abort path recorded the rollback only after it succeeded, so a throwing rollback had the run loop try again against the savepoint `rollbackCrank` had already discarded. The second attempt's "no such savepoint" then became the reported cause of death — and since only `error.message` crosses the wire, the real failure reached neither `getStatus` nor the daemon log - Reject the run loop's promise with the same `Error` its status reports, rather than re-throwing a non-`Error` for the embedder to normalize a second time ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) +- **BREAKING:** Make c-list reference accounting symmetric: creating an import c-list entry now takes a reference, as tearing one down has always released one ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) + - `initKernelObject` 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 ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) + - 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 ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) + - 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 ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Release a queued notification's reference before the paths that decide there is nothing to deliver, and stop decrementing references on promises retired alongside it that nobody had taken ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Transfer, rather than duplicate, the references a message carries when it is queued on an unresolved promise and later re-enqueued on resolution ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Fix the stale `cle.`/`clk.` key prefixes in `getPromisesByDecider` and `deleteEndpoint`, which no longer matched the `${endpointId}.c.` c-list layout ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) + + - `getPromisesByDecider` matched nothing, so promises a terminating vat or restarting peer was deciding were never rejected - 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 a312e60e65..94865cb049 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(); } @@ -249,6 +257,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 +273,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 d7beb0e2f1..1b3bd4a35a 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 3465e93cde..afda8139c7 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 ea833293a6..11aa8922c1 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 5cfb8335d4..6bd080e7c3 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/remotes/kernel/RemoteHandle.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts index c14539bc6c..009834a65b 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts @@ -536,7 +536,8 @@ describe('RemoteHandle', () => { for (const kref of krefs) { const { isPromise } = parseRef(kref); if (isPromise) { - expect(mockKernelStore.getRefCount(kref)).toBe(1); + // 1 for the unsettled promise, 1 for the remote's c-list entry + expect(mockKernelStore.getRefCount(kref)).toBe(2); } else { expect(mockKernelStore.getObjectRefCount(kref)).toStrictEqual({ reachable: 1, @@ -557,7 +558,7 @@ describe('RemoteHandle', () => { for (const kref of krefs) { const { isPromise } = parseRef(kref); if (isPromise) { - expect(mockKernelStore.getRefCount(kref)).toBe(1); + expect(mockKernelStore.getRefCount(kref)).toBe(2); } else { expect(mockKernelStore.getObjectRefCount(kref)).toStrictEqual({ reachable: 0, @@ -623,7 +624,6 @@ describe('RemoteHandle', () => { // As if we're no longer using it (which, in fact, we weren't), which is a // prequisite for a valid 'retireImports' delivery - mockKernelStore.decrementRefCount(koref, 'test'); mockKernelStore.clearReachableFlag(remote.remoteId, koref); // Now have the "other end" retire the import (include seq for incoming message) diff --git a/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts index 34e9e91502..29ca0b2da7 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts @@ -23,16 +23,13 @@ describe('RemoteManager', () => { let remoteManager: RemoteManager; let mockPlatformServices: PlatformServices; let kernelStore: ReturnType; - let kernelKVStore: ReturnType['kernelKVStore']; let mockKernelQueue: KernelQueue; let logger: Logger; let mockRemoteComms: RemoteComms; let mockFactory: ReturnType; beforeEach(() => { - const kernelDatabase = makeMapKernelDatabase(); - kernelKVStore = kernelDatabase.kernelKVStore; - kernelStore = makeKernelStore(kernelDatabase); + kernelStore = makeKernelStore(makeMapKernelDatabase()); logger = new Logger('test'); mockFactory = createMockRemotesFactory({ @@ -776,7 +773,7 @@ describe('RemoteManager', () => { // Set up a promise where the remote is the decider const [kpid] = kernelStore.initKernelPromise(); kernelStore.setPromiseDecider(kpid, remoteId); - kernelKVStore.set(`cle.${remoteId}.p+1`, kpid); + kernelStore.addCListEntry(remoteId, kpid, 'rp+1'); const resolvePromisesSpy = vi.spyOn(mockKernelQueue, 'resolvePromises'); @@ -882,7 +879,7 @@ describe('RemoteManager', () => { const [kpid] = kernelStore.initKernelPromise(); kernelStore.setPromiseDecider(kpid, remoteId); - kernelKVStore.set(`cle.${remoteId}.p+1`, kpid); + kernelStore.addCListEntry(remoteId, kpid, 'rp+1'); kernelStore.setPeerIncarnation(peerId, 'incarnation-A'); const resolvePromisesSpy = vi.spyOn(mockKernelQueue, 'resolvePromises'); diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 58fefc80c3..d4e68387cf 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', @@ -140,7 +143,7 @@ describe('kernel store', () => { 'isVatTerminated', 'kernelRefExists', 'krefToEref', - 'krefsToExistingErefs', + 'krefsToErefs', 'makeVatStore', 'markInitialized', 'markVatAsTerminated', @@ -148,6 +151,7 @@ describe('kernel store', () => { 'nextTerminatedVatCleanup', 'pinObject', 'provideIncarnationId', + 'recomputeRefCounts', 'recordLastActiveTime', 'releaseAllSavepoints', 'releaseSavepoint', @@ -167,6 +171,8 @@ describe('kernel store', () => { 'setPeerIncarnation', 'setPendingMessage', 'setPromiseDecider', + 'setReachableFlag', + 'setRefCountAuditing', 'setRelayEntries', 'setRemoteHighestReceivedSeq', 'setRemoteIdentityValue', @@ -206,31 +212,31 @@ describe('kernel store', () => { const ko2Owner = 'r23'; expect(ks.initKernelObject(ko1Owner)).toBe('ko1'); - // Check that the object is initialized with reachable=1, recognizable=1 - const refCounts = ks.getObjectRefCount('ko1'); - expect(refCounts.reachable).toBe(1); - expect(refCounts.recognizable).toBe(1); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); // Increment the reference count ks.incrementRefCount('ko1', 'test'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(2); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(2); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); // Increment again ks.incrementRefCount('ko1', 'test'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(3); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(3); - - // Decrement - ks.decrementRefCount('ko1', 'tess'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(2); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(2); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 2, + recognizable: 2, + }); - // Decrement twice more to reach 0 ks.decrementRefCount('ko1', 'test'); ks.decrementRefCount('ko1', 'test'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(0); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(0); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); // Create another object expect(ks.initKernelObject(ko2Owner)).toBe('ko2'); diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 5c8f49fc3d..9ac1085f18 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -38,9 +38,10 @@ * ${kpid}.decider = ${endid} // who decides on settlement * ${kpid}.value = JSON(CAPDATA) // value settled to, if settled * - * C-lists - * cle.${endid}.${eref} = ${kref} // ERef->KRef mapping - * clk.${endid}.${kref} = ${eref} // KRef->ERef mapping + * C-lists (both directions share one prefix; see `getCListPrefix`) + * ${endid}.c.${eref} = ${kref} // ERef->KRef mapping + * ${endid}.c.${kref} = R|_ ${eref} // KRef->ERef mapping, plus the + * // endpoint's reachable flag * * Vat bookkeeping * e.nextObjectId.${endid} = NN // allocation counter for imported object ERefs @@ -79,6 +80,7 @@ import { getPinMethods } from './methods/pinned.ts'; import { getPromiseMethods } from './methods/promise.ts'; import { getQueueMethods } from './methods/queue.ts'; import { getReachableMethods } from './methods/reachable.ts'; +import { getRefCountAuditMethods } from './methods/refcount-audit.ts'; import { getRefCountMethods } from './methods/refcount.ts'; import { getRelayMethods } from './methods/relay.ts'; import { getRemoteMethods } from './methods/remote.ts'; @@ -152,12 +154,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 +295,7 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { ...id, ...queue, ...refCount, + ...refCountAudit, ...object, ...promise, ...revocation, @@ -368,3 +373,4 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { export type KernelStore = ReturnType; export type { RelayEntry } from './types.ts'; +export type { RefCountViolation } from './methods/refcount-audit.ts'; diff --git a/packages/ocap-kernel/src/store/methods/base.ts b/packages/ocap-kernel/src/store/methods/base.ts index bd09b457a5..2ccd17f1f3 100644 --- a/packages/ocap-kernel/src/store/methods/base.ts +++ b/packages/ocap-kernel/src/store/methods/base.ts @@ -19,7 +19,18 @@ export function getBaseMethods(kv: KVStore) { * @returns The key for the reachable flag and vatSlot. */ function getSlotKey(endpointId: EndpointId, ref: Ref): string { - return `${endpointId}.c.${ref}`; + return `${getCListPrefix(endpointId)}${ref}`; + } + + /** + * Get the prefix shared by both directions of every entry in an endpoint's + * c-list, for iterating over the whole thing. + * + * @param endpointId - The endpoint whose c-list is of interest. + * @returns The prefix that all of that endpoint's c-list keys begin with. + */ + function getCListPrefix(endpointId: EndpointId): string { + return `${endpointId}.c.`; } /** @@ -206,6 +217,7 @@ export function getBaseMethods(kv: KVStore) { return { getSlotKey, + getCListPrefix, refCountKey, getOwnerKey, getRevokedKey, diff --git a/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts new file mode 100644 index 0000000000..49d4d384e5 --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts @@ -0,0 +1,278 @@ +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('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 e43d6e428a..4df8841bf9 100644 --- a/packages/ocap-kernel/src/store/methods/clist.test.ts +++ b/packages/ocap-kernel/src/store/methods/clist.test.ts @@ -27,39 +27,58 @@ describe('clist-methods', () => { }); describe('addCListEntry', () => { - it('adds a bidirectional mapping between KRef and ERef', () => { - const endpointId: EndpointId = 'v1'; - const kref: KRef = 'ko1'; - const eref: ERef = 'o-1'; + it.each([ + { what: 'an object import', kref: 'ko1', eref: 'o-1', flag: '_' }, + { what: 'an object export', kref: 'ko1', eref: 'o+1', flag: 'R' }, + { what: 'a promise import', kref: 'kp1', eref: 'p-2', flag: '_' }, + { what: 'a promise export', kref: 'kp1', eref: 'p+2', flag: 'R' }, + ] as { what: string; kref: KRef; eref: ERef; flag: string }[])( + 'adds a bidirectional mapping for $what', + ({ kref, eref, flag }) => { + const endpointId: EndpointId = 'v1'; + + clistMethods.addCListEntry(endpointId, kref, eref); + + // Only an export is born reachable; an import earns reachability when + // the reference is actually handed over + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`${flag} ${eref}`); + expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); + }, + ); + + it('works with remote endpoints', () => { + const endpointId: EndpointId = 'r1'; + const kref: KRef = 'ko2'; + const eref: ERef = 'ro+3'; clistMethods.addCListEntry(endpointId, kref, eref); - // Check that both mappings are stored expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); }); - it('works with promise refs', () => { - const endpointId: EndpointId = 'v1'; - const kref: KRef = 'kp1'; - const eref: ERef = 'p+2'; + it('takes a recognizable reference for an object import', () => { + clistMethods.addCListEntry('v1', 'ko1', 'o-1'); - clistMethods.addCListEntry(endpointId, kref, eref); + expect(kv.get('ko1.refCount')).toBe('0,1'); + }); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); - expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); + it('takes no reference for an object export', () => { + clistMethods.addCListEntry('v1', 'ko1', 'o+1'); + + expect(kv.get('ko1.refCount')).toBeUndefined(); }); - it('works with remote endpoints', () => { - const endpointId: EndpointId = 'r1'; - const kref: KRef = 'ko2'; - const eref: ERef = 'ro+3'; + it.each(['p-1', 'p+1'] as ERef[])( + 'takes a reference for a promise entry (%s)', + (eref) => { + kv.set('kp1.refCount', '1'); - clistMethods.addCListEntry(endpointId, kref, eref); + clistMethods.addCListEntry('v1', 'kp1', eref); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); - expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); - }); + expect(kv.get('kp1.refCount')).toBe('2'); + }, + ); }); describe('hasCListEntry', () => { @@ -96,7 +115,7 @@ describe('clist-methods', () => { expect(kv.get(`e.nextObjectId.${endpointId}`)).toBe('2'); // Check that the mapping was added - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); }); @@ -113,7 +132,7 @@ describe('clist-methods', () => { expect(kv.get(`e.nextPromiseId.${endpointId}`)).toBe('2'); // Check that the mapping was added - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); }); @@ -151,7 +170,7 @@ describe('clist-methods', () => { }); }); - describe('krefsToExistingErefs', () => { + describe('krefsToErefs', () => { it('returns the ERefs for existing KRefs', () => { const endpointId: EndpointId = 'v1'; const kref1: KRef = 'ko1'; @@ -163,25 +182,18 @@ describe('clist-methods', () => { clistMethods.addCListEntry(endpointId, kref2, eref2); expect( - clistMethods.krefsToExistingErefs(endpointId, [kref1, kref2]), + clistMethods.krefsToErefs(endpointId, [kref1, kref2]), ).toStrictEqual([eref1, eref2]); }); - it('returns an empty array for non-existent KRefs', () => { - const endpointId: EndpointId = 'v1'; - const kref: KRef = 'ko1'; - - expect( - clistMethods.krefsToExistingErefs(endpointId, [kref]), - ).toStrictEqual([]); + it('throws for an unmapped KRef', () => { + expect(() => clistMethods.krefsToErefs('v1', ['ko1'])).toThrow( + 'unmapped kref "ko1" in "v1" c-list', + ); }); it('returns an empty array for empty KRef array', () => { - const endpointId: EndpointId = 'v1'; - - expect(clistMethods.krefsToExistingErefs(endpointId, [])).toStrictEqual( - [], - ); + expect(clistMethods.krefsToErefs('v1', [])).toStrictEqual([]); }); }); @@ -191,7 +203,7 @@ describe('clist-methods', () => { const kref: KRef = 'ko1'; const eref: ERef = 'o-1'; clistMethods.addCListEntry(endpointId, kref, eref); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); clistMethods.forgetEref(endpointId, eref); expect(kv.get(`${endpointId}.c.${kref}`)).toBeUndefined(); @@ -214,7 +226,7 @@ describe('clist-methods', () => { const kref: KRef = 'ko1'; const eref: ERef = 'o-1'; clistMethods.addCListEntry(endpointId, kref, eref); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); clistMethods.forgetKref(endpointId, kref); expect(kv.get(`${endpointId}.c.${kref}`)).toBeUndefined(); diff --git a/packages/ocap-kernel/src/store/methods/clist.ts b/packages/ocap-kernel/src/store/methods/clist.ts index 425f4a1295..d079100ae4 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 a294920e9a..e91fb29bb9 100644 --- a/packages/ocap-kernel/src/store/methods/gc.test.ts +++ b/packages/ocap-kernel/src/store/methods/gc.test.ts @@ -76,21 +76,6 @@ describe('GC methods', () => { }); }); - describe('reachability tracking', () => { - it('manages reachable flags', () => { - const v1Object = kernelStore.initKernelObject('v1'); - kernelStore.addCListEntry('v1', v1Object, 'o-1'); - - expect(kernelStore.getReachableFlag('v1', v1Object)).toBe(true); - - kernelStore.clearReachableFlag('v1', v1Object); - expect(kernelStore.getReachableFlag('v1', v1Object)).toBe(false); - - const refCounts = kernelStore.getObjectRefCount(v1Object); - expect(refCounts.reachable).toBe(0); - }); - }); - describe('reaping', () => { it('processes reap queue in order', () => { const vatIds = ['v1', 'v2', 'v3']; diff --git a/packages/ocap-kernel/src/store/methods/gc.ts b/packages/ocap-kernel/src/store/methods/gc.ts index 04f3c0cc0c..31b21294c8 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 def40adaef..26d4fb9b16 100644 --- a/packages/ocap-kernel/src/store/methods/object.test.ts +++ b/packages/ocap-kernel/src/store/methods/object.test.ts @@ -29,7 +29,7 @@ describe('object-methods', () => { }); describe('initKernelObject', () => { - it('creates a new kernel object with initial reference counts', () => { + it('creates a new kernel object, unreferenced', () => { const owner: EndpointId = 'v1'; const koId = objectStore.initKernelObject(owner); @@ -39,13 +39,13 @@ describe('object-methods', () => { // Check the owner is set correctly expect(kv.get(`${koId}.owner`)).toBe(owner); - // Check reference counts are initialized to 1,1 - expect(kv.get(`${koId}.refCount`)).toBe('1,1'); - - // Check via the API - const refCounts = objectStore.getObjectRefCount(koId); - expect(refCounts.reachable).toBe(1); - expect(refCounts.recognizable).toBe(1); + // A new object has no referrers yet; the owner's own export entry is + // not one of them + expect(kv.get(`${koId}.refCount`)).toBe('0,0'); + expect(objectStore.getObjectRefCount(koId)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); }); it('initializes the revoked flag to false', () => { @@ -171,8 +171,10 @@ describe('object-methods', () => { it('returns reference counts for existing objects', () => { const koId = objectStore.initKernelObject('v1'); + objectStore.setObjectRefCount(koId, { reachable: 1, recognizable: 2 }); + const refCounts = objectStore.getObjectRefCount(koId); - expect(refCounts).toStrictEqual({ reachable: 1, recognizable: 1 }); + expect(refCounts).toStrictEqual({ reachable: 1, recognizable: 2 }); }); it('returns zero counts for non-existent objects', () => { @@ -276,8 +278,8 @@ describe('object-methods', () => { // Check initial state expect(objectStore.getOwner(koId)).toBe('v1'); expect(objectStore.getObjectRefCount(koId)).toStrictEqual({ - reachable: 1, - recognizable: 1, + reachable: 0, + recognizable: 0, }); // Update reference counts diff --git a/packages/ocap-kernel/src/store/methods/object.ts b/packages/ocap-kernel/src/store/methods/object.ts index 9e596f5e8f..57b7aaf88e 100644 --- a/packages/ocap-kernel/src/store/methods/object.ts +++ b/packages/ocap-kernel/src/store/methods/object.ts @@ -19,10 +19,11 @@ export function getObjectMethods(ctx: StoreContext) { getBaseMethods(ctx.kv); /** - * Create a new kernel object. The new object will be born with reference and - * recognizability counts of 1, on the assumption that the new object - * corresponds to an object that has just been imported from somewhere. The - * object is initially unrevoked. + * Create a new kernel object, born unreferenced at `(0, 0)`. Every unit of + * an object's counts is owed to a reference someone else holds — an + * importer's c-list entry, a queued message, a promise's resolution value, a + * pin — and the owner's own export entry is not one of them. The object is + * initially unrevoked. * * @param owner - The endpoint or 'kernel' that is the owner of the new object. * @returns The new object's KRef. @@ -30,7 +31,7 @@ export function getObjectMethods(ctx: StoreContext) { function initKernelObject(owner: EndpointId | 'kernel'): KRef { const koId = getNextObjectId(); ctx.kv.set(getOwnerKey(koId), owner); - setObjectRefCount(koId, { reachable: 1, recognizable: 1 }); + setObjectRefCount(koId, { reachable: 0, recognizable: 0 }); return koId; } diff --git a/packages/ocap-kernel/src/store/methods/promise.test.ts b/packages/ocap-kernel/src/store/methods/promise.test.ts index baeba6c2e5..d85e291ef5 100644 --- a/packages/ocap-kernel/src/store/methods/promise.test.ts +++ b/packages/ocap-kernel/src/store/methods/promise.test.ts @@ -58,6 +58,7 @@ describe('promise store methods', () => { }; let context: StoreContext; let promiseMethods: ReturnType; + const mockIncrementRefCount = vi.fn(); const mockDecrementRefCount = vi.fn(); beforeEach(() => { @@ -78,6 +79,7 @@ describe('promise store methods', () => { incCounter: mockIncCounter, provideStoredQueue: mockProvideStoredQueue, getPrefixedKeys: mockGetPrefixedKeys, + getCListPrefix: (endpointId: string) => `${endpointId}.c.`, }); (getQueueMethods as ReturnType).mockReturnValue({ @@ -85,6 +87,7 @@ describe('promise store methods', () => { }); (getRefCountMethods as ReturnType).mockReturnValue({ + incrementRefCount: mockIncrementRefCount, decrementRefCount: mockDecrementRefCount, }); @@ -304,11 +307,12 @@ describe('promise store methods', () => { slots: ['o+1', 'o+2'], }; const message1: KernelMessage = { - method: 'method1', - } as unknown as KernelMessage; + methargs: { body: 'method1', slots: ['ko7'] }, + result: 'kp8', + }; const message2: KernelMessage = { - method: 'method2', - } as unknown as KernelMessage; + methargs: { body: 'method2', slots: [] }, + }; mockKV.set(`${kpid}.state`, 'unresolved'); mockKV.set(`${kpid}.decider`, 'v1'); @@ -338,7 +342,15 @@ describe('promise store methods', () => { expect(mockKV.has(`${kpid}.decider`)).toBe(false); expect(mockKV.has(`${kpid}.subscribers`)).toBe(false); expect(mockQueue.delete).toHaveBeenCalled(); - expect(mockDecrementRefCount).toHaveBeenCalledTimes(1); + // Each dequeued message releases what its queue entry held, then the + // promise releases the decision it was owed + expect(mockDecrementRefCount.mock.calls).toStrictEqual([ + [kpid, 'resolve|dequeue|target'], + ['kp8', 'resolve|dequeue|result'], + ['ko7', 'resolve|dequeue|slot'], + [kpid, 'resolve|dequeue|target'], + [kpid, 'resolve|decider'], + ]); }); it('rejects a promise and enqueues pending messages', () => { @@ -372,6 +384,23 @@ describe('promise store methods', () => { expect(mockProvideStoredQueue).toHaveBeenCalledWith(kpid, false); expect(mockQueue.enqueue).toHaveBeenCalledWith(message); }); + + it('takes a reference on everything the queued message carries', () => { + const kpid = 'kp123'; + const message: KernelMessage = { + methargs: { body: 'test', slots: ['ko1', 'kp2'] }, + result: 'kp3', + }; + + promiseMethods.enqueuePromiseMessage(kpid, message); + + expect(mockIncrementRefCount.mock.calls).toStrictEqual([ + [kpid, 'promiseQueue|target'], + ['kp3', 'promiseQueue|result'], + ['ko1', 'promiseQueue|slot'], + ['kp2', 'promiseQueue|slot'], + ]); + }); }); describe('getKernelPromiseMessageQueue', () => { @@ -410,69 +439,71 @@ describe('promise store methods', () => { }); describe('getPromisesByDecider', () => { - it('yields promises decided by a specific vat', () => { - const vatId = 'v1' as VatId; - const kpid1 = 'kp101'; - const kpid2 = 'kp102'; - const kpid3 = 'kp103'; - - // Set up mock data - mockGetPrefixedKeys.mockReturnValue([ - `cle.${vatId}.p1`, - `cle.${vatId}.p2`, - `cle.${vatId}.p3`, - ]); - - mockKV.set(`cle.${vatId}.p1`, kpid1); - mockKV.set(`cle.${vatId}.p2`, kpid2); - mockKV.set(`cle.${vatId}.p3`, kpid3); - - // kpid1 is decided by vatId - mockKV.set(`${kpid1}.state`, 'unresolved'); - mockKV.set(`${kpid1}.decider`, vatId); - mockKV.set(`${kpid1}.subscribers`, '[]'); - - // kpid2 is also decided by vatId - mockKV.set(`${kpid2}.state`, 'unresolved'); - mockKV.set(`${kpid2}.decider`, vatId); - mockKV.set(`${kpid2}.subscribers`, '[]'); + /** + * Populate a c-list and an unresolved promise record, using the real key + * layout so the scan is exercised rather than mocked around. + * + * @param endpointId - The endpoint whose c-list to add to. + * @param eref - The endpoint's ref for the promise. + * @param kpid - The kernel promise. + * @param decider - The promise's decider, if it has one. + * @param state - The promise's state. + */ + function givenCListPromise( + endpointId: string, + eref: string, + kpid: string, + decider: string | undefined, + state = 'unresolved', + ): void { + mockKV.set(`${endpointId}.c.${eref}`, kpid); + mockKV.set(`${endpointId}.c.${kpid}`, `R ${eref}`); + mockKV.set(`${kpid}.state`, state); + mockKV.set(`${kpid}.subscribers`, '[]'); + if (state === 'unresolved') { + if (decider) { + mockKV.set(`${kpid}.decider`, decider); + } + } else { + mockKV.set(`${kpid}.value`, '{"body":"value","slots":[]}'); + } + mockGetPrefixedKeys.mockImplementation((prefix: string) => + [...mockKV.keys()].filter((key) => key.startsWith(prefix)).sort(), + ); + } - // kpid3 is unresolved but decided by a different vat - mockKV.set(`${kpid3}.state`, 'unresolved'); - mockKV.set(`${kpid3}.decider`, 'v2'); - mockKV.set(`${kpid3}.subscribers`, '[]'); + it.each([ + { context: 'a vat', endpointId: 'v1', erefs: ['p+1', 'p-2'] }, + { context: 'a remote', endpointId: 'r1', erefs: ['rp+1', 'rp-2'] }, + ])('yields promises decided by $context', ({ endpointId, erefs }) => { + givenCListPromise(endpointId, erefs[0] as string, 'kp101', endpointId); + givenCListPromise(endpointId, erefs[1] as string, 'kp102', endpointId); + givenCListPromise(endpointId, 'p+3', 'kp103', 'v2'); - const result = Array.from(promiseMethods.getPromisesByDecider(vatId)); + const result = Array.from( + promiseMethods.getPromisesByDecider(endpointId as VatId), + ); - expect(result).toStrictEqual([kpid1, kpid2]); - expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`cle.${vatId}.p`); + expect(result).toStrictEqual(['kp101', 'kp102']); }); it('does not yield resolved promises', () => { - const vatId = 'v1' as VatId; - const kpid1 = 'kp101'; - const kpid2 = 'kp102'; + givenCListPromise('v1', 'p+1', 'kp101', undefined, 'fulfilled'); + givenCListPromise('v1', 'p+2', 'kp102', 'v1'); - mockGetPrefixedKeys.mockReturnValue([ - `cle.${vatId}.p1`, - `cle.${vatId}.p2`, - ]); + const result = Array.from(promiseMethods.getPromisesByDecider('v1')); - mockKV.set(`cle.${vatId}.p1`, kpid1); - mockKV.set(`cle.${vatId}.p2`, kpid2); - - // kpid1 is fulfilled - mockKV.set(`${kpid1}.state`, 'fulfilled'); - mockKV.set(`${kpid1}.value`, '{"body":"value","slots":[]}'); + expect(result).toStrictEqual(['kp102']); + }); - // kpid2 is unresolved and decided by vatId - mockKV.set(`${kpid2}.state`, 'unresolved'); - mockKV.set(`${kpid2}.decider`, vatId); - mockKV.set(`${kpid2}.subscribers`, '[]'); + it('ignores object entries in the same c-list', () => { + givenCListPromise('v1', 'p+1', 'kp101', 'v1'); + mockKV.set('v1.c.o+1', 'ko1'); + mockKV.set('v1.c.ko1', 'R o+1'); - const result = Array.from(promiseMethods.getPromisesByDecider(vatId)); + const result = Array.from(promiseMethods.getPromisesByDecider('v1')); - expect(result).toStrictEqual([kpid2]); + expect(result).toStrictEqual(['kp101']); }); it('yields nothing if no promises are decided by the vat', () => { diff --git a/packages/ocap-kernel/src/store/methods/promise.ts b/packages/ocap-kernel/src/store/methods/promise.ts index c3aedbf6c3..40fa73b049 100644 --- a/packages/ocap-kernel/src/store/methods/promise.ts +++ b/packages/ocap-kernel/src/store/methods/promise.ts @@ -16,6 +16,9 @@ import { makeKernelSlot } from '../utils/kernel-slots.ts'; import { parseRef } from '../utils/parse-ref.ts'; import { isPromiseRef } from '../utils/promise-ref.ts'; +/** Matches the promise erefs in a c-list: `p+NN`/`p-NN`, or `rp+NN`/`rp-NN` for a remote. */ +const PROMISE_EREF = /^r?p[-+]\d+$/u; + /** * Create a promise store object that provides functionality for managing kernel promises. * @@ -25,14 +28,20 @@ import { isPromiseRef } from '../utils/promise-ref.ts'; */ // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getPromiseMethods(ctx: StoreContext) { - const { incCounter, provideStoredQueue, getPrefixedKeys, refCountKey } = - getBaseMethods(ctx.kv); - const { decrementRefCount } = getRefCountMethods(ctx); + const { + incCounter, + provideStoredQueue, + getPrefixedKeys, + getCListPrefix, + refCountKey, + } = getBaseMethods(ctx.kv); + const { incrementRefCount, decrementRefCount } = getRefCountMethods(ctx); /** - * Create a new, unresolved kernel promise. The new promise will be born with - * a reference count of 1 on the assumption that the promise has just been - * imported from somewhere. + * Create a new, unresolved kernel promise, born with a reference count of 1: + * an unsettled promise is owed a decision, and that obligation is itself a + * reference. Released, exactly once, when the promise settles in + * {@link resolveKernelPromise}. * * @returns A tuple of the new promise's KRef and an object describing the * new promise itself. @@ -159,16 +168,18 @@ export function getPromiseMethods(ctx: StoreContext) { value: CapData, ): [KRef, KernelMessage][] { const queue = provideStoredQueue(kpid, false); - // Collect messages that were queued on this promise + // Releasing each queue entry's references as we go: the caller re-enqueues + // these on the run queue, which takes its own. const queuedMessages: [KRef, KernelMessage][] = []; for (const message of getKernelPromiseMessageQueue(kpid)) { queuedMessages.push([kpid, message]); + releaseQueuedMessageRefs(kpid, message, 'resolve|dequeue'); } ctx.kv.set(`${kpid}.state`, rejected ? 'rejected' : 'fulfilled'); ctx.kv.set(`${kpid}.value`, JSON.stringify(value)); ctx.kv.delete(`${kpid}.decider`); ctx.kv.delete(`${kpid}.subscribers`); - // Drop the baseline "decider" refcount now that the promise is settled. + // The promise has been decided, so it is no longer owed a decision. decrementRefCount(kpid, 'resolve|decider'); queue.delete(); return queuedMessages; @@ -177,13 +188,46 @@ export function getPromiseMethods(ctx: StoreContext) { /** * Append a message to a promise's message queue. * + * The queue entry becomes the message's holder, so it takes references on + * everything the message carries, just as the run queue does. + * * @param kpid - The KRef of the promise to enqueue on. * @param message - The message to enqueue. */ function enqueuePromiseMessage(kpid: KRef, message: KernelMessage): void { + incrementRefCount(kpid, 'promiseQueue|target'); + if (message.result) { + incrementRefCount(message.result, 'promiseQueue|result'); + } + for (const slot of message.methargs.slots) { + incrementRefCount(slot, 'promiseQueue|slot'); + } provideStoredQueue(kpid, false).enqueue(message); } + /** + * Release the references a promise-queue entry held on the message it + * carried. + * + * @param kpid - The promise whose queue the message was on, and hence the + * message's target. + * @param message - The message being taken off the queue. + * @param tag - Tag for refcount logging. + */ + function releaseQueuedMessageRefs( + kpid: KRef, + message: KernelMessage, + tag: string, + ): void { + decrementRefCount(kpid, `${tag}|target`); + if (message.result) { + decrementRefCount(message.result, `${tag}|result`); + } + for (const slot of message.methargs.slots) { + decrementRefCount(slot, `${tag}|slot`); + } + } + /** * Fetch the messages in a kernel promise's message queue. * @@ -211,8 +255,13 @@ export function getPromiseMethods(ctx: StoreContext) { * @yields the kpids of all the unresolved promises decided by `decider`. */ function* getPromisesByDecider(decider: EndpointId): Generator { - const basePrefix = `cle.${decider}.`; - for (const key of getPrefixedKeys(`${basePrefix}p`)) { + const prefix = getCListPrefix(decider); + for (const key of getPrefixedKeys(prefix)) { + // A c-list holds both directions of each pair. Iterate by eref, and only + // the promise ones: `p+NN`/`p-NN` for a vat, `rp+NN`/`rp-NN` for a remote. + if (!PROMISE_EREF.test(key.slice(prefix.length))) { + continue; + } const kpid = ctx.kv.getRequired(key); const kp = getKernelPromise(kpid); if (kp.state === 'unresolved' && kp.decider === decider) { diff --git a/packages/ocap-kernel/src/store/methods/reachable.test.ts b/packages/ocap-kernel/src/store/methods/reachable.test.ts index fdaab477e5..1ec32bdb9f 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 4caa2d0960..05e4c5f986 100644 --- a/packages/ocap-kernel/src/store/methods/reachable.ts +++ b/packages/ocap-kernel/src/store/methods/reachable.ts @@ -54,6 +54,32 @@ export function getReachableMethods(ctx: StoreContext) { return parseReachableAndVatSlot(data); } + /** + * Set the reachable flag for a given endpoint and kref. + * + * The counterpart to {@link clearReachableFlag}: this is how an object + * regains reachability when a vat that dropped it is handed it again. + * Idempotent, so repeated translations don't inflate the count. + * + * @param endpointId - The endpoint for which the reachable flag is being set. + * @param kref - The kref. + */ + function setReachableFlag(endpointId: EndpointId, kref: KRef): void { + const key = getSlotKey(endpointId, kref); + const { isReachable, vatSlot } = getReachableAndVatSlot(endpointId, kref); + if (isReachable) { + return; + } + ctx.kv.set(key, buildReachableAndVatSlot(true, vatSlot)); + const { direction, isPromise } = parseRef(vatSlot); + // increment 'reachable' part of refcount, but only for object imports + if (!isPromise && direction === 'import' && kernelRefExists(kref)) { + const counts = getObjectRefCount(kref); + counts.reachable += 1; + setObjectRefCount(kref, counts); + } + } + /** * Clear the reachable flag for a given endpoint and kref. * @@ -84,6 +110,7 @@ export function getReachableMethods(ctx: StoreContext) { return { getReachableFlag, getReachableAndVatSlot, + setReachableFlag, clearReachableFlag, }; } diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts new file mode 100644 index 0000000000..70af7d4075 --- /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 0000000000..c43fd904de --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -0,0 +1,352 @@ +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. + * + * @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(); + + 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') { + // 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. + * + * This is how a store written under the pre-fix accounting is brought onto + * the current scheme: the references themselves are authoritative, so the + * counts can simply be rebuilt from them. Krefs that are referenced but have + * already been deleted cannot be repaired this way and are reported instead. + * + * @returns The violations that were corrected and those that could not be. + */ + function recomputeRefCounts(): { + corrected: RefCountViolation[]; + unfixable: RefCountViolation[]; + } { + const corrected: RefCountViolation[] = []; + const unfixable: RefCountViolation[] = []; + for (const violation of auditRefCounts()) { + if (violation.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/translators.test.ts b/packages/ocap-kernel/src/store/methods/translators.test.ts index 0ac95eafb0..fd1b5e1df6 100644 --- a/packages/ocap-kernel/src/store/methods/translators.test.ts +++ b/packages/ocap-kernel/src/store/methods/translators.test.ts @@ -14,6 +14,7 @@ import type { } from '../../types.ts'; import type { StoreContext } from '../types.ts'; import * as clistModule from './clist.ts'; +import * as reachableModule from './reachable.ts'; import { getTranslators } from './translators.ts'; import * as vatModule from './vat.ts'; @@ -22,6 +23,7 @@ describe('getTranslators', () => { const mockErefToKref = vi.fn(); const mockAllocateErefForKref = vi.fn(); const mockExportFromEndpoint = vi.fn(); + const mockSetReachableFlag = vi.fn(); const mockCtx = {} as StoreContext; beforeEach(() => { @@ -33,6 +35,10 @@ describe('getTranslators', () => { allocateErefForKref: mockAllocateErefForKref, } as unknown as ReturnType); + vi.spyOn(reachableModule, 'getReachableMethods').mockReturnValue({ + setReachableFlag: mockSetReachableFlag, + } as unknown as ReturnType); + vi.spyOn(vatModule, 'getVatMethods').mockReturnValue({ exportFromEndpoint: mockExportFromEndpoint, } as unknown as ReturnType); diff --git a/packages/ocap-kernel/src/store/methods/translators.ts b/packages/ocap-kernel/src/store/methods/translators.ts index c5948fedff..b7d418a1c3 100644 --- a/packages/ocap-kernel/src/store/methods/translators.ts +++ b/packages/ocap-kernel/src/store/methods/translators.ts @@ -21,6 +21,7 @@ import type { } from '../../types.ts'; import type { StoreContext } from '../types.ts'; import { getCListMethods } from './clist.ts'; +import { getReachableMethods } from './reachable.ts'; import { getVatMethods } from './vat.ts'; import { Fail, assert } from '../../utils/assert.ts'; @@ -35,6 +36,7 @@ import { Fail, assert } from '../../utils/assert.ts'; // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getTranslators(ctx: StoreContext) { const { krefToEref, erefToKref, allocateErefForKref } = getCListMethods(ctx); + const { setReachableFlag } = getReachableMethods(ctx); const { exportFromEndpoint } = getVatMethods(ctx); /** @@ -54,6 +56,11 @@ export function getTranslators(ctx: StoreContext) { /** * Translate a reference from kernel space into endpoint space. * + * Translating is how the kernel hands an endpoint a reference, so it also + * re-establishes reachability: a vat given an object it previously dropped + * holds it live again. Garbage collection deliveries must not do this, and + * don't — they map through `krefsToErefs`, which never touches the flag. + * * @param endpointId - The endpoint for whom translation is desired. * @param kref - The KRef of the entity of interest. * @param importIfNeeded - If true, allocate a new clist entry if necessary; @@ -74,6 +81,7 @@ export function getTranslators(ctx: StoreContext) { throw Fail`unmapped kref ${kref} endpoint=${endpointId}`; } } + setReachableFlag(endpointId, kref); if (isRemoteId(endpointId)) { // The import/export relationship between a vat and the kernel is // asymmetric -- the vat always exports to the kernel and imports from the diff --git a/packages/ocap-kernel/src/store/methods/vat.test.ts b/packages/ocap-kernel/src/store/methods/vat.test.ts index d04c609896..e2a3914f6e 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", () => { - seedClist([['rp+1', 'kp123']]); - mockGetKernelPromise.mockReturnValue({ decider: endpointId }); - - vatMethods.forgetEndpointImports(endpointId); - - 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', () => { + it("releases the peer's promise exports through the c-list", () => { seedClist([['rp+1', 'kp123']]); - mockGetKernelPromise.mockReturnValue({ decider: 'someoneElse' }); 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).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,26 @@ 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(); }); 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 48d431dc10..29e2a85dff 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 1ea54d19cc..3bf54862fa 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/VatManager.ts b/packages/ocap-kernel/src/vats/VatManager.ts index b90f6f30c8..314bfa87fb 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,14 @@ export class VatManager { } else if (terminating) { terminationError = new VatDeletedError(vatId); } + if (terminating) { + // Release the pin `launchVat` took, so the root can be collected once + // its importers let go. A restart keeps it: the same root comes back. + const rootRef = this.#kernelStore.getRootObject(vatId); + if (rootRef) { + this.#kernelStore.unpinObject(rootRef); + } + } await this.#platformServices .terminate(vatId, terminationError) .catch(this.#logger.error); From d921e9fe6be37b2d4ec72b359c757fc00dfeac0b Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 13 Aug 2026 17:09:30 +0200 Subject: [PATCH 02/17] fix(ocap-kernel): format the changelog and cite this PR Prettier wanted a blank line before the entry following a nested bullet, and the entries still cited #1010, which this PR replaces. --- packages/ocap-kernel/CHANGELOG.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 0b9328172a..9a7e0b3050 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -37,10 +37,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- 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 with no holder (a leak) - Exports the `RefCountViolation` type -- Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) ### Changed @@ -71,21 +71,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Report the database error when an aborted crank cannot be rolled back, instead of a spurious "no such savepoint" ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) - The abort path recorded the rollback only after it succeeded, so a throwing rollback had the run loop try again against the savepoint `rollbackCrank` had already discarded. The second attempt's "no such savepoint" then became the reported cause of death — and since only `error.message` crosses the wire, the real failure reached neither `getStatus` nor the daemon log - Reject the run loop's promise with the same `Error` its status reports, rather than re-throwing a non-`Error` for the embedder to normalize a second time ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) -- **BREAKING:** Make c-list reference accounting symmetric: creating an import c-list entry now takes a reference, as tearing one down has always released one ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- **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 ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- 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 ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- 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 ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) -- Release a queued notification's reference before the paths that decide there is nothing to deliver, and stop decrementing references on promises retired alongside it that nobody had taken ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) -- Transfer, rather than duplicate, the references a message carries when it is queued on an unresolved promise and later re-enqueued on resolution ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) -- Fix the stale `cle.`/`clk.` key prefixes in `getPromisesByDecider` and `deleteEndpoint`, which no longer matched the `${endpointId}.c.` c-list layout ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010)) +- Charge a delivered message's target reference against the run-queue item's own target rather than the routed target, 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 + - 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 From 1f9c8881327296af70ab0b56408696aaffb1d0f9 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 13 Aug 2026 17:20:12 +0200 Subject: [PATCH 03/17] fix(ocap-kernel): don't audit an importer entry the collector is retiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `retireKernelObjects` deletes an object and queues a `retireImport` for each importer in the same breath, so until that action is delivered an importer's c-list entry names a kref the kernel has already dropped. The audit counted those entries as holders and reported a violation against the collector's own output — and since `assertRefCountsIfAuditing` throws from inside the crank, that killed the run loop for good. Reachable from an ordinary `terminateVat` while a surviving vat holds the dying vat's export in liveslots' dropped-but-recognizable state. No current test produced it; found by Cursor Bugbot on #1020 and reproduced against the real store. Co-Authored-By: Claude Opus 5 (1M context) --- .../store/methods/clist-accounting.test.ts | 20 +++++++++++++++++++ .../src/store/methods/refcount-audit.ts | 14 ++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts index 49d4d384e5..c7d1cf22d1 100644 --- a/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts +++ b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts @@ -216,6 +216,26 @@ describe('c-list reference accounting', () => { 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); diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.ts index c43fd904de..e79a63e946 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -98,6 +98,15 @@ export function getRefCountAuditMethods(ctx: StoreContext) { */ 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, @@ -152,7 +161,10 @@ export function getRefCountAuditMethods(ctx: StoreContext) { if (isPromiseRef(kref)) { // Both directions count for a promise. credit(kref, holder); - } else if (direction === 'import') { + } 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 }); From e26c92924a4d55d68b3632fb4dedb13bee71805b Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Fri, 14 Aug 2026 16:26:06 +0200 Subject: [PATCH 04/17] fix(ocap-kernel): retain the holders the accounting cannot see MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebasing the baseline to (0, 0) made every reference explicit, which exposed the holders that were never references at all. An ocap URL carries its kref inside an encrypted bearer token and nothing else, so the kernel cannot discover from its own state that a holder exists: `issueOcapURL` took no reference of any kind. Under the old baseline nothing exported was collectable and it never showed; 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. The audit is silent on it by construction — the object genuinely has no holder it can see. Retain the target when the URL is issued, before the token exists, since the token is unretractable once it does. One pin per kref however many URLs name it, and no release: the token is persistent and unexpiring, so `revoke` is how the capability dies. Pinning also puts the holder inside the reference graph, so the audit can see it rather than being taught to excuse it. The same shape had a second door. `incrementRefCount` has no `kernelRefExists` guard where `decrementRefCount` does, so importing a deleted kref read its missing counts as (0, 0) and wrote them back, resurrecting a live-looking object with no owner — deliverable to by nobody, and endorsed by the audit, since the new c-list entry is a legitimate holder for exactly the count it finds. Reached by redeeming a URL issued for an object since collected. Guard the point of corruption, `translateRefKtoE`, rather than `incrementRefCount` itself: creating an entry for a deleted kref is the invariant, and releasing a reference to something already gone is how GC teardown is allowed to race deletion. Also release a vat's root pin when `deleteSubcluster` retires vats that never ran here. It 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. `stopVat` and `deleteSubcluster` now share `releaseVatRootPin`. Vat root pinning had no unit coverage at all, so pin-on-launch, release-on-terminate and keep-across-restart are asserted now; the last is what the comment claims and what would break silently. Restores the `maybeFreeKrefs` assertion on `forgetEndpointImports`' ownership-migrated branch, which lost its `not.toHaveBeenCalled` when that branch stopped returning early. Corrects three claims that the (0, 0) birth falsified and that shipped as documentation: both `KernelServiceManager` comments asserting its delete branch cannot fire, when it now does, and a changelog entry asserting (1, 1) birth two dozen lines above one asserting (0, 0). `recomputeRefCounts` no longer describes itself as a migration; nothing calls it, and opening an existing store does not migrate one. Co-Authored-By: Claude Opus 5 (1M context) --- packages/ocap-kernel/CHANGELOG.md | 17 ++++- packages/ocap-kernel/src/Kernel.ts | 1 + .../ocap-kernel/src/KernelServiceManager.ts | 12 ++-- .../src/remotes/kernel/OcapURLManager.test.ts | 63 ++++++++++++++++--- .../src/remotes/kernel/OcapURLManager.ts | 12 +++- .../src/remotes/kernel/RemoteHandle.test.ts | 7 ++- packages/ocap-kernel/src/store/index.test.ts | 2 + packages/ocap-kernel/src/store/index.ts | 31 +++++++++ .../src/store/methods/refcount-audit.ts | 20 ++++-- .../ocap-kernel/src/store/methods/refcount.ts | 5 ++ .../src/store/methods/translators.test.ts | 19 ++++++ .../src/store/methods/translators.ts | 11 ++++ .../ocap-kernel/src/store/methods/vat.test.ts | 4 ++ .../src/vats/SubclusterManager.test.ts | 6 ++ .../ocap-kernel/src/vats/SubclusterManager.ts | 5 ++ .../ocap-kernel/src/vats/VatManager.test.ts | 45 +++++++++++++ packages/ocap-kernel/src/vats/VatManager.ts | 34 +++++++--- packages/ocap-kernel/test/remotes-mocks.ts | 1 + 18 files changed, 263 insertions(+), 32 deletions(-) diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 9a7e0b3050..1079dd9db7 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -38,9 +38,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Caveat: a legitimate string argument that begins with `@@` followed by alphanumerics will be misinterpreted as a marker; wrap such literals inside an object - Reference-count auditing: `auditRefCounts`, `recomputeRefCounts`, `formatRefCountViolations`, `assertRefCountsIfAuditing`, and `setRefCountAuditing` on the kernel store, plus a `Kernel.make` option `auditRefCounts` that verifies every kref's counts against the references the kernel actually holds at the end of each crank ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) - - Reports drift in both directions: counts too low (a live capability can be collected) and counts too high with no holder (a leak) + - 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 @@ -55,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)) @@ -84,8 +87,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 +- 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)) diff --git a/packages/ocap-kernel/src/Kernel.ts b/packages/ocap-kernel/src/Kernel.ts index 94865cb049..c041d060be 100644 --- a/packages/ocap-kernel/src/Kernel.ts +++ b/packages/ocap-kernel/src/Kernel.ts @@ -176,6 +176,7 @@ export class Kernel { this.#ocapURLManager = new OcapURLManager({ remoteManager: this.#remoteManager, + kernelStore: this.#kernelStore, }); this.#kernelServiceManager = new KernelServiceManager({ diff --git a/packages/ocap-kernel/src/KernelServiceManager.ts b/packages/ocap-kernel/src/KernelServiceManager.ts index bc236e6113..6744c98c74 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 b97b79369c..8c59ab9389 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,42 @@ 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('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 +230,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 +248,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 +270,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 +338,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 +353,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 +374,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 eec66cae1b..2a2b97f3cc 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,6 +126,9 @@ export class OcapURLManager { */ async issueOcapURL(kref: KRef): Promise { const identity = this.#remoteManager.getRemoteIdentity(); + // Before minting the token, not after: the URL is unretractable once it + // exists, so the target must already be retained. See `retainForOcapURL`. + this.#kernelStore.retainForOcapURL(kref); return identity.issueOcapURL(kref); } diff --git a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts index 009834a65b..4f53572c0d 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts @@ -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/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index d4e68387cf..83b27dea79 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -103,6 +103,7 @@ describe('kernel store', () => { 'getNextRemoteId', 'getNextVatId', 'getObjectRefCount', + 'getOcapURLObjects', 'getOwner', 'getPeerIncarnation', 'getPendingMessage', @@ -159,6 +160,7 @@ describe('kernel store', () => { 'removeVatFromSubcluster', 'reset', 'resolveKernelPromise', + 'retainForOcapURL', 'retireKernelObjects', 'revoke', 'rollbackCrank', diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 9ac1085f18..00a8d591f2 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -65,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'; @@ -368,6 +369,36 @@ 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; there is deliberately no release here. + getOcapURLObjects(): KRef[] { + const raw = kv.get('ocapURLObjects'); + return raw ? (raw.split(',') as KRef[]) : []; + }, + retainForOcapURL(kref: KRef): void { + // 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; + } + krefs.add(kref); + kv.set('ocapURLObjects', [...krefs].sort().join(',')); + this.pinObject(kref); + }, }); } diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.ts index e79a63e946..b4178e5c6f 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -59,6 +59,12 @@ const REFCOUNT_KEY = /^(k[op]\d+)\.refCount$/u; * 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. */ @@ -287,10 +293,16 @@ export function getRefCountAuditMethods(ctx: StoreContext) { /** * Overwrite stored reference counts with the counts implied by ground truth. * - * This is how a store written under the pre-fix accounting is brought onto - * the current scheme: the references themselves are authoritative, so the - * counts can simply be rebuilt from them. Krefs that are referenced but have - * already been deleted cannot be repaired this way and are reported instead. + * 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. */ diff --git a/packages/ocap-kernel/src/store/methods/refcount.ts b/packages/ocap-kernel/src/store/methods/refcount.ts index b87d13c253..6cea54268b 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 fd1b5e1df6..b50f21b4a0 100644 --- a/packages/ocap-kernel/src/store/methods/translators.test.ts +++ b/packages/ocap-kernel/src/store/methods/translators.test.ts @@ -15,6 +15,7 @@ import type { 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'; @@ -24,10 +25,12 @@ describe('getTranslators', () => { 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, @@ -39,6 +42,10 @@ describe('getTranslators', () => { 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); @@ -70,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 b7d418a1c3..3ba551d28f 100644 --- a/packages/ocap-kernel/src/store/methods/translators.ts +++ b/packages/ocap-kernel/src/store/methods/translators.ts @@ -22,6 +22,7 @@ import type { 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'; @@ -37,6 +38,7 @@ import { Fail, assert } from '../../utils/assert.ts'; export function getTranslators(ctx: StoreContext) { const { krefToEref, erefToKref, allocateErefForKref } = getCListMethods(ctx); const { setReachableFlag } = getReachableMethods(ctx); + const { kernelRefExists } = getRefCountMethods(ctx); const { exportFromEndpoint } = getVatMethods(ctx); /** @@ -76,6 +78,15 @@ 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}`; diff --git a/packages/ocap-kernel/src/store/methods/vat.test.ts b/packages/ocap-kernel/src/store/methods/vat.test.ts index e2a3914f6e..fd1f05402e 100644 --- a/packages/ocap-kernel/src/store/methods/vat.test.ts +++ b/packages/ocap-kernel/src/store/methods/vat.test.ts @@ -600,6 +600,10 @@ describe('vat store methods', () => { expect(mockKV.has(`slot.${endpointId}.ko42`)).toBe(false); expect(mockKV.has(`slot.${endpointId}.ro+7`)).toBe(false); expect(mockDecrementRefCount).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/vats/SubclusterManager.test.ts b/packages/ocap-kernel/src/vats/SubclusterManager.test.ts index bea731738c..23af0e0402 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 2686fda5f3..1b21b6c26a 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 d5e92b1aac..21361f942c 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 314bfa87fb..5da2bebe2d 100644 --- a/packages/ocap-kernel/src/vats/VatManager.ts +++ b/packages/ocap-kernel/src/vats/VatManager.ts @@ -192,12 +192,8 @@ export class VatManager { terminationError = new VatDeletedError(vatId); } if (terminating) { - // Release the pin `launchVat` took, so the root can be collected once - // its importers let go. A restart keeps it: the same root comes back. - const rootRef = this.#kernelStore.getRootObject(vatId); - if (rootRef) { - this.#kernelStore.unpinObject(rootRef); - } + // A restart keeps the pin: the same root comes back. + this.releaseVatRootPin(vatId); } await this.#platformServices .terminate(vatId, terminationError) @@ -299,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. @@ -314,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 0e37b4a77c..3d3e332e13 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, }; } From 051d772dd0310b125db5f75b5e15259fbfeca70c Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Fri, 14 Aug 2026 16:47:19 +0200 Subject: [PATCH 05/17] fix(ocap-kernel): release the ocap URL retention a failed mint took Retaining before minting is right: minting awaits, so a collection crank can run in that window. But nothing undid the retention when minting then failed. 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. retainForOcapURL now reports whether this call took the pin, and undoOcapURLRetention unwinds one that never backed a URL. Guarded on the ledger rather than the pin list, so it can only remove the pin it put there: a kref some live URL already names keeps the pin that URL depends on, and a vat root keeps its lifetime pin. Co-Authored-By: Claude Opus 5 (1M context) --- packages/ocap-kernel/CHANGELOG.md | 1 + .../src/remotes/kernel/OcapURLManager.test.ts | 33 +++++++++++ .../src/remotes/kernel/OcapURLManager.ts | 19 ++++++- packages/ocap-kernel/src/store/index.test.ts | 57 +++++++++++++++++++ packages/ocap-kernel/src/store/index.ts | 20 ++++++- 5 files changed, 124 insertions(+), 6 deletions(-) diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 1079dd9db7..c48411a0ff 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -92,6 +92,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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)) diff --git a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts index 8c59ab9389..5bba9896a0 100644 --- a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts @@ -130,6 +130,39 @@ describe('OcapURLManager', () => { }); }); + 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); diff --git a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts index 2a2b97f3cc..aee9b1710f 100644 --- a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts +++ b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts @@ -127,9 +127,22 @@ export class OcapURLManager { async issueOcapURL(kref: KRef): Promise { const identity = this.#remoteManager.getRemoteIdentity(); // Before minting the token, not after: the URL is unretractable once it - // exists, so the target must already be retained. See `retainForOcapURL`. - this.#kernelStore.retainForOcapURL(kref); - return identity.issueOcapURL(kref); + // 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/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 83b27dea79..af06ace09c 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -192,6 +192,7 @@ describe('kernel store', () => { 'translateRefEtoK', 'translateRefKtoE', 'translateSyscallVtoK', + 'undoOcapURLRetention', 'unpinObject', 'waitForCrank', ]); @@ -366,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 00a8d591f2..6ba7c51775 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -378,12 +378,13 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { // 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; there is deliberately no release here. + // 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): void { + 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. @@ -393,11 +394,24 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { // 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; + 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); }, }); } From ee9c894fa398fc53c91b671da27731da61f56722 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 17 Aug 2026 11:45:09 +0200 Subject: [PATCH 06/17] fix(ocap-kernel): take an ocap URL retention per issuance, not per kref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retention was deduplicated by kref, and the failure path undid it if this call was the one that took it. Minting awaits, though, so issuances for the same target overlap: a second `issue` can mint a URL while the first is still in flight, having taken no retention of its own because the ledger already named the kref. If the first then fails it unwinds the retention the second's live URL depends on, and collection can take the capability out from under it. The ledger is a multiset now, one entry and one pin per issuance, so a failed mint releases only what it took. Pins were already a multiset, and each pin here is either released by its own failure or held by its own live URL, so none is left unreleasable — the concern that motivated deduplicating. Co-Authored-By: Claude Opus 5 (1M context) --- packages/ocap-kernel/CHANGELOG.md | 4 +-- .../src/remotes/kernel/OcapURLManager.test.ts | 35 ++++++++++++++++--- .../src/remotes/kernel/OcapURLManager.ts | 11 +++--- packages/ocap-kernel/src/store/index.test.ts | 31 +++++++++++++--- packages/ocap-kernel/src/store/index.ts | 31 ++++++++-------- 5 files changed, 82 insertions(+), 30 deletions(-) diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index c48411a0ff..1cab9b17ef 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -90,9 +90,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `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 + - One pin per URL, 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 + - The retention is taken before the token is minted, since minting awaits and a collection crank can run in that window, and released again if minting fails. It is a retention per issuance rather than per kref because that window lets issuances for the same target overlap: sharing one would let a failed mint release the retention a URL minted alongside it depends on - 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)) diff --git a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts index 5bba9896a0..79bd75222a 100644 --- a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts @@ -1,3 +1,4 @@ +import { makePromiseKit } from '@endo/promise-kit'; import { describe, it, expect, beforeEach, vi } from 'vitest'; import type { Mock } from 'vitest'; @@ -119,14 +120,17 @@ describe('OcapURLManager', () => { expect(mockKernelStore.kernelRefExists(objectKRef)).toBe(true); }); - it('retains a target named by several URLs only once', async () => { + it('retains a target once per URL naming it', async () => { await ocapURLManager.issueOcapURL(objectKRef); await ocapURLManager.issueOcapURL(objectKRef); - expect(mockKernelStore.getPinnedObjects()).toStrictEqual([objectKRef]); + expect(mockKernelStore.getPinnedObjects()).toStrictEqual([ + objectKRef, + objectKRef, + ]); expect(mockKernelStore.getObjectRefCount(objectKRef)).toStrictEqual({ - reachable: 1, - recognizable: 1, + reachable: 2, + recognizable: 2, }); }); @@ -163,6 +167,29 @@ describe('OcapURLManager', () => { expect(mockKernelStore.getOcapURLObjects()).toStrictEqual([objectKRef]); }); + it('keeps the retention of a URL minted while a failing issuance was in flight', async () => { + // Minting awaits, so a second issuance for the same target can run to + // completion inside the first's window. The first's failure may only + // release its own retention, not the one the second's live URL needs. + const firstMint = makePromiseKit(); + vi.spyOn(mockRemoteComms, 'issueOcapURL') + .mockImplementationOnce(async () => await firstMint.promise) + .mockImplementationOnce(async () => 'ocap:def456@local-peer-id'); + + const failing = ocapURLManager.issueOcapURL(objectKRef); + const url = await ocapURLManager.issueOcapURL(objectKRef); + firstMint.reject(new Error('Issue failed')); + + await expect(failing).rejects.toThrow('Issue failed'); + expect(url).toBe('ocap:def456@local-peer-id'); + expect(mockKernelStore.isObjectPinned(objectKRef)).toBe(true); + expect(mockKernelStore.getOcapURLObjects()).toStrictEqual([objectKRef]); + expect(mockKernelStore.getObjectRefCount(objectKRef)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }); + it('refuses to issue a URL for a kref the kernel has deleted', async () => { mockKernelStore.deleteKernelObject(objectKRef); diff --git a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts index aee9b1710f..8575bbe33d 100644 --- a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts +++ b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts @@ -129,18 +129,17 @@ export class OcapURLManager { // 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); + 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); - } + // existed. This unwinds exactly the one retention above took, so the + // retentions of URLs that were minted — including any issued for this + // same kref while this one was in flight — are left alone. + this.#kernelStore.undoOcapURLRetention(kref); throw error; } } diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index af06ace09c..0344da12d4 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -372,18 +372,41 @@ describe('kernel store', () => { const ks = makeKernelStore(mockKernelDatabase); const kref = ks.initKernelObject('v1'); - expect(ks.retainForOcapURL(kref)).toBe(true); + ks.retainForOcapURL(kref); + expect(ks.isObjectPinned(kref)).toBe(true); expect(ks.getOcapURLObjects()).toStrictEqual([kref]); }); - it('reports that a second URL for the same target took no pin', () => { + it('takes a retention of its own for each URL naming the same target', () => { const ks = makeKernelStore(mockKernelDatabase); const kref = ks.initKernelObject('v1'); + + ks.retainForOcapURL(kref); ks.retainForOcapURL(kref); - expect(ks.retainForOcapURL(kref)).toBe(false); - expect(ks.getPinnedObjects()).toStrictEqual([kref]); + expect(ks.getOcapURLObjects()).toStrictEqual([kref, kref]); + expect(ks.getPinnedObjects()).toStrictEqual([kref, kref]); + expect(ks.getObjectRefCount(kref)).toStrictEqual({ + reachable: 2, + recognizable: 2, + }); + }); + + it('undoes one retention of a target that several URLs name', () => { + const ks = makeKernelStore(mockKernelDatabase); + const kref = ks.initKernelObject('v1'); + ks.retainForOcapURL(kref); + ks.retainForOcapURL(kref); + + ks.undoOcapURLRetention(kref); + + expect(ks.getOcapURLObjects()).toStrictEqual([kref]); + expect(ks.isObjectPinned(kref)).toBe(true); + expect(ks.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); }); it('refuses to retain a kref the kernel has deleted', () => { diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 6ba7c51775..8e082aa8b1 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -380,36 +380,39 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { // 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. + // + // The ledger is a multiset, one entry and one pin per issuance, because + // issuances for the same kref overlap: minting awaits, so a second `issue` + // can run to completion while the first is still in flight. Deduplicating + // by kref would leave the second issuance holding no retention of its own + // and the first free to unwind, on failure, the one the second's live URL + // depends on. getOcapURLObjects(): KRef[] { const raw = kv.get('ocapURLObjects'); return raw ? (raw.split(',') as KRef[]) : []; }, - retainForOcapURL(kref: KRef): boolean { + retainForOcapURL(kref: KRef): void { // 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(',')); + const krefs = this.getOcapURLObjects(); + krefs.push(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)) { + const krefs = this.getOcapURLObjects(); + const index = krefs.indexOf(kref); + if (index === -1) { return; } - if (krefs.size === 0) { + krefs.splice(index, 1); + if (krefs.length === 0) { kv.delete('ocapURLObjects'); } else { - kv.set('ocapURLObjects', [...krefs].sort().join(',')); + kv.set('ocapURLObjects', krefs.join(',')); } this.unpinObject(kref); }, From 2bf8a5cf5ad3e0cffe2fb1f229b4fef409e1f5c0 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 17 Aug 2026 16:05:46 +0200 Subject: [PATCH 07/17] fix(ocap-kernel): refuse to increment a deleted object's refcount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getObjectRefCount` reads a missing row as (0, 0), so incrementing one writes it back and resurrects a live-looking object with no owner — deliverable to by nobody, and endorsed by the audit, since whatever took the reference is a legitimate holder for exactly the count it finds. `retireKernelObjects` deletes the object and queues the `retireImport` in the same breath, so there is always a window where the row is gone while an importer's entry is still live; an increment inside it loses that entry's recognizable unit, and the next `setReachableFlag` pushes reachable past recognizable and throws mid-crank. This PR guarded the two paths it had found — importing into a c-list, issuing an ocap URL — but `pinObject`, `resolve|slot` and everything else still resurrect. `decrementRefCount` has always guarded the same missing row at the primitive; `incrementRefCount` now does too, and fails rather than returning: releasing a reference to something already gone is ordinary teardown, taking one is always a bug. The two call-site guards stay. They refuse before an eref is allocated or a ledger entry written, and name what was attempted. Co-Authored-By: Claude Opus 5 (1M context) --- packages/ocap-kernel/src/store/index.ts | 6 ++-- .../src/store/methods/clist.test.ts | 8 ++++- .../src/store/methods/refcount.test.ts | 29 +++++++++++++++++++ .../ocap-kernel/src/store/methods/refcount.ts | 9 ++++++ .../src/store/methods/translators.ts | 11 ++++--- 5 files changed, 53 insertions(+), 10 deletions(-) diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 8e082aa8b1..c37135530f 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -392,9 +392,9 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { return raw ? (raw.split(',') as KRef[]) : []; }, retainForOcapURL(kref: KRef): void { - // 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. + // Refuse to mint a token for something already collected: the URL would + // name a capability that can never be delivered to. The pin's increment + // refuses it too, but only after the ledger entry has been written. this.kernelRefExists(kref) || Fail`cannot issue an ocap URL for deleted kref ${kref}`; const krefs = this.getOcapURLObjects(); diff --git a/packages/ocap-kernel/src/store/methods/clist.test.ts b/packages/ocap-kernel/src/store/methods/clist.test.ts index 4df8841bf9..ea2fdd0b41 100644 --- a/packages/ocap-kernel/src/store/methods/clist.test.ts +++ b/packages/ocap-kernel/src/store/methods/clist.test.ts @@ -19,6 +19,12 @@ describe('clist-methods', () => { kv.set('e.nextPromiseId.r1', '1'); kv.set('e.nextObjectId.r1', '1'); + // The objects these tests map, as `initKernelObject` leaves them: an entry + // may not be added for a kref the kernel has no record of, since taking a + // reference to one would resurrect it. + kv.set('ko1.refCount', '0,0'); + kv.set('ko2.refCount', '0,0'); + // Create the store with mocked dependencies clistMethods = getCListMethods({ kv, @@ -66,7 +72,7 @@ describe('clist-methods', () => { it('takes no reference for an object export', () => { clistMethods.addCListEntry('v1', 'ko1', 'o+1'); - expect(kv.get('ko1.refCount')).toBeUndefined(); + expect(kv.get('ko1.refCount')).toBe('0,0'); }); it.each(['p-1', 'p+1'] as ERef[])( diff --git a/packages/ocap-kernel/src/store/methods/refcount.test.ts b/packages/ocap-kernel/src/store/methods/refcount.test.ts index 24fe904dfd..5b79032fa9 100644 --- a/packages/ocap-kernel/src/store/methods/refcount.test.ts +++ b/packages/ocap-kernel/src/store/methods/refcount.test.ts @@ -204,6 +204,35 @@ describe('refcount-methods', () => { refCountMethods.incrementRefCount('' as KRef, 'test', {}), ).toThrow('incrementRefCount called with empty kref'); }); + + it('refuses to resurrect an object the kernel has deleted', () => { + const kref: KRef = 'ko99'; + + expect(() => refCountMethods.incrementRefCount(kref, 'test')).toThrow( + 'incrementRefCount on deleted kref "ko99" ("test")', + ); + expect(kv.get(baseStore.refCountKey(kref))).toBeUndefined(); + }); + + it('increments an object that exists', () => { + const kref: KRef = 'ko1'; + kv.set(baseStore.refCountKey(kref), '0,0'); + + refCountMethods.incrementRefCount(kref, 'test'); + + expect(kv.get(baseStore.refCountKey(kref))).toBe('1,1'); + }); + + it('increments only the recognizable count when asked', () => { + const kref: KRef = 'ko1'; + kv.set(baseStore.refCountKey(kref), '1,1'); + + refCountMethods.incrementRefCount(kref, 'test', { + onlyRecognizable: true, + }); + + expect(kv.get(baseStore.refCountKey(kref))).toBe('1,2'); + }); }); describe('decrementRefCount', () => { diff --git a/packages/ocap-kernel/src/store/methods/refcount.ts b/packages/ocap-kernel/src/store/methods/refcount.ts index 6cea54268b..6e095b95c4 100644 --- a/packages/ocap-kernel/src/store/methods/refcount.ts +++ b/packages/ocap-kernel/src/store/methods/refcount.ts @@ -81,6 +81,7 @@ export function getRefCountMethods(ctx: StoreContext) { * @param options - Options for the increment. * @param options.isExport - True if the reference comes from a clist export, which counts for promises but not objects. * @param options.onlyRecognizable - True if the reference provides only recognition, not reachability. + * @throws if `kref` names an object the kernel has already deleted. */ function incrementRefCount( kref: KRef, @@ -105,6 +106,14 @@ export function getRefCountMethods(ctx: StoreContext) { return; } + // A missing row reads as `(0, 0)`, so incrementing one writes it back and + // resurrects a live-looking object that nobody owns and nobody can be + // delivered to. `decrementRefCount` tolerates the same missing row because + // releasing a reference to something already gone is ordinary teardown; + // taking one is always a bug, so this refuses rather than returns. + kernelRefExists(kref) || + Fail`incrementRefCount on deleted kref ${kref} (${tag})`; + const counts = getObjectRefCount(kref); if (!onlyRecognizable) { counts.reachable += 1; diff --git a/packages/ocap-kernel/src/store/methods/translators.ts b/packages/ocap-kernel/src/store/methods/translators.ts index 3ba551d28f..e849b3b836 100644 --- a/packages/ocap-kernel/src/store/methods/translators.ts +++ b/packages/ocap-kernel/src/store/methods/translators.ts @@ -79,12 +79,11 @@ export function getTranslators(ctx: StoreContext) { 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. + // entry: the entry would be a legitimate holder for the resurrected + // count, so even the audit would endorse it. `incrementRefCount` + // refuses that increment too; checking here refuses before an eref is + // allocated, and says what was attempted. 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); From 6fb6332b49d42ff07db23c1519e6e173e31caeb7 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 17 Aug 2026 16:06:01 +0200 Subject: [PATCH 08/17] fix(ocap-kernel): report a corrupt refcount row instead of throwing on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit read each object's stored counts back through `getObjectRefCount`, which `Fail`s when reachable exceeds recognizable — one of the two drifts this module exists to diagnose. Hitting it meant the operator got `refMismatch(get) ko7 3,1` with no holder list, no expected value, and none of the other violations from the same sweep. Objects store the same "reachable,recognizable" encoding the audit renders, so the raw row compares directly. A malformed row is now reported as it stands rather than taking the sweep down with it. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/store/methods/refcount-audit.test.ts | 28 ++++++++++++++++++- .../src/store/methods/refcount-audit.ts | 11 ++++---- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts index 70af7d4075..5d637d79d0 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts @@ -1,3 +1,4 @@ +import type { KernelDatabase } from '@metamask/kernel-store'; import { describe, it, expect, beforeEach } from 'vitest'; import { makeMapKernelDatabase } from '../../../test/storage.ts'; @@ -5,6 +6,7 @@ import type { KRef, VatConfig, VatId } from '../../types.ts'; import { makeKernelStore } from '../index.ts'; describe('reference count audit', () => { + let kernelDatabase: KernelDatabase; let kernelStore: ReturnType; /** @@ -20,7 +22,8 @@ describe('reference count audit', () => { } beforeEach(() => { - kernelStore = makeKernelStore(makeMapKernelDatabase()); + kernelDatabase = makeMapKernelDatabase(); + kernelStore = makeKernelStore(kernelDatabase); kernelStore.markInitialized(); givenVats('v1', 'v2', 'v3'); }); @@ -152,6 +155,29 @@ describe('reference count audit', () => { ]); }); + it.each([ + { what: 'more reachable than recognizable', row: '3,1' }, + { what: 'not a pair of numbers', row: 'NaN,0' }, + { what: 'a promise count on an object', row: '1' }, + ])('reports a row that is $what', ({ row }) => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + // Written past the store's own guards, as a store drifted by an earlier + // kernel would arrive: `getObjectRefCount` throws on all three, so + // reading the row through it would take the whole sweep down with the + // one violation it exists to report. + kernelDatabase.kernelKVStore.set(`${kref}.refCount`, row); + + expect(kernelStore.auditRefCounts()).toStrictEqual([ + { + kref, + stored: row, + 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'); diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.ts index b4178e5c6f..5ebbbb6e1a 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -1,7 +1,6 @@ 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'; @@ -71,7 +70,6 @@ const REFCOUNT_KEY = /^(k[op]\d+)\.refCount$/u; // 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); /** @@ -275,9 +273,12 @@ export function getRefCountAuditMethods(ctx: StoreContext) { } continue; } - const storedText = isPromiseRef(kref) - ? raw - : renderCounts(kref, getObjectRefCount(kref)); + // The raw row, not `getObjectRefCount`: that `Fail`s on `reachable > + // recognizable`, which is one of the drifts this exists to report, and + // would take the whole sweep down with it. Objects store the same + // `"reachable,recognizable"` encoding `renderCounts` produces, so a + // malformed row is reported as it stands. + const storedText = raw; if (storedText !== expectedText) { violations.push({ kref, From b619532835a59e703b10c1239e36ec1edf7865d3 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 17 Aug 2026 16:06:07 +0200 Subject: [PATCH 09/17] fix(ocap-kernel): charge a resolution's slots only once the resolve is legal `resolvePromises` incremented every slot before checking the promise's state and decider, so a vat's illegal `syscall.resolve` threw out of those checks having already charged a unit per slot with nobody holding it. This PR removed the `resolve|kpid` increment from the same spot but left the slots, and the audit it adds is what makes the leftover fatal rather than merely leaky: the next crank reports a kref stored at (1, 1) with no holder and kills the kernel. Co-Authored-By: Claude Opus 5 (1M context) --- packages/ocap-kernel/src/KernelQueue.test.ts | 8 ++++++-- packages/ocap-kernel/src/KernelQueue.ts | 11 +++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index 1b3bd4a35a..4ff8a8e78a 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -771,7 +771,7 @@ describe('KernelQueue', () => { const resolution: VatOneResolution = [ kpid, false, - { body: 'resolved value', slots: [] } as CapData, + { body: 'resolved value', slots: ['ko1'] } as CapData, ]; (kernelStore.getKernelPromise as unknown as MockInstance).mockReturnValue( { @@ -782,6 +782,9 @@ describe('KernelQueue', () => { expect(() => kernelQueue.resolvePromises(endpointId, [resolution]), ).toThrow('"kp123" was already resolved'); + // A refused resolve charges nothing, so it leaves nothing behind for the + // refcount audit to find with no holder. + expect(kernelStore.incrementRefCount).not.toHaveBeenCalled(); }); it('throws error if the resolver is not the decider', () => { @@ -791,7 +794,7 @@ describe('KernelQueue', () => { const resolution: VatOneResolution = [ kpid, false, - { body: 'resolved value', slots: [] } as CapData, + { body: 'resolved value', slots: ['ko1'] } as CapData, ]; (kernelStore.getKernelPromise as unknown as MockInstance).mockReturnValue( { @@ -804,6 +807,7 @@ describe('KernelQueue', () => { ).toThrow( '"v1" not permitted to resolve "kp123" because "its decider is v2"', ); + expect(kernelStore.incrementRefCount).not.toHaveBeenCalled(); }); }); diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index afda8139c7..b4280ec029 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -505,10 +505,6 @@ export class KernelQueue { for (const resolution of resolutions) { const [kpid, rejected, data] = resolution; - for (const slot of data.slots || []) { - this.#kernelStore.incrementRefCount(slot, 'resolve|slot'); - } - const promise = this.#kernelStore.getKernelPromise(kpid); const { state, decider, subscribers } = promise; if (state !== 'unresolved') { @@ -522,6 +518,13 @@ export class KernelQueue { throw Fail`${kpid} subscribers not set`; } + // Charged only once the resolve is known to be legal: a vat's illegal + // `syscall.resolve` throws out of the checks above, and a unit taken + // before them would be left behind with nobody holding it. + for (const slot of data.slots || []) { + this.#kernelStore.incrementRefCount(slot, 'resolve|slot'); + } + // Enqueue notifications for each subscriber (immediate or buffered based on flag). for (const subscriber of subscribers) { this.enqueueNotify(subscriber, kpid, immediate); From 42cc2a2bec3c561a793262ae512dd8db959baeb9 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 17 Aug 2026 16:06:26 +0200 Subject: [PATCH 10/17] test(ocap-kernel): pin the send and notify accounting fixes Both were untested on exactly the paths they exist for. Every assertion on `deliver|send|target` used an object target, where the run queue item's target and the routed target are the same kref, so reverting that fix left the suite green; there was no delivery test at all where a message reaches an object through a promise that fulfilled to it. The notify fix is the same story: the two early returns it moved the release in front of asserted only the return value, and the sibling-promise decrement it deletes was never exercised, since the one batch test mocks `getKpidsToRetire` to return the notified promise itself. Co-Authored-By: Claude Opus 5 (1M context) --- packages/ocap-kernel/src/KernelRouter.test.ts | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/packages/ocap-kernel/src/KernelRouter.test.ts b/packages/ocap-kernel/src/KernelRouter.test.ts index 11aa8922c1..eddf28a6a4 100644 --- a/packages/ocap-kernel/src/KernelRouter.test.ts +++ b/packages/ocap-kernel/src/KernelRouter.test.ts @@ -3,6 +3,7 @@ import type { MockInstance } from 'vitest'; import { KernelQueue } from './KernelQueue.ts'; import { KernelRouter } from './KernelRouter.ts'; +import { kser, kslot } from './liveslots/kernel-marshal.ts'; import type { KernelStore } from './store/index.ts'; import type { KernelMessage, @@ -317,6 +318,37 @@ describe('KernelRouter', () => { ]); }); + it('charges the queued target, not the object a fulfilled promise routes to', async () => { + // The one delivery where the two differ: the message was queued against + // the promise, so the promise is what `enqueueSend` charged, while the + // object it fulfilled to is held by the resolution instead. + const target = 'kp123'; + const routedTarget = 'ko99'; + ( + kernelStore.getKernelPromise as unknown as MockInstance + ).mockReturnValueOnce({ + state: 'fulfilled', + value: kser(kslot(routedTarget)), + }); + (kernelStore.getOwner as unknown as MockInstance).mockReturnValueOnce( + 'v1', + ); + const message: KernelMessage = { + methargs: { body: 'method args', slots: [] }, + result: null, + }; + + await kernelRouter.deliver({ type: 'send', target, message }); + + expect(endpointHandle.deliverMessage).toHaveBeenCalledWith( + `translated-${routedTarget}`, + message, + ); + expect( + (kernelStore.decrementRefCount as unknown as MockInstance).mock.calls, + ).toStrictEqual([[target, 'deliver|send|target']]); + }); + it('splats message when promise resolves to a non-object', async () => { // Setup a fulfilled promise that doesn't resolve to an object const promiseId = 'kp123'; @@ -580,6 +612,11 @@ describe('KernelRouter', () => { // Verify no notification was delivered to the vat expect(endpointHandle.deliverNotify).not.toHaveBeenCalled(); expect(result).toStrictEqual({ didDelivery: endpointId }); + // The notification's own reference is released on the way out, not + // stranded by the early return. + expect( + (kernelStore.decrementRefCount as unknown as MockInstance).mock.calls, + ).toStrictEqual([[kpid, 'deliver|notify']]); }); it('returns didDelivery when no kpids to retire', async () => { @@ -618,6 +655,36 @@ describe('KernelRouter', () => { // Verify no notification was delivered to the vat expect(endpointHandle.deliverNotify).not.toHaveBeenCalled(); expect(result).toStrictEqual({ didDelivery: endpointId }); + expect( + (kernelStore.decrementRefCount as unknown as MockInstance).mock.calls, + ).toStrictEqual([[kpid, 'deliver|notify']]); + }); + + it('releases only the notified promise when others are retired with it', async () => { + const endpointId = 'v1'; + const kpid = 'kp123'; + const alsoRetired = 'kp456'; + const resolved = { + state: 'fulfilled', + value: { body: JSON.stringify({ value: 'resolved' }), slots: [] }, + }; + (kernelStore.getKernelPromise as unknown as MockInstance) + .mockReturnValueOnce(resolved) + .mockReturnValue(resolved); + (kernelStore.krefToEref as unknown as MockInstance).mockReturnValueOnce( + 'p+123', + ); + ( + kernelStore.getKpidsToRetire as unknown as MockInstance + ).mockReturnValueOnce([kpid, alsoRetired]); + + await kernelRouter.deliver({ type: 'notify', endpointId, kpid }); + + // Only `enqueueNotify` charges a notification, and only for its own + // kpid, so the promises settled alongside it are nobody's to release. + expect( + (kernelStore.decrementRefCount as unknown as MockInstance).mock.calls, + ).toStrictEqual([[kpid, 'deliver|notify']]); }); it('throws if notification is for an unresolved promise', async () => { From 3b516aba3bd7c2a5b4726c3e86e58f7e33c7884e Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 17 Aug 2026 16:06:31 +0200 Subject: [PATCH 11/17] test(kernel-test): make a refcount violation fail the run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turning the audit on for every kernel `kernel-test` builds did not make a violation fail the build. The audit reports by throwing, which kills the run loop, and the kernel deliberately hands run loop death to `onRunLoopFailure` rather than rethrowing it — so with no handler a violation surfaced only if that crank happened to have a caller waiting on it. On a garbage collection or reap crank, or one landing after a test's last assertion, it was logged into a mock nobody asserts on and forgotten. `makeAuditedKernelOptions` records the failure and hooks report it, so it fails the test with the message that names the drifted kref rather than an unhandled error that takes the worker down with a useless one. Two kernels built directly rather than through `makeKernel` were not audited at all; they are now. Verified by injecting a double increment into `pinObject`: two `kernel-test` tests fail with the violation, where before this they passed. Co-Authored-By: Claude Opus 5 (1M context) --- .../kernel-test/src/endowment-globals.test.ts | 7 ++- packages/kernel-test/src/io.test.ts | 7 ++- packages/kernel-test/src/utils.ts | 61 +++++++++++++++++-- 3 files changed, 68 insertions(+), 7 deletions(-) diff --git a/packages/kernel-test/src/endowment-globals.test.ts b/packages/kernel-test/src/endowment-globals.test.ts index 4eafeb90fb..18498867bb 100644 --- a/packages/kernel-test/src/endowment-globals.test.ts +++ b/packages/kernel-test/src/endowment-globals.test.ts @@ -12,7 +12,11 @@ import type { AllowedGlobalName, KRef, VatId } from '@metamask/ocap-kernel'; import { getWorkerFile } from '@ocap/nodejs-test-workers'; import { describe, expect, it } from 'vitest'; -import { extractTestLogs, getBundleSpec } from './utils.ts'; +import { + extractTestLogs, + getBundleSpec, + makeAuditedKernelOptions, +} from './utils.ts'; describe('global endowments', () => { const vatId: VatId = 'v1'; @@ -38,6 +42,7 @@ describe('global endowments', () => { resetStorage: true, logger, allowedGlobalNames, + ...makeAuditedKernelOptions(), }); await kernel.launchSubcluster({ diff --git a/packages/kernel-test/src/io.test.ts b/packages/kernel-test/src/io.test.ts index df94edb03a..47840b59aa 100644 --- a/packages/kernel-test/src/io.test.ts +++ b/packages/kernel-test/src/io.test.ts @@ -6,7 +6,11 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { describe, it, expect, afterEach } from 'vitest'; -import { getBundleSpec, makeTestLogger } from './utils.ts'; +import { + getBundleSpec, + makeAuditedKernelOptions, + makeTestLogger, +} from './utils.ts'; function tempSocketPath(): string { return path.join( @@ -79,6 +83,7 @@ async function makeIoKernel( resetStorage: true, logger, ioListenerFactory: makeIOListenerFactory(), + ...makeAuditedKernelOptions(), }, ); diff --git a/packages/kernel-test/src/utils.ts b/packages/kernel-test/src/utils.ts index 7d867f2c28..b840fa2e30 100644 --- a/packages/kernel-test/src/utils.ts +++ b/packages/kernel-test/src/utils.ts @@ -11,8 +11,61 @@ import { } from '@metamask/logger'; import type { LogEntry } from '@metamask/logger'; import { Kernel, kunser } from '@metamask/ocap-kernel'; -import type { ClusterConfig, PlatformServices } from '@metamask/ocap-kernel'; -import { vi } from 'vitest'; +import type { + ClusterConfig, + OnRunLoopFailure, + PlatformServices, +} from '@metamask/ocap-kernel'; +import { afterAll, afterEach, vi } from 'vitest'; + +/** + * The first run loop death seen since it was last reported, held here rather + * than passed to a test because the crank that kills the loop is often one no + * test is awaiting — a garbage collection or reap crank, or one that lands + * after the last assertion. The hooks below are the only thing guaranteed to + * look, so they are registered for every file that imports this module. + */ +let runLoopFailure: Error | undefined; + +/** + * Fail the current test if a kernel's run loop has died since the last check. + */ +function assertRunLoopAlive(): void { + const failure = runLoopFailure; + runLoopFailure = undefined; + if (failure) { + throw failure; + } +} + +afterEach(assertRunLoopAlive); +afterAll(assertRunLoopAlive); + +/** + * Kernel options under which reference count drift fails the test run. + * + * Drift is invisible to ordinary assertions until something gets collected out + * from under a live holder, so the audit runs every crank. It reports by + * throwing, which kills the run loop — and the kernel hands run loop death to + * `onRunLoopFailure` rather than rethrowing it, deliberately, so that an + * embedder can decide what to do. Without a handler a violation therefore + * surfaces only if the killed crank happened to have a caller waiting on it. + * + * @returns Options to pass to `Kernel.make`. + */ +export function makeAuditedKernelOptions(): { + auditRefCounts: true; + onRunLoopFailure: OnRunLoopFailure; +} { + return { + auditRefCounts: true, + // The first failure is the informative one: a dead loop cannot process + // anything, so whatever follows is downstream of it. + onRunLoopFailure: (failure: Error): void => { + runLoopFailure ??= failure; + }, + }; +} /** * Construct a bundle path URL from a bundle name. @@ -93,9 +146,7 @@ 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, + ...makeAuditedKernelOptions(), }); return kernel; } From 577c6274689f222ab71656c84aafffd1cb6e1ab9 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 17 Aug 2026 16:06:46 +0200 Subject: [PATCH 12/17] docs(ocap-kernel): state the migration decision and place the breaking rename A store written before this change has every object at (1, 1) and no root pins, and `kernel-store` has no schema version to notice: the second importer's `dropImports` underflows mid-crank, and a legacy store's roots have no pin for the last importer's drop to lose to. There is no migration and none is planned at this version, so say so where an upgrading consumer will read it. The `krefsToExistingErefs` rename moves to `### Changed`, where this file puts its other breaking API changes and where a consumer scanning for breakage looks. The audit entry claimed to catch leaks; it compares counts against the holders it finds, so a holder that should have been torn down but wasn't justifies its own count and is invisible to it. `undoOcapURLRetention` is as public as the two methods listed beside it, and `RefCountViolation` is now exported from the package root, as the entry said it was. Also reverts blank lines this branch's formatting commit inserted into an unrelated entry. Co-Authored-By: Claude Opus 5 (1M context) --- packages/ocap-kernel/CHANGELOG.md | 13 +++++-------- packages/ocap-kernel/src/index.ts | 2 +- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 1cab9b17ef..59c8877ee2 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -32,18 +32,16 @@ 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) + - Reports drift in both directions: counts too low, which lets a live capability be collected, and counts too high, which keeps a dead one alive. A holder that should have been torn down but wasn't is not detectable this way, since it justifies its own count - Only references visible in the kernel's own state are checkable, so a holder that keeps a kref outside them has to take a pin to be counted at all - - `recomputeRefCounts` is a repair tool for a drifted store, offered to embedders and never run automatically: opening an existing store does not migrate it + - `recomputeRefCounts` is a repair tool for a drifted store, offered to embedders and never run automatically: opening an existing store does not migrate it. Reach it by calling `makeKernelStore` over the kernel's own database - Exports the `RefCountViolation` type - Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) -- Add `getOcapURLObjects` and `retainForOcapURL` to the kernel store, and `VatManager.releaseVatRootPin` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- Add `getOcapURLObjects`, `retainForOcapURL` and `undoOcapURLRetention` to the kernel store, and `VatManager.releaseVatRootPin` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) ### Changed @@ -54,6 +52,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **BREAKING:** Type `VatConfig.globals` and `Kernel.make`'s `allowedGlobalNames` as `AllowedGlobalName[]` (a literal union) instead of `string[]`; unknown names are now rejected at the `initVat` RPC boundary ([#941](https://github.com/MetaMask/ocap-kernel/pull/941)) - Exports: `AllowedGlobalName`, `AllowedGlobalNameStruct`, `MakeAllowedGlobals`, `VatEndowmentsStruct` - Bound relay hints in OCAP URLs to a maximum of 3 and cap the relay pool at 20 entries with eviction of oldest non-bootstrap relays ([#929](https://github.com/MetaMask/ocap-kernel/pull/929)) +- **BREAKING:** Rename `krefsToExistingErefs` to `krefsToErefs`, which now throws on an unmapped kref instead of silently dropping it ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) ### Fixed @@ -78,7 +77,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `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 + - **A store written by an earlier version must be reset.** There is no migration: every object in it is still at `(1, 1)` and no vat root is pinned, so the second importer's `dropImports` underflows mid-crank and the last importer's drop can retire a live vat's root. `recomputeRefCounts` can rebuild the counts, but not the root pins, so it is a diagnostic rather than an upgrade path - 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)) @@ -96,9 +95,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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/index.ts b/packages/ocap-kernel/src/index.ts index 6ea9594f3f..230a111332 100644 --- a/packages/ocap-kernel/src/index.ts +++ b/packages/ocap-kernel/src/index.ts @@ -64,7 +64,7 @@ export type { KernelFacet } from './kernel-facet.ts'; export { makeKernelFacet } from './kernel-facet.ts'; export type { PingVatResult } from './rpc/index.ts'; export { makeKernelStore } from './store/index.ts'; -export type { KernelStore } from './store/index.ts'; +export type { KernelStore, RefCountViolation } from './store/index.ts'; export { parseRef } from './store/utils/parse-ref.ts'; export { generateMnemonic, From 266eaad6455049abf9986d57de807b2e2640cf22 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 18 Aug 2026 15:58:06 +0200 Subject: [PATCH 13/17] fix(ocap-kernel): count retentions per object rather than listing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An ocap URL retains its target for as long as any URL names it, and which objects get URLs is the holder's choice, not the kernel's — so neither the retention ledger nor the pin list it writes into is bounded by anything the kernel controls. Both kept every entry in a single row, so each issuance read, rewrote and re-sorted the whole thing, and the row grew without limit. Both are now a count per object in a row of its own, which is one write per issuance whatever else is retained. Counting keeps the per-issuance semantics the previous shape needed a multiset for: overlapping issuances for one target share the single pin, and a failed mint spends its own issuance without touching the retention a URL minted alongside it depends on. Ending a retention was one method doing two jobs it cannot both do. `undoOcapURLRetention` unwinds a single issuance, which is right for a mint that failed and wrong for disavowing an object, where every URL naming it goes at once — that case is `releaseOcapURLRetentions`. Revocation is still neither: it writes only its flag, so a revoked object's URLs stay retained. `getPinnedObjects` now names each object once however many pins it holds, and `getPinCount` reports that number, which is what the audit needs to credit a unit per pin. Co-Authored-By: Claude Opus 5 (1M context) --- packages/ocap-kernel/CHANGELOG.md | 12 +- .../src/remotes/kernel/OcapURLManager.test.ts | 12 +- packages/ocap-kernel/src/store/index.test.ts | 57 +++++++-- packages/ocap-kernel/src/store/index.ts | 79 ++++++++---- .../src/store/methods/pinned.test.ts | 115 ++++++++++++------ .../ocap-kernel/src/store/methods/pinned.ts | 65 +++++++--- .../src/store/methods/refcount-audit.ts | 8 +- .../ocap-kernel/src/vats/SubclusterManager.ts | 4 +- 8 files changed, 249 insertions(+), 103 deletions(-) diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 59c8877ee2..2fe4e74d39 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -41,7 +41,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `recomputeRefCounts` is a repair tool for a drifted store, offered to embedders and never run automatically: opening an existing store does not migrate it. Reach it by calling `makeKernelStore` over the kernel's own database - Exports the `RefCountViolation` type - Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) -- Add `getOcapURLObjects`, `retainForOcapURL` and `undoOcapURLRetention` to the kernel store, and `VatManager.releaseVatRootPin` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- Add `getOcapURLObjects`, `getOcapURLIssuanceCount`, `retainForOcapURL`, `undoOcapURLRetention` and `releaseOcapURLRetentions` to the kernel store, and `VatManager.releaseVatRootPin` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - The two ways a retention ends are separate operations: `undoOcapURLRetention` unwinds one issuance whose URL was never minted, and `releaseOcapURLRetentions` drops a target's whole retention, for disavowing every URL naming it at once +- Add `getPinCount` to the kernel store, which reports how many pins are held on an object ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) ### Changed @@ -53,6 +55,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Exports: `AllowedGlobalName`, `AllowedGlobalNameStruct`, `MakeAllowedGlobals`, `VatEndowmentsStruct` - Bound relay hints in OCAP URLs to a maximum of 3 and cap the relay pool at 20 entries with eviction of oldest non-bootstrap relays ([#929](https://github.com/MetaMask/ocap-kernel/pull/929)) - **BREAKING:** Rename `krefsToExistingErefs` to `krefsToErefs`, which now throws on an unmapped kref instead of silently dropping it ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- **BREAKING:** `getPinnedObjects` now names each pinned object once, however many pins it holds; `getPinCount` gives the number of pins ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - Pins and ocap URL retentions are each stored as a count per object (`pinned.${koid}`, `ocapURLObjects.${koid}`) rather than in one row listing every pin, so taking or spending one is a single write regardless of how many others there are. What an ocap URL retains is chosen by whoever holds a URL, so neither list is bounded by anything the kernel controls. Covered by the reset above: the old `pinnedObjects` row is not read ### Fixed @@ -89,13 +93,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `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 URL, and no release: the token is persistent and unexpiring, so `revoke` is the way to kill the capability + - One pin for as long as any URL names the target, and no release: the token is persistent and unexpiring, so revocation is what kills the capability — though it only stops deliveries, and leaves the target retained - 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, and released again if minting fails. It is a retention per issuance rather than per kref because that window lets issuances for the same target overlap: sharing one would let a failed mint release the retention a URL minted alongside it depends on + - The retention is taken before the token is minted, since minting awaits and a collection crank can run in that window, and unwound again if minting fails. The store counts issuances per target rather than sharing one retention between them, because that window lets issuances for the same target overlap: sharing one would let a failed mint release the retention a URL minted alongside it depends on - 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 + - `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 a pin outstanding on 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/remotes/kernel/OcapURLManager.test.ts b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts index 79bd75222a..a35f137e7c 100644 --- a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts @@ -120,17 +120,15 @@ describe('OcapURLManager', () => { expect(mockKernelStore.kernelRefExists(objectKRef)).toBe(true); }); - it('retains a target once per URL naming it', async () => { + it('counts a retention per URL naming the target, on the one pin', async () => { await ocapURLManager.issueOcapURL(objectKRef); await ocapURLManager.issueOcapURL(objectKRef); - expect(mockKernelStore.getPinnedObjects()).toStrictEqual([ - objectKRef, - objectKRef, - ]); + expect(mockKernelStore.getOcapURLIssuanceCount(objectKRef)).toBe(2); + expect(mockKernelStore.getPinnedObjects()).toStrictEqual([objectKRef]); expect(mockKernelStore.getObjectRefCount(objectKRef)).toStrictEqual({ - reachable: 2, - recognizable: 2, + reachable: 1, + recognizable: 1, }); }); diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 0344da12d4..ab7d8d040d 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -103,10 +103,12 @@ describe('kernel store', () => { 'getNextRemoteId', 'getNextVatId', 'getObjectRefCount', + 'getOcapURLIssuanceCount', 'getOcapURLObjects', 'getOwner', 'getPeerIncarnation', 'getPendingMessage', + 'getPinCount', 'getPinnedObjects', 'getPromisesByDecider', 'getQueueLength', @@ -155,6 +157,7 @@ describe('kernel store', () => { 'recomputeRefCounts', 'recordLastActiveTime', 'releaseAllSavepoints', + 'releaseOcapURLRetentions', 'releaseSavepoint', 'removeAnonymousKernelObject', 'removeVatFromSubcluster', @@ -378,18 +381,19 @@ describe('kernel store', () => { expect(ks.getOcapURLObjects()).toStrictEqual([kref]); }); - it('takes a retention of its own for each URL naming the same target', () => { + it('counts an issuance for each URL naming the same target, and pins it once', () => { const ks = makeKernelStore(mockKernelDatabase); const kref = ks.initKernelObject('v1'); ks.retainForOcapURL(kref); ks.retainForOcapURL(kref); - expect(ks.getOcapURLObjects()).toStrictEqual([kref, kref]); - expect(ks.getPinnedObjects()).toStrictEqual([kref, kref]); + expect(ks.getOcapURLObjects()).toStrictEqual([kref]); + expect(ks.getOcapURLIssuanceCount(kref)).toBe(2); + expect(ks.getPinCount(kref)).toBe(1); expect(ks.getObjectRefCount(kref)).toStrictEqual({ - reachable: 2, - recognizable: 2, + reachable: 1, + recognizable: 1, }); }); @@ -401,7 +405,7 @@ describe('kernel store', () => { ks.undoOcapURLRetention(kref); - expect(ks.getOcapURLObjects()).toStrictEqual([kref]); + expect(ks.getOcapURLIssuanceCount(kref)).toBe(1); expect(ks.isObjectPinned(kref)).toBe(true); expect(ks.getObjectRefCount(kref)).toStrictEqual({ reachable: 1, @@ -415,6 +419,7 @@ describe('kernel store', () => { expect(() => ks.retainForOcapURL('ko99')).toThrow( 'cannot issue an ocap URL for deleted kref "ko99"', ); + expect(ks.getOcapURLObjects()).toStrictEqual([]); }); it('undoing the last retention clears the record entirely', () => { @@ -431,7 +436,7 @@ describe('kernel store', () => { ks.undoOcapURLRetention(otherKref); expect(ks.getOcapURLObjects()).toStrictEqual([]); expect( - mockKernelDatabase.kernelKVStore.get('ocapURLObjects'), + mockKernelDatabase.kernelKVStore.get(`ocapURLObjects.${otherKref}`), ).toBeUndefined(); }); @@ -444,6 +449,44 @@ describe('kernel store', () => { expect(ks.isObjectPinned(kref)).toBe(true); }); + + it('releases every retention of a target at once', () => { + const ks = makeKernelStore(mockKernelDatabase); + const kref = ks.initKernelObject('v1'); + ks.retainForOcapURL(kref); + ks.retainForOcapURL(kref); + ks.retainForOcapURL(kref); + + ks.releaseOcapURLRetentions(kref); + + expect(ks.getOcapURLObjects()).toStrictEqual([]); + expect(ks.isObjectPinned(kref)).toBe(false); + expect(ks.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + }); + + it('releasing retentions of a target that has none leaves other pins alone', () => { + const ks = makeKernelStore(mockKernelDatabase); + const kref = ks.initKernelObject('v1'); + ks.pinObject(kref); + + ks.releaseOcapURLRetentions(kref); + + expect(ks.isObjectPinned(kref)).toBe(true); + }); + + it('does not mistake the ocap URL cipher key for a retained object', () => { + const ks = makeKernelStore(mockKernelDatabase); + const kref = ks.initKernelObject('v1'); + // `ocapURLKey` is a neighbour of the retention keys in key order, and + // holds the key the tokens are encrypted with rather than a kref. + mockKernelDatabase.kernelKVStore.set('ocapURLKey', 'some-cipher-key'); + ks.retainForOcapURL(kref); + + expect(ks.getOcapURLObjects()).toStrictEqual([kref]); + }); }); describe('reset', () => { diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index c37135530f..32b8dd585c 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -61,7 +61,8 @@ * nextRemoteId = NN // allocation counter for remote IDs * k.nextObjectId = NN // allocation counter for object KRefs * k.nextPromiseId = NN // allocation counter for promise KRefs - * pinnedObjects = ${kref}[,${kref}]* // pinned object list + * pinned.${koid} = NN // number of pins held on ${koid} + * ocapURLObjects.${koid} = NN // number of ocap URLs naming ${koid} * kernelService.${serviceName} = ${koid} // kref of kernel service object ${serviceName} */ @@ -91,6 +92,14 @@ import { getTranslators } from './methods/translators.ts'; import { getVatMethods } from './methods/vat.ts'; import type { StoreContext } from './types.ts'; +/** + * The prefix shared by the issuance count of every object an ocap URL names, + * for iterating over them. A count per object, rather than one row listing + * every issuance, because the number of URLs a kernel hands out is bounded by + * nothing the kernel controls. + */ +const OCAP_URL_PREFIX = 'ocapURLObjects.'; + /** * Create a new KernelStore object wrapped around a raw kernel database. The * resulting object provides a variety of operations for accessing various @@ -113,7 +122,8 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { /** KV store in which all the kernel's own state is kept. */ const kv: KVStore = kdb.kernelKVStore; - const { provideCachedStoredValue, provideStoredQueue } = getBaseMethods(kv); + const { getPrefixedKeys, provideCachedStoredValue, provideStoredQueue } = + getBaseMethods(kv); const context: StoreContext = { kv, @@ -377,44 +387,59 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { // 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. + // not running when it was issued. // - // The ledger is a multiset, one entry and one pin per issuance, because - // issuances for the same kref overlap: minting awaits, so a second `issue` - // can run to completion while the first is still in flight. Deduplicating - // by kref would leave the second issuance holding no retention of its own - // and the first free to unwind, on failure, the one the second's live URL - // depends on. + // The ledger counts, per target, how many URLs name it, and holds one pin + // for as long as that count is nonzero. Counting is what makes overlapping + // issuances safe: minting awaits, so a second `issue` for the same target + // can run to completion while the first is still in flight, and the first's + // failure must then unwind its own issuance without disturbing the + // retention the second's live URL depends on. + // + // Ending a retention comes in two kinds, so it comes in two methods. + // `undoOcapURLRetention` unwinds one issuance whose URL was never minted + // and leaves the others standing; `releaseOcapURLRetentions` drops the + // target's whole retention at once, for disavowing every URL naming it. + // Revocation is neither: it writes only the revoked flag, so a revoked + // object's URLs stay retained, redeemable, and undeliverable. getOcapURLObjects(): KRef[] { - const raw = kv.get('ocapURLObjects'); - return raw ? (raw.split(',') as KRef[]) : []; + return [...getPrefixedKeys(OCAP_URL_PREFIX)].map( + (key) => key.slice(OCAP_URL_PREFIX.length) as KRef, + ); + }, + getOcapURLIssuanceCount(kref: KRef): number { + return Number(kv.get(`${OCAP_URL_PREFIX}${kref}`) ?? 0); }, retainForOcapURL(kref: KRef): void { // Refuse to mint a token for something already collected: the URL would - // name a capability that can never be delivered to. The pin's increment - // refuses it too, but only after the ledger entry has been written. + // name a capability that can never be delivered to. Refusing here names + // what was attempted, and does it before anything has been written. this.kernelRefExists(kref) || Fail`cannot issue an ocap URL for deleted kref ${kref}`; - const krefs = this.getOcapURLObjects(); - krefs.push(kref); - kv.set('ocapURLObjects', krefs.sort().join(',')); - this.pinObject(kref); + const issuances = this.getOcapURLIssuanceCount(kref); + if (issuances === 0) { + this.pinObject(kref); + } + kv.set(`${OCAP_URL_PREFIX}${kref}`, `${issuances + 1}`); }, undoOcapURLRetention(kref: KRef): void { - const krefs = this.getOcapURLObjects(); - const index = krefs.indexOf(kref); - if (index === -1) { + const issuances = this.getOcapURLIssuanceCount(kref); + if (issuances > 1) { + kv.set(`${OCAP_URL_PREFIX}${kref}`, `${issuances - 1}`); return; } - krefs.splice(index, 1); - if (krefs.length === 0) { - kv.delete('ocapURLObjects'); - } else { - kv.set('ocapURLObjects', krefs.join(',')); + // The last issuance, or none at all: either way, what is left of the + // retention is exactly what a release drops. + this.releaseOcapURLRetentions(kref); + }, + releaseOcapURLRetentions(kref: KRef): void { + if (this.getOcapURLIssuanceCount(kref) === 0) { + return; } + // Spend the pin before forgetting the retention, so a failure to release + // it leaves a ledger that still says the retention is held. this.unpinObject(kref); + kv.delete(`${OCAP_URL_PREFIX}${kref}`); }, }); } diff --git a/packages/ocap-kernel/src/store/methods/pinned.test.ts b/packages/ocap-kernel/src/store/methods/pinned.test.ts index b9dfcd3f19..a0f7141037 100644 --- a/packages/ocap-kernel/src/store/methods/pinned.test.ts +++ b/packages/ocap-kernel/src/store/methods/pinned.test.ts @@ -8,15 +8,39 @@ vi.mock('./refcount.ts', () => ({ getRefCountMethods: vi.fn(), })); -describe('getPinMethods', () => { - const mockKv = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), +type MockKv = { + get: (key: string) => string | undefined; + set: (key: string, value: string) => void; + delete: (key: string) => void; + getNextKey: (key: string) => string | undefined; +}; + +/** + * Make a key/value store over a map, enough of one for the pin methods: reads, + * writes, deletes, and the ordered key traversal `getPinnedObjects` iterates. + * + * @param entries - The entries the store starts out holding. + * @returns The store. + */ +function makeMockKv(entries: Record = {}): MockKv { + const map = new Map(Object.entries(entries)); + return { + get: (key) => map.get(key), + set: (key, value) => { + map.set(key, value); + }, + delete: (key) => { + map.delete(key); + }, + getNextKey: (key) => + [...map.keys()].sort().find((candidate) => candidate > key), }; +} +describe('getPinMethods', () => { const mockIncrementRefCount = vi.fn(); const mockDecrementRefCount = vi.fn(); + let mockKv: MockKv; let methods: ReturnType; beforeEach(() => { @@ -27,72 +51,89 @@ describe('getPinMethods', () => { decrementRefCount: mockDecrementRefCount, }, ); - mockKv.get.mockImplementation((key) => { - if (key === 'pinnedObjects') { - return 'ko1,ko2,ko3'; - } - return undefined; + mockKv = makeMockKv({ + 'pinned.ko1': '1', + 'pinned.ko2': '1', + 'pinned.ko3': '1', }); // @ts-expect-error - We don't need to provide a full StoreContext for testing methods = getPinMethods({ kv: mockKv }); }); describe('pinObject', () => { - it('should pin an object by adding it to the pinned objects list', () => { + it('records a pin on the object and takes a reference', () => { methods.pinObject('ko4'); expect(mockIncrementRefCount).toHaveBeenCalledWith('ko4', 'pin'); - expect(mockKv.set).toHaveBeenCalledWith( - 'pinnedObjects', - 'ko1,ko2,ko3,ko4', - ); + expect(mockKv.get('pinned.ko4')).toBe('1'); }); - it('should always pin and increment even if object is already pinned', () => { + it('pins and increments again for an object already pinned', () => { methods.pinObject('ko2'); expect(mockIncrementRefCount).toHaveBeenCalledWith('ko2', 'pin'); - expect(mockKv.set).toHaveBeenCalledWith( - 'pinnedObjects', - 'ko1,ko2,ko2,ko3', - ); + expect(methods.getPinCount('ko2')).toBe(2); + expect(methods.getPinnedObjects()).toStrictEqual(['ko1', 'ko2', 'ko3']); + }); + + it('records no pin if taking the reference is refused', () => { + mockIncrementRefCount.mockImplementation(() => { + throw Error('deleted kref'); + }); + expect(() => methods.pinObject('ko4')).toThrow('deleted kref'); + expect(methods.getPinCount('ko4')).toBe(0); }); }); describe('unpinObject', () => { - it('should unpin an object by removing it from the pinned objects list', () => { + it('drops the object from the pinned objects and releases its reference', () => { methods.unpinObject('ko2'); expect(mockDecrementRefCount).toHaveBeenCalledWith('ko2', 'unpin'); - expect(mockKv.set).toHaveBeenCalledWith('pinnedObjects', 'ko1,ko3'); + expect(methods.getPinnedObjects()).toStrictEqual(['ko1', 'ko3']); }); - it('should not modify the list or decrement refCount if object is not in the list', () => { + it('spends one pin of several, leaving the object pinned', () => { + methods.pinObject('ko2'); + + methods.unpinObject('ko2'); + + expect(mockDecrementRefCount).toHaveBeenCalledWith('ko2', 'unpin'); + expect(methods.getPinCount('ko2')).toBe(1); + expect(methods.isObjectPinned('ko2')).toBe(true); + }); + + it('does not release a reference for an object that is not pinned', () => { methods.unpinObject('ko4'); expect(mockDecrementRefCount).not.toHaveBeenCalled(); - expect(mockKv.set).not.toHaveBeenCalled(); + expect(methods.getPinCount('ko4')).toBe(0); }); }); describe('getPinnedObjects', () => { - it('should return all pinned objects', () => { - const pinnedObjects = methods.getPinnedObjects(); - expect(pinnedObjects).toStrictEqual(['ko1', 'ko2', 'ko3']); + it('returns all pinned objects', () => { + expect(methods.getPinnedObjects()).toStrictEqual(['ko1', 'ko2', 'ko3']); + }); + + it('returns an empty array if no objects are pinned', () => { + // @ts-expect-error - We don't need to provide a full StoreContext for testing + methods = getPinMethods({ kv: makeMockKv() }); + expect(methods.getPinnedObjects()).toStrictEqual([]); }); - it('should return an empty array if no objects are pinned', () => { - mockKv.get.mockReturnValue(''); - const pinnedObjects = methods.getPinnedObjects(); - expect(pinnedObjects).toStrictEqual([]); + it('names an object once however many pins it holds', () => { + methods.pinObject('ko2'); + methods.pinObject('ko2'); + + expect(methods.getPinnedObjects()).toStrictEqual(['ko1', 'ko2', 'ko3']); + expect(methods.getPinCount('ko2')).toBe(3); }); }); describe('isObjectPinned', () => { - it('should return true if the object is pinned', () => { - const isPinned = methods.isObjectPinned('ko2'); - expect(isPinned).toBe(true); + it('returns true if the object is pinned', () => { + expect(methods.isObjectPinned('ko2')).toBe(true); }); - it('should return false if the object is not pinned', () => { - const isPinned = methods.isObjectPinned('ko4'); - expect(isPinned).toBe(false); + it('returns false if the object is not pinned', () => { + expect(methods.isObjectPinned('ko4')).toBe(false); }); }); }); diff --git a/packages/ocap-kernel/src/store/methods/pinned.ts b/packages/ocap-kernel/src/store/methods/pinned.ts index a2ba783c3c..25d75acc4b 100644 --- a/packages/ocap-kernel/src/store/methods/pinned.ts +++ b/packages/ocap-kernel/src/store/methods/pinned.ts @@ -1,16 +1,18 @@ +import { getBaseMethods } from './base.ts'; import { getRefCountMethods } from './refcount.ts'; import type { KRef } from '../../types.ts'; import type { StoreContext } from '../types.ts'; /** - * Split a comma-separated string into an array. + * The prefix shared by every object's pin count, for iterating over them. * - * @param str - The string to split. - * @returns An array of strings. + * A pin count lives in a row of its own rather than in one row listing every + * pin, so pinning is a single write whatever else is pinned. Anything a holder + * outside the kernel's own state keeps alive is pinned — an ocap URL's target, + * for one — so the number of pinned objects is not bounded by the kernel's + * own structure. */ -function commaSplit(str: string = ''): string[] { - return str ? str.split(',') : []; -} +const PIN_PREFIX = 'pinned.'; /** * Create a pinned store that provides high-level functionality for managing pinned objects. @@ -21,6 +23,27 @@ function commaSplit(str: string = ''): string[] { // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getPinMethods(ctx: StoreContext) { const { incrementRefCount, decrementRefCount } = getRefCountMethods(ctx); + const { getPrefixedKeys } = getBaseMethods(ctx.kv); + + /** + * Generate the storage key for an object's pin count. + * + * @param kref - The KRef of interest. + * @returns the key to store the indicated pin count at. + */ + function pinCountKey(kref: KRef): string { + return `${PIN_PREFIX}${kref}`; + } + + /** + * Get the number of pins held on an object. + * + * @param kref - The KRef of the object to count the pins of. + * @returns How many times the object has been pinned and not unpinned. + */ + function getPinCount(kref: KRef): number { + return Number(ctx.kv.get(pinCountKey(kref)) ?? 0); + } /** * Pin a kernel object to prevent it from being garbage collected. @@ -29,10 +52,10 @@ export function getPinMethods(ctx: StoreContext) { * @param kref - The KRef of the object to pin. */ function pinObject(kref: KRef): void { - const pinList = commaSplit(ctx.kv.get('pinnedObjects')); - pinList.push(kref); + const pins = getPinCount(kref); + // Before the count, so a refused increment records no pin. incrementRefCount(kref, 'pin'); - ctx.kv.set('pinnedObjects', pinList.sort().join(',')); + ctx.kv.set(pinCountKey(kref), `${pins + 1}`); } /** @@ -43,21 +66,28 @@ export function getPinMethods(ctx: StoreContext) { * @param kref - The KRef of the object to unpin. */ function unpinObject(kref: KRef): void { - const pinList = commaSplit(ctx.kv.get('pinnedObjects')); - if (pinList.includes(kref)) { - decrementRefCount(kref, 'unpin'); - pinList.splice(pinList.indexOf(kref), 1); - ctx.kv.set('pinnedObjects', pinList.sort().join(',')); + const pins = getPinCount(kref); + if (pins === 0) { + return; + } + decrementRefCount(kref, 'unpin'); + if (pins === 1) { + ctx.kv.delete(pinCountKey(kref)); + } else { + ctx.kv.set(pinCountKey(kref), `${pins - 1}`); } } /** * Get all pinned objects. * - * @returns An array of KRefs for all pinned objects. + * @returns An array of KRefs for all pinned objects, each named once however + * many pins it holds; `getPinCount` gives that number. */ function getPinnedObjects(): KRef[] { - return commaSplit(ctx.kv.get('pinnedObjects')) as KRef[]; + return [...getPrefixedKeys(PIN_PREFIX)].map( + (key) => key.slice(PIN_PREFIX.length) as KRef, + ); } /** @@ -67,12 +97,13 @@ export function getPinMethods(ctx: StoreContext) { * @returns True if the object is pinned, false otherwise. */ function isObjectPinned(kref: KRef): boolean { - return getPinnedObjects().includes(kref); + return getPinCount(kref) > 0; } return { pinObject, unpinObject, + getPinCount, getPinnedObjects, isObjectPinned, }; diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.ts index 5ebbbb6e1a..b8824747ec 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -70,7 +70,7 @@ const REFCOUNT_KEY = /^(k[op]\d+)\.refCount$/u; // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getRefCountAuditMethods(ctx: StoreContext) { const { getPrefixedKeys, refCountKey } = getBaseMethods(ctx.kv); - const { getPinnedObjects } = getPinMethods(ctx); + const { getPinCount, getPinnedObjects } = getPinMethods(ctx); /** * Render a tally the way the store encodes it, so expected and stored values @@ -219,7 +219,11 @@ export function getRefCountAuditMethods(ctx: StoreContext) { } for (const kref of getPinnedObjects()) { - credit(kref, 'pin'); + // One unit per pin: each `pinObject` call increments once, and the + // object's count is how many of those calls are outstanding. + for (let pins = getPinCount(kref); pins > 0; pins -= 1) { + credit(kref, 'pin'); + } } return tallies; diff --git a/packages/ocap-kernel/src/vats/SubclusterManager.ts b/packages/ocap-kernel/src/vats/SubclusterManager.ts index 1b21b6c26a..aa22f18a0e 100644 --- a/packages/ocap-kernel/src/vats/SubclusterManager.ts +++ b/packages/ocap-kernel/src/vats/SubclusterManager.ts @@ -297,8 +297,8 @@ export class SubclusterManager { 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. + // the root's count never reaches zero and the pin outlives the vat it + // was taken for. this.#vatManager.releaseVatRootPin(vatId); this.#kernelStore.deleteVatConfig(vatId); this.#kernelStore.markVatAsTerminated(vatId); From 508a8c6ad5dcde15f0f3ef44e718bdf11eb4f04f Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 18 Aug 2026 16:12:03 +0200 Subject: [PATCH 14/17] docs(ocap-kernel): keep the new entries to consumer-facing changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Where pins and ocap URL retentions are stored is the store's own business: a consumer sees the methods and what they mean, not the keys they write. The entries keep the API changes — the new methods, and `getPinnedObjects` naming each object once — and drop the key layout and the reasoning behind it, which live in the code that implements them. Co-Authored-By: Claude Opus 5 (1M context) --- packages/ocap-kernel/CHANGELOG.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 2fe4e74d39..d08d3d596f 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -42,7 +42,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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`, `getOcapURLIssuanceCount`, `retainForOcapURL`, `undoOcapURLRetention` and `releaseOcapURLRetentions` to the kernel store, and `VatManager.releaseVatRootPin` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) - - The two ways a retention ends are separate operations: `undoOcapURLRetention` unwinds one issuance whose URL was never minted, and `releaseOcapURLRetentions` drops a target's whole retention, for disavowing every URL naming it at once + - `undoOcapURLRetention` unwinds one issuance whose URL was never minted; `releaseOcapURLRetentions` drops a target's whole retention, for disavowing every URL naming it at once - Add `getPinCount` to the kernel store, which reports how many pins are held on an object ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) ### Changed @@ -56,7 +56,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bound relay hints in OCAP URLs to a maximum of 3 and cap the relay pool at 20 entries with eviction of oldest non-bootstrap relays ([#929](https://github.com/MetaMask/ocap-kernel/pull/929)) - **BREAKING:** Rename `krefsToExistingErefs` to `krefsToErefs`, which now throws on an unmapped kref instead of silently dropping it ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) - **BREAKING:** `getPinnedObjects` now names each pinned object once, however many pins it holds; `getPinCount` gives the number of pins ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) - - Pins and ocap URL retentions are each stored as a count per object (`pinned.${koid}`, `ocapURLObjects.${koid}`) rather than in one row listing every pin, so taking or spending one is a single write regardless of how many others there are. What an ocap URL retains is chosen by whoever holds a URL, so neither list is bounded by anything the kernel controls. Covered by the reset above: the old `pinnedObjects` row is not read ### Fixed @@ -95,7 +94,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 for as long as any URL names the target, and no release: the token is persistent and unexpiring, so revocation is what kills the capability — though it only stops deliveries, and leaves the target retained - 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, and unwound again if minting fails. The store counts issuances per target rather than sharing one retention between them, because that window lets issuances for the same target overlap: sharing one would let a failed mint release the retention a URL minted alongside it depends on + - The retention is taken before the token is minted, since minting awaits and a collection crank can run in that window, and unwound again if minting fails. A failed issuance never disturbs the retention a URL minted for the same target alongside it depends on - 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)) From ceb6c50d20d14c8ec1d8d02c9a98a1830c2a17f5 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 18 Aug 2026 17:49:47 +0200 Subject: [PATCH 15/17] fix(ocap-kernel): close the gaps a fresh review of this PR found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three behaviour fixes, each the sibling of something already fixed here. `incrementRefCount`'s deleted-kref guard now covers promises. It sat below the `isPromise` early return, so a promise increment on a missing row still wrote `NaN` back — a row that reads as existing and that no decrement can bring to zero, so the promise could never be collected. The guard is placed in front of both paths that read a row and write it back, rather than at the top: an object export mutates no count, so it still needs no row to exist. The audit no longer dies on a settled promise that lost its value. Reading it with `getRequired` took the whole sweep down over one row, in the module whose premise is that the store might be wrong; `gc.ts` and `getKpidsToRetire` both allow that state. Read tolerantly, the slots it would have credited are reported as counts too high, which is what they are. `deleteEndpoint` releases the references its c-list entries hold instead of deleting the keys. The prefix fix made this loop live for the first time, and a bare delete leaves the target held by a holder that no longer exists — pinned alive forever, and reported by the audit as a count nothing accounts for. No in-tree change: `cleanupTerminatedVat` has emptied the c-list before it gets here. `clist.test.ts` needed a promise refcount row for the same reason the object tests needed one in 2bf8a5cf5. Also: `unpinVatRoot`'s doc claimed the opposite of what it does, since pins are fungible and an unbalanced call spends the lifetime pin; four keys in the store's layout block were wrong, which is the class of staleness that caused two of the bugs this PR fixes; and the e2e asserts a root's pin, which is the one invariant the audit cannot check — a root that lost its pin agrees with its refcount and the audit stays silent. Co-Authored-By: Claude Opus 5 (1M context) --- .../extension/test/e2e/control-panel.test.ts | 5 +++ packages/ocap-kernel/CHANGELOG.md | 6 ++- packages/ocap-kernel/src/store/index.ts | 18 +++++---- .../store/methods/clist-accounting.test.ts | 17 ++++++++ .../src/store/methods/clist.test.ts | 1 + .../ocap-kernel/src/store/methods/clist.ts | 9 +++-- .../src/store/methods/refcount-audit.test.ts | 23 +++++++++++ .../src/store/methods/refcount-audit.ts | 37 +++++++++++++++--- .../src/store/methods/refcount.test.ts | 11 ++++++ .../ocap-kernel/src/store/methods/refcount.ts | 39 ++++++++++++------- .../ocap-kernel/src/store/methods/vat.test.ts | 7 ++++ packages/ocap-kernel/src/store/methods/vat.ts | 24 +++++++++++- packages/ocap-kernel/src/vats/VatManager.ts | 7 +++- 13 files changed, 166 insertions(+), 38 deletions(-) diff --git a/packages/extension/test/e2e/control-panel.test.ts b/packages/extension/test/e2e/control-panel.test.ts index 754a4017b5..d5dfa4ad39 100644 --- a/packages/extension/test/e2e/control-panel.test.ts +++ b/packages/extension/test/e2e/control-panel.test.ts @@ -253,6 +253,11 @@ test.describe('Control Panel', () => { `{"key":"v3.c.${v3Promise}","value":"R p-1"}`, `{"key":"v3.c.p-1","value":"${v3Promise}"}`, `{"key":"${v3Promise}.refCount","value":"2"}`, + // A root is pinned once for its vat's lifetime, by `launchVat`. Asserted + // here because nothing else can: a pin is the audit's own ground truth, + // so a root that lost its pin agrees with its refcount and the audit + // stays silent — while the last importer's drop can retire it. + `{"key":"pinned.${v3Root}","value":"1"}`, ]; // Derived too: v1 imports the two roots as the bootstrap's calls are // answered, so which of `o-1`/`o-2` names which root varies with the same diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index d08d3d596f..8b1b30b2bb 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -56,6 +56,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bound relay hints in OCAP URLs to a maximum of 3 and cap the relay pool at 20 entries with eviction of oldest non-bootstrap relays ([#929](https://github.com/MetaMask/ocap-kernel/pull/929)) - **BREAKING:** Rename `krefsToExistingErefs` to `krefsToErefs`, which now throws on an unmapped kref instead of silently dropping it ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) - **BREAKING:** `getPinnedObjects` now names each pinned object once, however many pins it holds; `getPinCount` gives the number of pins ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- **BREAKING:** `incrementRefCount` now throws on a kref the kernel has already deleted, rather than writing a resurrected row ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - A missing object row read as `(0, 0)` and was written back as a live-looking object with no owner; a missing promise row read as `NaN`, which no decrement can bring to zero, so the promise could never be collected. `decrementRefCount` still tolerates a missing object row, since releasing a reference to something already gone is ordinary teardown ### Fixed @@ -80,7 +82,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `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 - - **A store written by an earlier version must be reset.** There is no migration: every object in it is still at `(1, 1)` and no vat root is pinned, so the second importer's `dropImports` underflows mid-crank and the last importer's drop can retire a live vat's root. `recomputeRefCounts` can rebuild the counts, but not the root pins, so it is a diagnostic rather than an upgrade path + - **A store written by an earlier version must be reset.** There is no migration: every object in it is still at `(1, 1)` and no vat root is pinned, so the second importer's `dropImports` underflows mid-crank and the last importer's drop can retire a live vat's root. Pins also moved from a single `pinnedObjects` row to a count per object at `pinned.${kref}`, and the old row is no longer read by anything — so every pin in such a store is silently lost on open while the refcount unit each one took remains. `recomputeRefCounts` can rebuild the counts, but not the pins, so it is a diagnostic rather than an upgrade path - 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)) @@ -92,7 +94,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `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 for as long as any URL names the target, and no release: the token is persistent and unexpiring, so revocation is what kills the capability — though it only stops deliveries, and leaves the target retained + - One pin for as long as any URL names the target, and nothing releases it automatically: the token is persistent and unexpiring, so revocation is what kills the capability — though it only stops deliveries, and leaves the target retained. `releaseOcapURLRetentions` drops a target's retention for a caller that is disavowing every URL naming it - 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, and unwound again if minting fails. A failed issuance never disturbs the retention a URL minted for the same target alongside it depends on - Refuse to import a kref the kernel has deleted into an endpoint's c-list ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 32b8dd585c..75f946eed6 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -25,17 +25,19 @@ * Queues * queue.${queueName}.head = NN // queue head index * queue.${queueName}.tail = NN // queue tail index - * queue.${queueName}.${NN} = JSON(CAPDATA) // queue entry #NN + * queue.${queueName}.${NN} = JSON(ITEM) // queue entry #NN: a RunQueueItem + * // on `run`, a KernelMessage on a + * // ${kpid} queue * * Kernel objects - * ${koid}.refCount = NN // reference count - * ${koid}.owner = ${vatid} // owner (where the object is) + * ${koid}.refCount = NN,NN // reachable,recognizable counts + * ${koid}.owner = ${endid} | kernel // owner (where the object is) * * Kernel promises * ${kpid}.refCount = NN // reference count * ${kpid}.state = unresolved | fulfilled | rejected // current state of settlement * ${kpid}.subscribers = JSON([${endid}]) // array of who is waiting for settlement - * ${kpid}.decider = ${endid} // who decides on settlement + * ${kpid}.decider = ${endid} | kernel // who decides on settlement * ${kpid}.value = JSON(CAPDATA) // value settled to, if settled * * C-lists (both directions share one prefix; see `getCListPrefix`) @@ -59,10 +61,10 @@ * initialized = true // if set, indicates the store has been initialized * nextVatId = NN // allocation counter for vat IDs * nextRemoteId = NN // allocation counter for remote IDs - * k.nextObjectId = NN // allocation counter for object KRefs - * k.nextPromiseId = NN // allocation counter for promise KRefs - * pinned.${koid} = NN // number of pins held on ${koid} - * ocapURLObjects.${koid} = NN // number of ocap URLs naming ${koid} + * nextObjectId = NN // allocation counter for object KRefs + * nextPromiseId = NN // allocation counter for promise KRefs + * pinned.${kref} = NN // number of pins held on ${kref} + * ocapURLObjects.${kref} = NN // number of ocap URLs naming ${kref} * kernelService.${serviceName} = ${koid} // kref of kernel service object ${serviceName} */ diff --git a/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts index c7d1cf22d1..ac1972c4e1 100644 --- a/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts +++ b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts @@ -34,6 +34,23 @@ describe('c-list reference accounting', () => { givenVats('v1', 'v2', 'v3'); }); + it('releases an endpoint c-list entry rather than dropping it', () => { + kernelStore.initEndpoint('r1'); + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('r1', kref, true); + + kernelStore.deleteEndpoint('r1'); + + // A bare key delete would leave the count behind, pinning the object alive + // on the strength of a holder that no longer exists. + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + expect(kernelStore.krefToEref('r1', kref)).toBeUndefined(); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + it('counts each importer separately', () => { const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); diff --git a/packages/ocap-kernel/src/store/methods/clist.test.ts b/packages/ocap-kernel/src/store/methods/clist.test.ts index ea2fdd0b41..7a03bd3559 100644 --- a/packages/ocap-kernel/src/store/methods/clist.test.ts +++ b/packages/ocap-kernel/src/store/methods/clist.test.ts @@ -24,6 +24,7 @@ describe('clist-methods', () => { // reference to one would resurrect it. kv.set('ko1.refCount', '0,0'); kv.set('ko2.refCount', '0,0'); + kv.set('kp1.refCount', '0'); // Create the store with mocked dependencies clistMethods = getCListMethods({ diff --git a/packages/ocap-kernel/src/store/methods/clist.ts b/packages/ocap-kernel/src/store/methods/clist.ts index d079100ae4..b4e104dcc9 100644 --- a/packages/ocap-kernel/src/store/methods/clist.ts +++ b/packages/ocap-kernel/src/store/methods/clist.ts @@ -30,10 +30,11 @@ export function getCListMethods(ctx: StoreContext) { * 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. + * {@link deleteCListEntry}. An object import is born recognizing but not + * reaching: reachability is `setReachableFlag`'s job, when the reference is + * handed over. An object export takes no count — the owner is not one of its + * own referrers — and is born flagged. A promise entry takes a full count + * whichever direction it faces, since a promise has only the one count. * * @param endpointId - The endpoint whose c-list is to be added to. * @param kref - The KRef. diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts index 5d637d79d0..ac6cf99e59 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts @@ -116,6 +116,29 @@ describe('reference count audit', () => { expect(kernelStore.auditRefCounts()).toStrictEqual([]); }); + it('reports a settled promise that lost its value instead of throwing', () => { + 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], + }); + // `gc.ts` and `getKpidsToRetire` both allow this state, so the audit has + // to survive it: the slot's holder is gone, which is drift to report, not + // a reason to take the whole sweep down. + kernelDatabase.kernelKVStore.delete(`${kpid}.value`); + + expect(kernelStore.auditRefCounts()).toStrictEqual([ + { + kref: koid, + stored: '1,1', + expected: '0,0', + holders: [], + }, + ]); + }); + it('reports counts that are too low', () => { const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); kernelStore.translateRefKtoE('v2', kref, true); diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.ts index b8824747ec..17f86f5456 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -91,12 +91,35 @@ export function getRefCountAuditMethods(ctx: StoreContext) { : `${counts.reachable},${counts.recognizable}`; } + /** + * Read the slots a settled promise's resolution value carries, tolerating a + * value that is missing or unreadable. + * + * @param kpid - The settled promise. + * @returns Its resolution slots, or none if the value cannot be read. + */ + function readResolutionSlots(kpid: KRef): KRef[] { + const raw = ctx.kv.get(`${kpid}.value`); + if (raw === undefined) { + return []; + } + try { + return (JSON.parse(raw) as CapData).slots; + } catch { + return []; + } + } + /** * 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. + * function's rules change, these have to change with it. Two of the rules + * mirrored here live elsewhere: the unsettled-promise unit is written + * directly by `initKernelPromise`, and an object import's reachable half is + * carried by `setReachableFlag`/`clearReachableFlag`, which is why this reads + * each import entry's flag rather than assuming it. * * @returns A tally per kref that anything refers to. */ @@ -208,10 +231,14 @@ export function getRefCountAuditMethods(ctx: StoreContext) { // 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) { + // A settled promise's stored value is what holds its resolution + // slots. Read it the way the rest of the kernel does — `gc.ts` and + // `getKpidsToRetire` both allow a settled promise to have lost its + // value — rather than requiring it: this exists to report drift, not + // to die on it, and `getRequired` here would take the whole sweep + // down over one row. With nothing to credit, whatever those slots + // still hold is reported as a count too high, which is what it is. + for (const slot of readResolutionSlots(kpid)) { credit(slot, `${kpid} resolution slot`); } } diff --git a/packages/ocap-kernel/src/store/methods/refcount.test.ts b/packages/ocap-kernel/src/store/methods/refcount.test.ts index 5b79032fa9..4a9cfba169 100644 --- a/packages/ocap-kernel/src/store/methods/refcount.test.ts +++ b/packages/ocap-kernel/src/store/methods/refcount.test.ts @@ -214,6 +214,17 @@ describe('refcount-methods', () => { expect(kv.get(baseStore.refCountKey(kref))).toBeUndefined(); }); + it('refuses to resurrect a promise the kernel has deleted', () => { + const kref: KRef = 'kp99'; + + expect(() => refCountMethods.incrementRefCount(kref, 'test')).toThrow( + 'incrementRefCount on deleted kref "kp99" ("test")', + ); + // A written-back `NaN` would read as existing and never reach zero, so + // the promise could never be collected. + expect(kv.get(baseStore.refCountKey(kref))).toBeUndefined(); + }); + it('increments an object that exists', () => { const kref: KRef = 'ko1'; kv.set(baseStore.refCountKey(kref), '0,0'); diff --git a/packages/ocap-kernel/src/store/methods/refcount.ts b/packages/ocap-kernel/src/store/methods/refcount.ts index 6e095b95c4..a54d8ef77f 100644 --- a/packages/ocap-kernel/src/store/methods/refcount.ts +++ b/packages/ocap-kernel/src/store/methods/refcount.ts @@ -74,14 +74,17 @@ export function getRefCountMethods(ctx: StoreContext) { * 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. + * violations against correct accounting. Two rules the audit mirrors do not + * live here, and move with it too: `initKernelPromise` writes an unsettled + * promise's first unit directly, and `setReachableFlag`/`clearReachableFlag` + * carry the reachable half of an object import on their own. * * @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. * @param options.isExport - True if the reference comes from a clist export, which counts for promises but not objects. * @param options.onlyRecognizable - True if the reference provides only recognition, not reachability. - * @throws if `kref` names an object the kernel has already deleted. + * @throws if `kref` names an object or promise the kernel has already deleted. */ function incrementRefCount( kref: KRef, @@ -94,26 +97,32 @@ export function getRefCountMethods(ctx: StoreContext) { kref || Fail`incrementRefCount called with empty kref`; const { isPromise } = parseRef(kref); - if (isPromise) { - const refCount = Number(ctx.kv.get(refCountKey(kref))) + 1; - ctx.logger?.debug('++', refCountKey(kref), refCount, tag); - ctx.kv.set(refCountKey(kref), `${refCount}`); - return; - } - // If `isExport` the reference comes from a clist export, which counts for promises but not objects - if (isExport) { + // If `isExport` the reference comes from a clist export, which counts for + // promises but not objects. An object export changes no count, so it needs + // no row to change. + if (!isPromise && isExport) { return; } - // A missing row reads as `(0, 0)`, so incrementing one writes it back and - // resurrects a live-looking object that nobody owns and nobody can be - // delivered to. `decrementRefCount` tolerates the same missing row because - // releasing a reference to something already gone is ordinary teardown; - // taking one is always a bug, so this refuses rather than returns. + // Everything past here reads a row and writes it back, so a row that is not + // there gets resurrected: a missing object row reads as `(0, 0)` and becomes + // a live-looking object that nobody owns and nobody can be delivered to, + // and a missing promise row reads as `NaN`, which no decrement can bring to + // zero, so the promise can never be collected. `decrementRefCount` tolerates + // a missing object row because releasing a reference to something already + // gone is ordinary teardown; taking one is always a bug, so this refuses + // rather than returns. kernelRefExists(kref) || Fail`incrementRefCount on deleted kref ${kref} (${tag})`; + if (isPromise) { + const refCount = Number(ctx.kv.get(refCountKey(kref))) + 1; + ctx.logger?.debug('++', refCountKey(kref), refCount, tag); + ctx.kv.set(refCountKey(kref), `${refCount}`); + return; + } + const counts = getObjectRefCount(kref); if (!onlyRecognizable) { counts.reachable += 1; diff --git a/packages/ocap-kernel/src/store/methods/vat.test.ts b/packages/ocap-kernel/src/store/methods/vat.test.ts index fd1f05402e..3af461aa9b 100644 --- a/packages/ocap-kernel/src/store/methods/vat.test.ts +++ b/packages/ocap-kernel/src/store/methods/vat.test.ts @@ -91,6 +91,7 @@ describe('vat store methods', () => { // Mock method implementations const mockDeleteCListEntry = vi.fn(); + const mockForgetKref = vi.fn(); const mockGetKernelPromise = vi.fn(); const mockGetReachableAndVatSlot = vi.fn(); const mockDecrementRefCount = vi.fn(); @@ -126,6 +127,7 @@ describe('vat store methods', () => { (clistModule.getCListMethods as ReturnType).mockReturnValue({ deleteCListEntry: mockDeleteCListEntry, addCListEntry: mockAddCListEntry, + forgetKref: mockForgetKref, }); ( @@ -294,6 +296,11 @@ describe('vat store methods', () => { expect(mockKV.has(`e.nextPromiseId.${endpointId}`)).toBe(false); expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`${endpointId}.c.`); + // The kref-keyed half goes through the c-list teardown, so the entry's + // reference is released rather than dropped along with the key. The + // eref-keyed half of that pair goes with it; `p+1` here has no kref side + // to release through, so it is deleted outright. + expect(mockForgetKref.mock.calls).toStrictEqual([[endpointId, 'ko1']]); }); it('does nothing if endpoint has no associated keys', () => { diff --git a/packages/ocap-kernel/src/store/methods/vat.ts b/packages/ocap-kernel/src/store/methods/vat.ts index 29e2a85dff..e603ce5248 100644 --- a/packages/ocap-kernel/src/store/methods/vat.ts +++ b/packages/ocap-kernel/src/store/methods/vat.ts @@ -36,7 +36,7 @@ export function getVatMethods(ctx: StoreContext) { const { kv } = ctx; const { getPrefixedKeys, getSlotKey, getCListPrefix, getOwnerKey } = getBaseMethods(ctx.kv); - const { deleteCListEntry } = getCListMethods(ctx); + const { deleteCListEntry, forgetKref } = getCListMethods(ctx); const { getReachableAndVatSlot } = getReachableMethods(ctx); const { initKernelPromise, setPromiseDecider, addPromiseSubscriber } = getPromiseMethods(ctx); @@ -46,10 +46,30 @@ export function getVatMethods(ctx: StoreContext) { /** * Delete all persistent state associated with an endpoint. * + * Each surviving c-list entry is torn down rather than merely deleted: an + * entry is a reference, so dropping the key without releasing its count + * leaves the target held by a holder that no longer exists — pinned alive + * forever, and reported by the audit as a count nothing accounts for. + * `cleanupTerminatedVat` has already emptied the c-list by the time it calls + * this, so in practice there is nothing here to release; a caller that has + * not done that work first depends on this. + * * @param endpointId - The endpoint whose state is to be deleted. */ function deleteEndpoint(endpointId: EndpointId): void { - for (const key of getPrefixedKeys(getCListPrefix(endpointId))) { + const prefix = getCListPrefix(endpointId); + // Snapshot the keys: forgetKref deletes both halves of a pair, so mutating + // while walking the live key sequence would step over entries. + for (const key of [...getPrefixedKeys(prefix)]) { + const ref = key.slice(prefix.length); + // The kref-keyed half of each pair, which is the half that names the + // reference to release. The eref-keyed half goes with it. + if (parseRef(ref).context === 'kernel') { + forgetKref(endpointId, ref as KRef); + } + } + // Any half-pair left over has no kref side to release through. + for (const key of getPrefixedKeys(prefix)) { kv.delete(key); } kv.delete(`e.nextObjectId.${endpointId}`); diff --git a/packages/ocap-kernel/src/vats/VatManager.ts b/packages/ocap-kernel/src/vats/VatManager.ts index 5da2bebe2d..f080dbf177 100644 --- a/packages/ocap-kernel/src/vats/VatManager.ts +++ b/packages/ocap-kernel/src/vats/VatManager.ts @@ -331,8 +331,11 @@ export class VatManager { /** * 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. + * Removes a single pin, and pins are fungible: this is only safe to call + * against a pin `pinVatRoot` took. Called without one, it spends the pin + * `launchVat` holds for the vat's lifetime, and the root becomes collectable + * while the vat is still running — silently, since `unpinObject` tolerates a + * count that has already reached zero. * * @param vatId - The ID of the vat. */ From 619c647bfbacfabe0262282848409a5982a00921 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 18 Aug 2026 17:55:15 +0200 Subject: [PATCH 16/17] test(extension): assert a vat root's pin across its vat's life MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `v3Values` is checked three times, spanning v3's termination, so a pin assertion cannot live there: terminating the vat releases the pin its launch took. Asserted on either side of the termination instead, which is worth more than one reading anyway — it pins the release too. The counts come from an actual run rather than derivation: a live root is its own pin plus v1's import at `2,2`, and `1,1` once the pin is spent. That run also showed bob's root at `3,3` behind two pins, the second being the ocap URL retention its issued URL holds, which is the accounting this branch added. Co-Authored-By: Claude Opus 5 (1M context) --- .../extension/test/e2e/control-panel.test.ts | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/packages/extension/test/e2e/control-panel.test.ts b/packages/extension/test/e2e/control-panel.test.ts index d5dfa4ad39..fdd96bdcd1 100644 --- a/packages/extension/test/e2e/control-panel.test.ts +++ b/packages/extension/test/e2e/control-panel.test.ts @@ -253,11 +253,6 @@ test.describe('Control Panel', () => { `{"key":"v3.c.${v3Promise}","value":"R p-1"}`, `{"key":"v3.c.p-1","value":"${v3Promise}"}`, `{"key":"${v3Promise}.refCount","value":"2"}`, - // A root is pinned once for its vat's lifetime, by `launchVat`. Asserted - // here because nothing else can: a pin is the audit's own ground truth, - // so a root that lost its pin agrees with its refcount and the audit - // stays silent — while the last importer's drop can retire it. - `{"key":"pinned.${v3Root}","value":"1"}`, ]; // Derived too: v1 imports the two roots as the bootstrap's calls are // answered, so which of `o-1`/`o-2` names which root varies with the same @@ -286,6 +281,21 @@ test.describe('Control Panel', () => { popupPage.locator('[data-testid="message-output"]'), ).toContainText(value); } + // A live vat's root is pinned once, by `launchVat`, and its count is that + // pin plus v1's import. Both are asserted only while v3 is alive, since + // terminating it releases the pin — which is the point of the pair of + // assertions after the termination below. Worth asserting at all because a + // pin is the audit's own ground truth: a root that lost its pin agrees with + // its own refcount, so the audit stays silent while the last importer's + // drop can retire a live vat's root. + for (const value of [ + `{"key":"pinned.${v3Root}","value":"1"}`, + `{"key":"${v3Root}.refCount","value":"2,2"}`, + ]) { + await expect( + popupPage.locator('[data-testid="message-output"]'), + ).toContainText(value); + } await popupPage.click('button:text("Control Panel")'); await popupPage.locator('[data-testid="accordion-header"]').first().click(); await popupPage @@ -311,6 +321,14 @@ test.describe('Control Panel', () => { popupPage.locator('[data-testid="message-output"]'), ).toContainText(value); } + // Terminating the vat released the pin its launch took, leaving the root + // held only by v1's import — so it can now be collected once v1 lets go. + await expect( + popupPage.locator('[data-testid="message-output"]'), + ).not.toContainText(`{"key":"pinned.${v3Root}"`); + await expect( + popupPage.locator('[data-testid="message-output"]'), + ).toContainText(`{"key":"${v3Root}.refCount","value":"1,1"}`); await popupPage.click('button:text("Control Panel")'); await popupPage.click('button:text("Collect Garbage")'); From 2f9ef11bf6a06ec8d963096eb74575405669fcac Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 18 Aug 2026 18:06:10 +0200 Subject: [PATCH 17/17] fix(ocap-kernel): tolerate a resolution value with no slots array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tolerant read added for a missing value row still asserted `slots` was there, so a row that parsed but carried none returned `undefined` for the caller to iterate — throwing outside the `try`, which is the crash the helper exists to prevent. Reported by Bugbot. A row whose `slots` is a string was worse and unreported: it iterated character by character and credited krefs that never existed, so the audit invented violations against `k`, `o` and `1` instead of dying. A tool whose only value is being believed must not do that, so this checks for an array rather than trusting a cast. Parameterized over all six shapes a value row can take; three of them fail against the previous code, including the fabricated-kref one. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/store/methods/refcount-audit.test.ts | 90 ++++++++++++++----- .../src/store/methods/refcount-audit.ts | 9 +- 2 files changed, 75 insertions(+), 24 deletions(-) diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts index ac6cf99e59..03368852bb 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts @@ -1,4 +1,4 @@ -import type { KernelDatabase } from '@metamask/kernel-store'; +import type { KernelDatabase, KVStore } from '@metamask/kernel-store'; import { describe, it, expect, beforeEach } from 'vitest'; import { makeMapKernelDatabase } from '../../../test/storage.ts'; @@ -9,6 +9,27 @@ describe('reference count audit', () => { let kernelDatabase: KernelDatabase; let kernelStore: ReturnType; + /** + * The raw KV store, for writing rows the store's own methods never would. + * Read through a function because the cases below are built before + * `beforeEach` has made a database. + * + * @returns The KV store the running test is using. + */ + const kv = (): KVStore => kernelDatabase.kernelKVStore; + + /** + * Corrupt a row by overwriting it with a value the store would not produce. + * + * @param value - The raw value to write. + * @returns A function that writes it at a given key. + */ + const setValue = + (value: string) => + (key: string): void => { + kv().set(key, value); + }; + /** * Register and initialize an endpoint so it can hold c-list entries. * @@ -116,28 +137,51 @@ describe('reference count audit', () => { expect(kernelStore.auditRefCounts()).toStrictEqual([]); }); - it('reports a settled promise that lost its value instead of throwing', () => { - 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], - }); - // `gc.ts` and `getKpidsToRetire` both allow this state, so the audit has - // to survive it: the slot's holder is gone, which is drift to report, not - // a reason to take the whole sweep down. - kernelDatabase.kernelKVStore.delete(`${kpid}.value`); - - expect(kernelStore.auditRefCounts()).toStrictEqual([ - { - kref: koid, - stored: '1,1', - expected: '0,0', - holders: [], - }, - ]); - }); + // `gc.ts` and `getKpidsToRetire` both tolerate a settled promise whose + // value cannot be read, so the audit has to: the slot's holder is gone, + // which is drift to report, not a reason to take the whole sweep down. A + // `slots` that parses but is not an array is the dangerous one — iterating + // a string would credit krefs that were never there, and a wrong audit is + // worse than a loud one. + it.each([ + { + what: 'no value row at all', + corrupt: (key: string) => kv().delete(key), + }, + { what: 'a value that is not JSON', corrupt: setValue('{') }, + { what: 'a value of null', corrupt: setValue('null') }, + { what: 'a value that is not an object', corrupt: setValue('"gone"') }, + { + what: 'an object carrying no slots', + corrupt: setValue('{"body":"#null"}'), + }, + { + what: 'a slots that is a string', + corrupt: setValue('{"body":"#null","slots":"ko1"}'), + }, + ])( + 'reports a settled promise with $what rather than throwing', + ({ corrupt }) => { + 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], + }); + + corrupt(`${kpid}.value`); + + expect(kernelStore.auditRefCounts()).toStrictEqual([ + { + kref: koid, + stored: '1,1', + expected: '0,0', + holders: [], + }, + ]); + }, + ); it('reports counts that are too low', () => { const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.ts index 17f86f5456..5c88376bf7 100644 --- a/packages/ocap-kernel/src/store/methods/refcount-audit.ts +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -104,7 +104,14 @@ export function getRefCountAuditMethods(ctx: StoreContext) { return []; } try { - return (JSON.parse(raw) as CapData).slots; + // `Array.isArray` rather than a cast asserting `slots` is there: a row + // that parses but carries no slots array would otherwise return + // `undefined` for the caller to iterate, throwing outside this `try` — + // the very crash this exists to prevent — and a row whose `slots` is a + // string would be iterated character by character, crediting krefs that + // were never there. A wrong audit is worse than a loud one. + const { slots } = JSON.parse(raw) as Partial>; + return Array.isArray(slots) ? slots : []; } catch { return []; }