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-browser-runtime/CHANGELOG.md b/packages/kernel-browser-runtime/CHANGELOG.md index e101fdcb30..31e6f07c54 100644 --- a/packages/kernel-browser-runtime/CHANGELOG.md +++ b/packages/kernel-browser-runtime/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- The kernel worker gives the kernel store a logger, so the wasm SQLite driver's diagnostics — including the transaction aborts it reports on the kernel's dying path — reach the log rather than nowhere ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021)) - Process platform-services RPC request handlers in the background so a request handler that fires a reentrant outbound RPC (e.g. transport handshake calling back into the kernel) cannot deadlock waiting for its response ([#948](https://github.com/MetaMask/ocap-kernel/pull/948)) ## [0.6.0] diff --git a/packages/kernel-browser-runtime/src/kernel-worker/kernel-worker.ts b/packages/kernel-browser-runtime/src/kernel-worker/kernel-worker.ts index 832a57a906..afffa24e32 100644 --- a/packages/kernel-browser-runtime/src/kernel-worker/kernel-worker.ts +++ b/packages/kernel-browser-runtime/src/kernel-worker/kernel-worker.ts @@ -44,7 +44,10 @@ async function main(): Promise { isJsonRpcMessage, ), PlatformServicesClient.make(globalThis as PostMessageTarget), - makeSQLKernelDatabase({ dbFilename: DB_FILENAME }), + makeSQLKernelDatabase({ + dbFilename: DB_FILENAME, + logger: logger.subLogger({ tags: ['kernel-store'] }), + }), ]); setupConsoleForwarding({ diff --git a/packages/kernel-node-runtime/CHANGELOG.md b/packages/kernel-node-runtime/CHANGELOG.md index 5a75a45781..54296e31c0 100644 --- a/packages/kernel-node-runtime/CHANGELOG.md +++ b/packages/kernel-node-runtime/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `makeKernel` gives the kernel store a logger, so the SQLite driver's diagnostics — including the transaction aborts it reports on the kernel's dying path — reach the log rather than nowhere ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021)) - The RPC socket server refuses to bind a Unix socket that has a live listener, rather than unlinking it and orphaning the previous owner; stale socket files with no listener are still cleaned up automatically ([#952](https://github.com/MetaMask/ocap-kernel/pull/952)) ## [0.1.0] diff --git a/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts b/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts index 57b0293d65..672313a02a 100644 --- a/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts +++ b/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts @@ -1,3 +1,5 @@ +import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; +import { Logger } from '@metamask/logger'; import { Kernel } from '@metamask/ocap-kernel'; import { describe, expect, it, vi } from 'vitest'; @@ -8,7 +10,9 @@ vi.mock('@metamask/kernel-store/sqlite/nodejs', async () => { '../../../ocap-kernel/test/storage.ts' ); return { - makeSQLKernelDatabase: makeMapKernelDatabase, + // Wrapped so that a test can see what the database was constructed with, + // while still getting a real store back. + makeSQLKernelDatabase: vi.fn(makeMapKernelDatabase), }; }); @@ -18,4 +22,19 @@ describe('makeKernel', () => { expect(kernel).toBeInstanceOf(Kernel); }); + + // FAILING REPRO. + // + // The kernel store is the only collaborator `makeKernel` builds without + // handing it a logger, so every `logger?.` call inside the SQLite driver is + // dead code in production — including the four abort failures #1012 added + // logging for. `kernel-worker.ts` omits it too, which keeps the wasm driver's + // pair dead even once this passes. + it('gives the kernel store a logger', async () => { + await makeKernel({}); + + expect(vi.mocked(makeSQLKernelDatabase)).toHaveBeenCalledWith( + expect.objectContaining({ logger: expect.any(Logger) }), + ); + }); }); diff --git a/packages/kernel-node-runtime/src/kernel/make-kernel.ts b/packages/kernel-node-runtime/src/kernel/make-kernel.ts index 901e79982a..529eb7af2d 100644 --- a/packages/kernel-node-runtime/src/kernel/make-kernel.ts +++ b/packages/kernel-node-runtime/src/kernel/make-kernel.ts @@ -60,7 +60,10 @@ export async function makeKernel({ }); // Initialize kernel store. - const kernelDatabase = await makeSQLKernelDatabase({ dbFilename }); + const kernelDatabase = await makeSQLKernelDatabase({ + dbFilename, + logger: rootLogger.subLogger({ tags: ['kernel-store'] }), + }); // Create and start kernel. const kernel = await Kernel.make(platformServicesClient, kernelDatabase, { diff --git a/packages/kernel-store/CHANGELOG.md b/packages/kernel-store/CHANGELOG.md index 5cfe39eb5e..df9b23b671 100644 --- a/packages/kernel-store/CHANGELOG.md +++ b/packages/kernel-store/CHANGELOG.md @@ -12,6 +12,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `rollbackSavepoint` discards the enclosing transaction when `ROLLBACK TO` itself fails, instead of leaving the savepoint on its stack and the transaction open ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) - Nothing would ever commit or abort that transaction, so every later write on the connection silently joined it, reported success, and vanished on close. Discarding it is no wider than the caller asked for: the transaction begins with the outermost savepoint, so it holds only the work the rollback was abandoning - The rollback failure is still what gets thrown, even if aborting the transaction fails too +- `releaseSavepoint` discards the enclosing transaction when `RELEASE` fails, as `rollbackSavepoint` already did for `ROLLBACK TO` ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021)) + - Otherwise the savepoint stayed on the stack and the transaction open with nothing left to commit or abort it. The release failure is still what gets thrown +- The wasm driver clears `_inTx` when aborting or committing a transaction throws, instead of believing it is still in one ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021)) + - Left true, `beginIfNeeded` became a permanent no-op and later writes autocommitted — including the next savepoint, which was then created bare, where its `RELEASE` commits and no rollback could undo the delivery. The nodejs driver reads `db.inTransaction` and was never affected +- An abort that fails while recovering from a failed savepoint operation is now logged in both drivers ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021)) ## [0.6.0] diff --git a/packages/kernel-store/src/sqlite/nodejs.test.ts b/packages/kernel-store/src/sqlite/nodejs.test.ts index a62392fe19..270f018ae6 100644 --- a/packages/kernel-store/src/sqlite/nodejs.test.ts +++ b/packages/kernel-store/src/sqlite/nodejs.test.ts @@ -360,6 +360,45 @@ describe('makeSQLKernelDatabase', () => { expect(mockDb._spStack).toStrictEqual([]); }); + // The hazard `rollbackSavepoint` guards against, by the other door. + it('releaseSavepoint discards the transaction when the release fails', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb.inTransaction = true; + mockDb._spStack = ['point1']; + mockStatement.run.mockClear(); + mockDb.exec.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + + expect(() => db.releaseSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + expect(mockDb._spStack).toStrictEqual([]); + // The abort is the only prepared statement this path runs. + expect(mockStatement.run).toHaveBeenCalledOnce(); + mockDb.inTransaction = false; + }); + + it('releaseSavepoint reports the release failure even if the abort fails too', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb.inTransaction = true; + mockDb._spStack = ['point1']; + mockDb.exec.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + mockStatement.run.mockImplementationOnce(() => { + throw new Error('cannot rollback'); + }); + + expect(() => db.releaseSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + expect(mockDb._spStack).toStrictEqual([]); + mockDb.inTransaction = false; + }); + it('supports nested savepoints', async () => { const db = await makeSQLKernelDatabase({}); db.createSavepoint('outer'); diff --git a/packages/kernel-store/src/sqlite/nodejs.ts b/packages/kernel-store/src/sqlite/nodejs.ts index ec863edc7c..fa754bb88f 100644 --- a/packages/kernel-store/src/sqlite/nodejs.ts +++ b/packages/kernel-store/src/sqlite/nodejs.ts @@ -298,8 +298,14 @@ export async function makeSQLKernelDatabase({ db._spStack.length = 0; try { rollbackIfNeeded(); - } catch { - // The rollback failure below is the one worth reporting. + } catch (abortError) { + // The rollback failure below is the one worth reporting, but a failed + // abort leaves SQLite holding a transaction the next crank would + // silently write into. Nothing here can repair that. + logger?.error( + 'failed to discard transaction after rollback', + abortError, + ); } throw error; } @@ -321,7 +327,28 @@ export async function makeSQLKernelDatabase({ throw new Error(`No such savepoint: ${name}`); } const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', name); - db.exec(query); + try { + db.exec(query); + } catch (error) { + // The hazard `rollbackSavepoint` guards against, by the other door: left as + // it was, the savepoint stays on the stack and the transaction open with + // nothing to ever commit or abort it, so every later write on this + // connection joins it, reports success, and vanishes on close. There is no + // committing this transaction now, so discard it. + db._spStack.length = 0; + try { + rollbackIfNeeded(); + } catch (abortError) { + // The release failure below is the one worth reporting, but a failed + // abort leaves SQLite holding a transaction the next crank would + // silently write into. Nothing here can repair that. + logger?.error( + 'failed to discard transaction after release', + abortError, + ); + } + throw error; + } db._spStack.splice(idx); if (db._spStack.length === 0) { commitIfNeeded(); diff --git a/packages/kernel-store/src/sqlite/wasm.test.ts b/packages/kernel-store/src/sqlite/wasm.test.ts index 2cbc96d658..1cf496dc95 100644 --- a/packages/kernel-store/src/sqlite/wasm.test.ts +++ b/packages/kernel-store/src/sqlite/wasm.test.ts @@ -488,7 +488,6 @@ describe('makeSQLKernelDatabase', () => { ); expect(mockDb._spStack).toStrictEqual([]); - mockDb._inTx = false; }); it('releaseSavepoint validates savepoint exists', async () => { @@ -518,6 +517,126 @@ describe('makeSQLKernelDatabase', () => { expect(mockDb._inTx).toBe(false); }); + // The hazard `rollbackSavepoint` guards against, by the other door. + it('releaseSavepoint discards the transaction when the release fails', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + mockDb.exec.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + + expect(() => db.releaseSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + expect(mockDb._spStack).toStrictEqual([]); + expect(mockDb._inTx).toBe(false); + }); + + it('releaseSavepoint reports the release failure even if the abort fails too', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + mockDb.exec.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + mockStatement.step.mockImplementationOnce(() => { + throw new Error('cannot rollback'); + }); + + expect(() => db.releaseSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + expect(mockDb._inTx).toBe(false); + }); + + // A failed abort is the one case that can leave `_inTx` disagreeing with the + // database. Left true, `beginIfNeeded` is a no-op forever after. + it('stops believing it is in a transaction when the abort fails too', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + mockDb.exec.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + mockStatement.step.mockImplementationOnce(() => { + throw new Error('cannot rollback'); + }); + + expect(() => db.rollbackSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + expect(mockDb._inTx).toBe(false); + }); + + // Why that matters: a savepoint created outside a transaction commits when + // released (Agoric/agoric-sdk#8423), so an aborted crank would keep its + // writes. + it('begins a transaction for the next savepoint after a failed abort', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + mockDb.exec.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + mockStatement.step.mockImplementationOnce(() => { + throw new Error('cannot rollback'); + }); + expect(() => db.rollbackSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + mockDb.exec.mockClear(); + mockStatement.step.mockClear(); + db.createSavepoint('next'); + + // BEGIN is the only prepared statement `createSavepoint` runs; the + // SAVEPOINT itself goes through `exec`. + expect(mockStatement.step).toHaveBeenCalledOnce(); + expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT next'); + }); + + // FAILING REPRO. + // + // `commitIfNeeded` still steps the COMMIT before clearing `_inTx`, the exact + // ordering `rollbackIfNeeded` was corrected to avoid. A COMMIT that throws + // therefore leaves `_inTx` true against a database that may hold no + // transaction, `beginIfNeeded` is a no-op forever after, and the next + // savepoint is created outside a transaction — which commits when released + // (Agoric/agoric-sdk#8423). This is the crank's commit point, so the writes + // that leak are a whole crank's. + // + // A failed abort is therefore not, as the abort case above claims, the one + // case that can leave `_inTx` disagreeing with the database. This is the + // second. + it('stops believing it is in a transaction when the commit fails', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + // The RELEASE goes through `exec` and succeeds; COMMIT is the first + // prepared statement this path steps, and it is what fails. + mockStatement.step.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + + expect(() => db.releaseSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + expect(mockDb._inTx).toBe(false); + + // And so the next savepoint gets a transaction of its own rather than + // being created bare. + mockDb.exec.mockClear(); + mockStatement.step.mockClear(); + db.createSavepoint('next'); + expect(mockStatement.step).toHaveBeenCalledOnce(); + expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT next'); + }); + it('supports nested savepoints', async () => { const db = await makeSQLKernelDatabase({}); db.createSavepoint('outer'); diff --git a/packages/kernel-store/src/sqlite/wasm.ts b/packages/kernel-store/src/sqlite/wasm.ts index c0c32b8a72..a047d6cbcf 100644 --- a/packages/kernel-store/src/sqlite/wasm.ts +++ b/packages/kernel-store/src/sqlite/wasm.ts @@ -199,9 +199,13 @@ export async function makeSQLKernelDatabase({ */ function commitIfNeeded(): void { if (db._inTx && db._spStack.length === 0) { + // Cleared before the commit is attempted, for the reason `rollbackIfNeeded` + // gives: a throwing COMMIT would otherwise wedge `_inTx` true, and every + // later savepoint would be created bare — where its RELEASE commits + // (Agoric/agoric-sdk#8423) and no rollback can undo the delivery. + db._inTx = false; sqlCommitTransaction.step(); sqlCommitTransaction.reset(); - db._inTx = false; } } @@ -210,10 +214,16 @@ export async function makeSQLKernelDatabase({ */ function rollbackIfNeeded(): void { if (db._inTx) { - sqlAbortTransaction.step(); - sqlAbortTransaction.reset(); + // Cleared before the abort is attempted, because the abort can throw and + // `_inTx` is tracked here rather than read from SQLite as the nodejs driver + // does. Left true, `beginIfNeeded` is a no-op forever after and writes + // autocommit one statement at a time (see `createSavepoint`). Cleared, a + // still-open transaction surfaces as a failed `BEGIN` — the louder + // failure. db._inTx = false; db._spStack.length = 0; + sqlAbortTransaction.step(); + sqlAbortTransaction.reset(); } } @@ -380,8 +390,14 @@ export async function makeSQLKernelDatabase({ db._spStack.length = 0; try { rollbackIfNeeded(); - } catch { - // The rollback failure below is the one worth reporting. + } catch (abortError) { + // The rollback failure below is the one worth reporting. The next + // `BEGIN` will fail if SQLite really is still in a transaction, but that + // is a crank away and this is where the evidence is. + logger?.error( + 'failed to discard transaction after rollback', + abortError, + ); } throw error; } @@ -403,7 +419,28 @@ export async function makeSQLKernelDatabase({ throw new Error(`No such savepoint: ${name}`); } const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', name); - db.exec(query); + try { + db.exec(query); + } catch (error) { + // The hazard `rollbackSavepoint` guards against, by the other door: left as + // it was, the savepoint stays on the stack and the transaction open with + // nothing to ever commit or abort it, so every later write on this + // connection joins it, reports success, and vanishes on close. There is no + // committing this transaction now, so discard it. + db._spStack.length = 0; + try { + rollbackIfNeeded(); + } catch (abortError) { + // The release failure below is the one worth reporting. The next + // `BEGIN` will fail if SQLite really is still in a transaction, but that + // is a crank away and this is where the evidence is. + logger?.error( + 'failed to discard transaction after release', + abortError, + ); + } + throw error; + } db._spStack.splice(idx); if (db._spStack.length === 0) { commitIfNeeded(); diff --git a/packages/kernel-test/src/crank-rollback.test.ts b/packages/kernel-test/src/crank-rollback.test.ts index cd2a708aa0..36131a8a16 100644 --- a/packages/kernel-test/src/crank-rollback.test.ts +++ b/packages/kernel-test/src/crank-rollback.test.ts @@ -138,6 +138,106 @@ describe('crank rollback against a real database', () => { expect(kdb.kernelKVStore.get('second')).toBe('yes'); }); + // An aborted crank still owes work after the rollback — terminating the vat, + // collecting garbage — whose writes have to survive it. + it('keeps the writes a crank makes after rolling its delivery back', async () => { + const { kernelStore, kdb } = await makeStore(); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('crank'); + kernelStore.createCrankSavepoint('delivery'); + kdb.kernelKVStore.set('delivered', 'yes'); + + kernelStore.rollbackCrank('delivery'); + kdb.kernelKVStore.set('terminated', 'yes'); + kernelStore.endCrank(); + + expect(kdb.kernelKVStore.get('delivered')).toBeUndefined(); + expect(kdb.kernelKVStore.get('terminated')).toBe('yes'); + }); + + // And survive it *inside the crank's transaction*, not as autocommitted + // statements. Rolling back `crank` is the only way to observe that from here; + // the run loop never does it. + it('holds those writes in the transaction rather than autocommitting them', async () => { + const { kernelStore, kdb } = await makeStore(); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('crank'); + kernelStore.createCrankSavepoint('delivery'); + kernelStore.rollbackCrank('delivery'); + kdb.kernelKVStore.set('terminated', 'yes'); + + kernelStore.rollbackCrank('crank'); + kernelStore.endCrank(); + + expect(kdb.kernelKVStore.get('terminated')).toBeUndefined(); + }); + + // Every `provideCachedStoredValue` keeps its value in a closure and writes + // through to kv, so a rollback that only reverts the database leaves the cache + // holding the abandoned crank's value — and the next `set` persists it. The GC + // action set is the case that matters: `processGCActionSet` consumes an action + // before delivering it, so losing the rollback loses the action outright. + it('restores the GC action set consumed by a rolled-back crank', async () => { + const { kernelStore } = await makeStore(); + kernelStore.addGCActions(['v1 dropExport ko1']); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('crank'); + kernelStore.createCrankSavepoint('delivery'); + // Consume the action the way `processGCActionSet` does. + kernelStore.setGCActions(new Set()); + + kernelStore.rollbackCrank('delivery'); + kernelStore.endCrank(); + + expect([...kernelStore.getGCActions()]).toStrictEqual([ + 'v1 dropExport ko1', + ]); + }); + + // Same closure, same failure: a reap scheduled and then consumed by a crank + // that rolls back must still be pending afterwards. + it('restores the reap queue consumed by a rolled-back crank', async () => { + const { kernelStore } = await makeStore(); + kernelStore.scheduleReap('v1'); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('crank'); + kernelStore.createCrankSavepoint('delivery'); + expect(kernelStore.nextReapAction()).toBeDefined(); + + kernelStore.rollbackCrank('delivery'); + kernelStore.endCrank(); + + expect(kernelStore.nextReapAction()).toBeDefined(); + }); + + // `maybeFreeKrefs` is RAM-only, so nothing rolls it back. Left populated, the + // next crank's `collectGarbage` visits krefs whose decrements were undone — + // and `getKernelPromise` throws outright for one the rollback deleted, which + // kills the run loop. + it('discards GC candidates accumulated by a rolled-back crank', async () => { + const { kernelStore } = await makeStore(); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('crank'); + kernelStore.createCrankSavepoint('delivery'); + // Born at 1, so this drops it to 0 and leaves `kpid` in `maybeFreeKrefs` + // while the rollback removes the promise record it names. + const kpid = kernelStore.initKernelPromise()[0]; + kernelStore.decrementRefCount(kpid, 'test'); + + kernelStore.rollbackCrank('delivery'); + kernelStore.endCrank(); + + kernelStore.startCrank(); + kernelStore.createCrankSavepoint('crank'); + expect(() => kernelStore.collectGarbage()).not.toThrow(); + kernelStore.endCrank(); + }); + // `createCrankSavepoint` records the name only once the database has the // savepoint. Asking to roll back one that was never created must therefore say // so, rather than releasing someone else's savepoint. diff --git a/packages/kernel-test/src/garbage-collection.test.ts b/packages/kernel-test/src/garbage-collection.test.ts index 268ed7920e..44345bfcdd 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]); @@ -105,6 +117,29 @@ describe('Garbage Collection', () => { expect(parseReplyBody(useResult.body)).toBe(objectId); }); + /** + * Reap the importer vat until the kernel's bookkeeping catches up with the + * vat's own garbage collection, or the attempts run out. + * + * `bringOutYourDead` reports an import as dropped only once the engine has + * collected the vat's presence and run its finalizer, which `gcAndFinalize` + * does not guarantee on the first attempt. Each attempt needs its own reap — + * `nextReapAction` shifts the one scheduled entry off, so cranking again finds + * nothing to do — plus a message to wake the run loop and consume it. + * + * Gives up after five attempts; the caller's assertion reports the failure. + * + * @param settled - Whether the state under test has arrived yet. + */ + async function reapImporterUntil(settled: () => boolean): Promise { + const isImporter = (vatId: VatId): boolean => vatId === importerVatId; + for (let attempt = 0; attempt < 5 && !settled(); attempt += 1) { + kernel.reapVats(isImporter); + await kernel.queueMessage(importerKRef, 'noop', []); + await waitUntilQuiescent(500); + } + } + it('should trigger GC syscalls through bringOutYourDead', async () => { // Create an object in the exporter vat with a known ID const objectId = 'test-object'; @@ -116,10 +151,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); @@ -149,14 +184,10 @@ describe('Garbage Collection', () => { await kernel.queueMessage(importerKRef, 'makeWeak', [objectId]); await waitUntilQuiescent(); - // Schedule reap to trigger bringOutYourDead on next crank - kernel.reapVats((vatId) => vatId === importerVatId); - - // Run 3 cranks to allow bringOutYourDead to be processed - for (let i = 0; i < 3; i++) { - await kernel.queueMessage(importerKRef, 'noop', []); - await waitUntilQuiescent(500); - } + // Reap until the importer reports the drop + await reapImporterUntil( + () => kernelStore.getObjectRefCount(createObjectRef).reachable === 1, + ); // Check reference counts after dropImports const afterWeakRefCounts = kernelStore.getObjectRefCount(createObjectRef); @@ -168,13 +199,10 @@ describe('Garbage Collection', () => { await kernel.queueMessage(importerKRef, 'forgetImport', []); await waitUntilQuiescent(); - // Schedule another reap - kernel.reapVats((vatId) => vatId === importerVatId); - - for (let i = 0; i < 3; i++) { - await kernel.queueMessage(importerKRef, 'noop', []); - await waitUntilQuiescent(500); - } + // Reap until the importer reports the retirement + await reapImporterUntil( + () => kernelStore.getObjectRefCount(createObjectRef).recognizable === 1, + ); // Check reference counts after retireImports const afterForgetRefCounts = kernelStore.getObjectRefCount(createObjectRef); @@ -201,4 +229,129 @@ describe('Garbage Collection', () => { ); expect(parseReplyBody(exporterFinalCheck.body)).toBe(false); }, 40000); + + describe('an object shared by two importers', () => { + let secondImporterKRef: KRef; + let secondImporterVatId: VatId; + + beforeEach(async () => { + kernelDatabase = await makeSQLKernelDatabase({ dbFilename: ':memory:' }); + kernelStore = makeKernelStore(kernelDatabase); + kernel = await makeKernel(kernelDatabase, true, makeMockLogger()); + await runTestVats(kernel, makeTestSubcluster(['Importer2'])); + + const vats = kernel.getVats(); + const idOf = (name: string): VatId => + vats.find((row) => row.config.parameters?.name === name)?.id as VatId; + exporterVatId = idOf('Exporter'); + importerVatId = idOf('Importer'); + secondImporterVatId = idOf('Importer2'); + exporterKRef = kernelStore.getRootObject(exporterVatId) as KRef; + importerKRef = kernelStore.getRootObject(importerVatId) as KRef; + secondImporterKRef = kernelStore.getRootObject( + secondImporterVatId, + ) as KRef; + }); + + /** + * Give an importer a chance to notice a dropped object and tell the kernel, + * then keep cranking until the resulting GC actions have all been consumed. + * + * @param vatId - The vat to reap. + * @param rootKRef - That vat's root, to poke with cranks afterwards. + */ + async function reapAndSettle(vatId: VatId, rootKRef: KRef): Promise { + kernel.reapVats((id) => id === vatId); + // BOYD has to reach the vat, the vat has to answer, and the kernel has to + // act on the answer — but a round can queue more work, so loop until the + // queue is actually empty rather than guessing at a crank count. + const maxRounds = 10; + for (let round = 0; round < maxRounds; round++) { + await kernel.queueMessage(rootKRef, 'noop', []); + await waitUntilQuiescent(500); + if ([...kernelStore.getGCActions()].length === 0) { + return; + } + } + throw Error( + `GC actions still pending after ${maxRounds} rounds: ${[ + ...kernelStore.getGCActions(), + ].join(', ')}`, + ); + } + + it('survives until both importers let go', async () => { + const objectId = 'shared-object'; + const createObjectData = await kernel.queueMessage( + exporterKRef, + 'createObject', + [objectId], + ); + const sharedKRef = createObjectData.slots[0] as KRef; + const objectRef = kunser(createObjectData); + + for (const importer of [importerKRef, secondImporterKRef]) { + await kernel.queueMessage(importer, 'storeImport', [ + objectRef, + objectId, + ]); + } + await waitUntilQuiescent(); + + expect(kernelStore.getImporters(sharedKRef)).toStrictEqual( + [importerVatId, secondImporterVatId].sort(), + ); + // Two importers, plus the resolved createObject promise whose value + // still carries the slot + expect(kernelStore.getObjectRefCount(sharedKRef)).toStrictEqual({ + reachable: 3, + recognizable: 3, + }); + + await kernel.queueMessage(importerKRef, 'makeWeak', [objectId]); + await kernel.queueMessage(importerKRef, 'forgetImport', []); + await waitUntilQuiescent(); + await reapAndSettle(importerVatId, importerKRef); + + // The exporter must not have been told to drop it: the second importer + // legitimately still holds it + expect(kernelStore.getReachableFlag(exporterVatId, sharedKRef)).toBe( + true, + ); + expect(kernelStore.getImporters(sharedKRef)).toStrictEqual([ + secondImporterVatId, + ]); + expect( + parseReplyBody( + ( + await kernel.queueMessage(exporterKRef, 'isObjectPresent', [ + objectId, + ]) + ).body, + ), + ).toBe(true); + + expect( + parseReplyBody( + ( + await kernel.queueMessage(secondImporterKRef, 'useImport', [ + objectId, + ]) + ).body, + ), + ).toBe(objectId); + + await kernel.queueMessage(secondImporterKRef, 'makeWeak', [objectId]); + await kernel.queueMessage(secondImporterKRef, 'forgetImport', []); + await waitUntilQuiescent(); + await reapAndSettle(secondImporterVatId, secondImporterKRef); + + expect(kernelStore.getImporters(sharedKRef)).toStrictEqual([]); + // Only the createObject result's stored value still names it + expect(kernelStore.getObjectRefCount(sharedKRef)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }, 60000); + }); }); diff --git a/packages/kernel-test/src/persistence.test.ts b/packages/kernel-test/src/persistence.test.ts index 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/refcount-audit.test.ts b/packages/kernel-test/src/refcount-audit.test.ts new file mode 100644 index 0000000000..3ab09fdba6 --- /dev/null +++ b/packages/kernel-test/src/refcount-audit.test.ts @@ -0,0 +1,63 @@ +import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; +import { makeKernelStore } from '@metamask/ocap-kernel'; +import type { KRef, VatId } from '@metamask/ocap-kernel'; +import { expect, describe, it } from 'vitest'; + +import { + getBundleSpec, + makeKernel, + makeMockLogger, + runTestVats, +} from './utils.ts'; + +/** + * The per-crank audit throws from inside the run loop, which nothing restarts. + * Unless that failure is reported to whoever is waiting on the kernel, the only + * symptom is a test that hangs until its timeout, with no mention of reference + * counts anywhere — which would make the audit worthless as a build gate. + */ +describe('reference count audit', () => { + it('reports a violation to kernel callers rather than hanging', async () => { + const kernelDatabase = await makeSQLKernelDatabase({ + dbFilename: ':memory:', + }); + const kernelStore = makeKernelStore(kernelDatabase); + const kernel = await makeKernel(kernelDatabase, true, makeMockLogger()); + await runTestVats(kernel, { + bootstrap: 'exporter', + forceReset: true, + vats: { + exporter: { + bundleSpec: getBundleSpec('exporter-vat'), + parameters: { name: 'Exporter' }, + }, + }, + }); + + const exporterVatId = kernel.getVats()[0]?.id as VatId; + const exporterKRef = kernelStore.getRootObject(exporterVatId) as KRef; + + kernelStore.setObjectRefCount(exporterKRef, { + reachable: 7, + recognizable: 9, + }); + + // The crank carrying this message settles its result before the + // end-of-crank audit runs, so this one may still succeed. + await kernel + .queueMessage(exporterKRef, 'createObject', ['x']) + .catch(() => undefined); + + // What a caller is told directly is that the run loop is gone; the audit + // failure that killed it rides along as the `cause`. That chain is the part + // that has to survive, since "run loop died" on its own names nothing. + const failure = (await kernel + .queueMessage(exporterKRef, 'createObject', ['y']) + .catch((error) => error)) as Error; + + expect(failure.message).toMatch(/Kernel run loop died/u); + expect(String(failure.cause)).toMatch( + /reference count invariant violated/u, + ); + }, 30000); +}); diff --git a/packages/kernel-test/src/utils.ts b/packages/kernel-test/src/utils.ts index c255347b89..7d867f2c28 100644 --- a/packages/kernel-test/src/utils.ts +++ b/packages/kernel-test/src/utils.ts @@ -93,6 +93,9 @@ export async function makeKernel( resetStorage, logger, keySeed, + // Refcount drift is invisible to ordinary assertions until something gets + // collected out from under a live holder, so check it every crank. + auditRefCounts: true, }); return kernel; } diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 8af7998ec7..86794f9103 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -32,10 +32,20 @@ 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 with no holder (an orphaned count). It compares counts against the holders it finds, so a holder that should have been torn down but wasn't justifies its own count and is not detectable this way + - Only references visible in the kernel's own state are checkable, so a holder that keeps a kref outside them has to take a pin to be counted at all + - `recomputeRefCounts` is a repair tool for a drifted store, offered to embedders and never run automatically: opening an existing store does not migrate it + - Exports the `RefCountViolation` type, a union over `kind: 'mismatch' | 'dangling'` +- Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- Add `getOcapURLObjects` and `retainForOcapURL` to the kernel store, and `VatManager.releaseVatRootPin` ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- Add `orphanKernelObject` to the kernel store, which drops an object's owner mapping and hands it to the collector ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) + ### Changed - **BREAKING:** `Kernel.make`'s `ioChannelFactory` option is now `ioListenerFactory`, and the exported `IOChannelFactory` type is replaced by `IOListener` and `IOListenerFactory`. A cluster config's `io` entries now create listeners; vats call `accept()` to obtain a channel instead of reading and writing the endowment directly ([#1007](https://github.com/MetaMask/ocap-kernel/pull/1007)) @@ -49,7 +59,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)) @@ -57,6 +67,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Roll back the crank the run loop died in instead of committing it, so a restart resumes from a consistent boundary ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) - Because the killing item is no longer consumed, a restart re-dequeues it; an item that reliably kills a crank needs `clearState`/`reset` rather than a restart - Store state only — a crank that had already flushed its buffer settled JS-side subscriptions irreversibly +- Keep a crank's store work inside one transaction ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021)) + - A crank now takes two savepoints, `crank` and `delivery`, and rolls back only `delivery`. Rolling back the outermost one ends the transaction, so terminating the vat and collecting garbage — which follow the rollback and must survive it — were autocommitting a statement at a time + - Buffered vat outputs are flushed after that work rather than before it, so a later failure can no longer roll the crank back underneath an answer already given. Termination still settles the dying vat's own promises immediately +- A failing `endCrank` no longer replaces the error that killed the run loop; it is reported with that error as its `cause`, as the rollback path already did ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021)) + - Now that the delivery rollback spares the `crank` savepoint, `endCrank`'s release is a real RELEASE and COMMIT on the dying path where it used to be a no-op — and the run loop called it from a bare `finally` +- A failing savepoint rollback in `RemoteHandle.handleRemoteMessage` and `RemoteManager`'s incarnation change is logged rather than thrown, so the failure it was cleaning up after is what reaches the caller ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021)) + - Both release inside the `try` and roll back in the `catch`, so once a failed RELEASE discarded the whole savepoint stack, the rollback reported a savepoint that no longer existed in place of the real error. The rollback is still attempted: a release that failed for a reason of its own may well have left the savepoint standing +- Forget a crank's savepoints when the store call that ends them fails, in both `rollbackCrank` and `endCrank` ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021)) + - A failed rollback or release discards the whole transaction, savepoints included. Keeping them listed had `endCrank` throw `No such savepoint: t0` from the run loop's `finally`, over whatever really killed the kernel +- `rollbackCrank` now also reverts the two pieces of state a database rollback cannot reach, whether or not the rollback itself succeeded: every cached stored value is re-read, and the crank's accumulated garbage-collection candidates are discarded ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021)) + - A cached stored value answers reads from a closure and only writes through to the database, so reverting the database left it holding the abandoned crank's value and the next write persisted that. `processGCActionSet` takes an action out of the set before delivering it, so an aborted delivery lost the action outright instead of retrying it + - `maybeFreeKrefs` lives in RAM, so nothing reverted it either. Its entries are collection candidates only because of decrements the rollback undid, and a later `collectGarbage` threw outright on a promise the rollback had deleted, killing the run loop + - A failed rollback discards the whole transaction, moving the database back at least as far as a successful rollback would have — so reverting only on success left exactly the state that is least able to tolerate it - Refuse inbound remote deliveries once the run loop is dead, rolling back without acknowledging them, so the peer retries and gives up instead of waiting on a kernel that will never deliver ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) - Covers `bringOutYourDead` as well as `message` and `notify`: a reap is queue work too, consumed only by the run loop. The remaining GC arms need no guard, since they only touch refcounts - Refuse `launchSubcluster` once the run loop is dead ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) @@ -65,6 +88,39 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Report the database error when an aborted crank cannot be rolled back, instead of a spurious "no such savepoint" ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) - The abort path recorded the rollback only after it succeeded, so a throwing rollback had the run loop try again against the savepoint `rollbackCrank` had already discarded. The second attempt's "no such savepoint" then became the reported cause of death — and since only `error.message` crosses the wire, the real failure reached neither `getStatus` nor the daemon log - Reject the run loop's promise with the same `Error` its status reports, rather than re-throwing a non-`Error` for the embedder to normalize a second time ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) +- **BREAKING:** Make c-list reference accounting symmetric: creating an import c-list entry now takes a reference, as tearing one down has always released one ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - `initKernelObject` now births objects at `(0, 0)` instead of `(1, 1)`. The old constant made the arithmetic come out right for exactly one importer, masking the missing increment; with two importers a live capability could be dropped and retired out from under a holder + - Removes the owner-side baseline decrements in `cleanupTerminatedVat` and `forgetEndpointImports`, which double-claimed the same unit an importer's drop also spent — the source of `"koNN" underflow -1,0` escaping mid-cleanup and leaving a vat half-cleaned + - `translateRefKtoE` now re-establishes reachability, so a vat handed an object it previously dropped is counted as holding it live again + - Renames `krefsToExistingErefs` to `krefsToErefs`, which now throws on an unmapped kref instead of silently dropping it +- Pin vat root objects for the lifetime of their vat, and release the pin on termination ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - A root is addressable while its vat lives whether or not anyone imports it; the old `(1, 1)` birth baseline was standing in for this +- Garbage-collection action delivery now moves the kernel's own c-list: `dropExports` clears the owner's reachable flag and `retireExports`/`retireImports` tear the entry down ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) + - Previously the owner's flag never cleared, so the same action could be re-derived, and retired entries outlived the objects they named +- Orphan a kernel object when its owner stops naming it (a delivered `retireExport`, or a `retireExports`/`abandonExports` syscall), so its `owner` and `refCount` records no longer outlive the c-list entry they were reachable through ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) + - They leaked, and the next collection to visit such a kref read a c-list entry that was no longer there and killed the run loop. Reproduces on `main`, so it predates this stack +- Reject a `retireExports`/`abandonExports` syscall for an object the calling endpoint does not own, instead of letting it erase another endpoint's claim to an object it is still exporting ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) + - Nothing upstream of `performExportCleanup` checked that the vref it was handed is even an export, and the audit could not see the damage, because an export entry carries no count +- Garbage-collection action delivery releases the kernel's side even when the endpoint has vanished, and rolls back rather than committing a release the endpoint was never told about ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) + - It releases only where the endpoint is genuinely gone: a terminated vat, whose cleanup tears the whole c-list down anyway, or a remote, which reconciles on its next incarnation. A vat that is absent yet not terminated is one `restartVat` has taken out of the kernel's reach while keeping its c-list, so the crank fails there instead of committing a release the returning incarnation would disagree with +- A failed garbage-collection delivery to a remote is logged and survived rather than escaping the crank and stopping the run loop ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) +- Tear down and mark for cleanup a vat whose worker launched but whose kernel-side registration then failed ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022)) +- Charge a delivered message's target reference against the run-queue item's own target rather than the routed target, so a message routed through a resolved promise no longer decrements an object nobody charged while leaking the promise ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- Release a queued notification's reference before the paths that decide there is nothing to deliver, and stop decrementing references on promises retired alongside it that nobody had taken ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- Transfer, rather than duplicate, the references a message carries when it is queued on an unresolved promise and later re-enqueued on resolution ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020)) +- 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 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, 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)) + + - `deleteSubcluster` bypasses `stopVat`, so nothing released the pin `launchVat` took in the incarnation that did run them, leaving the root's count permanently above zero and `pinnedObjects` naming a vat that no longer exists + - Deserialize CapData rejections in `Kernel.queueMessage` so vat errors surface as plain `Error` objects to all callers ([#928](https://github.com/MetaMask/ocap-kernel/pull/928)) - Detect peer restart across receiver state loss so the receiving kernel no longer silently drops a restarted peer's `seq=1` messages ([#948](https://github.com/MetaMask/ocap-kernel/pull/948)) - Persist the peer's last-observed incarnation and compare it on every successful handshake; on a detected restart, clear the peer's c-list contributions and reject the promises it was deciding before the new incarnation reuses any erefs diff --git a/packages/ocap-kernel/src/Kernel.ts b/packages/ocap-kernel/src/Kernel.ts index a312e60e65..f615ea6cbf 100644 --- a/packages/ocap-kernel/src/Kernel.ts +++ b/packages/ocap-kernel/src/Kernel.ts @@ -109,6 +109,14 @@ export class Kernel { * @param options.ioListenerFactory - Optional factory for creating IO listeners. * @param options.allowedGlobalNames - Optional list of allowed global names for vat endowments. * @param options.onRunLoopFailure - Optional handler called if the run loop dies. + * @param options.auditRefCounts - If true, verify every kref's reference + * counts against the references the kernel actually holds at the end of each + * crank, and throw on any mismatch. This is the check standing in for the + * accounting invariant `collectGarbage` still cannot assert (see the comment + * on its `retireExport` branch), so it is not optional + * instrumentation: it is off by default only because it walks the whole store + * every crank. Any kernel whose accounting is under test wants it on, and + * every kernel `kernel-test` builds enables it. */ // eslint-disable-next-line no-restricted-syntax private constructor( @@ -122,6 +130,7 @@ export class Kernel { ioListenerFactory?: IOListenerFactory; allowedGlobalNames?: AllowedGlobalName[]; onRunLoopFailure?: OnRunLoopFailure; + auditRefCounts?: boolean; } = {}, ) { this.#platformServices = platformServices; @@ -129,6 +138,9 @@ export class Kernel { this.#onRunLoopFailure = options.onRunLoopFailure; this.#logger = options.logger ?? new Logger('ocap-kernel'); this.#kernelStore = makeKernelStore(kernelDatabase, this.#logger); + if (options.auditRefCounts) { + this.#kernelStore.setRefCountAuditing(true); + } if (!this.#kernelStore.isInitialized()) { this.#kernelStore.markInitialized(); } @@ -168,6 +180,7 @@ export class Kernel { this.#ocapURLManager = new OcapURLManager({ remoteManager: this.#remoteManager, + kernelStore: this.#kernelStore, }); this.#kernelServiceManager = new KernelServiceManager({ @@ -249,6 +262,8 @@ export class Kernel { * @param options.systemSubclusters - Optional array of system subcluster configurations. * @param options.allowedGlobalNames - Optional list of allowed global names for vat endowments. When set, only these names from the `VatSupervisor`'s configured endowments (see `createDefaultEndowments`) are available to vats. * @param options.onRunLoopFailure - Optional handler called if the run loop dies. The kernel must be restarted after that, so an embedder that outlives it (e.g. a daemon) should use this to terminate or restart. + * @param options.auditRefCounts - If true, verify reference counts against + * ground truth at the end of each crank and throw on any mismatch. * @returns A promise for the new kernel instance. */ static async make( @@ -263,6 +278,7 @@ export class Kernel { systemSubclusters?: SystemSubclusterConfig[]; allowedGlobalNames?: AllowedGlobalName[]; onRunLoopFailure?: OnRunLoopFailure; + auditRefCounts?: boolean; } = {}, ): Promise { const kernel = new Kernel(platformServices, kernelDatabase, options); diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index d7beb0e2f1..9e4de322eb 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -25,6 +25,24 @@ vi.mock('@endo/promise-kit', () => ({ */ const STOP_RUN_LOOP = 'test: stop run loop'; +/** + * Collect an error and every error reachable through its `cause` chain, so that + * a test can assert a root cause survived without pinning how its reporter + * chose to wrap it. + * + * @param error - The error to walk. + * @returns The chain, outermost first. + */ +const causeChain = (error: unknown): Error[] => { + const chain: Error[] = []; + let current = error; + while (current instanceof Error) { + chain.push(current); + current = current.cause; + } + return chain; +}; + describe('KernelQueue', () => { let kernelStore: KernelStore; let kernelQueue: KernelQueue; @@ -44,6 +62,7 @@ describe('KernelQueue', () => { kernelStore = { nextTerminatedVatCleanup: vi.fn(), collectGarbage: vi.fn(), + assertRefCountsIfAuditing: vi.fn(), runQueueLength: vi.fn(), dequeueRun: vi.fn(), enqueueRun: vi.fn(), @@ -91,6 +110,23 @@ describe('KernelQueue', () => { }; }; + /** + * Stop the run loop by failing the *next* crank's start, so that the crank + * under test runs to completion. Throwing from one of a crank's own store calls + * cuts it short, which hides everything the crank does after that call. + */ + const stopAfterOneCrank = (): void => { + let cranks = 0; + (kernelStore.startCrank as unknown as MockInstance).mockImplementation( + () => { + cranks += 1; + if (cranks > 1) { + throw new Error(STOP_RUN_LOOP); + } + }, + ); + }; + /** * Run a single crank whose delivery blows up, killing the run loop. * @@ -127,7 +163,8 @@ describe('KernelQueue', () => { const deliver = vi.fn().mockRejectedValue(deliverError); await expect(kernelQueue.run(deliver)).rejects.toBe(deliverError); expect(kernelStore.startCrank).toHaveBeenCalled(); - expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('start'); + expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('crank'); + expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('delivery'); expect(processGCActionSetSpy).toHaveBeenCalled(); expect(kernelStore.nextReapAction).toHaveBeenCalled(); expect(kernelStore.nextTerminatedVatCleanup).toHaveBeenCalled(); @@ -155,9 +192,9 @@ describe('KernelQueue', () => { }); await expect(kernelQueue.run(deliver)).rejects.toThrow(STOP_RUN_LOOP); expect(kernelStore.startCrank).toHaveBeenCalled(); - expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('start'); + expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('delivery'); expect(deliver).toHaveBeenCalledWith(mockItem); - expect(kernelStore.rollbackCrank).toHaveBeenCalledWith('start'); + expect(kernelStore.rollbackCrank).toHaveBeenCalledWith('delivery'); expect(kernelStore.collectGarbage).toHaveBeenCalled(); expect(kernelStore.endCrank).toHaveBeenCalled(); }); @@ -194,6 +231,170 @@ describe('KernelQueue', () => { expect(kernelStore.collectGarbage).toHaveBeenCalled(); expect(kernelStore.endCrank).toHaveBeenCalled(); }); + + // Why the flush comes after the crank's fallible work: see + // `#processCrankResult`. Here the terminate is what fails. + it('answers no caller from a crank it then rolls back', async () => { + const mockItem: RunQueueItem = { + type: 'send', + target: 'ko123', + message: { result: 'kp1' } as KernelMessage, + }; + (kernelStore.runQueueLength as unknown as MockInstance) + .mockReturnValueOnce(1) + .mockReturnValue(0); + (kernelStore.dequeueRun as unknown as MockInstance).mockReturnValueOnce( + mockItem, + ); + + // A caller is awaiting this message's result. + const resolve = vi.fn(); + const reject = vi.fn(); + kernelQueue.subscriptions.set('kp1', { resolve, reject }); + + // The delivery succeeds and its result is there for the flush to hand over... + ( + kernelStore.flushCrankBuffer as unknown as MockInstance + ).mockReturnValueOnce([ + { type: 'notify', endpointId: 'v1', kpid: 'kp1' }, + ]); + (kernelStore.getKernelPromise as unknown as MockInstance).mockReturnValue( + { + state: 'fulfilled', + value: { body: '"answer"', slots: [] }, + }, + ); + + // ...but the crank still has fallible work left, and it dies there. + const terminationError = new Error('vat worker already gone'); + (terminateVat as unknown as MockInstance).mockRejectedValueOnce( + terminationError, + ); + const deliver = vi.fn().mockResolvedValue({ + terminate: { vatId: 'v1', info: { body: '"exit"', slots: [] } }, + }); + + await expect(kernelQueue.run(deliver)).rejects.toBe(terminationError); + + expect(kernelStore.rollbackCrank).toHaveBeenCalledWith('delivery'); + expect(resolve).not.toHaveBeenCalled(); + // Told the result will never come, rather than left waiting on a crank the + // store no longer has any record of. + expect(reject).toHaveBeenCalledWith( + expect.objectContaining({ cause: terminationError }), + ); + }); + + // The same invariant inside the flush: `#enqueueRun` is store work and can + // fail part-way, so no caller may be answered until all of it lands. + it('answers no caller until every buffered item is enqueued', async () => { + const mockItem: RunQueueItem = { + type: 'send', + target: 'ko123', + message: { result: 'kp1' } as KernelMessage, + }; + (kernelStore.runQueueLength as unknown as MockInstance) + .mockReturnValueOnce(1) + .mockReturnValue(0); + (kernelStore.dequeueRun as unknown as MockInstance).mockReturnValueOnce( + mockItem, + ); + + const resolve = vi.fn(); + const reject = vi.fn(); + kernelQueue.subscriptions.set('kp1', { resolve, reject }); + + // Two resolutions to hand over, the caller's first. + ( + kernelStore.flushCrankBuffer as unknown as MockInstance + ).mockReturnValueOnce([ + { type: 'notify', endpointId: 'v1', kpid: 'kp1' }, + { type: 'notify', endpointId: 'v2', kpid: 'kp2' }, + ]); + (kernelStore.getKernelPromise as unknown as MockInstance).mockReturnValue( + { state: 'fulfilled', value: { body: '"answer"', slots: [] } }, + ); + + // The second enqueue is the write that fails. + const enqueueError = new Error('database is gone'); + let enqueued = 0; + (kernelStore.enqueueRun as unknown as MockInstance).mockImplementation( + () => { + enqueued += 1; + if (enqueued > 1) { + throw enqueueError; + } + }, + ); + + const deliver = vi.fn().mockResolvedValue(undefined); + await expect(kernelQueue.run(deliver)).rejects.toBe(enqueueError); + + expect(kernelStore.rollbackCrank).toHaveBeenCalledWith('delivery'); + expect(resolve).not.toHaveBeenCalled(); + }); + + // Why two savepoints: see `#runLoop`. This pins that the rollback spares the + // transaction, so the work an aborted crank still owes stays inside it. + it.each([ + { + label: 'an abort', + crankResult: { abort: true }, + storeOrder: ['rollbackCrank', 'collectGarbage'], + }, + { + label: 'an abort that also terminates', + crankResult: { + abort: true, + terminate: { vatId: 'v1', info: { body: '"exit"', slots: [] } }, + }, + storeOrder: ['rollbackCrank', 'terminateVat', 'collectGarbage'], + }, + ])( + 'keeps the crank transactional after rolling back $label', + async ({ crankResult, storeOrder }) => { + const mockItem: RunQueueItem = { + type: 'send', + target: 'ko123', + message: { result: 'kp99' } as KernelMessage, + }; + (kernelStore.runQueueLength as unknown as MockInstance) + .mockReturnValueOnce(1) + .mockReturnValue(0); + (kernelStore.dequeueRun as unknown as MockInstance).mockReturnValueOnce( + mockItem, + ); + + const storeCalls: string[] = []; + ( + kernelStore.rollbackCrank as unknown as MockInstance + ).mockImplementation(() => { + storeCalls.push('rollbackCrank'); + }); + (terminateVat as unknown as MockInstance).mockImplementation( + async () => { + storeCalls.push('terminateVat'); + }, + ); + ( + kernelStore.collectGarbage as unknown as MockInstance + ).mockImplementation(() => { + storeCalls.push('collectGarbage'); + throw new Error(STOP_RUN_LOOP); + }); + + const deliver = vi.fn().mockResolvedValue(crankResult); + await expect(kernelQueue.run(deliver)).rejects.toThrow(STOP_RUN_LOOP); + + expect(storeCalls).toStrictEqual(storeOrder); + expect( + ( + kernelStore.createCrankSavepoint as unknown as MockInstance + ).mock.calls.flat(), + ).toStrictEqual(['crank', 'delivery']); + expect(kernelStore.rollbackCrank).not.toHaveBeenCalledWith('crank'); + }, + ); }); describe('getRunLoopStatus', () => { @@ -286,7 +487,7 @@ describe('KernelQueue', () => { await killRunLoop(new Error('crank exploded')); // Without this, endCrank's savepoint release commits the half-finished // crank and the dequeued item is lost. - expect(kernelStore.rollbackCrank).toHaveBeenCalledWith('start'); + expect(kernelStore.rollbackCrank).toHaveBeenCalledWith('delivery'); }); it('does not roll back when the savepoint was never created', async () => { @@ -357,6 +558,43 @@ describe('KernelQueue', () => { }); }); + // FAILING REPRO. + // + // The companion of the case above, at the other end of the crank. Since the + // delivery rollback now spares `crank`, `endCrank`'s release is a real + // RELEASE + COMMIT on the dying path where it used to be a no-op, and + // `#runLoop` calls it from a bare `finally` — so when it throws it replaces + // the error that killed the kernel instead of being reported alongside it. + it('reports both failures when endCrank also fails', async () => { + ( + kernelStore.runQueueLength as unknown as MockInstance + ).mockReturnValueOnce(1); + (kernelStore.dequeueRun as unknown as MockInstance).mockReturnValueOnce({ + type: 'send', + target: 'ko123', + message: {} as KernelMessage, + }); + (kernelStore.endCrank as unknown as MockInstance).mockImplementation( + () => { + throw new Error('database is gone'); + }, + ); + const crankError = new Error('crank exploded'); + const deliver = vi.fn().mockRejectedValue(crankError); + + const failure = await kernelQueue + .run(deliver) + .catch((error: unknown) => error); + + // However the release failure is named, the error that actually killed the + // kernel has to stay reachable — as the rollback path already manages. + expect(causeChain(failure)).toContain(crankError); + expect(kernelQueue.getRunLoopStatus()).toMatchObject({ + state: 'failed', + detail: expect.stringContaining('crank exploded'), + }); + }); + // `rollbackCrank` discards the savepoint even when its database call throws, // so a second attempt could only report a missing savepoint. Without the // `finally` that records the attempt, the abort path leaves the flag unset, @@ -652,10 +890,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 +943,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', @@ -846,7 +1076,7 @@ describe('KernelQueue', () => { throw new Error(STOP_RUN_LOOP); }); await expect(kernelQueue.run(deliver)).rejects.toThrow(STOP_RUN_LOOP); - expect(kernelStore.rollbackCrank).toHaveBeenCalledWith('start'); + expect(kernelStore.rollbackCrank).toHaveBeenCalledWith('delivery'); expect(rejectSpy).toHaveBeenCalledWith(terminateInfo); expect(kernelQueue.subscriptions.has('kp99')).toBe(false); }); @@ -882,7 +1112,7 @@ describe('KernelQueue', () => { throw new Error(STOP_RUN_LOOP); }); await expect(kernelQueue.run(deliver)).rejects.toThrow(STOP_RUN_LOOP); - expect(kernelStore.rollbackCrank).toHaveBeenCalledWith('start'); + expect(kernelStore.rollbackCrank).toHaveBeenCalledWith('delivery'); expect(rejectedAfterAbort).toBe(false); expect(resolveSpy).not.toHaveBeenCalled(); expect(subscribedAfterAbort).toBe(true); @@ -952,11 +1182,7 @@ describe('KernelQueue', () => { mockItem, ); const deliver = vi.fn().mockResolvedValue(undefined); - ( - kernelStore.collectGarbage as unknown as MockInstance - ).mockImplementation(() => { - throw new Error(STOP_RUN_LOOP); - }); + stopAfterOneCrank(); await expect(kernelQueue.run(deliver)).rejects.toThrow(STOP_RUN_LOOP); expect(rejectSpy).toHaveBeenCalledWith(rejectedValue); expect(resolveSpy).not.toHaveBeenCalled(); @@ -991,11 +1217,7 @@ describe('KernelQueue', () => { mockItem, ); const deliver = vi.fn().mockResolvedValue(undefined); - ( - kernelStore.collectGarbage as unknown as MockInstance - ).mockImplementation(() => { - throw new Error(STOP_RUN_LOOP); - }); + stopAfterOneCrank(); await expect(kernelQueue.run(deliver)).rejects.toThrow(STOP_RUN_LOOP); expect(resolveSpy).toHaveBeenCalledWith(fulfilledValue); expect(rejectSpy).not.toHaveBeenCalled(); diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index 3465e93cde..f4e1eff9f0 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -56,14 +56,10 @@ export class KernelQueue { /** * Whether this crank's savepoint has already been handed to `rollbackCrank`. - * Attempted, not necessarily succeeded: `rollbackCrank` forgets the savepoint - * whether or not the database call throws, so after either outcome a second - * attempt can only report "no such savepoint" over the real error. - * - * This has to be recorded at the moment of the attempt rather than returned - * from `#processCrankResult`, because that method can throw after rolling back - * (`#terminateVat`, `collectGarbage`), and the catch below must still know not - * to ask twice. + * Attempted, not necessarily succeeded: it is forgotten either way, so a second + * attempt could only report "no such savepoint" over the real error. Recorded + * at the attempt rather than returned, because `#processCrankResult` can throw + * after rolling back. */ #crankRollbackAttempted: boolean = false; @@ -126,17 +122,25 @@ export class KernelQueue { ): Promise { for (;;) { let wakeUpPromise: Promise | undefined; + // Boxed rather than left `undefined`, so that a crank which threw + // `undefined` is still distinguishable from one that did not throw. + let crankFailure: { error: unknown } | undefined; this.#kernelStore.startCrank(); this.#crankRollbackAttempted = false; try { - this.#kernelStore.createCrankSavepoint('start'); + // Two savepoints, because rolling back the outermost one discards the + // enclosing transaction (see `rollbackSavepoint`) and an aborted crank + // still has writes to make. Only `delivery` is ever rolled back; + // releasing `crank` in `endCrank` is this crank's one commit point. + // `releaseAllSavepoints` names it by ordinal, so `crank` must stay first. + this.#kernelStore.createCrankSavepoint('crank'); + this.#kernelStore.createCrankSavepoint('delivery'); // The savepoint exists from here on, so a throw can be undone. Without - // this, `endCrank`'s savepoint release commits the half-finished crank: - // the item this crank dequeued is gone for good, refcount increments - // stick, and promises resolved during it stay resolved while their - // notifies die unflushed. A restart would resume from that. + // this, `endCrank`'s release commits the half-finished crank: the + // dequeued item is gone for good, refcount increments stick, and + // resolved promises keep their unflushed notifies. try { const queueItem = this.#getNextRunQueueItem(); if (queueItem) { @@ -153,15 +157,14 @@ export class KernelQueue { wakeUpPromise = promise; } } catch (error) { - // An aborted crank already asked, and `rollbackCrank` discards the - // savepoint either way; asking again could only throw "no such - // savepoint" over the real error. + // An aborted crank already asked; asking again could only throw "no + // such savepoint" over the real error. if (!this.#crankRollbackAttempted) { try { - this.#kernelStore.rollbackCrank('start'); + this.#kernelStore.rollbackCrank('delivery'); } catch (rollbackError) { - // The original failure stays the `cause`, since that is the root - // cause an operator needs; the rollback failure is named here. + // The original failure stays the `cause`; the rollback failure is + // named here. throw new Error( `Run loop died and its crank could not be rolled back: ${String(rollbackError)}`, { cause: error }, @@ -170,8 +173,11 @@ export class KernelQueue { } throw error; } + } catch (error) { + crankFailure = { error }; + throw error; } finally { - this.#kernelStore.endCrank(); + this.#endCrank(crankFailure); if (wakeUpPromise) { await wakeUpPromise; } @@ -179,6 +185,31 @@ export class KernelQueue { } } + /** + * End the crank without losing the error that is already unwinding. Since the + * delivery rollback now spares `crank`, `endCrank`'s release is a real RELEASE + * and COMMIT on the dying path, where it used to be a no-op — and from a bare + * `finally` a failing one would silently replace whatever killed the kernel. + * + * @param crankFailure - The error already in flight, if the crank threw. + * @param crankFailure.error - That error. + */ + #endCrank(crankFailure?: { error: unknown }): void { + try { + this.#kernelStore.endCrank(); + } catch (endCrankError) { + if (!crankFailure) { + throw endCrankError; + } + // The original failure stays the `cause`, as on the rollback path; the + // release failure is named here. + throw new Error( + `Run loop died and its crank could not be ended: ${String(endCrankError)}`, + { cause: crankFailure.error }, + ); + } + } + /** * Record the death of the run loop and fail the kernel's own message-result * subscriptions, which would otherwise hang forever. Kernel promises in the @@ -304,13 +335,11 @@ export class KernelQueue { // For active vats, this allows the message to be retried in a future crank. // For terminated vats, the message will just go splat. try { - this.#kernelStore.rollbackCrank('start'); + this.#kernelStore.rollbackCrank('delivery'); } finally { - // Set even when the rollback threw. `rollbackCrank` forgets the - // savepoint in its own `finally`, so "attempted" and "the savepoint is - // gone" now coincide exactly — and a second attempt from the run loop's - // catch would report a missing savepoint as the reason the kernel died, - // burying the database error that actually killed it. + // Set even when the rollback threw: the savepoint is gone either way, so + // a second attempt would report a missing savepoint as the reason the + // kernel died, burying the error that actually killed it. this.#crankRollbackAttempted = true; } // Discard kernel subscriptions that were queued for invocation @@ -333,17 +362,30 @@ export class KernelQueue { // TODO: Currently all errors terminate the vat, but instead we could // restart it and terminate the vat only after a certain number of failed // retries. This is probably where we should implement the vat restart logic. - } else { - // Upon on successful crank completion, enqueue buffered vat outputs for delivery. - this.#flushCrankBuffer(); } - // Vat termination during delivery is triggered by an illegal syscall - // or by syscall.exit(). + // This call kills the worker, so its writes must outlive the rollback above: + // a store that still believed the vat was alive would relaunch it after a + // restart and redeliver what killed it. if (crankResult?.terminate) { const { vatId, info } = crankResult.terminate; await this.#terminateVat(vatId, info); } this.#kernelStore.collectGarbage(); + if (!crankResult?.abort) { + // After the fallible work above, not before it. The flush settles the + // promise `enqueueMessage` gave an external caller, so a later rollback + // would discard the state that answer was computed from. + // + // Not airtight: `#terminateVat` resolves the dying vat's promises through + // `resolvePromises`, which defaults to `immediate` and invokes their + // subscriptions before `collectGarbage`. Deferring those too would change + // termination semantics, not crank ordering. + this.#flushCrankBuffer(); + } + // After the flush, because the audit reads the run queue as ground truth + // while a buffered item's references were already counted when it was + // enqueued: audited mid-flush, every buffered item reads as a leak. + this.#kernelStore.assertRefCountsIfAuditing(); } /** @@ -370,21 +412,23 @@ export class KernelQueue { */ #flushCrankBuffer(): void { const items = this.#kernelStore.flushCrankBuffer(); + const resolved: KRef[] = []; for (const item of items) { this.#enqueueRun(item); if (item.type === 'notify') { - // Invoke kernel subscription callback if any, reading resolution - // data from the (now committed) promise state - this.#invokeKernelSubscription(item.kpid); + resolved.push(item.kpid); } } + // Plus promises with no vat subscriber to notify, which the kernel is + // nonetheless waiting on (e.g. from `enqueueMessage`). + resolved.push(...this.#resolvedWithKernelSubscription); + this.#resolvedWithKernelSubscription = []; - // Invoke kernel subscriptions for promises resolved during this crank - // that don't have kernel-level subscribers (e.g., promises from enqueueMessage) - for (const kpid of this.#resolvedWithKernelSubscription) { + // Callbacks only once every `#enqueueRun` is done: one that threw partway + // would roll the crank back underneath answers already given. + for (const kpid of resolved) { this.#invokeKernelSubscription(kpid); } - this.#resolvedWithKernelSubscription = []; } /** @@ -504,7 +548,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..e88c54b52c 100644 --- a/packages/ocap-kernel/src/KernelRouter.test.ts +++ b/packages/ocap-kernel/src/KernelRouter.test.ts @@ -59,9 +59,15 @@ describe('KernelRouter', () => { krefToEref: vi.fn() as unknown as MockInstance, getKpidsToRetire: vi.fn().mockReturnValue([]), translateCapDataKtoE: vi.fn(), - krefsToExistingErefs: vi.fn((_endpointId: string, krefs: string[]) => + krefsToErefs: vi.fn((_endpointId: string, krefs: string[]) => krefs.map((kref: string) => `translated-${kref}`), ) as unknown as MockInstance, + clearReachableFlag: vi.fn(), + deleteCListEntry: vi.fn(), + forgetKref: vi.fn(), + orphanKernelObject: vi.fn(), + hasCListEntry: vi.fn().mockReturnValue(true), + isVatTerminated: vi.fn().mockReturnValue(false), createCrankSavepoint: vi.fn(), } as unknown as KernelStore; @@ -283,8 +289,67 @@ describe('KernelRouter', () => { expect(endpointHandle.deliverMessage).not.toHaveBeenCalled(); expect(result).toBeUndefined(); - // Verify that no refcount decrementation happened since we're requeuing - expect(kernelStore.decrementRefCount).not.toHaveBeenCalled(); + expect(kernelStore.decrementRefCount).toHaveBeenCalledWith( + target, + 'requeue|target', + ); + }); + + it('hands over every reference a requeued message carries', async () => { + const target = 'kp123'; + ( + kernelStore.getKernelPromise as unknown as MockInstance + ).mockReturnValueOnce({ state: 'unresolved' }); + const message: KernelMessage = { + methargs: { body: 'method args', slots: ['ko1', 'ko2'] }, + result: 'kp9', + }; + await kernelRouter.deliver({ type: 'send', target, message }); + + expect(kernelStore.enqueuePromiseMessage).toHaveBeenCalledWith( + target, + message, + ); + expect( + (kernelStore.decrementRefCount as unknown as MockInstance).mock.calls, + ).toStrictEqual([ + [target, 'requeue|target'], + ['kp9', 'requeue|result'], + ['ko1', 'requeue|slot'], + ['ko2', 'requeue|slot'], + ]); + }); + + it('charges the promise, not the object it resolved to', async () => { + const promiseId = 'kp123'; + const resolvedObject = 'ko456'; + ( + kernelStore.getKernelPromise as unknown as MockInstance + ).mockReturnValueOnce({ + state: 'fulfilled', + value: { body: '#"$0"', slots: [resolvedObject] }, + }); + (kernelStore.getOwner as unknown as MockInstance).mockReturnValue('v1'); + + await kernelRouter.deliver({ + type: 'send', + target: promiseId, + message: { + methargs: { body: 'method args', slots: [] }, + result: null, + }, + }); + + // The run queue item was charged against the promise it named, so that + // is what has to be released — not whatever routing resolved it to. + expect(kernelStore.decrementRefCount).toHaveBeenCalledWith( + promiseId, + 'deliver|send|target', + ); + expect(kernelStore.decrementRefCount).not.toHaveBeenCalledWith( + resolvedObject, + 'deliver|send|target', + ); }); it('splats message when promise resolves to a non-object', async () => { @@ -550,6 +615,12 @@ describe('KernelRouter', () => { // Verify no notification was delivered to the vat expect(endpointHandle.deliverNotify).not.toHaveBeenCalled(); expect(result).toStrictEqual({ didDelivery: endpointId }); + // Nothing was delivered, but the queued notification is gone either + // way, so its reference has to be released on this path too. + expect(kernelStore.decrementRefCount).toHaveBeenCalledWith( + kpid, + 'deliver|notify', + ); }); it('returns didDelivery when no kpids to retire', async () => { @@ -588,6 +659,10 @@ describe('KernelRouter', () => { // Verify no notification was delivered to the vat expect(endpointHandle.deliverNotify).not.toHaveBeenCalled(); expect(result).toStrictEqual({ didDelivery: endpointId }); + expect(kernelStore.decrementRefCount).toHaveBeenCalledWith( + kpid, + 'deliver|notify', + ); }); it('throws if notification is for an unresolved promise', async () => { @@ -649,6 +724,204 @@ describe('KernelRouter', () => { expect(result).toStrictEqual(mockCrankResult); }, ); + + it('clears the reachable flag when delivering dropExports', async () => { + await kernelRouter.deliver({ + type: 'dropExports', + endpointId: 'v1', + krefs: ['ko1', 'ko2'], + }); + + expect( + (kernelStore.clearReachableFlag as unknown as MockInstance).mock + .calls, + ).toStrictEqual([ + ['v1', 'ko1'], + ['v1', 'ko2'], + ]); + expect(kernelStore.deleteCListEntry).not.toHaveBeenCalled(); + }); + + it.each(['retireExports', 'retireImports'] as const)( + 'tears down the c-list entry when delivering %s', + async (actionType) => { + await kernelRouter.deliver({ + type: actionType, + endpointId: 'v1', + krefs: ['ko1', 'ko2'], + }); + + expect( + (kernelStore.deleteCListEntry as unknown as MockInstance).mock + .calls, + ).toStrictEqual([ + ['v1', 'ko1', 'translated-ko1'], + ['v1', 'ko2', 'translated-ko2'], + ]); + }, + ); + + it('orphans the object when delivering retireExports', async () => { + await kernelRouter.deliver({ + type: 'retireExports', + endpointId: 'v1', + krefs: ['ko1', 'ko2'], + }); + + // The owner has given up the last name for the object, so the kernel's + // record of who owns it must go too or it outlives every reference. + expect( + (kernelStore.orphanKernelObject as unknown as MockInstance).mock + .calls, + ).toStrictEqual([ + ['ko1', 'v1'], + ['ko2', 'v1'], + ]); + }); + + it('leaves ownership alone when delivering retireImports', async () => { + await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'v1', + krefs: ['ko1'], + }); + + expect(kernelStore.orphanKernelObject).not.toHaveBeenCalled(); + }); + + it('still releases the kernel side when a terminated vat has vanished', async () => { + getEndpoint.mockImplementationOnce(() => { + throw Error('vat v1 not found'); + }); + ( + kernelStore.isVatTerminated as unknown as MockInstance + ).mockReturnValue(true); + + const result = await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'v1', + krefs: ['ko1'], + }); + + expect(result).toStrictEqual({ didDelivery: 'v1' }); + // The action has already been consumed, so skipping the teardown would + // lose it and leave the entry behind for good + expect(kernelStore.deleteCListEntry).toHaveBeenCalledWith( + 'v1', + 'ko1', + 'translated-ko1', + ); + }); + + it('still releases the kernel side when a remote has vanished', async () => { + getEndpoint.mockImplementationOnce(() => { + throw Error('remote r1 not found'); + }); + + const result = await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'r1', + krefs: ['ko1'], + }); + + expect(result).toStrictEqual({ didDelivery: 'r1' }); + expect(kernelStore.deleteCListEntry).toHaveBeenCalledWith( + 'r1', + 'ko1', + 'translated-ko1', + ); + }); + + it.each(['dropExports', 'retireExports', 'retireImports'] as const)( + 'refuses to release %s for a vat that is absent but not terminated', + async (actionType) => { + // A vat between incarnations still holds every one of these krefs, so + // committing the kernel's release would leave the two disagreeing. + getEndpoint.mockImplementationOnce(() => { + throw Error('vat v1 not found'); + }); + + await expect( + kernelRouter.deliver({ + type: actionType, + endpointId: 'v1', + krefs: ['ko1'], + }), + ).rejects.toThrow('vat v1 not found'); + + expect(kernelStore.clearReachableFlag).not.toHaveBeenCalled(); + expect(kernelStore.deleteCListEntry).not.toHaveBeenCalled(); + expect(kernelStore.orphanKernelObject).not.toHaveBeenCalled(); + }, + ); + + it('skips krefs already cleaned up before delivery', async () => { + ( + kernelStore.hasCListEntry as unknown as MockInstance + ).mockImplementation( + (_endpointId: string, kref: string) => kref === 'ko1', + ); + + await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'v1', + krefs: ['ko1', 'ko2'], + }); + + expect( + (kernelStore.deleteCListEntry as unknown as MockInstance).mock.calls, + ).toStrictEqual([['v1', 'ko1', 'translated-ko1']]); + }); + + it('does nothing when every kref is already gone', async () => { + (kernelStore.hasCListEntry as unknown as MockInstance).mockReturnValue( + false, + ); + + const result = await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'v1', + krefs: ['ko1'], + }); + + expect(result).toStrictEqual({ didDelivery: 'v1' }); + expect(kernelStore.deleteCListEntry).not.toHaveBeenCalled(); + expect(endpointHandle.deliverRetireImports).not.toHaveBeenCalled(); + }); + + it('rolls back and terminates the vat when delivery fails', async () => { + ( + endpointHandle.deliverRetireImports as unknown as MockInstance + ).mockRejectedValueOnce(Error('endpoint went away mid-delivery')); + + const result = await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'v1', + krefs: ['ko1'], + }); + + // Committing the release while v1 still holds the eref would leave the + // two disagreeing, and v1 would mint a fresh kref for the same object + expect(result?.abort).toBe(true); + expect(result?.terminate?.vatId).toBe('v1'); + }); + + it('does not retry a remote that refuses the delivery', async () => { + ( + endpointHandle.deliverRetireImports as unknown as MockInstance + ).mockRejectedValueOnce(Error('remote queue full')); + + const result = await kernelRouter.deliver({ + type: 'retireImports', + endpointId: 'r1', + krefs: ['ko1'], + }); + + // Aborting would restore the action, and GC actions are selected ahead + // of all other work, so a remote that keeps refusing would be handed + // this same item every crank and nothing else would ever run + expect(result).toStrictEqual({ didDelivery: 'r1' }); + }); }); describe('bringOutYourDead', () => { diff --git a/packages/ocap-kernel/src/KernelRouter.ts b/packages/ocap-kernel/src/KernelRouter.ts index 5cfb8335d4..5117905f36 100644 --- a/packages/ocap-kernel/src/KernelRouter.ts +++ b/packages/ocap-kernel/src/KernelRouter.ts @@ -3,7 +3,10 @@ import type { CapData } from '@endo/marshal'; import { Logger } from '@metamask/logger'; import { KernelQueue } from './KernelQueue.ts'; -import { makeKernelError } from './liveslots/kernel-marshal.ts'; +import { + makeFatalKernelError, + makeKernelError, +} from './liveslots/kernel-marshal.ts'; import type { KernelStore } from './store/index.ts'; import { extractSingleRef } from './store/utils/extract-ref.ts'; import { parseRef } from './store/utils/parse-ref.ts'; @@ -11,6 +14,7 @@ import { isPromiseRef } from './store/utils/promise-ref.ts'; import type { EndpointId, EndpointHandle, + ERef, KRef, KernelMessage, RunQueueItem, @@ -20,6 +24,7 @@ import type { RunQueueItemGCAction, CrankResult, } from './types.ts'; +import { isVatId } from './types.ts'; import { assert, Fail } from './utils/assert.ts'; type MessageRoute = { @@ -255,7 +260,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 +322,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 +382,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 +408,15 @@ export class KernelRouter { tPromise.state === 'rejected', this.#kernelStore.translateCapDataKtoE(endpointId, tPromise.value), ]); - // decrement refcount for the promise being notified - if (toResolve !== kpid) { - this.#kernelStore.decrementRefCount(toResolve, 'deliver|notify|slot'); - } } + // TODO: SwingSet also tears down the c-list entry for each promise in the + // batch here, since the endpoint can never refer to a settled promise by + // that eref again. Left alone for now because the debug UI discovers + // exported ocap URLs by scanning these entries. The cost of keeping them is + // that a settled promise reached this way holds a count forever, so it is + // never collected and its resolution slots are never released. 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); } /** @@ -408,15 +430,123 @@ export class KernelRouter { this.#logger?.log( `@@@@ deliver ${endpointId} ${type} ${JSON.stringify(krefs)}`, ); - const endpoint = this.#getEndpoint(endpointId); - const erefs = this.#kernelStore.krefsToExistingErefs(endpointId, krefs); + // This action was selected while the endpoint's c-list held every one of + // these krefs, but `nextTerminatedVatCleanup` runs between selection and + // here and can take the entries — and the endpoint — with it. Whatever + // survives still has to be released on the kernel's side: the action has + // already been consumed from the durable set, so skipping the teardown + // would lose it and leave the entry behind for good. + const live = krefs.filter((kref) => + this.#kernelStore.hasCListEntry(endpointId, kref), + ); + if (live.length < krefs.length) { + this.#logger?.error( + `${type} for ${endpointId}: ${krefs.length - live.length} of ${krefs.length} kref(s) were cleaned up before delivery`, + ); + } + if (live.length === 0) { + return { didDelivery: endpointId }; + } + // Resolved before anything is torn down, so a lookup that fails has nothing + // to undo, and so the two outcomes below are decided rather than discovered + // halfway through. + let endpoint: EndpointHandle | undefined; + try { + endpoint = this.#getEndpoint(endpointId); + } catch (error) { + // A vat absent from the kernel's vat table but not marked terminated is a + // vat between incarnations, and its c-list is whole: every kref here is one + // the returning incarnation still has in its own tables. `restartVat` + // takes a vat out of that table for as long as launching a worker and + // negotiating with it takes, so this is reachable, and releasing the + // kernel's side would commit exactly the disagreement the failed delivery + // below rolls back to avoid — the vat would mint fresh krefs for objects + // the kernel thinks it let go of. Fail the crank rather than commit that. + // Nothing here can make the restart safe: the action is already spent from + // the durable set, and a crank that neither delivers nor releases would + // simply be handed the same action again on the next one. + if ( + isVatId(endpointId) && + !this.#kernelStore.isVatTerminated(endpointId) + ) { + throw error; + } + // A terminated vat's cleanup tears its c-list down wholesale, and a remote + // reconciles on its next incarnation, so for those the release below is + // safe to commit — and has to be, since the action is already spent from + // the durable set. + this.#logger?.error( + `Endpoint ${endpointId} vanished before ${type} of ${JSON.stringify(live)}; releasing the kernel's side anyway:`, + error, + ); + } + const erefs = this.#kernelStore.krefsToErefs(endpointId, live); + // Telling an endpoint to let go is also the kernel letting go. Otherwise a + // dropped export stays flagged reachable, so the same action gets derived + // again, and retired entries outlive the objects they name. + live.forEach((kref, index) => { + if (type === 'dropExports') { + this.#kernelStore.clearReachableFlag(endpointId, kref); + return; + } + // `erefs` is parallel to `live`: krefsToErefs throws rather than + // returning a short array, so every index is populated. + this.#kernelStore.deleteCListEntry( + endpointId, + kref, + erefs[index] as ERef, + ); + if (type === 'retireExports') { + // Retiring an export is the owner giving up the last name for the + // object, so the kernel's record of who owns it goes too. + this.#kernelStore.orphanKernelObject(kref, endpointId); + } + }); + if (!endpoint) { + return { didDelivery: endpointId }; + } const method = `deliver${(type[0] as string).toUpperCase()}${type.slice(1)}` as | 'deliverDropExports' | 'deliverRetireExports' | 'deliverRetireImports'; - const crankResult = await endpoint[method](erefs); - return crankResult; + try { + return await endpoint[method](erefs); + } catch (error) { + if (!isVatId(endpointId)) { + // A remote is a separate kernel across a link that can drop messages, + // so its protocol already has to tolerate one going missing — it + // reconciles on the next incarnation change. Retrying instead would + // starve the kernel: GC actions are selected ahead of all other work, + // so a remote that keeps refusing (a full send queue, say) would be + // handed the same item every crank and nothing else would ever run. + this.#logger?.error( + `Delivery of ${type} to remote ${endpointId} failed; the kernel has released ${JSON.stringify(live)} regardless:`, + error, + ); + return { didDelivery: endpointId }; + } + // A vat is local and reliable, so a refusal means it is broken. Undo the + // teardown rather than commit it: leaving the two disagreeing would have + // the vat mint fresh krefs for objects the kernel thinks it let go of. + // Aborting restores the entries and the action; terminating the vat is + // what stops that restored action from being retried forever. + this.#logger?.error( + `Delivery of ${type} to ${endpointId} failed; rolling back the kernel's release of ${JSON.stringify(live)} and terminating it:`, + error, + ); + return { + abort: true, + terminate: { + vatId: endpointId, + reject: true, + info: makeFatalKernelError( + 'INTERNAL_ERROR', + `failed to accept ${type}: ${error instanceof Error ? error.message : String(error)}`, + ), + }, + }; + } } /** diff --git a/packages/ocap-kernel/src/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/garbage-collection/gc-handlers.test.ts b/packages/ocap-kernel/src/garbage-collection/gc-handlers.test.ts new file mode 100644 index 0000000000..8b5231eec9 --- /dev/null +++ b/packages/ocap-kernel/src/garbage-collection/gc-handlers.test.ts @@ -0,0 +1,115 @@ +import { describe, it, expect, beforeEach } from 'vitest'; + +import { makeMapKernelDatabase } from '../../test/storage.ts'; +import { makeKernelStore } from '../store/index.ts'; +import type { VatConfig, VatId } from '../types.ts'; +import { performExportCleanup } from './gc-handlers.ts'; + +describe('performExportCleanup', () => { + let kernelStore: ReturnType; + + /** + * Register and initialize an endpoint so it can hold c-list entries. + * + * @param vatIds - The vats to bring into existence. + */ + function givenVats(...vatIds: VatId[]): void { + for (const vatId of vatIds) { + kernelStore.setVatConfig(vatId, { sourceSpec: 'x' } as VatConfig); + kernelStore.initEndpoint(vatId); + } + } + + beforeEach(() => { + kernelStore = makeKernelStore(makeMapKernelDatabase()); + kernelStore.markInitialized(); + givenVats('v1', 'v2'); + }); + + // `checkReachable` is what separates a retire from an abandon; the ownership + // check precedes it, so both syscalls have to be covered. + const actions = [ + { name: 'retireExports', checkReachable: true }, + { name: 'abandonExports', checkReachable: false }, + ] as const; + + it.each(actions)( + 'lets an owner give up its own export via $name', + ({ checkReachable }) => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.clearReachableFlag('v1', kref); + + performExportCleanup([kref], checkReachable, 'v1', kernelStore); + + expect(kernelStore.getOwner(kref)).toBeUndefined(); + expect(kernelStore.hasCListEntry('v1', kref)).toBe(false); + }, + ); + + it.each(actions)( + 'refuses $name for an object owned by another endpoint', + ({ name, checkReachable }) => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + // v2 holds it as an import, which is what makes the kref nameable in a + // syscall from v2 at all. + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + + expect(() => + performExportCleanup([kref], checkReachable, 'v2', kernelStore), + ).toThrow(`endpoint v2 issued ${name} for ${kref}, which is owned by v1`); + + // v1's claim survives intact, entry and ownership both. + expect(kernelStore.getOwner(kref)).toBe('v1'); + expect(kernelStore.hasCListEntry('v1', kref)).toBe(true); + expect(kernelStore.hasCListEntry('v2', kref)).toBe(true); + }, + ); + + it.each(actions)( + 'allows $name for an already-orphaned object', + ({ checkReachable }) => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + kernelStore.clearReachableFlag('v1', kref); + kernelStore.forgetKref('v1', kref); + kernelStore.orphanKernelObject(kref, 'v1'); + + // No claim is left to erase, so there is nothing for the guard to protect. + expect(() => + performExportCleanup([kref], checkReachable, 'v2', kernelStore), + ).not.toThrow(); + expect(kernelStore.hasCListEntry('v2', kref)).toBe(false); + }, + ); + + it('refuses retireExports for an object the owner still reaches', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + + expect(() => performExportCleanup([kref], true, 'v1', kernelStore)).toThrow( + `retireExports but ${kref} is still reachable`, + ); + expect(kernelStore.getOwner(kref)).toBe('v1'); + }); + + it('abandons an export the owner still reaches', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + + performExportCleanup([kref], false, 'v1', kernelStore); + + expect(kernelStore.getOwner(kref)).toBeUndefined(); + }); + + it.each(actions)( + 'refuses $name for a promise', + ({ name, checkReachable }) => { + const kpid = kernelStore.initKernelPromise()[0]; + kernelStore.exportFromEndpoint('v1', 'p+1'); + + expect(() => + performExportCleanup([kpid], checkReachable, 'v1', kernelStore), + ).toThrow(`endpoint v1 issued invalid ${name} for ${kpid}`); + }, + ); +}); diff --git a/packages/ocap-kernel/src/garbage-collection/gc-handlers.ts b/packages/ocap-kernel/src/garbage-collection/gc-handlers.ts index c96b6e26c8..15e254eb3b 100644 --- a/packages/ocap-kernel/src/garbage-collection/gc-handlers.ts +++ b/packages/ocap-kernel/src/garbage-collection/gc-handlers.ts @@ -78,11 +78,26 @@ export function performExportCleanup( `endpoint ${endpointId} issued invalid ${action}Exports for ${kref}`, ); } + // Only an owner may give up an object. Nothing upstream of here checks that + // the vref is even an export — `translateSyscallVtoK` maps import and + // export directions alike — so without this a vat could disown an object + // belonging to a different, live vat. An already-orphaned object is fine: + // there is no claim left to erase. + const owner = kernelStore.getOwner(kref); + if (owner !== undefined && owner !== endpointId) { + throw Error( + `endpoint ${endpointId} issued ${action}Exports for ${kref}, which is owned by ${owner}`, + ); + } if (checkReachable) { if (kernelStore.getReachableFlag(endpointId, kref)) { throw Error(`${action}Exports but ${kref} is still reachable`); } } kernelStore.forgetKref(endpointId, kref); + // The owner no longer names the object, so nothing can reach it through + // this endpoint again. Drop the owner mapping too, or the kernel's record + // of the object outlives the only c-list entry it was reachable through. + kernelStore.orphanKernelObject(kref, endpointId); } } diff --git a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.test.ts index b97b79369c..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'; @@ -7,6 +8,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 +26,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 +42,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 +61,7 @@ describe('OcapURLManager', () => { ocapURLManager = new OcapURLManager({ remoteManager: mockRemoteManager, + kernelStore: mockKernelStore, }); }); @@ -92,8 +106,101 @@ 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 once per URL naming it', async () => { + await ocapURLManager.issueOcapURL(objectKRef); + await ocapURLManager.issueOcapURL(objectKRef); + + expect(mockKernelStore.getPinnedObjects()).toStrictEqual([ + objectKRef, + objectKRef, + ]); + expect(mockKernelStore.getObjectRefCount(objectKRef)).toStrictEqual({ + reachable: 2, + recognizable: 2, + }); + }); + + 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('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); + + 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 +290,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 +308,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 +330,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 +398,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 +413,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 +434,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..8575bbe33d 100644 --- a/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts +++ b/packages/ocap-kernel/src/remotes/kernel/OcapURLManager.ts @@ -4,6 +4,7 @@ import { parseOcapURL } from './remote-comms.ts'; import type { RemoteManager } from './RemoteManager.ts'; import { kslot, krefOf } from '../../liveslots/kernel-marshal.ts'; import type { SlotValue } from '../../liveslots/kernel-marshal.ts'; +import type { KernelStore } from '../../store/index.ts'; import type { KRef } from '../../types.ts'; /** @@ -25,6 +26,7 @@ export type OcapURLRedemptionService = { type OcapURLManagerConstructorProps = { remoteManager: RemoteManager; + kernelStore: KernelStore; }; /** @@ -34,6 +36,9 @@ export class OcapURLManager { /** Remote manager for handling remote connections */ readonly #remoteManager: RemoteManager; + /** The kernel's store, for retaining the objects issued URLs name */ + readonly #kernelStore: KernelStore; + /** OCAP URL issuer service object */ readonly #ocapURLIssuerService: object; @@ -45,9 +50,11 @@ export class OcapURLManager { * * @param options - Constructor options. * @param options.remoteManager - The remote manager for handling cross-kernel communications. + * @param options.kernelStore - The kernel's store. */ - constructor({ remoteManager }: OcapURLManagerConstructorProps) { + constructor({ remoteManager, kernelStore }: OcapURLManagerConstructorProps) { this.#remoteManager = remoteManager; + this.#kernelStore = kernelStore; // Create the OCAP URL issuer service this.#ocapURLIssuerService = Far('ocapURLIssuerService', { @@ -119,7 +126,22 @@ export class OcapURLManager { */ async issueOcapURL(kref: KRef): Promise { const identity = this.#remoteManager.getRemoteIdentity(); - return identity.issueOcapURL(kref); + // Before minting the token, not after: the URL is unretractable once it + // exists, so the target must already be retained. Minting also awaits, and + // a collection crank can run in that window. See `retainForOcapURL`. + 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. 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/remotes/kernel/RemoteHandle.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts index c14539bc6c..90f4ef3bc5 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts @@ -5,6 +5,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { RemoteHandle } from './RemoteHandle.ts'; import { createMockRemotesFactory } from '../../../test/remotes-mocks.ts'; +import { withFailingSavepointRelease } from '../../../test/savepoint-stack.ts'; import type { KernelQueue } from '../../KernelQueue.ts'; import type { KernelStore } from '../../store/index.ts'; import { parseRef } from '../../store/utils/parse-ref.ts'; @@ -220,6 +221,34 @@ describe('RemoteHandle', () => { }); }); + // `handleRemoteMessage` releases its savepoint inside the `try` and rolls + // back in the `catch`. #1012 made a failed `RELEASE` discard the whole + // savepoint stack, so that rollback reports a savepoint that no longer + // exists, and left unguarded it throws out of the `catch` in place of the + // failure that brought it there. + it('reports the release failure rather than a missing savepoint', async () => { + const failing = withFailingSavepointRelease(mockKernelStore); + const { releaseFailure, rollbackSavepoint } = failing; + mockKernelStore = failing.kernelStore; + const remote = makeRemote(); + + const delivery = JSON.stringify({ + seq: 1, + method: 'deliver', + params: ['bringOutYourDead'], + }); + + // The error an operator needs is the one the database gave, not the + // bookkeeping artefact of trying to clean up after it. + await expect(remote.handleRemoteMessage(delivery)).rejects.toBe( + releaseFailure, + ); + // Still attempted, so that abandoning the rollback is not a way to pass + // this test: a release that failed for a reason of its own may well have + // left the savepoint standing. + expect(rollbackSavepoint).toHaveBeenCalledWith('receive_r0_1'); + }); + // A dead run loop will never deliver the message, and `handleRemoteMessage` // rolls back without advancing the received sequence number, so the peer // retries and gives up rather than being acknowledged by a black hole. @@ -536,7 +565,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 +587,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 +653,6 @@ describe('RemoteHandle', () => { // As if we're no longer using it (which, in fact, we weren't), which is a // prequisite for a valid 'retireImports' delivery - mockKernelStore.decrementRefCount(koref, 'test'); mockKernelStore.clearReachableFlag(remote.remoteId, koref); // Now have the "other end" retire the import (include seq for incoming message) @@ -660,7 +689,12 @@ describe('RemoteHandle', () => { const remote = makeRemote(); const mockOcapURL = 'as if it was a URL'; const mockReplyKey = 'replyKey'; - const replyKRef = 'ko100'; + // A URL only ever names an object the kernel still has, so redeem one that + // exists: importing a deleted kref is refused outright. + const replyKRef = mockKernelStore.initKernelObject('kernel'); + vi.spyOn(mockRemoteComms, 'redeemLocalOcapURL').mockResolvedValue( + replyKRef, + ); const replyRRef = 'ro+1'; // Include seq for incoming message const request = JSON.stringify({ diff --git a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.ts index d92359b5c0..34181df661 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.ts @@ -1032,7 +1032,19 @@ export class RemoteHandle implements EndpointHandle { this.#kernelStore.releaseSavepoint(savepointName); } catch (error) { // Rollback on any error - in-memory state unchanged since we didn't update it yet - this.#kernelStore.rollbackSavepoint(savepointName); + try { + this.#kernelStore.rollbackSavepoint(savepointName); + } catch (rollbackError) { + // The release above is inside the `try`, and a failed RELEASE discards + // the whole savepoint stack — so this rollback reports a savepoint that + // is already gone, over the database failure an operator actually needs. + // Still attempted, because a release that failed for a reason of its own + // may well have left the savepoint standing. + this.#logger.error( + `${this.#peerId.slice(0, 8)}:: rollback of ${savepointName} failed`, + rollbackError, + ); + } throw error; } diff --git a/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts index 34e9e91502..c3257d85df 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteManager.test.ts @@ -4,6 +4,7 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import * as remoteComms from './remote-comms.ts'; import { RemoteManager } from './RemoteManager.ts'; import { createMockRemotesFactory } from '../../../test/remotes-mocks.ts'; +import { withFailingSavepointRelease } from '../../../test/savepoint-stack.ts'; import { makeMapKernelDatabase } from '../../../test/storage.ts'; import type { KernelQueue } from '../../KernelQueue.ts'; import { makeKernelStore } from '../../store/index.ts'; @@ -23,16 +24,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 +774,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 +880,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'); @@ -951,5 +949,44 @@ describe('RemoteManager', () => { // mutations would otherwise drift from the rolled-back kv view. expect(finalizeSpy).not.toHaveBeenCalled(); }); + + // The same shape `RemoteHandle.handleRemoteMessage` has: the release sits + // inside the `try` and the rollback in the `catch`, so a failed RELEASE — + // which discards the whole savepoint stack — leaves the rollback naming a + // savepoint that is gone. + it('reports the release failure rather than a missing savepoint', async () => { + const peerId = 'peer-whose-release-fails'; + const { + kernelStore: failingStore, + releaseFailure, + rollbackSavepoint, + } = withFailingSavepointRelease(kernelStore); + remoteManager = new RemoteManager({ + platformServices: mockPlatformServices, + kernelStore: failingStore, + kernelQueue: mockKernelQueue, + logger, + }); + remoteManager.setMessageHandler(vi.fn()); + await remoteManager.initRemoteComms(); + const onIncarnationChange = vi + .mocked(remoteComms.initRemoteComms) + .mock.calls.at(-1)?.[8] as ( + peerId: string, + observedIncarnation: string, + ) => Promise; + + // The error an operator needs is the one the database gave, not the + // bookkeeping artefact of trying to clean up after it. + await expect(onIncarnationChange(peerId, 'incarnation-A')).rejects.toBe( + releaseFailure, + ); + // Still attempted, so that abandoning the rollback is not a way to pass: + // a release that failed for a reason of its own may well have left the + // savepoint standing. + expect(rollbackSavepoint).toHaveBeenCalledWith( + `peerIncarnation_${peerId}`, + ); + }); }); }); diff --git a/packages/ocap-kernel/src/remotes/kernel/RemoteManager.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteManager.ts index d9b7c3c946..9a477cf398 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteManager.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteManager.ts @@ -261,7 +261,16 @@ export class RemoteManager { this.#kernelStore.setPeerIncarnation(peerId, observedIncarnation); this.#kernelStore.releaseSavepoint(savepoint); } catch (error) { - this.#kernelStore.rollbackSavepoint(savepoint); + try { + this.#kernelStore.rollbackSavepoint(savepoint); + } catch (rollbackError) { + // The release above is inside the `try`, and a failed RELEASE discards + // the whole savepoint stack — so this rollback reports a savepoint that + // is already gone, over the database failure an operator actually needs. + // Still attempted, because a release that failed for a reason of its own + // may well have left the savepoint standing. + this.#logger?.error(`Rollback of ${savepoint} failed`, rollbackError); + } throw error; } diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 58fefc80c3..d2fb46efe3 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -47,6 +47,8 @@ describe('kernel store', () => { 'addSubcluster', 'addSubclusterVat', 'allocateErefForKref', + 'assertRefCountsIfAuditing', + 'auditRefCounts', 'bufferCrankOutput', 'cleanupOrphanMessages', 'cleanupTerminatedVat', @@ -84,6 +86,7 @@ describe('kernel store', () => { 'forgetEref', 'forgetKref', 'forgetTerminatedVat', + 'formatRefCountViolations', 'getAllRemoteRecords', 'getAllSystemSubclusterMappings', 'getAllVatRecords', @@ -100,6 +103,7 @@ describe('kernel store', () => { 'getNextRemoteId', 'getNextVatId', 'getObjectRefCount', + 'getOcapURLObjects', 'getOwner', 'getPeerIncarnation', 'getPendingMessage', @@ -140,14 +144,16 @@ describe('kernel store', () => { 'isVatTerminated', 'kernelRefExists', 'krefToEref', - 'krefsToExistingErefs', + 'krefsToErefs', 'makeVatStore', 'markInitialized', 'markVatAsTerminated', 'nextReapAction', 'nextTerminatedVatCleanup', + 'orphanKernelObject', 'pinObject', 'provideIncarnationId', + 'recomputeRefCounts', 'recordLastActiveTime', 'releaseAllSavepoints', 'releaseSavepoint', @@ -155,6 +161,7 @@ describe('kernel store', () => { 'removeVatFromSubcluster', 'reset', 'resolveKernelPromise', + 'retainForOcapURL', 'retireKernelObjects', 'revoke', 'rollbackCrank', @@ -167,6 +174,8 @@ describe('kernel store', () => { 'setPeerIncarnation', 'setPendingMessage', 'setPromiseDecider', + 'setReachableFlag', + 'setRefCountAuditing', 'setRelayEntries', 'setRemoteHighestReceivedSeq', 'setRemoteIdentityValue', @@ -184,6 +193,7 @@ describe('kernel store', () => { 'translateRefEtoK', 'translateRefKtoE', 'translateSyscallVtoK', + 'undoOcapURLRetention', 'unpinObject', 'waitForCrank', ]); @@ -206,31 +216,31 @@ describe('kernel store', () => { const ko2Owner = 'r23'; expect(ks.initKernelObject(ko1Owner)).toBe('ko1'); - // Check that the object is initialized with reachable=1, recognizable=1 - const refCounts = ks.getObjectRefCount('ko1'); - expect(refCounts.reachable).toBe(1); - expect(refCounts.recognizable).toBe(1); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); // Increment the reference count ks.incrementRefCount('ko1', 'test'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(2); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(2); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); // Increment again ks.incrementRefCount('ko1', 'test'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(3); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(3); - - // Decrement - ks.decrementRefCount('ko1', 'tess'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(2); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(2); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 2, + recognizable: 2, + }); - // Decrement twice more to reach 0 ks.decrementRefCount('ko1', 'test'); ks.decrementRefCount('ko1', 'test'); - expect(ks.getObjectRefCount('ko1').reachable).toBe(0); - expect(ks.getObjectRefCount('ko1').recognizable).toBe(0); + expect(ks.getObjectRefCount('ko1')).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); // Create another object expect(ks.initKernelObject(ko2Owner)).toBe('ko2'); @@ -358,6 +368,85 @@ describe('kernel store', () => { }); }); + describe('ocap URL retention', () => { + it('pins the target and records it', () => { + const ks = makeKernelStore(mockKernelDatabase); + const kref = ks.initKernelObject('v1'); + + ks.retainForOcapURL(kref); + + expect(ks.isObjectPinned(kref)).toBe(true); + expect(ks.getOcapURLObjects()).toStrictEqual([kref]); + }); + + 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.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', () => { + 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 5c8f49fc3d..436a21cc86 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -38,9 +38,10 @@ * ${kpid}.decider = ${endid} // who decides on settlement * ${kpid}.value = JSON(CAPDATA) // value settled to, if settled * - * C-lists - * cle.${endid}.${eref} = ${kref} // ERef->KRef mapping - * clk.${endid}.${kref} = ${eref} // KRef->ERef mapping + * C-lists (both directions share one prefix; see `getCListPrefix`) + * ${endid}.c.${eref} = ${kref} // ERef->KRef mapping + * ${endid}.c.${kref} = R|_ ${eref} // KRef->ERef mapping, plus the + * // endpoint's reachable flag * * Vat bookkeeping * e.nextObjectId.${endid} = NN // allocation counter for imported object ERefs @@ -64,6 +65,7 @@ * kernelService.${serviceName} = ${koid} // kref of kernel service object ${serviceName} */ +import { Fail } from '@endo/errors'; import type { KernelDatabase, KVStore, VatStore } from '@metamask/kernel-store'; import { Logger } from '@metamask/logger'; @@ -79,6 +81,7 @@ import { getPinMethods } from './methods/pinned.ts'; import { getPromiseMethods } from './methods/promise.ts'; import { getQueueMethods } from './methods/queue.ts'; import { getReachableMethods } from './methods/reachable.ts'; +import { getRefCountAuditMethods } from './methods/refcount-audit.ts'; import { getRefCountMethods } from './methods/refcount.ts'; import { getRelayMethods } from './methods/relay.ts'; import { getRemoteMethods } from './methods/remote.ts'; @@ -86,7 +89,7 @@ import { getRevocationMethods } from './methods/revocation.ts'; import { getSubclusterMethods } from './methods/subclusters.ts'; import { getTranslators } from './methods/translators.ts'; import { getVatMethods } from './methods/vat.ts'; -import type { StoreContext } from './types.ts'; +import type { StoreContext, StoredValue } from './types.ts'; /** * Create a new KernelStore object wrapped around a raw kernel database. The @@ -112,6 +115,48 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { const { provideCachedStoredValue, provideStoredQueue } = getBaseMethods(kv); + /** + * Every cached stored value the context holds, as `field: [key, initial]`. + * Declared once so that initialization and `refreshCachedValues` cannot + * disagree about which values exist: adding one here does both. + */ + const CACHED_VALUES = { + /** Counter for allocating kernel object IDs */ + nextObjectId: ['nextObjectId', '1'], + /** Counter for allocating kernel promise IDs */ + nextPromiseId: ['nextPromiseId', '1'], + /** Counter for allocating VatIDs */ + nextVatId: ['nextVatId', '1'], + /** Counter for allocating RemoteIDs */ + nextRemoteId: ['nextRemoteId', '1'], + // Garbage collection + gcActions: ['gcActions', '[]'], + reapQueue: ['reapQueue', '[]'], + terminatedVats: ['vats.terminated', '[]'], + // Subclusters + subclusters: ['subclusters', '[]'], + nextSubclusterId: ['nextSubclusterId', '1'], + vatToSubclusterMap: ['vatToSubclusterMap', '{}'], + } as const satisfies Record; + + /** + * Provide a fresh stored value for each of {@link CACHED_VALUES}, reading its + * current setting out of the database. + * + * @returns The stored values, keyed by the context field that holds each. + */ + function provideCachedValues(): Record< + keyof typeof CACHED_VALUES, + StoredValue + > { + return Object.fromEntries( + Object.entries(CACHED_VALUES).map(([field, [key, init]]) => [ + field, + provideCachedStoredValue(key, init), + ]), + ) as Record; + } + const context: StoreContext = { kv, /** The kernel's run queue. */ @@ -122,14 +167,16 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { refreshRunQueue: () => { context.runQueue = provideStoredQueue('run', true); }, - /** Counter for allocating kernel object IDs */ - nextObjectId: provideCachedStoredValue('nextObjectId', '1'), - /** Counter for allocating kernel promise IDs */ - nextPromiseId: provideCachedStoredValue('nextPromiseId', '1'), - /** Counter for allocating VatIDs */ - nextVatId: provideCachedStoredValue('nextVatId', '1'), - /** Counter for allocating RemoteIDs */ - nextRemoteId: provideCachedStoredValue('nextRemoteId', '1'), + ...provideCachedValues(), + /** + * Re-read every cached stored value from the database. Each one closes over + * the last value written through it (see `provideCachedStoredValue`), so + * reverting the database alone is not enough: the closure would still hold + * the abandoned value and the next `set` would write it straight back. + */ + refreshCachedValues: () => { + Object.assign(context, provideCachedValues()); + }, // As refcounts are decremented, we accumulate a set of krefs for which // action might need to be taken: // * promises which are now resolved and unreferenced can be deleted @@ -141,23 +188,17 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { // the change, else removals might be lost (not performed during the next // replay). maybeFreeKrefs: new Set(), - // Garbage collection - gcActions: provideCachedStoredValue('gcActions', '[]'), - reapQueue: provideCachedStoredValue('reapQueue', '[]'), - terminatedVats: provideCachedStoredValue('vats.terminated', '[]'), inCrank: false, savepoints: [], crankBuffer: [], - // Subclusters - subclusters: provideCachedStoredValue('subclusters', '[]'), - nextSubclusterId: provideCachedStoredValue('nextSubclusterId', '1'), - vatToSubclusterMap: provideCachedStoredValue('vatToSubclusterMap', '{}'), + 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); @@ -209,23 +250,8 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { })); kdb.clear(); context.maybeFreeKrefs.clear(); - context.runQueue = provideStoredQueue('run', true); - context.gcActions = provideCachedStoredValue('gcActions', '[]'); - context.reapQueue = provideCachedStoredValue('reapQueue', '[]'); - context.terminatedVats = provideCachedStoredValue('vats.terminated', '[]'); - context.nextObjectId = provideCachedStoredValue('nextObjectId', '1'); - context.nextPromiseId = provideCachedStoredValue('nextPromiseId', '1'); - context.nextVatId = provideCachedStoredValue('nextVatId', '1'); - context.nextRemoteId = provideCachedStoredValue('nextRemoteId', '1'); - context.subclusters = provideCachedStoredValue('subclusters', '[]'); - context.nextSubclusterId = provideCachedStoredValue( - 'nextSubclusterId', - '1', - ); - context.vatToSubclusterMap = provideCachedStoredValue( - 'vatToSubclusterMap', - '{}', - ); + context.refreshRunQueue(); + context.refreshCachedValues(); crank.releaseAllSavepoints(); context.crankBuffer.length = 0; preservedState?.forEach(({ key, value }) => { @@ -291,6 +317,7 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { ...id, ...queue, ...refCount, + ...refCountAudit, ...object, ...promise, ...revocation, @@ -363,8 +390,56 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { kv.set('anonymousKernelObjects', [...krefs].sort().join(',')); } }, + + // Objects named by an issued ocap URL + // + // An ocap URL is a durable bearer token: it carries an encrypted kref and + // nothing else, so the kernel cannot discover from its own state that a + // holder exists. Retaining the target is therefore the only thing keeping + // the URL redeemable, and it has to outlive every other reference — the + // token is persistent, unexpiring, and may be redeemed by a peer that was + // not running when it was issued. `revoke` is the way to kill the + // capability; `undoOcapURLRetention` is not a release, only an unwind of a + // retention whose URL was never minted. + // + // 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): 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 = this.getOcapURLObjects(); + krefs.push(kref); + kv.set('ocapURLObjects', krefs.sort().join(',')); + this.pinObject(kref); + }, + undoOcapURLRetention(kref: KRef): void { + const krefs = this.getOcapURLObjects(); + const index = krefs.indexOf(kref); + if (index === -1) { + return; + } + krefs.splice(index, 1); + if (krefs.length === 0) { + kv.delete('ocapURLObjects'); + } else { + kv.set('ocapURLObjects', krefs.join(',')); + } + this.unpinObject(kref); + }, }); } export type KernelStore = ReturnType; export type { RelayEntry } from './types.ts'; +export type { RefCountViolation } from './methods/refcount-audit.ts'; diff --git a/packages/ocap-kernel/src/store/methods/base.ts b/packages/ocap-kernel/src/store/methods/base.ts index 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..5ce0dc12b1 --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/clist-accounting.test.ts @@ -0,0 +1,374 @@ +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 an asymmetry in c-list accounting: creating an import c-list + * entry changed no refcount while tearing one down decremented both, and + * `initKernelObject` compensated by minting every object at (1, 1). That + * constant came out right for exactly one importer, which is why nothing + * noticed. + */ +describe('c-list reference accounting', () => { + let kernelStore: ReturnType; + + /** + * Register and initialize an endpoint so it can hold c-list entries. + * + * @param vatIds - The vats to bring into existence. + */ + function givenVats(...vatIds: VatId[]): void { + for (const vatId of vatIds) { + kernelStore.setVatConfig(vatId, { sourceSpec: 'x' } as VatConfig); + kernelStore.initEndpoint(vatId); + } + } + + beforeEach(() => { + kernelStore = makeKernelStore(makeMapKernelDatabase()); + kernelStore.markInitialized(); + kernelStore.setRefCountAuditing(true); + givenVats('v1', 'v2', 'v3'); + }); + + it('counts each importer separately', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + + kernelStore.translateRefKtoE('v2', kref, true); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + + kernelStore.translateRefKtoE('v3', kref, true); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 2, + recognizable: 2, + }); + }); + + it('keeps an object alive for a second importer after the first lets go', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.translateRefKtoE('v3', kref, true); + + kernelStore.clearReachableFlag('v2', kref); + kernelStore.forgetKref('v2', kref); + kernelStore.collectGarbage(); + + // v3 still holds it, so the owner must not be told to drop or retire + expect([...kernelStore.getGCActions()]).toStrictEqual([]); + expect(kernelStore.getReachableFlag('v3', kref)).toBe(true); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('drops an object once the last of several importers lets go', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + for (const vatId of ['v2', 'v3'] as VatId[]) { + kernelStore.translateRefKtoE(vatId, kref, true); + kernelStore.clearReachableFlag(vatId, kref); + kernelStore.forgetKref(vatId, kref); + } + kernelStore.collectGarbage(); + + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v1 dropExport ${kref}`, + `v1 retireExport ${kref}`, + ]); + }); + + it('cleans up a terminated owner whose importer had already dropped', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + kernelStore.markVatAsTerminated('v1'); + + // Previously the owner's baseline decrement drove this below zero and threw + // out of the middle of the export loop, leaving the vat half-cleaned + expect(kernelStore.cleanupTerminatedVat('v1')).toStrictEqual({ + exports: 1, + imports: 0, + promises: 0, + kv: 0, + }); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 1, + }); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('restores reachability when a dropped import is handed over again', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + const eref = kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + + expect(kernelStore.getReachableFlag('v2', kref)).toBe(false); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 1, + }); + + expect(kernelStore.translateRefKtoE('v2', kref, true)).toBe(eref); + expect(kernelStore.getReachableFlag('v2', kref)).toBe(true); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }); + + it('does not inflate the count when the same import is translated twice', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.translateRefKtoE('v2', kref, true); + + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }); + + it('collects an object whose only reference went splat', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.incrementRefCount(kref, 'queue|slot'); + kernelStore.decrementRefCount(kref, 'deliver|splat|slot'); + + // Previously this settled at (1,1) with no holder, forever + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + kernelStore.collectGarbage(); + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v1 dropExport ${kref}`, + `v1 retireExport ${kref}`, + ]); + expect(kernelStore.getImporters(kref)).toStrictEqual([]); + }); + + describe('an owner that gives up its own export', () => { + it('frees the object once the last importer lets go', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + kernelStore.collectGarbage(); + + // The owner is told to drop, which clears its flag, and it then retires + // the export itself — leaving nothing naming the object from its side. + kernelStore.clearReachableFlag('v1', kref); + kernelStore.forgetKref('v1', kref); + kernelStore.orphanKernelObject(kref, 'v1'); + + kernelStore.forgetKref('v2', kref); + kernelStore.collectGarbage(); + + expect(kernelStore.kernelRefExists(kref)).toBe(false); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('collects an orphan that no importer ever recognized', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + + kernelStore.forgetKref('v1', kref); + kernelStore.orphanKernelObject(kref, 'v1'); + kernelStore.collectGarbage(); + + expect(kernelStore.getOwner(kref)).toBeUndefined(); + expect(kernelStore.kernelRefExists(kref)).toBe(false); + }); + + it('retires stragglers that still recognize an orphaned object', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + + kernelStore.clearReachableFlag('v1', kref); + kernelStore.forgetKref('v1', kref); + kernelStore.orphanKernelObject(kref, 'v1'); + kernelStore.collectGarbage(); + + // v2 can still recognize it, so it has to be told the name is dead + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v2 retireImport ${kref}`, + ]); + // v2's entry outlives the object it names until that action is delivered. + // The audit has to tolerate that window, or the end-of-crank check throws + // on a state the collector itself just created. + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('rejects an endpoint disowning an object it does not own', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + + expect(() => kernelStore.orphanKernelObject(kref, 'v2')).toThrow( + 'owned by "v1"', + ); + expect(kernelStore.getOwner(kref)).toBe('v1'); + }); + + it('survives an owner mapping left behind without a c-list entry', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + kernelStore.clearReachableFlag('v1', kref); + // Tear the owner's side down but leave the ownership record, the shape + // that used to make the next collection read a key that wasn't there. + kernelStore.forgetKref('v1', kref); + kernelStore.forgetKref('v2', kref); + + expect(() => kernelStore.collectGarbage()).not.toThrow(); + expect(kernelStore.getOwner(kref)).toBeUndefined(); + expect(kernelStore.kernelRefExists(kref)).toBe(false); + }); + }); + + describe('cleanupTerminatedVat', () => { + it('does nothing for a vat that is not terminated', () => { + expect(kernelStore.cleanupTerminatedVat('v1')).toStrictEqual({ + exports: 0, + imports: 0, + promises: 0, + kv: 0, + }); + }); + + it('orphans exports, releases imports, and forgets the vat', () => { + const mine = kernelStore.exportFromEndpoint('v1', 'o+1'); + const theirs = kernelStore.exportFromEndpoint('v2', 'o+1'); + kernelStore.translateRefKtoE('v1', theirs, true); + kernelStore.translateRefKtoE('v3', mine, true); + kernelStore.markVatAsTerminated('v1'); + + const work = kernelStore.cleanupTerminatedVat('v1'); + + expect(work).toMatchObject({ exports: 1, imports: 1, promises: 0 }); + // v1's export is orphaned but still recognized by v3 + expect(kernelStore.getOwner(mine)).toBeUndefined(); + expect(kernelStore.getObjectRefCount(mine)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + // v1's import of v2's object is released + expect(kernelStore.getObjectRefCount(theirs)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + expect(kernelStore.hasCListEntry('v1', mine)).toBe(false); + expect(kernelStore.hasCListEntry('v1', theirs)).toBe(false); + expect(kernelStore.isVatTerminated('v1')).toBe(false); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('releases the c-list entry of a promise the vat was deciding', () => { + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.setPromiseDecider(kpid, 'v1'); + kernelStore.translateRefKtoE('v2', kpid, true); + // The caller rejects the orphans first, which is what releases the + // unsettled-promise reference and clears the decider + expect([...kernelStore.getPromisesByDecider('v1')]).toStrictEqual([kpid]); + kernelStore.resolveKernelPromise(kpid, true, { + body: '#"gone"', + slots: [], + }); + kernelStore.markVatAsTerminated('v1'); + + const work = kernelStore.cleanupTerminatedVat('v1'); + + expect(work).toMatchObject({ exports: 0, imports: 0, promises: 1 }); + expect(kernelStore.getRefCount(kpid)).toBe(1); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('tolerates an importer entry that outlives the object it names', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v3', kref, true); + // v3 has let go of the object but can still recognize the name + kernelStore.clearReachableFlag('v3', kref); + kernelStore.markVatAsTerminated('v1'); + kernelStore.cleanupTerminatedVat('v1'); + + kernelStore.collectGarbage(); + + // The collector deletes the object and queues the retirement together, so + // v3's entry names a kref the kernel has already dropped until that action + // is delivered. Counting it as a holder fails the end-of-crank audit on a + // state the collector itself just created, which kills the run loop. + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v3 retireImport ${kref}`, + ]); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('leaves a live vat that shares the object untouched', () => { + const kref = kernelStore.exportFromEndpoint('v2', 'o+1'); + kernelStore.translateRefKtoE('v1', kref, true); + kernelStore.translateRefKtoE('v3', kref, true); + kernelStore.markVatAsTerminated('v1'); + + kernelStore.cleanupTerminatedVat('v1'); + kernelStore.collectGarbage(); + + expect(kernelStore.getReachableFlag('v3', kref)).toBe(true); + expect([...kernelStore.getGCActions()]).toStrictEqual([]); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }); + }); + + describe('three endpoints sharing one object', () => { + it('accounts for every hand-off and release in turn', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + const importers = ['v2', 'v3'] as VatId[]; + + for (const vatId of importers) { + kernelStore.translateRefKtoE(vatId, kref, true); + } + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 2, + recognizable: 2, + }); + expect(kernelStore.getImporters(kref)).toStrictEqual(importers); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + + // v2 drops but still recognizes + kernelStore.clearReachableFlag('v2', kref); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 1, + recognizable: 2, + }); + kernelStore.collectGarbage(); + expect([...kernelStore.getGCActions()]).toStrictEqual([]); + + // v2 retires; v3 keeps it alive + kernelStore.forgetKref('v2', kref); + kernelStore.collectGarbage(); + expect([...kernelStore.getGCActions()]).toStrictEqual([]); + expect(kernelStore.getImporters(kref)).toStrictEqual(['v3']); + + // v3 lets go too, and only now is the owner told + kernelStore.clearReachableFlag('v3', kref); + kernelStore.forgetKref('v3', kref); + kernelStore.collectGarbage(); + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v1 dropExport ${kref}`, + `v1 retireExport ${kref}`, + ]); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + }); +}); diff --git a/packages/ocap-kernel/src/store/methods/clist.test.ts b/packages/ocap-kernel/src/store/methods/clist.test.ts index e43d6e428a..4df8841bf9 100644 --- a/packages/ocap-kernel/src/store/methods/clist.test.ts +++ b/packages/ocap-kernel/src/store/methods/clist.test.ts @@ -27,39 +27,58 @@ describe('clist-methods', () => { }); describe('addCListEntry', () => { - it('adds a bidirectional mapping between KRef and ERef', () => { - const endpointId: EndpointId = 'v1'; - const kref: KRef = 'ko1'; - const eref: ERef = 'o-1'; + it.each([ + { what: 'an object import', kref: 'ko1', eref: 'o-1', flag: '_' }, + { what: 'an object export', kref: 'ko1', eref: 'o+1', flag: 'R' }, + { what: 'a promise import', kref: 'kp1', eref: 'p-2', flag: '_' }, + { what: 'a promise export', kref: 'kp1', eref: 'p+2', flag: 'R' }, + ] as { what: string; kref: KRef; eref: ERef; flag: string }[])( + 'adds a bidirectional mapping for $what', + ({ kref, eref, flag }) => { + const endpointId: EndpointId = 'v1'; + + clistMethods.addCListEntry(endpointId, kref, eref); + + // Only an export is born reachable; an import earns reachability when + // the reference is actually handed over + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`${flag} ${eref}`); + expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); + }, + ); + + it('works with remote endpoints', () => { + const endpointId: EndpointId = 'r1'; + const kref: KRef = 'ko2'; + const eref: ERef = 'ro+3'; clistMethods.addCListEntry(endpointId, kref, eref); - // Check that both mappings are stored expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); }); - it('works with promise refs', () => { - const endpointId: EndpointId = 'v1'; - const kref: KRef = 'kp1'; - const eref: ERef = 'p+2'; + it('takes a recognizable reference for an object import', () => { + clistMethods.addCListEntry('v1', 'ko1', 'o-1'); - clistMethods.addCListEntry(endpointId, kref, eref); + expect(kv.get('ko1.refCount')).toBe('0,1'); + }); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); - expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); + it('takes no reference for an object export', () => { + clistMethods.addCListEntry('v1', 'ko1', 'o+1'); + + expect(kv.get('ko1.refCount')).toBeUndefined(); }); - it('works with remote endpoints', () => { - const endpointId: EndpointId = 'r1'; - const kref: KRef = 'ko2'; - const eref: ERef = 'ro+3'; + it.each(['p-1', 'p+1'] as ERef[])( + 'takes a reference for a promise entry (%s)', + (eref) => { + kv.set('kp1.refCount', '1'); - clistMethods.addCListEntry(endpointId, kref, eref); + clistMethods.addCListEntry('v1', 'kp1', eref); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); - expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); - }); + expect(kv.get('kp1.refCount')).toBe('2'); + }, + ); }); describe('hasCListEntry', () => { @@ -96,7 +115,7 @@ describe('clist-methods', () => { expect(kv.get(`e.nextObjectId.${endpointId}`)).toBe('2'); // Check that the mapping was added - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); }); @@ -113,7 +132,7 @@ describe('clist-methods', () => { expect(kv.get(`e.nextPromiseId.${endpointId}`)).toBe('2'); // Check that the mapping was added - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); }); @@ -151,7 +170,7 @@ describe('clist-methods', () => { }); }); - describe('krefsToExistingErefs', () => { + describe('krefsToErefs', () => { it('returns the ERefs for existing KRefs', () => { const endpointId: EndpointId = 'v1'; const kref1: KRef = 'ko1'; @@ -163,25 +182,18 @@ describe('clist-methods', () => { clistMethods.addCListEntry(endpointId, kref2, eref2); expect( - clistMethods.krefsToExistingErefs(endpointId, [kref1, kref2]), + clistMethods.krefsToErefs(endpointId, [kref1, kref2]), ).toStrictEqual([eref1, eref2]); }); - it('returns an empty array for non-existent KRefs', () => { - const endpointId: EndpointId = 'v1'; - const kref: KRef = 'ko1'; - - expect( - clistMethods.krefsToExistingErefs(endpointId, [kref]), - ).toStrictEqual([]); + it('throws for an unmapped KRef', () => { + expect(() => clistMethods.krefsToErefs('v1', ['ko1'])).toThrow( + 'unmapped kref "ko1" in "v1" c-list', + ); }); it('returns an empty array for empty KRef array', () => { - const endpointId: EndpointId = 'v1'; - - expect(clistMethods.krefsToExistingErefs(endpointId, [])).toStrictEqual( - [], - ); + expect(clistMethods.krefsToErefs('v1', [])).toStrictEqual([]); }); }); @@ -191,7 +203,7 @@ describe('clist-methods', () => { const kref: KRef = 'ko1'; const eref: ERef = 'o-1'; clistMethods.addCListEntry(endpointId, kref, eref); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); clistMethods.forgetEref(endpointId, eref); expect(kv.get(`${endpointId}.c.${kref}`)).toBeUndefined(); @@ -214,7 +226,7 @@ describe('clist-methods', () => { const kref: KRef = 'ko1'; const eref: ERef = 'o-1'; clistMethods.addCListEntry(endpointId, kref, eref); - expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`R ${eref}`); + expect(kv.get(`${endpointId}.c.${kref}`)).toBe(`_ ${eref}`); expect(kv.get(`${endpointId}.c.${eref}`)).toBe(kref); clistMethods.forgetKref(endpointId, kref); expect(kv.get(`${endpointId}.c.${kref}`)).toBeUndefined(); diff --git a/packages/ocap-kernel/src/store/methods/clist.ts b/packages/ocap-kernel/src/store/methods/clist.ts index 425f4a1295..d7e3a52af0 100644 --- a/packages/ocap-kernel/src/store/methods/clist.ts +++ b/packages/ocap-kernel/src/store/methods/clist.ts @@ -1,3 +1,5 @@ +import { Fail } from '@endo/errors'; + import { getBaseMethods } from './base.ts'; import { getReachableMethods } from './reachable.ts'; import { getRefCountMethods } from './refcount.ts'; @@ -20,23 +22,32 @@ import { export function getCListMethods(ctx: StoreContext) { const { getSlotKey } = getBaseMethods(ctx.kv); const { clearReachableFlag } = getReachableMethods(ctx); - const { decrementRefCount } = getRefCountMethods(ctx); + const { incrementRefCount, decrementRefCount } = getRefCountMethods(ctx); /** * Add an entry to an endpoint's c-list, creating a new bidirectional mapping * between an ERef belonging to the endpoint and a KRef belonging to the * kernel. * + * The entry is itself a reference, so creating one takes a count, mirroring + * {@link deleteCListEntry}. An import is born recognizing but not reaching: + * reachability is `setReachableFlag`'s job, when the reference is handed + * over. An export takes no count for an object — the owner is not one of its + * own referrers — and is born flagged. For a promise both directions count; + * only objects exempt the owner. + * * @param endpointId - The endpoint whose c-list is to be added to. * @param kref - The KRef. * @param eref - The ERef. */ function addCListEntry(endpointId: EndpointId, kref: KRef, eref: ERef): void { + const isExport = parseRef(eref).direction === 'export'; ctx.kv.set( getSlotKey(endpointId, kref), - buildReachableAndVatSlot(true, eref), + buildReachableAndVatSlot(isExport, eref), ); ctx.kv.set(getSlotKey(endpointId, eref), kref); + incrementRefCount(kref, 'add|kref', { isExport, onlyRecognizable: true }); } /** @@ -133,16 +144,23 @@ export function getCListMethods(ctx: StoreContext) { } /** - * Look up the ERefs that an endpoint's c-list maps aa list of KRefs to. + * Look up the ERefs that an endpoint's c-list maps a list of KRefs to, + * without allocating entries or disturbing reachability. + * + * Every kref must already be mapped: a missing entry means the caller's list + * of krefs and the c-list disagree, which is worth hearing about rather than + * silently dropping the one that got away. * * @param endpointId - The endpoint in question. * @param krefs - The KRefs to look up. - * @returns The given endpoint's ERefs corresponding to `krefs` + * @returns The given endpoint's ERefs corresponding to `krefs`. */ - function krefsToExistingErefs(endpointId: EndpointId, krefs: KRef[]): ERef[] { - return krefs - .map((kref) => krefToEref(endpointId, kref)) - .filter((eref): eref is ERef => Boolean(eref)); + function krefsToErefs(endpointId: EndpointId, krefs: KRef[]): ERef[] { + return krefs.map( + (kref) => + krefToEref(endpointId, kref) ?? + Fail`unmapped kref ${kref} in ${endpointId} c-list`, + ); } /** @@ -182,6 +200,6 @@ export function getCListMethods(ctx: StoreContext) { krefToEref, forgetEref, forgetKref, - krefsToExistingErefs, + krefsToErefs, }; } diff --git a/packages/ocap-kernel/src/store/methods/crank.test.ts b/packages/ocap-kernel/src/store/methods/crank.test.ts index db3de86450..426890d209 100644 --- a/packages/ocap-kernel/src/store/methods/crank.test.ts +++ b/packages/ocap-kernel/src/store/methods/crank.test.ts @@ -17,6 +17,8 @@ describe('crank methods', () => { savepoints: [], crankBuffer: mockCrankBuffer, refreshRunQueue: vi.fn(), + refreshCachedValues: vi.fn(), + maybeFreeKrefs: new Set(), } as unknown as StoreContext; kdb = { @@ -153,6 +155,69 @@ describe('crank methods', () => { expect(mockCrankBuffer).toHaveLength(0); }); + + // A failed rollback discards every savepoint, not just this one. Truncating + // to the ordinal would have `endCrank` release a `t0` the database lacks and + // throw over whatever really killed the kernel. + it('forgets every savepoint when the rollback fails', () => { + context.inCrank = true; + crankMethods.createCrankSavepoint('crank'); + crankMethods.createCrankSavepoint('delivery'); + vi.mocked(kdb.rollbackSavepoint).mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + + expect(() => crankMethods.rollbackCrank('delivery')).toThrow( + 'disk I/O error', + ); + + expect(context.savepoints).toStrictEqual([]); + // The release `endCrank` would otherwise attempt, and throw over. + crankMethods.endCrank(); + expect(kdb.releaseSavepoint).not.toHaveBeenCalled(); + }); + + // The two halves of this function compose the wrong way round if the + // failure path simply rethrows: a failed rollback discards the whole + // transaction, so the database has moved back at least as far as a + // successful rollback would have taken it and the caches it left behind are + // at least as stale. + it('reverts the caches the database cannot reach even when the rollback fails', () => { + context.inCrank = true; + context.maybeFreeKrefs.add('kp1'); + crankMethods.createCrankSavepoint('crank'); + crankMethods.createCrankSavepoint('delivery'); + vi.mocked(kdb.rollbackSavepoint).mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + + expect(() => crankMethods.rollbackCrank('delivery')).toThrow( + 'disk I/O error', + ); + + expect(context.refreshCachedValues).toHaveBeenCalled(); + expect(context.refreshRunQueue).toHaveBeenCalled(); + expect(context.runQueueLengthCache).toBe(-1); + expect([...context.maybeFreeKrefs]).toStrictEqual([]); + }); + + // Reverting must not become a way to lose the database error either. + it('keeps the rollback failure as the cause when reverting also fails', () => { + context.inCrank = true; + const rollbackFailure = new Error('disk I/O error'); + crankMethods.createCrankSavepoint('crank'); + crankMethods.createCrankSavepoint('delivery'); + vi.mocked(kdb.rollbackSavepoint).mockImplementationOnce(() => { + throw rollbackFailure; + }); + vi.mocked(context.refreshCachedValues).mockImplementationOnce(() => { + throw new Error('database is gone'); + }); + + expect(() => crankMethods.rollbackCrank('delivery')).toThrow( + expect.objectContaining({ cause: rollbackFailure }), + ); + }); }); describe('endCrank', () => { @@ -206,6 +271,21 @@ describe('crank methods', () => { expect(context.resolveCrank).toBeUndefined(); expect(await waiter).toBeUndefined(); }); + + // As `rollbackCrank` does. Left listed, the next crank numbers its savepoint + // `t1` against a database that has none, and every later release and rollback + // aims one crank past its target. + it('forgets its savepoints even if releasing them fails', () => { + crankMethods.startCrank(); + context.savepoints = ['test']; + vi.mocked(kdb.releaseSavepoint).mockImplementationOnce(() => { + throw new Error('database is gone'); + }); + + expect(() => crankMethods.endCrank()).toThrow('database is gone'); + + expect(context.savepoints).toStrictEqual([]); + }); }); describe('releaseAllSavepoints', () => { diff --git a/packages/ocap-kernel/src/store/methods/crank.ts b/packages/ocap-kernel/src/store/methods/crank.ts index 87d2bc65b8..a3bba1956b 100644 --- a/packages/ocap-kernel/src/store/methods/crank.ts +++ b/packages/ocap-kernel/src/store/methods/crank.ts @@ -51,34 +51,84 @@ export function getCrankMethods(ctx: StoreContext, kdb: KernelDatabase) { if (ctx.savepoints[ordinal] === savepoint) { try { kdb.rollbackSavepoint(`t${ordinal}`); - } finally { - // Forget the savepoint even if the rollback failed. Leaving it listed - // would have `endCrank`'s release commit the crank we just abandoned — - // the half-finished state this rollback exists to discard. A failed - // rollback discards the whole transaction instead (see - // `rollbackSavepoint`), which for a crank is the same boundary. + // Left listed, `endCrank`'s release would commit the crank we just + // abandoned. ctx.savepoints.length = ordinal; + } catch (error) { + // A failed rollback discards the whole transaction, so every savepoint + // is gone, not just this one. Truncating to `ordinal` would have + // `endCrank` release a `t0` the database lacks and throw over whatever + // really killed the kernel. + ctx.savepoints.length = 0; + // Before the rethrow, and not only on the path below. A failed + // rollback discards the whole transaction, so the database has moved + // back at least as far as a successful rollback would have taken it + // and these caches are at least as stale. Rethrowing ahead of this + // would leave the dying crank holding the GC action it consumed and + // the freed krefs it was about to collect. + revertStateBeneathRollback(error); + throw error; } - // The rollback reverted DB state but in-memory caches are stale. - // Recreate the run queue so its cached head/tail are re-read from DB. - ctx.refreshRunQueue(); - // Invalidate the run queue length cache so it's recalculated from - // the database on next access, since the rollback may have restored - // dequeued items. - ctx.runQueueLengthCache = -1; + revertStateBeneathRollback(); return; } } Fail`no such savepoint as "${q(savepoint)}"`; } + /** + * Revert what a database rollback cannot reach: the in-memory caches built + * over the abandoned crank's writes. + * + * @param rollbackError - The error the rollback threw, if it threw. Kept as + * the `cause` should reverting fail too, since it is the root cause an + * operator needs. + */ + function revertStateBeneathRollback(rollbackError?: unknown): void { + try { + // Recreate the run queue so its cached head/tail are re-read from the + // database, and invalidate the length cache, since the rollback may have + // restored dequeued items. + ctx.refreshRunQueue(); + ctx.runQueueLengthCache = -1; + // Same staleness, worse consequence: a cached value reads from its + // closure and only writes through to kv, so one this crank consumed stays + // consumed and the next `set` persists that. `processGCActionSet` takes an + // action out of the set before delivering it, so an action not restored + // here is lost rather than retried. + ctx.refreshCachedValues(); + // Nothing rolls back RAM. These krefs are collection candidates only + // because this crank decremented them, and that is precisely what was just + // undone. Left in place, `collectGarbage` throws on a later crank for any + // promise this one created — killing the run loop over work that no longer + // exists. Correct only while every rollback discards the whole delivery, + // which is all any caller asks for. + ctx.maybeFreeKrefs.clear(); + } catch (revertError) { + if (rollbackError === undefined) { + throw revertError; + } + throw new Error( + `Crank rollback failed and its caches could not be reverted: ${String(revertError)}`, + { cause: rollbackError }, + ); + } + } + /** * Release all savepoints. */ function releaseAllSavepoints(): void { if (ctx.savepoints.length > 0) { - kdb.releaseSavepoint('t0'); - ctx.savepoints.length = 0; + try { + kdb.releaseSavepoint('t0'); + } finally { + // A failed release discards the transaction too, so the database has no + // savepoints left either. Left listed, the next crank would number its + // savepoint `t1` and aim every later release and rollback one crank past + // the one it meant to end. + ctx.savepoints.length = 0; + } } } diff --git a/packages/ocap-kernel/src/store/methods/gc.test.ts b/packages/ocap-kernel/src/store/methods/gc.test.ts index a294920e9a..e91fb29bb9 100644 --- a/packages/ocap-kernel/src/store/methods/gc.test.ts +++ b/packages/ocap-kernel/src/store/methods/gc.test.ts @@ -76,21 +76,6 @@ describe('GC methods', () => { }); }); - describe('reachability tracking', () => { - it('manages reachable flags', () => { - const v1Object = kernelStore.initKernelObject('v1'); - kernelStore.addCListEntry('v1', v1Object, 'o-1'); - - expect(kernelStore.getReachableFlag('v1', v1Object)).toBe(true); - - kernelStore.clearReachableFlag('v1', v1Object); - expect(kernelStore.getReachableFlag('v1', v1Object)).toBe(false); - - const refCounts = kernelStore.getObjectRefCount(v1Object); - expect(refCounts.reachable).toBe(0); - }); - }); - describe('reaping', () => { it('processes reap queue in order', () => { const vatIds = ['v1', 'v2', 'v3']; diff --git a/packages/ocap-kernel/src/store/methods/gc.ts b/packages/ocap-kernel/src/store/methods/gc.ts index 04f3c0cc0c..92a093c968 100644 --- a/packages/ocap-kernel/src/store/methods/gc.ts +++ b/packages/ocap-kernel/src/store/methods/gc.ts @@ -1,6 +1,7 @@ import { Fail } from '@endo/errors'; import { getBaseMethods } from './base.ts'; +import { getCListMethods } from './clist.ts'; import { getObjectMethods } from './object.ts'; import { getPromiseMethods } from './promise.ts'; import { getReachableMethods } from './reachable.ts'; @@ -33,6 +34,37 @@ export function getGCMethods(ctx: StoreContext) { const { getImporters, isVatTerminated } = getVatMethods(ctx); const { getReachableFlag, getReachableAndVatSlot } = getReachableMethods(ctx); const { clearEmptySubclusters } = getSubclusterMethods(ctx); + const { hasCListEntry } = getCListMethods(ctx); + + /** + * Give up the kernel's record of who owns an object. The object survives only + * as long as something still names it; the collector disposes of it from + * there, retiring any stragglers that still recognize it. + * + * Called when an owner stops naming its own export — it retired or abandoned + * it, or a GC `retireExport` was delivered. Without this the owner mapping + * outlives the c-list entry it was reachable through, which both leaks the + * object record and leaves `collectGarbage` reading a c-list entry that is no + * longer there. + * + * Disowning an object is only ever the owner's own doing, so `expectedOwner` + * is required: taking it on trust would let one endpoint erase another's claim + * to an object it is still exporting. An object that is already orphaned is + * left alone — the caller and the kernel agree it has no owner. + * + * @param kref - The object whose owner mapping is to be dropped. + * @param expectedOwner - The endpoint the caller believes owns `kref`. + */ + function orphanKernelObject(kref: KRef, expectedOwner: EndpointId): void { + const owner = getOwner(kref); + if (owner === undefined) { + return; + } + owner === expectedOwner || + Fail`cannot orphan ${kref} for ${expectedOwner}: owned by ${owner}`; + ctx.kv.delete(getOwnerKey(kref)); + ctx.maybeFreeKrefs.add(kref); + } /** * Get the set of GC actions to perform. @@ -158,15 +190,28 @@ export function getGCMethods(ctx: StoreContext) { // might still alive, or might be terminated and in the // process of being deleted. These two clauses are // mutually exclusive. - if (ownerVatID && !terminated) { + if (ownerVatID && !terminated && !hasCListEntry(ownerVatID, kref)) { + // Should be unreachable: every path that tears down an owner's + // export entry orphans the object with it. Repair it so the + // collector can keep going, but say so — absorbing this in silence + // would hide whatever upstream broke the pairing. + ctx.logger?.error( + `${kref} is owned by live endpoint ${ownerVatID} which has no ` + + `c-list entry for it; treating it as orphaned`, + ); + orphanKernelObject(kref, ownerVatID); + ownerVatID = undefined; + } else if (ownerVatID && !terminated) { const vatConsidersReachable = getReachableFlag(ownerVatID, kref); if (vatConsidersReachable) { // the reachable count is zero, but the vat doesn't realize it actions.add(makeGCAction(ownerVatID, 'dropExport', kref)); } if (recognizable === 0) { - // TODO: rethink this assert - // assert.equal(vatConsidersReachable, false, `${kref} is reachable but not recognizable`); + // No assertion that the owner has stopped considering this + // reachable: when the last holder both drops and retires before + // we run, we queue dropExport and retireExport together and the + // owner's flag is still set until the first of them is delivered. actions.add(makeGCAction(ownerVatID, 'retireExport', kref)); } } else if (ownerVatID && terminated) { @@ -219,6 +264,7 @@ export function getGCMethods(ctx: StoreContext) { scheduleReap, nextReapAction, retireKernelObjects, + orphanKernelObject, collectGarbage, }; } diff --git a/packages/ocap-kernel/src/store/methods/object.test.ts b/packages/ocap-kernel/src/store/methods/object.test.ts index def40adaef..26d4fb9b16 100644 --- a/packages/ocap-kernel/src/store/methods/object.test.ts +++ b/packages/ocap-kernel/src/store/methods/object.test.ts @@ -29,7 +29,7 @@ describe('object-methods', () => { }); describe('initKernelObject', () => { - it('creates a new kernel object with initial reference counts', () => { + it('creates a new kernel object, unreferenced', () => { const owner: EndpointId = 'v1'; const koId = objectStore.initKernelObject(owner); @@ -39,13 +39,13 @@ describe('object-methods', () => { // Check the owner is set correctly expect(kv.get(`${koId}.owner`)).toBe(owner); - // Check reference counts are initialized to 1,1 - expect(kv.get(`${koId}.refCount`)).toBe('1,1'); - - // Check via the API - const refCounts = objectStore.getObjectRefCount(koId); - expect(refCounts.reachable).toBe(1); - expect(refCounts.recognizable).toBe(1); + // A new object has no referrers yet; the owner's own export entry is + // not one of them + expect(kv.get(`${koId}.refCount`)).toBe('0,0'); + expect(objectStore.getObjectRefCount(koId)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); }); it('initializes the revoked flag to false', () => { @@ -171,8 +171,10 @@ describe('object-methods', () => { it('returns reference counts for existing objects', () => { const koId = objectStore.initKernelObject('v1'); + objectStore.setObjectRefCount(koId, { reachable: 1, recognizable: 2 }); + const refCounts = objectStore.getObjectRefCount(koId); - expect(refCounts).toStrictEqual({ reachable: 1, recognizable: 1 }); + expect(refCounts).toStrictEqual({ reachable: 1, recognizable: 2 }); }); it('returns zero counts for non-existent objects', () => { @@ -276,8 +278,8 @@ describe('object-methods', () => { // Check initial state expect(objectStore.getOwner(koId)).toBe('v1'); expect(objectStore.getObjectRefCount(koId)).toStrictEqual({ - reachable: 1, - recognizable: 1, + reachable: 0, + recognizable: 0, }); // Update reference counts diff --git a/packages/ocap-kernel/src/store/methods/object.ts b/packages/ocap-kernel/src/store/methods/object.ts index 9e596f5e8f..57b7aaf88e 100644 --- a/packages/ocap-kernel/src/store/methods/object.ts +++ b/packages/ocap-kernel/src/store/methods/object.ts @@ -19,10 +19,11 @@ export function getObjectMethods(ctx: StoreContext) { getBaseMethods(ctx.kv); /** - * Create a new kernel object. The new object will be born with reference and - * recognizability counts of 1, on the assumption that the new object - * corresponds to an object that has just been imported from somewhere. The - * object is initially unrevoked. + * Create a new kernel object, born unreferenced at `(0, 0)`. Every unit of + * an object's counts is owed to a reference someone else holds — an + * importer's c-list entry, a queued message, a promise's resolution value, a + * pin — and the owner's own export entry is not one of them. The object is + * initially unrevoked. * * @param owner - The endpoint or 'kernel' that is the owner of the new object. * @returns The new object's KRef. @@ -30,7 +31,7 @@ export function getObjectMethods(ctx: StoreContext) { function initKernelObject(owner: EndpointId | 'kernel'): KRef { const koId = getNextObjectId(); ctx.kv.set(getOwnerKey(koId), owner); - setObjectRefCount(koId, { reachable: 1, recognizable: 1 }); + setObjectRefCount(koId, { reachable: 0, recognizable: 0 }); return koId; } diff --git a/packages/ocap-kernel/src/store/methods/promise.test.ts b/packages/ocap-kernel/src/store/methods/promise.test.ts index baeba6c2e5..d85e291ef5 100644 --- a/packages/ocap-kernel/src/store/methods/promise.test.ts +++ b/packages/ocap-kernel/src/store/methods/promise.test.ts @@ -58,6 +58,7 @@ describe('promise store methods', () => { }; let context: StoreContext; let promiseMethods: ReturnType; + const mockIncrementRefCount = vi.fn(); const mockDecrementRefCount = vi.fn(); beforeEach(() => { @@ -78,6 +79,7 @@ describe('promise store methods', () => { incCounter: mockIncCounter, provideStoredQueue: mockProvideStoredQueue, getPrefixedKeys: mockGetPrefixedKeys, + getCListPrefix: (endpointId: string) => `${endpointId}.c.`, }); (getQueueMethods as ReturnType).mockReturnValue({ @@ -85,6 +87,7 @@ describe('promise store methods', () => { }); (getRefCountMethods as ReturnType).mockReturnValue({ + incrementRefCount: mockIncrementRefCount, decrementRefCount: mockDecrementRefCount, }); @@ -304,11 +307,12 @@ describe('promise store methods', () => { slots: ['o+1', 'o+2'], }; const message1: KernelMessage = { - method: 'method1', - } as unknown as KernelMessage; + methargs: { body: 'method1', slots: ['ko7'] }, + result: 'kp8', + }; const message2: KernelMessage = { - method: 'method2', - } as unknown as KernelMessage; + methargs: { body: 'method2', slots: [] }, + }; mockKV.set(`${kpid}.state`, 'unresolved'); mockKV.set(`${kpid}.decider`, 'v1'); @@ -338,7 +342,15 @@ describe('promise store methods', () => { expect(mockKV.has(`${kpid}.decider`)).toBe(false); expect(mockKV.has(`${kpid}.subscribers`)).toBe(false); expect(mockQueue.delete).toHaveBeenCalled(); - expect(mockDecrementRefCount).toHaveBeenCalledTimes(1); + // Each dequeued message releases what its queue entry held, then the + // promise releases the decision it was owed + expect(mockDecrementRefCount.mock.calls).toStrictEqual([ + [kpid, 'resolve|dequeue|target'], + ['kp8', 'resolve|dequeue|result'], + ['ko7', 'resolve|dequeue|slot'], + [kpid, 'resolve|dequeue|target'], + [kpid, 'resolve|decider'], + ]); }); it('rejects a promise and enqueues pending messages', () => { @@ -372,6 +384,23 @@ describe('promise store methods', () => { expect(mockProvideStoredQueue).toHaveBeenCalledWith(kpid, false); expect(mockQueue.enqueue).toHaveBeenCalledWith(message); }); + + it('takes a reference on everything the queued message carries', () => { + const kpid = 'kp123'; + const message: KernelMessage = { + methargs: { body: 'test', slots: ['ko1', 'kp2'] }, + result: 'kp3', + }; + + promiseMethods.enqueuePromiseMessage(kpid, message); + + expect(mockIncrementRefCount.mock.calls).toStrictEqual([ + [kpid, 'promiseQueue|target'], + ['kp3', 'promiseQueue|result'], + ['ko1', 'promiseQueue|slot'], + ['kp2', 'promiseQueue|slot'], + ]); + }); }); describe('getKernelPromiseMessageQueue', () => { @@ -410,69 +439,71 @@ describe('promise store methods', () => { }); describe('getPromisesByDecider', () => { - it('yields promises decided by a specific vat', () => { - const vatId = 'v1' as VatId; - const kpid1 = 'kp101'; - const kpid2 = 'kp102'; - const kpid3 = 'kp103'; - - // Set up mock data - mockGetPrefixedKeys.mockReturnValue([ - `cle.${vatId}.p1`, - `cle.${vatId}.p2`, - `cle.${vatId}.p3`, - ]); - - mockKV.set(`cle.${vatId}.p1`, kpid1); - mockKV.set(`cle.${vatId}.p2`, kpid2); - mockKV.set(`cle.${vatId}.p3`, kpid3); - - // kpid1 is decided by vatId - mockKV.set(`${kpid1}.state`, 'unresolved'); - mockKV.set(`${kpid1}.decider`, vatId); - mockKV.set(`${kpid1}.subscribers`, '[]'); - - // kpid2 is also decided by vatId - mockKV.set(`${kpid2}.state`, 'unresolved'); - mockKV.set(`${kpid2}.decider`, vatId); - mockKV.set(`${kpid2}.subscribers`, '[]'); + /** + * Populate a c-list and an unresolved promise record, using the real key + * layout so the scan is exercised rather than mocked around. + * + * @param endpointId - The endpoint whose c-list to add to. + * @param eref - The endpoint's ref for the promise. + * @param kpid - The kernel promise. + * @param decider - The promise's decider, if it has one. + * @param state - The promise's state. + */ + function givenCListPromise( + endpointId: string, + eref: string, + kpid: string, + decider: string | undefined, + state = 'unresolved', + ): void { + mockKV.set(`${endpointId}.c.${eref}`, kpid); + mockKV.set(`${endpointId}.c.${kpid}`, `R ${eref}`); + mockKV.set(`${kpid}.state`, state); + mockKV.set(`${kpid}.subscribers`, '[]'); + if (state === 'unresolved') { + if (decider) { + mockKV.set(`${kpid}.decider`, decider); + } + } else { + mockKV.set(`${kpid}.value`, '{"body":"value","slots":[]}'); + } + mockGetPrefixedKeys.mockImplementation((prefix: string) => + [...mockKV.keys()].filter((key) => key.startsWith(prefix)).sort(), + ); + } - // kpid3 is unresolved but decided by a different vat - mockKV.set(`${kpid3}.state`, 'unresolved'); - mockKV.set(`${kpid3}.decider`, 'v2'); - mockKV.set(`${kpid3}.subscribers`, '[]'); + it.each([ + { context: 'a vat', endpointId: 'v1', erefs: ['p+1', 'p-2'] }, + { context: 'a remote', endpointId: 'r1', erefs: ['rp+1', 'rp-2'] }, + ])('yields promises decided by $context', ({ endpointId, erefs }) => { + givenCListPromise(endpointId, erefs[0] as string, 'kp101', endpointId); + givenCListPromise(endpointId, erefs[1] as string, 'kp102', endpointId); + givenCListPromise(endpointId, 'p+3', 'kp103', 'v2'); - const result = Array.from(promiseMethods.getPromisesByDecider(vatId)); + const result = Array.from( + promiseMethods.getPromisesByDecider(endpointId as VatId), + ); - expect(result).toStrictEqual([kpid1, kpid2]); - expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`cle.${vatId}.p`); + expect(result).toStrictEqual(['kp101', 'kp102']); }); it('does not yield resolved promises', () => { - const vatId = 'v1' as VatId; - const kpid1 = 'kp101'; - const kpid2 = 'kp102'; + givenCListPromise('v1', 'p+1', 'kp101', undefined, 'fulfilled'); + givenCListPromise('v1', 'p+2', 'kp102', 'v1'); - mockGetPrefixedKeys.mockReturnValue([ - `cle.${vatId}.p1`, - `cle.${vatId}.p2`, - ]); + const result = Array.from(promiseMethods.getPromisesByDecider('v1')); - mockKV.set(`cle.${vatId}.p1`, kpid1); - mockKV.set(`cle.${vatId}.p2`, kpid2); - - // kpid1 is fulfilled - mockKV.set(`${kpid1}.state`, 'fulfilled'); - mockKV.set(`${kpid1}.value`, '{"body":"value","slots":[]}'); + expect(result).toStrictEqual(['kp102']); + }); - // kpid2 is unresolved and decided by vatId - mockKV.set(`${kpid2}.state`, 'unresolved'); - mockKV.set(`${kpid2}.decider`, vatId); - mockKV.set(`${kpid2}.subscribers`, '[]'); + it('ignores object entries in the same c-list', () => { + givenCListPromise('v1', 'p+1', 'kp101', 'v1'); + mockKV.set('v1.c.o+1', 'ko1'); + mockKV.set('v1.c.ko1', 'R o+1'); - const result = Array.from(promiseMethods.getPromisesByDecider(vatId)); + const result = Array.from(promiseMethods.getPromisesByDecider('v1')); - expect(result).toStrictEqual([kpid2]); + expect(result).toStrictEqual(['kp101']); }); it('yields nothing if no promises are decided by the vat', () => { diff --git a/packages/ocap-kernel/src/store/methods/promise.ts b/packages/ocap-kernel/src/store/methods/promise.ts index c3aedbf6c3..40fa73b049 100644 --- a/packages/ocap-kernel/src/store/methods/promise.ts +++ b/packages/ocap-kernel/src/store/methods/promise.ts @@ -16,6 +16,9 @@ import { makeKernelSlot } from '../utils/kernel-slots.ts'; import { parseRef } from '../utils/parse-ref.ts'; import { isPromiseRef } from '../utils/promise-ref.ts'; +/** Matches the promise erefs in a c-list: `p+NN`/`p-NN`, or `rp+NN`/`rp-NN` for a remote. */ +const PROMISE_EREF = /^r?p[-+]\d+$/u; + /** * Create a promise store object that provides functionality for managing kernel promises. * @@ -25,14 +28,20 @@ import { isPromiseRef } from '../utils/promise-ref.ts'; */ // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getPromiseMethods(ctx: StoreContext) { - const { incCounter, provideStoredQueue, getPrefixedKeys, refCountKey } = - getBaseMethods(ctx.kv); - const { decrementRefCount } = getRefCountMethods(ctx); + const { + incCounter, + provideStoredQueue, + getPrefixedKeys, + getCListPrefix, + refCountKey, + } = getBaseMethods(ctx.kv); + const { incrementRefCount, decrementRefCount } = getRefCountMethods(ctx); /** - * Create a new, unresolved kernel promise. The new promise will be born with - * a reference count of 1 on the assumption that the promise has just been - * imported from somewhere. + * Create a new, unresolved kernel promise, born with a reference count of 1: + * an unsettled promise is owed a decision, and that obligation is itself a + * reference. Released, exactly once, when the promise settles in + * {@link resolveKernelPromise}. * * @returns A tuple of the new promise's KRef and an object describing the * new promise itself. @@ -159,16 +168,18 @@ export function getPromiseMethods(ctx: StoreContext) { value: CapData, ): [KRef, KernelMessage][] { const queue = provideStoredQueue(kpid, false); - // Collect messages that were queued on this promise + // Releasing each queue entry's references as we go: the caller re-enqueues + // these on the run queue, which takes its own. const queuedMessages: [KRef, KernelMessage][] = []; for (const message of getKernelPromiseMessageQueue(kpid)) { queuedMessages.push([kpid, message]); + releaseQueuedMessageRefs(kpid, message, 'resolve|dequeue'); } ctx.kv.set(`${kpid}.state`, rejected ? 'rejected' : 'fulfilled'); ctx.kv.set(`${kpid}.value`, JSON.stringify(value)); ctx.kv.delete(`${kpid}.decider`); ctx.kv.delete(`${kpid}.subscribers`); - // Drop the baseline "decider" refcount now that the promise is settled. + // The promise has been decided, so it is no longer owed a decision. decrementRefCount(kpid, 'resolve|decider'); queue.delete(); return queuedMessages; @@ -177,13 +188,46 @@ export function getPromiseMethods(ctx: StoreContext) { /** * Append a message to a promise's message queue. * + * The queue entry becomes the message's holder, so it takes references on + * everything the message carries, just as the run queue does. + * * @param kpid - The KRef of the promise to enqueue on. * @param message - The message to enqueue. */ function enqueuePromiseMessage(kpid: KRef, message: KernelMessage): void { + incrementRefCount(kpid, 'promiseQueue|target'); + if (message.result) { + incrementRefCount(message.result, 'promiseQueue|result'); + } + for (const slot of message.methargs.slots) { + incrementRefCount(slot, 'promiseQueue|slot'); + } provideStoredQueue(kpid, false).enqueue(message); } + /** + * Release the references a promise-queue entry held on the message it + * carried. + * + * @param kpid - The promise whose queue the message was on, and hence the + * message's target. + * @param message - The message being taken off the queue. + * @param tag - Tag for refcount logging. + */ + function releaseQueuedMessageRefs( + kpid: KRef, + message: KernelMessage, + tag: string, + ): void { + decrementRefCount(kpid, `${tag}|target`); + if (message.result) { + decrementRefCount(message.result, `${tag}|result`); + } + for (const slot of message.methargs.slots) { + decrementRefCount(slot, `${tag}|slot`); + } + } + /** * Fetch the messages in a kernel promise's message queue. * @@ -211,8 +255,13 @@ export function getPromiseMethods(ctx: StoreContext) { * @yields the kpids of all the unresolved promises decided by `decider`. */ function* getPromisesByDecider(decider: EndpointId): Generator { - const basePrefix = `cle.${decider}.`; - for (const key of getPrefixedKeys(`${basePrefix}p`)) { + const prefix = getCListPrefix(decider); + for (const key of getPrefixedKeys(prefix)) { + // A c-list holds both directions of each pair. Iterate by eref, and only + // the promise ones: `p+NN`/`p-NN` for a vat, `rp+NN`/`rp-NN` for a remote. + if (!PROMISE_EREF.test(key.slice(prefix.length))) { + continue; + } const kpid = ctx.kv.getRequired(key); const kp = getKernelPromise(kpid); if (kp.state === 'unresolved' && kp.decider === decider) { diff --git a/packages/ocap-kernel/src/store/methods/reachable.test.ts b/packages/ocap-kernel/src/store/methods/reachable.test.ts index fdaab477e5..2464339b68 100644 --- a/packages/ocap-kernel/src/store/methods/reachable.test.ts +++ b/packages/ocap-kernel/src/store/methods/reachable.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { makeMapKernelDatabase } from '../../../test/storage.ts'; +import type { KRef } from '../../types.ts'; import { makeKernelStore } from '../index.ts'; describe('GC methods', () => { @@ -15,13 +16,82 @@ describe('GC methods', () => { const ko1 = kernelStore.initKernelObject('v1'); kernelStore.addCListEntry('v1', ko1, 'o-1'); + // An import entry is born recognizing but not yet reaching + expect(kernelStore.getReachableFlag('v1', ko1)).toBe(false); + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 0, + recognizable: 1, + }); + + kernelStore.setReachableFlag('v1', ko1); expect(kernelStore.getReachableFlag('v1', ko1)).toBe(true); + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); kernelStore.clearReachableFlag('v1', ko1); expect(kernelStore.getReachableFlag('v1', ko1)).toBe(false); + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 0, + recognizable: 1, + }); + }); + + /** + * Give v1 an import entry it reaches, the state both idempotence tests + * start from. + * + * @returns The kref of the reached import. + */ + function givenReachedImport(): KRef { + const ko1 = kernelStore.initKernelObject('v1'); + kernelStore.addCListEntry('v1', ko1, 'o-1'); + kernelStore.setReachableFlag('v1', ko1); + return ko1; + } + + it('setReachableFlag is idempotent', () => { + const ko1 = givenReachedImport(); + + kernelStore.setReachableFlag('v1', ko1); + kernelStore.setReachableFlag('v1', ko1); + + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 1, + recognizable: 1, + }); + }); + + it('clearReachableFlag is idempotent', () => { + const ko1 = givenReachedImport(); - const refCounts = kernelStore.getObjectRefCount(ko1); - expect(refCounts.reachable).toBe(0); + kernelStore.clearReachableFlag('v1', ko1); + kernelStore.clearReachableFlag('v1', ko1); + + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 0, + recognizable: 1, + }); + }); + + it('leaves an export entry alone: it carries no reachable count', () => { + const ko1 = kernelStore.initKernelObject('v1'); + kernelStore.addCListEntry('v1', ko1, 'o+1'); + + expect(kernelStore.getReachableFlag('v1', ko1)).toBe(true); + kernelStore.setReachableFlag('v1', ko1); + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + + kernelStore.clearReachableFlag('v1', ko1); + expect(kernelStore.getReachableFlag('v1', ko1)).toBe(false); + expect(kernelStore.getObjectRefCount(ko1)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); }); }); }); diff --git a/packages/ocap-kernel/src/store/methods/reachable.ts b/packages/ocap-kernel/src/store/methods/reachable.ts index 4caa2d0960..05e4c5f986 100644 --- a/packages/ocap-kernel/src/store/methods/reachable.ts +++ b/packages/ocap-kernel/src/store/methods/reachable.ts @@ -54,6 +54,32 @@ export function getReachableMethods(ctx: StoreContext) { return parseReachableAndVatSlot(data); } + /** + * Set the reachable flag for a given endpoint and kref. + * + * The counterpart to {@link clearReachableFlag}: this is how an object + * regains reachability when a vat that dropped it is handed it again. + * Idempotent, so repeated translations don't inflate the count. + * + * @param endpointId - The endpoint for which the reachable flag is being set. + * @param kref - The kref. + */ + function setReachableFlag(endpointId: EndpointId, kref: KRef): void { + const key = getSlotKey(endpointId, kref); + const { isReachable, vatSlot } = getReachableAndVatSlot(endpointId, kref); + if (isReachable) { + return; + } + ctx.kv.set(key, buildReachableAndVatSlot(true, vatSlot)); + const { direction, isPromise } = parseRef(vatSlot); + // increment 'reachable' part of refcount, but only for object imports + if (!isPromise && direction === 'import' && kernelRefExists(kref)) { + const counts = getObjectRefCount(kref); + counts.reachable += 1; + setObjectRefCount(kref, counts); + } + } + /** * Clear the reachable flag for a given endpoint and kref. * @@ -84,6 +110,7 @@ export function getReachableMethods(ctx: StoreContext) { return { getReachableFlag, getReachableAndVatSlot, + setReachableFlag, clearReachableFlag, }; } diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts new file mode 100644 index 0000000000..868034e55c --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.test.ts @@ -0,0 +1,496 @@ +import type { KernelDatabase } from '@metamask/kernel-store'; +import { describe, it, expect, beforeEach } from 'vitest'; + +import { makeMapKernelDatabase } from '../../../test/storage.ts'; +import type { KRef, VatConfig, VatId } from '../../types.ts'; +import { makeKernelStore } from '../index.ts'; + +describe('reference count audit', () => { + let kernelStore: ReturnType; + let kdb: KernelDatabase; + + /** + * Register and initialize an endpoint so it can hold c-list entries. + * + * @param vatIds - The vats to bring into existence. + */ + function givenVats(...vatIds: VatId[]): void { + for (const vatId of vatIds) { + kernelStore.setVatConfig(vatId, { sourceSpec: 'x' } as VatConfig); + kernelStore.initEndpoint(vatId); + } + } + + /** + * Overwrite a kref's stored count, going around the store's own arithmetic so + * that drift can be introduced in either direction regardless of what the + * current count happens to be. + * + * @param kref - The kref whose count to overwrite. + * @param counts - The count text, in the store's encoding. + */ + function setStoredCount(kref: KRef, counts: string): void { + kdb.kernelKVStore.set(`${kref}.refCount`, counts); + } + + /** + * Shift every component of a count by the same amount. + * + * @param counts - The count text, in the store's encoding. + * @param delta - How far to shift each component. + * @returns The shifted count text. + */ + function shift(counts: string, delta: number): string { + return counts + .split(',') + .map((part) => `${Number(part) + delta}`) + .join(','); + } + + beforeEach(() => { + kdb = makeMapKernelDatabase(); + kernelStore = makeKernelStore(kdb); + kernelStore.markInitialized(); + givenVats('v1', 'v2', 'v3'); + }); + + describe('auditRefCounts', () => { + it('finds nothing wrong in an empty store', () => { + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it.each([ + { + what: 'an export', + act: (kref: KRef) => kref, + }, + { + what: 'an export plus one importer', + act: (kref: KRef) => { + kernelStore.translateRefKtoE('v2', kref, true); + return kref; + }, + }, + { + what: 'an export plus two importers', + act: (kref: KRef) => { + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.translateRefKtoE('v3', kref, true); + return kref; + }, + }, + { + what: 'a dropped import', + act: (kref: KRef) => { + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + return kref; + }, + }, + { + what: 'a retired import', + act: (kref: KRef) => { + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + kernelStore.forgetKref('v2', kref); + return kref; + }, + }, + { + what: 'a pin', + act: (kref: KRef) => { + kernelStore.pinObject(kref); + return kref; + }, + }, + { + what: 'a queued message', + act: (kref: KRef) => { + kernelStore.enqueueRun({ + type: 'send', + target: kref, + message: { methargs: { body: '#[]', slots: [kref] }, result: null }, + }); + kernelStore.incrementRefCount(kref, 'queue|target'); + kernelStore.incrementRefCount(kref, 'queue|slot'); + return kref; + }, + }, + ])('holds for $what', ({ act }) => { + act(kernelStore.exportFromEndpoint('v1', 'o+1')); + + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('holds for a queued notification', () => { + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.enqueueRun({ type: 'notify', endpointId: 'v2', kpid }); + kernelStore.incrementRefCount(kpid, 'notify'); + + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('holds for an unsettled promise with importers', () => { + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.translateRefKtoE('v2', kpid, true); + + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + expect(kernelStore.getRefCount(kpid)).toBe(3); + }); + + it('holds for a settled promise whose value carries a slot', () => { + const koid = kernelStore.exportFromEndpoint('v1', 'o+1'); + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.incrementRefCount(koid, 'resolve|slot'); + kernelStore.resolveKernelPromise(kpid, false, { + body: '#"$0"', + slots: [koid], + }); + + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('reports counts that are too low', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.setObjectRefCount(kref, { reachable: 0, recognizable: 0 }); + + expect(kernelStore.auditRefCounts()).toStrictEqual([ + { + kind: 'mismatch', + kref, + stored: '0,0', + expected: '1,1', + holders: ['v2 c-list import o-1'], + }, + ]); + }); + + it('reports counts that are too high even though nothing underflowed', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.setObjectRefCount(kref, { reachable: 1, recognizable: 1 }); + + expect(kernelStore.auditRefCounts()).toStrictEqual([ + { kind: 'mismatch', kref, stored: '1,1', expected: '0,0', holders: [] }, + ]); + }); + + it('reports a reference to a kref that has been deleted', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.deleteKernelObject(kref); + + expect(kernelStore.auditRefCounts()).toStrictEqual([ + { + kind: 'dangling', + kref, + expected: '1,1', + holders: ['v2 c-list import o-1'], + }, + ]); + }); + + it('does not mistake an owner for a referrer', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 0, + recognizable: 0, + }); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + }); + + // The clean-audit cases above prove each rule agrees with whatever the store + // did, which stays true if a rule and the code it mirrors are wrong by the + // same constant. These pin each credit source to a literal count and holder + // label, and check drift in both directions: too low collects a live + // capability, too high leaks it. + describe('each credit source, on its own', () => { + const sources: { + what: string; + hold: () => KRef; + expected: string; + holders: string[]; + }[] = [ + { + what: 'an object import a vat still reaches', + hold: () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + return kref; + }, + expected: '1,1', + holders: ['v2 c-list import o-1'], + }, + { + what: 'an object import a vat has dropped but not retired', + hold: () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.clearReachableFlag('v2', kref); + return kref; + }, + expected: '0,1', + holders: ['v2 c-list import o-1'], + }, + { + what: 'a pinned object', + hold: () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.pinObject(kref); + return kref; + }, + expected: '1,1', + holders: ['pin'], + }, + { + what: "a run-queue send's target and slot", + hold: () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.enqueueRun({ + type: 'send', + target: kref, + message: { methargs: { body: '#[]', slots: [kref] }, result: null }, + }); + kernelStore.incrementRefCount(kref, 'queue|target'); + kernelStore.incrementRefCount(kref, 'queue|slot'); + return kref; + }, + expected: '2,2', + holders: ['run queue #1 send target', 'run queue #1 send slot'], + }, + { + what: "a run-queue send's result promise", + hold: () => { + const target = kernelStore.exportFromEndpoint('v1', 'o+1'); + const kpid = kernelStore.initKernelPromise()[0]; + kernelStore.enqueueRun({ + type: 'send', + target, + message: { methargs: { body: '#[]', slots: [] }, result: kpid }, + }); + kernelStore.incrementRefCount(target, 'queue|target'); + kernelStore.incrementRefCount(kpid, 'queue|result'); + return kpid; + }, + expected: '2', + holders: ['unsettled promise', 'run queue #1 send result'], + }, + { + what: 'a queued notification', + hold: () => { + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.enqueueRun({ type: 'notify', endpointId: 'v2', kpid }); + kernelStore.incrementRefCount(kpid, 'notify'); + return kpid; + }, + expected: '3', + holders: [ + 'unsettled promise', + 'run queue #1 notify', + 'v1 c-list export p+1', + ], + }, + { + // `enqueuePromiseMessage` takes the references itself, which is the + // point of the transfer-don't-duplicate fix; incrementing here too + // would be the double-count it exists to prevent. + what: 'a message parked on an unresolved promise', + hold: () => { + const target = kernelStore.exportFromEndpoint('v1', 'o+1'); + const kpid = kernelStore.initKernelPromise()[0]; + kernelStore.enqueuePromiseMessage(kpid, { + methargs: { body: '#[]', slots: [target] }, + result: null, + }); + return kpid; + }, + expected: '2', + holders: ['unsettled promise', 'kp1 queue #1 target'], + }, + { + what: 'a promise nobody has settled yet', + hold: () => kernelStore.initKernelPromise()[0], + expected: '1', + holders: ['unsettled promise'], + }, + { + what: "a settled promise's resolution slot", + hold: () => { + const koid = kernelStore.exportFromEndpoint('v1', 'o+1'); + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.incrementRefCount(koid, 'resolve|slot'); + kernelStore.resolveKernelPromise(kpid, false, { + body: '#"$0"', + slots: [koid], + }); + return koid; + }, + expected: '1,1', + holders: ['kp1 resolution slot'], + }, + { + what: "a promise's own c-list entries", + hold: () => { + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.translateRefKtoE('v2', kpid, true); + return kpid; + }, + expected: '3', + holders: [ + 'unsettled promise', + 'v1 c-list export p+1', + 'v2 c-list import p-1', + ], + }, + ]; + + it.each(sources)('credits $what exactly', ({ hold, expected, holders }) => { + const kref = hold(); + + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + + for (const delta of [1, -1]) { + const stored = shift(expected, delta); + setStoredCount(kref, stored); + expect(kernelStore.auditRefCounts()).toStrictEqual([ + { kind: 'mismatch', kref, stored, expected, holders }, + ]); + } + }); + }); + + describe('assertRefCountsIfAuditing', () => { + it('does nothing while auditing is off', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.setObjectRefCount(kref, { reachable: 9, recognizable: 9 }); + + expect(() => kernelStore.assertRefCountsIfAuditing()).not.toThrow(); + }); + + it('throws with the offending krefs once auditing is on', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.setObjectRefCount(kref, { reachable: 9, recognizable: 9 }); + kernelStore.setRefCountAuditing(true); + + expect(() => kernelStore.assertRefCountsIfAuditing()).toThrow( + `${kref}: stored 9,9, expected 0,0`, + ); + }); + + it('stays quiet when the counts agree', () => { + kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.setRefCountAuditing(true); + + expect(() => kernelStore.assertRefCountsIfAuditing()).not.toThrow(); + }); + }); + + describe('recomputeRefCounts', () => { + it('rebuilds counts written under the old accounting', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.translateRefKtoE('v3', kref, true); + // As the pre-fix kernel would have left it: born (1,1), with neither + // importer's c-list entry taking a reference. Two importers is the + // smallest topology where that disagrees with the truth — with one, the + // phantom baseline happens to come out to the right number. + kernelStore.setObjectRefCount(kref, { reachable: 1, recognizable: 1 }); + + const { corrected, unfixable } = kernelStore.recomputeRefCounts(); + + expect(corrected).toStrictEqual([ + { + kind: 'mismatch', + kref, + stored: '1,1', + expected: '2,2', + holders: ['v2 c-list import o-1', 'v3 c-list import o-1'], + }, + ]); + expect(unfixable).toStrictEqual([]); + expect(kernelStore.getObjectRefCount(kref)).toStrictEqual({ + reachable: 2, + recognizable: 2, + }); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('reports references it cannot repair', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.deleteKernelObject(kref); + + const { corrected, unfixable } = kernelStore.recomputeRefCounts(); + + expect(corrected).toStrictEqual([]); + expect(unfixable).toHaveLength(1); + expect(unfixable[0]?.kref).toBe(kref); + }); + + it('rebuilds a promise count', () => { + const kpid = kernelStore.exportFromEndpoint('v1', 'p+1'); + kernelStore.translateRefKtoE('v2', kpid, true); + // A promise has one undifferentiated count, so its repair goes down a + // different path from an object's pair. + kernelStore.incrementRefCount(kpid, 'phantom'); + kernelStore.incrementRefCount(kpid, 'phantom'); + + const { corrected, unfixable } = kernelStore.recomputeRefCounts(); + + expect(corrected).toStrictEqual([ + { + kind: 'mismatch', + kref: kpid, + stored: '5', + expected: '3', + holders: [ + 'unsettled promise', + 'v1 c-list export p+1', + 'v2 c-list import p-1', + ], + }, + ]); + expect(unfixable).toStrictEqual([]); + expect(kernelStore.getRefCount(kpid)).toBe(3); + expect(kernelStore.auditRefCounts()).toStrictEqual([]); + }); + + it('queues krefs it zeroes for collection', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.setObjectRefCount(kref, { reachable: 1, recognizable: 1 }); + + kernelStore.recomputeRefCounts(); + kernelStore.collectGarbage(); + + expect([...kernelStore.getGCActions()]).toStrictEqual([ + `v1 dropExport ${kref}`, + `v1 retireExport ${kref}`, + ]); + }); + }); + + describe('formatRefCountViolations', () => { + it('names the holders behind a mismatch', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.translateRefKtoE('v2', kref, true); + kernelStore.setObjectRefCount(kref, { reachable: 0, recognizable: 0 }); + + expect( + kernelStore.formatRefCountViolations(kernelStore.auditRefCounts()), + ).toBe( + `${kref}: stored 0,0, expected 1,1 (held by: v2 c-list import o-1)`, + ); + }); + + it('says so when a mismatch has no holders at all', () => { + const kref = kernelStore.exportFromEndpoint('v1', 'o+1'); + kernelStore.setObjectRefCount(kref, { reachable: 1, recognizable: 1 }); + + expect( + kernelStore.formatRefCountViolations(kernelStore.auditRefCounts()), + ).toBe(`${kref}: stored 1,1, expected 0,0 (held by: nothing)`); + }); + }); +}); diff --git a/packages/ocap-kernel/src/store/methods/refcount-audit.ts b/packages/ocap-kernel/src/store/methods/refcount-audit.ts new file mode 100644 index 0000000000..27a6d912f4 --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/refcount-audit.ts @@ -0,0 +1,406 @@ +import type { CapData } from '@endo/marshal'; + +import { getBaseMethods } from './base.ts'; +import { getObjectMethods } from './object.ts'; +import { getPinMethods } from './pinned.ts'; +import type { KRef, KernelMessage, RunQueueItem } from '../../types.ts'; +import type { StoreContext } from '../types.ts'; +import { parseRef } from '../utils/parse-ref.ts'; +import { isPromiseRef } from '../utils/promise-ref.ts'; +import { parseReachableAndVatSlot } from '../utils/reachable.ts'; + +/** + * A kref whose stored reference counts disagree with the counts implied by the + * references the kernel can actually be seen to hold. + */ +export type RefCountViolation = + | { + /** The kref is still counted, just by the wrong amount. */ + kind: 'mismatch'; + kref: KRef; + /** + * The counts as stored, in the store's own encoding: + * `"reachable,recognizable"` for objects, a single number for promises. + */ + stored: string; + /** The counts implied by `holders`, in the same encoding as `stored`. */ + expected: string; + /** One entry per reference found, so a mismatch can be traced to its source. */ + holders: string[]; + } + | { + /** + * The kref has no refcount entry, so each entry in `holders` names + * something the kernel has already deleted. Rewriting a count cannot + * repair this. + */ + kind: 'dangling'; + kref: KRef; + /** The counts `holders` imply, which there is nothing left to credit. */ + expected: string; + /** One entry per dangling reference found. */ + holders: string[]; + }; + +/** + * The running total of references found for one kref. For a promise, which has + * only a single count, that count accumulates in `reachable`. + */ +type Tally = { + reachable: number; + recognizable: number; + holders: string[]; +}; + +/** Matches the kref-keyed half of a c-list entry, e.g. `v1.c.ko3`. */ +const CLIST_KREF_KEY = /^([vr]\d+)\.c\.(k[op]\d+)$/u; + +/** Matches a queue entry (but not the queue's `head`/`tail` bookkeeping). */ +const QUEUE_ENTRY_KEY = /^queue\.([^.]+)\.(\d+)$/u; + +/** Matches the state record that exists for every live kernel promise. */ +const PROMISE_STATE_KEY = /^(kp\d+)\.state$/u; + +/** Matches the refcount record that exists for every live kernel object or promise. */ +const REFCOUNT_KEY = /^(k[op]\d+)\.refCount$/u; + +/** + * Get the methods that audit reference counts against ground truth. + * + * The kernel's reference counts are a cache: every unit of every count is owed + * to some reference the kernel is holding somewhere else in the store — a + * c-list entry, a queued message, a promise's resolution value, a pin. This + * module recomputes those counts from the references themselves and reports + * where the cache has drifted, in either direction. Counts that are too low + * let a live capability be collected; counts that are too high leak it. + * + * Only references the kernel can see in its own state are checkable, so a + * holder that keeps a kref outside them is invisible here and the audit will + * pronounce its target unreferenced. Anything of that shape has to take a pin + * to be counted at all — see `retainForOcapURL`, where an issued URL's + * encrypted kref does exactly that. + * + * @param ctx - The store context. + * @returns The reference count audit methods. + */ +// eslint-disable-next-line @typescript-eslint/explicit-function-return-type +export function getRefCountAuditMethods(ctx: StoreContext) { + const { getPrefixedKeys, refCountKey } = getBaseMethods(ctx.kv); + const { getObjectRefCount } = getObjectMethods(ctx); + const { getPinnedObjects } = getPinMethods(ctx); + + /** + * Render a tally the way the store encodes it, so expected and stored values + * can be compared and reported as like for like. + * + * @param kref - The kref the counts belong to. + * @param counts - The counts to render. + * @param counts.reachable - The reachable count (the only count, for a promise). + * @param counts.recognizable - The recognizable count (ignored for a promise). + * @returns The encoded counts. + */ + function renderCounts( + kref: KRef, + counts: { reachable: number; recognizable: number }, + ): string { + return isPromiseRef(kref) + ? `${counts.reachable}` + : `${counts.reachable},${counts.recognizable}`; + } + + /** + * Walk the whole store and total up, for every kref, the references the + * kernel is holding to it. + * + * The credits below mirror `incrementRefCount` case for case; when that + * function's rules change, these have to change with it. + * + * @returns A tally per kref that anything refers to. + */ + function computeExpectedRefCounts(): Map { + const tallies = new Map(); + // `retireKernelObjects` deletes an object and queues a `retireImport` for + // each importer in the same breath, so between then and the delivery an + // importer's c-list entry legitimately names a kref the kernel has already + // dropped. Those entries are scheduled for teardown and are not holders. + const retiring = new Set( + (JSON.parse(ctx.gcActions.get() ?? '[]') as string[]).filter((action) => + action.includes(' retireImport '), + ), + ); + + const credit = ( + kref: KRef, + holder: string, + { onlyRecognizable = false }: { onlyRecognizable?: boolean } = {}, + ): void => { + let tally = tallies.get(kref); + if (!tally) { + tally = { reachable: 0, recognizable: 0, holders: [] }; + tallies.set(kref, tally); + } + tally.holders.push(holder); + if (isPromiseRef(kref)) { + // Promises have a single count and no reachable/recognizable split. + tally.reachable += 1; + return; + } + if (!onlyRecognizable) { + tally.reachable += 1; + } + tally.recognizable += 1; + }; + + /** + * A queued message holds its result promise and every slot it carries. + * + * @param message - The queued message. + * @param holder - Description of the queue entry holding it. + */ + const creditMessage = (message: KernelMessage, holder: string): void => { + if (message.result) { + credit(message.result, `${holder} result`); + } + for (const slot of message.methargs.slots) { + credit(slot, `${holder} slot`); + } + }; + + for (const key of getPrefixedKeys('')) { + const clistMatch = CLIST_KREF_KEY.exec(key); + if (clistMatch) { + const [, endpointId, kref] = clistMatch as unknown as [ + string, + string, + KRef, + ]; + const { isReachable, vatSlot } = parseReachableAndVatSlot( + ctx.kv.getRequired(key), + ); + const { direction } = parseRef(vatSlot); + const holder = `${endpointId} c-list ${direction} ${vatSlot}`; + if (isPromiseRef(kref)) { + // Both directions count for a promise. + credit(kref, holder); + } else if ( + direction === 'import' && + !retiring.has(`${endpointId} retireImport ${kref}`) + ) { + // An object export is the owner's own entry and carries no count; + // an object import always recognizes and, while flagged, reaches. + credit(kref, holder, { onlyRecognizable: !isReachable }); + } + continue; + } + + const queueMatch = QUEUE_ENTRY_KEY.exec(key); + if (queueMatch) { + const [, queueName, seq] = queueMatch as unknown as [ + string, + string, + string, + ]; + const entry = ctx.kv.getRequired(key); + if (queueName === 'run') { + const item = JSON.parse(entry) as RunQueueItem; + if (item.type === 'send') { + credit(item.target, `run queue #${seq} send target`); + creditMessage(item.message, `run queue #${seq} send`); + } else if (item.type === 'notify') { + credit(item.kpid, `run queue #${seq} notify`); + } + } else { + const kpid = queueName as KRef; + const message = JSON.parse(entry) as KernelMessage; + credit(kpid, `${kpid} queue #${seq} target`); + creditMessage(message, `${kpid} queue #${seq}`); + } + continue; + } + + const promiseMatch = PROMISE_STATE_KEY.exec(key); + if (promiseMatch) { + const kpid = promiseMatch[1] as KRef; + if (ctx.kv.getRequired(key) === 'unresolved') { + // The unit `initKernelPromise` mints, released when the promise settles. + credit(kpid, 'unsettled promise'); + } else { + const value = JSON.parse( + ctx.kv.getRequired(`${kpid}.value`), + ) as CapData; + for (const slot of value.slots) { + credit(slot, `${kpid} resolution slot`); + } + } + } + } + + for (const kref of getPinnedObjects()) { + credit(kref, 'pin'); + } + + return tallies; + } + + /** + * Collect every kref the store has a refcount entry for. + * + * @returns The krefs with refcount entries. + */ + function getCountedKrefs(): KRef[] { + const krefs: KRef[] = []; + for (const key of getPrefixedKeys('')) { + const match = REFCOUNT_KEY.exec(key); + if (match) { + krefs.push(match[1] as KRef); + } + } + return krefs; + } + + /** + * Compare every kref's stored reference counts against the references the + * kernel can be seen to hold. + * + * What this can and cannot find is worth being precise about, because the + * ground truth here *is* the holder set. A count that disagrees with its + * holders is caught in either direction: too low, and a live capability can be + * collected; too high with no holder left, and the count itself is orphaned. + * But a holder that should have been torn down and wasn't justifies its own + * count — at any value — so a leaked *reference* is invisible to this by + * construction. A c-list entry that outlives what it names is the case that + * matters: see the settled-promise TODO in `KernelRouter`. + * + * @returns The krefs whose counts disagree with ground truth, in kref order. + */ + function auditRefCounts(): RefCountViolation[] { + const expected = computeExpectedRefCounts(); + const violations: RefCountViolation[] = []; + const krefs = new Set([...expected.keys(), ...getCountedKrefs()]); + + for (const kref of [...krefs].sort()) { + const tally = expected.get(kref) ?? { + reachable: 0, + recognizable: 0, + holders: [], + }; + const expectedText = renderCounts(kref, tally); + const raw = ctx.kv.get(refCountKey(kref)); + if (raw === undefined) { + // The kref has been deleted from the kernel, so anything still + // pointing at it is a dangling reference. + if (tally.holders.length > 0) { + violations.push({ + kind: 'dangling', + kref, + expected: expectedText, + holders: tally.holders, + }); + } + continue; + } + const storedText = isPromiseRef(kref) + ? raw + : renderCounts(kref, getObjectRefCount(kref)); + if (storedText !== expectedText) { + violations.push({ + kind: 'mismatch', + kref, + stored: storedText, + expected: expectedText, + holders: tally.holders, + }); + } + } + return violations; + } + + /** + * Overwrite stored reference counts with the counts implied by ground truth. + * + * A repair tool for a store whose counts have drifted, offered to embedders + * and never run automatically: nothing calls it, and opening an existing + * store does not migrate it. Krefs that are referenced but have already been + * deleted cannot be repaired this way and are reported instead. + * + * Ground truth here means the references the kernel can see in its own + * state. A holder the store cannot see — an issued ocap URL names its target + * only inside an encrypted bearer token — is not among them, which is why + * such a target is pinned when the URL is issued rather than left for this to + * infer. + * + * @returns The violations that were corrected and those that could not be. + */ + function recomputeRefCounts(): { + corrected: RefCountViolation[]; + unfixable: RefCountViolation[]; + } { + const corrected: RefCountViolation[] = []; + const unfixable: RefCountViolation[] = []; + for (const violation of auditRefCounts()) { + if (violation.kind === 'dangling') { + unfixable.push(violation); + continue; + } + ctx.kv.set(refCountKey(violation.kref), violation.expected); + if (violation.expected.startsWith('0')) { + ctx.maybeFreeKrefs.add(violation.kref); + } + corrected.push(violation); + } + return { corrected, unfixable }; + } + + /** + * Render violations as a human-readable report. + * + * @param violations - The violations to describe. + * @returns A newline-separated report, one line per violation. + */ + function formatRefCountViolations(violations: RefCountViolation[]): string { + return violations + .map((violation) => { + const { kref, expected, holders } = violation; + const stored = + violation.kind === 'dangling' ? '(deleted)' : violation.stored; + const held = holders.length > 0 ? holders.join(', ') : 'nothing'; + return `${kref}: stored ${stored}, expected ${expected} (held by: ${held})`; + }) + .join('\n'); + } + + /** + * Audit reference counts and throw if any have drifted. Enabled per kernel + * via the `auditRefCounts` option, and run at the end of every crank. + */ + function assertRefCountsIfAuditing(): void { + if (!ctx.auditRefCounts) { + return; + } + const violations = auditRefCounts(); + if (violations.length > 0) { + const report = formatRefCountViolations(violations); + // Logged as well as thrown: if this is the last crank before the kernel + // goes idle, nobody sends another message and the log is the only record. + ctx.logger?.error(`reference count invariant violated:\n${report}`); + throw Error(`reference count invariant violated:\n${report}`); + } + } + + /** + * Turn the per-crank reference count audit on or off. + * + * @param enabled - Whether to audit after every crank. + */ + function setRefCountAuditing(enabled: boolean): void { + ctx.auditRefCounts = enabled; + } + + return { + auditRefCounts, + recomputeRefCounts, + formatRefCountViolations, + assertRefCountsIfAuditing, + setRefCountAuditing, + }; +} diff --git a/packages/ocap-kernel/src/store/methods/refcount.ts b/packages/ocap-kernel/src/store/methods/refcount.ts index 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 0ac95eafb0..b50f21b4a0 100644 --- a/packages/ocap-kernel/src/store/methods/translators.test.ts +++ b/packages/ocap-kernel/src/store/methods/translators.test.ts @@ -14,6 +14,8 @@ import type { } from '../../types.ts'; import type { StoreContext } from '../types.ts'; import * as clistModule from './clist.ts'; +import * as reachableModule from './reachable.ts'; +import * as refCountModule from './refcount.ts'; import { getTranslators } from './translators.ts'; import * as vatModule from './vat.ts'; @@ -22,10 +24,13 @@ describe('getTranslators', () => { const mockErefToKref = vi.fn(); const mockAllocateErefForKref = vi.fn(); const mockExportFromEndpoint = vi.fn(); + const mockSetReachableFlag = vi.fn(); + const mockKernelRefExists = vi.fn(); const mockCtx = {} as StoreContext; beforeEach(() => { vi.clearAllMocks(); + mockKernelRefExists.mockReturnValue(true); vi.spyOn(clistModule, 'getCListMethods').mockReturnValue({ krefToEref: mockKrefToEref, @@ -33,6 +38,14 @@ describe('getTranslators', () => { allocateErefForKref: mockAllocateErefForKref, } as unknown as ReturnType); + vi.spyOn(reachableModule, 'getReachableMethods').mockReturnValue({ + setReachableFlag: mockSetReachableFlag, + } as unknown as ReturnType); + + vi.spyOn(refCountModule, 'getRefCountMethods').mockReturnValue({ + kernelRefExists: mockKernelRefExists, + } as unknown as ReturnType); + vi.spyOn(vatModule, 'getVatMethods').mockReturnValue({ exportFromEndpoint: mockExportFromEndpoint, } as unknown as ReturnType); @@ -64,6 +77,18 @@ describe('getTranslators', () => { expect(result).toStrictEqual(expectedEref); }); + it('refuses to import a kref the kernel has deleted', () => { + const vatId: VatId = 'v1'; + const kref: KRef = 'ko1' as KRef; + mockKrefToEref.mockReturnValue(null); + mockKernelRefExists.mockReturnValue(false); + const { translateRefKtoE } = getTranslators(mockCtx); + expect(() => translateRefKtoE(vatId, kref, true)).toThrow( + `cannot import deleted kref "${kref}" into "${vatId}"`, + ); + expect(mockAllocateErefForKref).not.toHaveBeenCalled(); + }); + it('throws error when not found and importIfNeeded is false', () => { const vatId: VatId = 'v1'; const kref: KRef = 'ko1' as KRef; diff --git a/packages/ocap-kernel/src/store/methods/translators.ts b/packages/ocap-kernel/src/store/methods/translators.ts index c5948fedff..3ba551d28f 100644 --- a/packages/ocap-kernel/src/store/methods/translators.ts +++ b/packages/ocap-kernel/src/store/methods/translators.ts @@ -21,6 +21,8 @@ import type { } from '../../types.ts'; import type { StoreContext } from '../types.ts'; import { getCListMethods } from './clist.ts'; +import { getReachableMethods } from './reachable.ts'; +import { getRefCountMethods } from './refcount.ts'; import { getVatMethods } from './vat.ts'; import { Fail, assert } from '../../utils/assert.ts'; @@ -35,6 +37,8 @@ import { Fail, assert } from '../../utils/assert.ts'; // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getTranslators(ctx: StoreContext) { const { krefToEref, erefToKref, allocateErefForKref } = getCListMethods(ctx); + const { setReachableFlag } = getReachableMethods(ctx); + const { kernelRefExists } = getRefCountMethods(ctx); const { exportFromEndpoint } = getVatMethods(ctx); /** @@ -54,6 +58,11 @@ export function getTranslators(ctx: StoreContext) { /** * Translate a reference from kernel space into endpoint space. * + * Translating is how the kernel hands an endpoint a reference, so it also + * re-establishes reachability: a vat given an object it previously dropped + * holds it live again. Garbage collection deliveries must not do this, and + * don't — they map through `krefsToErefs`, which never touches the flag. + * * @param endpointId - The endpoint for whom translation is desired. * @param kref - The KRef of the entity of interest. * @param importIfNeeded - If true, allocate a new clist entry if necessary; @@ -69,11 +78,21 @@ export function getTranslators(ctx: StoreContext) { let eref = krefToEref(endpointId, kref); if (!eref) { if (importIfNeeded) { + // A kref the kernel has already deleted must not acquire a new c-list + // entry. `getObjectRefCount` reads a missing row as `(0, 0)`, so the + // entry's own increment would write it back and resurrect a + // live-looking object that nobody owns — one the audit then endorses, + // since the entry is a legitimate holder for exactly the count it + // finds. Reached by redeeming an ocap URL issued for an object that + // has since been collected. + kernelRefExists(kref) || + Fail`cannot import deleted kref ${kref} into ${endpointId}`; eref = allocateErefForKref(endpointId, kref); } else { throw Fail`unmapped kref ${kref} endpoint=${endpointId}`; } } + setReachableFlag(endpointId, kref); if (isRemoteId(endpointId)) { // The import/export relationship between a vat and the kernel is // asymmetric -- the vat always exports to the kernel and imports from the diff --git a/packages/ocap-kernel/src/store/methods/vat.test.ts b/packages/ocap-kernel/src/store/methods/vat.test.ts index d04c609896..fd1f05402e 100644 --- a/packages/ocap-kernel/src/store/methods/vat.test.ts +++ b/packages/ocap-kernel/src/store/methods/vat.test.ts @@ -118,6 +118,7 @@ describe('vat store methods', () => { (getBaseMethods as ReturnType).mockReturnValue({ getPrefixedKeys: mockGetPrefixedKeys, getSlotKey: mockGetSlotKey, + getCListPrefix: (endpointId: string) => `${endpointId}.c.`, getOwnerKey: mockGetOwnerKey, }); @@ -273,33 +274,26 @@ describe('vat store methods', () => { it('deletes all keys related to the endpoint', () => { const endpointId = 'e1'; - // Setup mock data - mockKV.set(`cle.${endpointId}.obj1`, 'data1'); - mockKV.set(`cle.${endpointId}.obj2`, 'data2'); - mockKV.set(`clk.${endpointId}.prom1`, 'data3'); + // The c-list holds both directions of each pair under one prefix + mockKV.set(`${endpointId}.c.o-1`, 'ko1'); + mockKV.set(`${endpointId}.c.ko1`, 'R o-1'); + mockKV.set(`${endpointId}.c.p+1`, 'kp1'); mockKV.set(`e.nextObjectId.${endpointId}`, '10'); mockKV.set(`e.nextPromiseId.${endpointId}`, '5'); - mockGetPrefixedKeys.mockImplementation((prefix: string) => { - if (prefix === `cle.${endpointId}.`) { - return [`cle.${endpointId}.obj1`, `cle.${endpointId}.obj2`]; - } - if (prefix === `clk.${endpointId}.`) { - return [`clk.${endpointId}.prom1`]; - } - return []; - }); + mockGetPrefixedKeys.mockImplementation((prefix: string) => + [...mockKV.keys()].filter((key) => key.startsWith(prefix)), + ); vatMethods.deleteEndpoint(endpointId); - expect(mockKV.has(`cle.${endpointId}.obj1`)).toBe(false); - expect(mockKV.has(`cle.${endpointId}.obj2`)).toBe(false); - expect(mockKV.has(`clk.${endpointId}.prom1`)).toBe(false); + expect(mockKV.has(`${endpointId}.c.o-1`)).toBe(false); + expect(mockKV.has(`${endpointId}.c.ko1`)).toBe(false); + expect(mockKV.has(`${endpointId}.c.p+1`)).toBe(false); expect(mockKV.has(`e.nextObjectId.${endpointId}`)).toBe(false); expect(mockKV.has(`e.nextPromiseId.${endpointId}`)).toBe(false); - expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`cle.${endpointId}.`); - expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`clk.${endpointId}.`); + expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`${endpointId}.c.`); }); it('does nothing if endpoint has no associated keys', () => { @@ -309,8 +303,7 @@ describe('vat store methods', () => { expect(() => vatMethods.deleteEndpoint(endpointId)).not.toThrow(); - expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`cle.${endpointId}.`); - expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`clk.${endpointId}.`); + expect(mockGetPrefixedKeys).toHaveBeenCalledWith(`${endpointId}.c.`); }); }); @@ -470,11 +463,10 @@ describe('vat store methods', () => { expect(result).toBe('kp123'); expect(mockInitKernelPromise).toHaveBeenCalled(); expect(mockSetPromiseDecider).toHaveBeenCalledWith('kp123', vatId); + // addCListEntry takes the entry's reference; exportFromEndpoint no + // longer takes one of its own expect(mockAddCListEntry).toHaveBeenCalledWith(vatId, 'kp123', vref); - expect(mockIncrementRefCount).toHaveBeenCalledWith('kp123', 'export', { - isExport: true, - onlyRecognizable: true, - }); + expect(mockIncrementRefCount).not.toHaveBeenCalled(); }); it('creates a kernel object for an exported object', () => { @@ -486,10 +478,7 @@ describe('vat store methods', () => { expect(result).toBe('ko456'); expect(mockInitKernelObject).toHaveBeenCalledWith(vatId); expect(mockAddCListEntry).toHaveBeenCalledWith(vatId, 'ko456', vref); - expect(mockIncrementRefCount).toHaveBeenCalledWith('ko456', 'export', { - isExport: true, - onlyRecognizable: true, - }); + expect(mockIncrementRefCount).not.toHaveBeenCalled(); }); it('throws an error for non-export reference', () => { @@ -566,38 +555,22 @@ describe('vat store methods', () => { }); } - it("decrements the decider refcount for the peer's promise exports", () => { + it("releases the peer's promise exports through the c-list", () => { seedClist([['rp+1', 'kp123']]); - mockGetKernelPromise.mockReturnValue({ decider: endpointId }); vatMethods.forgetEndpointImports(endpointId); + // The caller rejected the promises the peer was deciding first, which + // released the unsettled-promise reference; the entry's own reference is + // all that is left, and deleteCListEntry releases it. expect(mockDeleteCListEntry).toHaveBeenCalledWith( endpointId, 'kp123', 'rp+1', ); - expect(mockDecrementRefCount).toHaveBeenCalledWith( - 'kp123', - 'cleanup|peerRestart|promise|decider', - ); }); - it('skips the decider decrement when the peer is no longer the decider', () => { - seedClist([['rp+1', 'kp123']]); - mockGetKernelPromise.mockReturnValue({ decider: 'someoneElse' }); - - vatMethods.forgetEndpointImports(endpointId); - - expect(mockDeleteCListEntry).toHaveBeenCalledWith( - endpointId, - 'kp123', - 'rp+1', - ); - expect(mockDecrementRefCount).not.toHaveBeenCalled(); - }); - - it("releases the peer's object exports: owner, c-list, baseline refcount, GC", () => { + it("releases the peer's object exports: owner, c-list, GC", () => { seedClist([['ro+7', 'ko42']]); mockKV.set(`owner.ko42`, endpointId); mockGetReachableAndVatSlot.mockReturnValue({ vatSlot: 'ro+7' }); @@ -607,33 +580,30 @@ describe('vat store methods', () => { expect(mockKV.has(`owner.ko42`)).toBe(false); expect(mockKV.has(`slot.${endpointId}.ko42`)).toBe(false); expect(mockKV.has(`slot.${endpointId}.ro+7`)).toBe(false); - expect(mockDecrementRefCount).toHaveBeenCalledWith( - 'ko42', - 'cleanup|peerRestart|export|baseline', - ); expect(mockMaybeFreeKrefs.add).toHaveBeenCalledWith('ko42'); - // Object-export tear-down handles the c-list pair directly; we don't - // also call deleteCListEntry (which uses the recognizable-only path - // and would corrupt the count). + // An export entry carries no reference, so tearing it down changes no + // count; the object is simply orphaned for GC to retire. + expect(mockDecrementRefCount).not.toHaveBeenCalled(); expect(mockDeleteCListEntry).not.toHaveBeenCalled(); }); - it('preserves baseline refcount when ownership has migrated', () => { + it('leaves the owner mapping alone when ownership has migrated', () => { seedClist([['ro+7', 'ko42']]); mockKV.set(`owner.ko42`, 'someoneElse'); mockGetReachableAndVatSlot.mockReturnValue({ vatSlot: 'ro+7' }); vatMethods.forgetEndpointImports(endpointId); - // Foreign owner survives — the baseline reference is theirs now. expect(mockKV.get(`owner.ko42`)).toBe('someoneElse'); - // Our c-list pair is still torn down (the peer can't reach the kref - // through us anymore), but the refcount stays untouched so we don't - // corrupt the new owner's accounting. + // Our c-list pair is still torn down: the peer can't reach the kref + // through us anymore. expect(mockKV.has(`slot.${endpointId}.ko42`)).toBe(false); expect(mockKV.has(`slot.${endpointId}.ro+7`)).toBe(false); expect(mockDecrementRefCount).not.toHaveBeenCalled(); - expect(mockMaybeFreeKrefs.add).not.toHaveBeenCalled(); + // Queued for GC even though the new owner keeps the kref: tearing our + // pair down may have been what took its last reference, and the new + // owner's accounting decides the outcome. + expect(mockMaybeFreeKrefs.add).toHaveBeenCalledWith('ko42'); }); it('preserves our exports to the peer (import-direction entries)', () => { diff --git a/packages/ocap-kernel/src/store/methods/vat.ts b/packages/ocap-kernel/src/store/methods/vat.ts index 48d431dc10..c1875d8581 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,16 @@ export function getVatMethods(ctx: StoreContext) { work.imports += 1; } - // The caller used enumeratePromisesByDecider() before calling us, - // so they have already rejected the orphan promises, but those - // kpids are still present in the dead vat's c-list. Clean those up now. + // The caller looked the orphan promises up with getPromisesByDecider() and + // rejected them before calling us; that rejection is what released each + // promise's unsettled reference. Their kpids are still in the dead vat's + // c-list, so clean those up now. for (const key of getPrefixedKeys(promisePrefix)) { const krefStr = ctx.kv.get(key) ?? Fail`getNextKey ensures get`; assert(key.startsWith(clistPrefix), key); const vref = key.slice(clistPrefix.length) as ERef; // the following will also delete both db keys deleteCListEntry(vatID, krefStr, vref); - // If the dead vat was still the decider, drop the decider’s refcount, too. - const kp = getKernelPromise(krefStr); - if (kp.decider === vatID) { - decrementRefCount(krefStr, 'cleanup|promise|decider'); - } work.promises += 1; } @@ -377,49 +362,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 +408,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..b9886c174f 100644 --- a/packages/ocap-kernel/src/store/types.ts +++ b/packages/ocap-kernel/src/store/types.ts @@ -11,6 +11,7 @@ export type StoreContext = { runQueue: StoredQueue; // Holds RunAction[] runQueueLengthCache: number; // Holds number refreshRunQueue: () => void; + refreshCachedValues: () => void; nextObjectId: StoredValue; // Holds string nextPromiseId: StoredValue; // Holds string nextVatId: StoredValue; // Holds string @@ -27,6 +28,7 @@ export type StoreContext = { subclusters: StoredValue; // Holds Subcluster[] nextSubclusterId: StoredValue; // Holds string vatToSubclusterMap: StoredValue; // Holds Record + auditRefCounts: boolean; // If set, verify refcounts against ground truth every crank logger?: Logger | undefined; }; diff --git a/packages/ocap-kernel/src/vats/SubclusterManager.test.ts b/packages/ocap-kernel/src/vats/SubclusterManager.test.ts index 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..e437d042b9 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'); @@ -199,6 +207,55 @@ describe('VatManager', () => { expect((error as Error).cause).toBe(cause); }); + + it('tears the worker down when kernel-side registration fails', async () => { + const config = createMockVatConfig(); + const cause = new Error('initEndpoint threw'); + mockKernelStore.initEndpoint.mockImplementationOnce(() => { + throw cause; + }); + + const error = await vatManager + .launchVat(config, 'bob', 's1') + .catch((reason: unknown) => reason); + + expect((error as Error).message).toBe('Failed to launch vat v1 (bob)'); + expect((error as Error).cause).toBe(cause); + // The worker is already running by this point, so it has to be stopped, + // and the vat marked so the terminated-vat cleanup reclaims what the + // partial launch wrote. + expect(mockPlatformServices.terminate).toHaveBeenCalledWith( + 'v1', + expect.any(Error), + ); + expect(vatHandles[0]?.terminate).toHaveBeenCalled(); + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + expect(vatManager.hasVat('v1')).toBe(false); + }); + + it('still marks the vat terminated when the cleanup itself fails', async () => { + const config = createMockVatConfig(); + const cause = new Error('setVatConfig threw'); + mockKernelStore.setVatConfig.mockImplementationOnce(() => { + throw cause; + }); + // `stopVat` unpins the root it was launched with, which is the first + // thing in the teardown that can fail. + mockKernelStore.unpinObject.mockImplementationOnce(() => { + throw new Error('worker will not die'); + }); + + const error = await vatManager + .launchVat(config, 'bob', 's1') + .catch((reason: unknown) => reason); + + expect((error as Error).message).toBe( + 'Failed to launch vat v1 (bob) (cleanup also failed)', + ); + // The launch failure, not the cleanup failure, is what the caller needs. + expect((error as Error).cause).toBe(cause); + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + }); }); describe('runVat', () => { @@ -236,6 +293,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 +504,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 b90f6f30c8..0870a9dacf 100644 --- a/packages/ocap-kernel/src/vats/VatManager.ts +++ b/packages/ocap-kernel/src/vats/VatManager.ts @@ -123,13 +123,42 @@ export class VatManager { cause: error, }); } - this.#kernelStore.initEndpoint(vatId); - const rootRef = this.#kernelStore.exportFromEndpoint( - vatId, - ROOT_OBJECT_VREF, - ); - this.#kernelStore.setVatConfig(vatId, vatConfig); - return rootRef; + try { + this.#kernelStore.initEndpoint(vatId); + const rootRef = this.#kernelStore.exportFromEndpoint( + vatId, + ROOT_OBJECT_VREF, + ); + // A root is addressable for as long as its vat lives, whether or not + // anyone currently imports it: the kernel's own API hands out root krefs + // and `getRootObject` resolves them through this c-list entry. Without a + // pin, GC would retire the entry the moment the last importer let go. + this.#kernelStore.pinObject(rootRef); + this.#kernelStore.setVatConfig(vatId, vatConfig); + return rootRef; + } catch (error) { + // The worker is already running, so leaving it would strand a vat the + // kernel has no record of. Tear it down before reporting the failure. + let stopFailure: unknown; + try { + await this.stopVat(vatId, true); + } catch (caught) { + stopFailure = caught; + this.#logger.error( + `Failed to stop vat ${vatId} after incomplete launch; its worker may still be running:`, + caught, + ); + } + // `stopVat` only tears down the worker. Whatever store records the + // partial launch did write — the endpoint counters, the root's c-list + // pair, its owner entry — are reclaimed by the terminated-vat cleanup, + // which never runs unless the vat is marked. + this.#kernelStore.markVatAsTerminated(vatId); + throw new Error( + `Failed to launch vat ${vatId} (${vatName})${stopFailure ? ' (cleanup also failed)' : ''}`, + { cause: error }, + ); + } } /** @@ -186,6 +215,10 @@ export class VatManager { } else if (terminating) { terminationError = new VatDeletedError(vatId); } + if (terminating) { + // A restart keeps the pin: the same root comes back. + this.releaseVatRootPin(vatId); + } await this.#platformServices .terminate(vatId, terminationError) .catch(this.#logger.error); @@ -286,7 +319,26 @@ export class VatManager { } /** - * Pin a vat root. + * Release the pin `launchVat` took on a vat's root, so the root can be + * collected once its importers let go. + * + * For paths that end a vat's life. Tolerant of a root that is already gone, + * since a vat can be torn down after the kernel has lost track of it. + * + * @param vatId - The ID of the vat whose life is ending. + */ + releaseVatRootPin(vatId: VatId): void { + const rootRef = this.#kernelStore.getRootObject(vatId); + if (rootRef) { + this.#kernelStore.unpinObject(rootRef); + } + } + + /** + * Pin a vat root, on behalf of an embedder that wants to keep it addressable. + * + * Pins are counted, and `launchVat` already holds one for the vat's lifetime, + * so this adds to that rather than replacing it. * * @param vatId - The ID of the vat. * @returns The KRef of the vat root. @@ -301,7 +353,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/src/vats/VatSyscall.test.ts b/packages/ocap-kernel/src/vats/VatSyscall.test.ts index 04a0e8bf42..bf425bc23b 100644 --- a/packages/ocap-kernel/src/vats/VatSyscall.test.ts +++ b/packages/ocap-kernel/src/vats/VatSyscall.test.ts @@ -30,6 +30,9 @@ describe('VatSyscall', () => { clearReachableFlag: vi.fn(), getReachableFlag: vi.fn(), forgetKref: vi.fn(), + // Only an owner may disown an object, so the cleanup syscalls check first + getOwner: vi.fn().mockReturnValue('v1'), + orphanKernelObject: vi.fn(), getVatConfig: vi.fn(() => ({})), isVatActive: vi.fn(() => true), isInCrank: vi.fn(() => true), 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, }; } diff --git a/packages/ocap-kernel/test/savepoint-stack.ts b/packages/ocap-kernel/test/savepoint-stack.ts new file mode 100644 index 0000000000..c4b7e587fc --- /dev/null +++ b/packages/ocap-kernel/test/savepoint-stack.ts @@ -0,0 +1,53 @@ +import type { MockedFunction } from 'vitest'; +import { vi } from 'vitest'; + +import type { KernelStore } from '../src/store/index.ts'; + +export type FailingReleaseStore = { + /** The store to hand the subject under test. */ + kernelStore: KernelStore; + /** The error every `releaseSavepoint` throws. */ + releaseFailure: Error; + /** Exposed so a test can assert the rollback was still attempted. */ + rollbackSavepoint: MockedFunction<(name: string) => void>; +}; + +/** + * Wrap a kernel store so that `releaseSavepoint` fails the way a full disk does, + * modelling the drivers' bookkeeping as of #1012 and verified against both: a + * failed RELEASE clears the savepoint stack, and rolling back a name that is no + * longer on it throws `No such savepoint`. What is under test is therefore the + * caller's error handling, not an expected outcome baked into the mock. + * + * Replaces the store wholesale rather than assigning over its methods, because + * `makeKernelStore` hardens what it returns. + * + * @param kernelStore - The store to wrap. + * @returns The wrapped store and the handles a test needs to assert against. + */ +export function withFailingSavepointRelease( + kernelStore: KernelStore, +): FailingReleaseStore { + const savepoints: string[] = []; + const releaseFailure = new Error('database or disk is full'); + const rollbackSavepoint = vi.fn((name: string) => { + if (!savepoints.includes(name)) { + throw new Error(`No such savepoint: ${name}`); + } + }); + return { + kernelStore: { + ...kernelStore, + createSavepoint: (name: string) => { + savepoints.push(name); + }, + releaseSavepoint: () => { + savepoints.length = 0; + throw releaseFailure; + }, + rollbackSavepoint, + }, + releaseFailure, + rollbackSavepoint, + }; +}