From b0668c157fefcd0cb065a4ad249e4d0fe7f50ae6 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 13 May 2025 11:49:53 +0200 Subject: [PATCH 01/25] feat: Add crank savepoints --- .../kernel-store/src/sqlite/common.test.ts | 31 ++++- packages/kernel-store/src/sqlite/common.ts | 48 ++++--- .../kernel-store/src/sqlite/nodejs.test.ts | 93 +++++++++++++ packages/kernel-store/src/sqlite/nodejs.ts | 38 +++++- packages/kernel-store/src/sqlite/wasm.test.ts | 111 +++++++++++++++ packages/kernel-store/src/sqlite/wasm.ts | 56 +++++--- packages/kernel-store/src/types.ts | 3 + packages/kernel-test/src/exo.test.ts | 1 - .../src/garbage-collection.test.ts | 1 - packages/kernel-test/src/liveslots.test.ts | 1 - packages/kernel-test/src/logger.test.ts | 1 - packages/kernel-test/src/resume.test.ts | 1 - packages/kernel-test/src/savepoint.test.ts | 82 ++++++++++++ packages/kernel-test/src/supervisor.test.ts | 1 - packages/kernel-test/vitest.config.ts | 2 + packages/ocap-kernel/src/KernelQueue.test.ts | 6 + packages/ocap-kernel/src/KernelQueue.ts | 52 ++++---- packages/ocap-kernel/src/KernelRouter.test.ts | 39 ++++++ packages/ocap-kernel/src/KernelRouter.ts | 1 + packages/ocap-kernel/src/store/index.test.ts | 4 + packages/ocap-kernel/src/store/index.ts | 10 ++ .../src/store/methods/crank.test.ts | 126 ++++++++++++++++++ .../ocap-kernel/src/store/methods/crank.ts | 70 ++++++++++ packages/ocap-kernel/src/store/types.ts | 2 + packages/ocap-kernel/test/storage.ts | 9 ++ vitest.config.ts | 8 +- 26 files changed, 723 insertions(+), 74 deletions(-) create mode 100644 packages/kernel-test/src/savepoint.test.ts create mode 100644 packages/ocap-kernel/src/store/methods/crank.test.ts create mode 100644 packages/ocap-kernel/src/store/methods/crank.ts diff --git a/packages/kernel-store/src/sqlite/common.test.ts b/packages/kernel-store/src/sqlite/common.test.ts index 5fa1a4fdb..4a05ef1ad 100644 --- a/packages/kernel-store/src/sqlite/common.test.ts +++ b/packages/kernel-store/src/sqlite/common.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from 'vitest'; -import { SQL_QUERIES } from './common.ts'; +import { SQL_QUERIES, safeIdentifier } from './common.ts'; describe('SQL_QUERIES', () => { // XXX Is this test actually useful? It's basically testing that the source code matches itself. @@ -44,6 +44,7 @@ describe('SQL_QUERIES', () => { 'CLEAR', 'CLEAR_VS', 'COMMIT_TRANSACTION', + 'CREATE_SAVEPOINT', 'CREATE_TABLE', 'CREATE_TABLE_VS', 'DELETE', @@ -54,8 +55,36 @@ describe('SQL_QUERIES', () => { 'GET', 'GET_ALL_VS', 'GET_NEXT', + 'RELEASE_SAVEPOINT', + 'ROLLBACK_SAVEPOINT', 'SET', 'SET_VS', ]); }); }); + +describe('safeIdentifier', () => { + it('accepts valid SQL identifiers', () => { + expect(safeIdentifier('valid')).toBe('valid'); + expect(safeIdentifier('Valid')).toBe('Valid'); + expect(safeIdentifier('valid_name')).toBe('valid_name'); + expect(safeIdentifier('valid_name_123')).toBe('valid_name_123'); + expect(safeIdentifier('_leading_underscore')).toBe('_leading_underscore'); + }); + + it('rejects invalid SQL identifiers', () => { + // Starting with a number + expect(() => safeIdentifier('123invalid')).toThrow('Invalid identifier'); + + // Containing invalid characters + expect(() => safeIdentifier('invalid-name')).toThrow('Invalid identifier'); + expect(() => safeIdentifier('invalid.name')).toThrow('Invalid identifier'); + expect(() => safeIdentifier('invalid;name')).toThrow('Invalid identifier'); + expect(() => safeIdentifier('invalid name')).toThrow('Invalid identifier'); + + // Containing SQL injection attempts + expect(() => safeIdentifier("name'; DROP TABLE users--")).toThrow( + 'Invalid identifier', + ); + }); +}); diff --git a/packages/kernel-store/src/sqlite/common.ts b/packages/kernel-store/src/sqlite/common.ts index 860c7d042..9803b3345 100644 --- a/packages/kernel-store/src/sqlite/common.ts +++ b/packages/kernel-store/src/sqlite/common.ts @@ -52,30 +52,36 @@ export const SQL_QUERIES = { DELETE FROM kv_vatstore WHERE vatID = ? `, - CLEAR: ` - DELETE FROM kv - `, - CLEAR_VS: ` - DELETE FROM kv_vatstore - `, - DROP: ` - DROP TABLE kv - `, - DROP_VS: ` - DROP TABLE kv_vatstore - `, - BEGIN_TRANSACTION: ` - BEGIN TRANSACTION - `, - COMMIT_TRANSACTION: ` - COMMIT TRANSACTION - `, - ABORT_TRANSACTION: ` - ROLLBACK TRANSACTION - `, + CLEAR: `DELETE FROM kv`, + CLEAR_VS: `DELETE FROM kv_vatstore`, + DROP: `DROP TABLE kv`, + DROP_VS: `DROP TABLE kv_vatstore`, + BEGIN_TRANSACTION: `BEGIN TRANSACTION`, + COMMIT_TRANSACTION: `COMMIT TRANSACTION`, + ABORT_TRANSACTION: `ROLLBACK TRANSACTION`, + // SQLite’s parameter markers (?, ?NNN, :name, @name, $name) can only be used + // in places where a literal value is allowed. We can’t bind identifiers + // for table names, column names, or savepoint names. We use %NAME% as a + // placeholder for the savepoint name. + CREATE_SAVEPOINT: `SAVEPOINT %NAME%`, + ROLLBACK_SAVEPOINT: `ROLLBACK TO SAVEPOINT %NAME%`, + RELEASE_SAVEPOINT: `RELEASE SAVEPOINT %NAME%`, } as const; /** * The default filename for the SQLite database; ":memory:" is an ephemeral in-memory database. */ export const DEFAULT_DB_FILENAME = ':memory:'; + +/** + * Check if a string is a valid SQLite identifier. + * + * @param name - The string to check. + * @returns The string if it is a valid identifier. + */ +export function safeIdentifier(name: string): string { + if (!/^[A-Za-z_]\w*$/u.test(name)) { + throw new Error(`Invalid identifier: ${name}`); + } + return name; +} diff --git a/packages/kernel-store/src/sqlite/nodejs.test.ts b/packages/kernel-store/src/sqlite/nodejs.test.ts index a3727e423..6639e5927 100644 --- a/packages/kernel-store/src/sqlite/nodejs.test.ts +++ b/packages/kernel-store/src/sqlite/nodejs.test.ts @@ -25,6 +25,7 @@ const mockStatement = { const mockDb = { prepare: vi.fn(() => mockStatement), transaction: vi.fn((fn) => fn), + exec: vi.fn(), }; vi.mock('better-sqlite3', () => ({ @@ -137,6 +138,49 @@ describe('makeSQLKernelDatabase', () => { expect(mockStatement.run).toHaveBeenCalled(); // commit transaction }); + describe('deleteVatStore functionality', () => { + beforeEach(() => { + Object.values(mockStatement).forEach((mock) => mock.mockReset()); + }); + + it('deleteVatStore removes all data for a given vat', async () => { + const db = await makeSQLKernelDatabase({}); + const vatId = 'test-vat'; + db.deleteVatStore(vatId); + expect(mockDb.prepare).toHaveBeenCalledWith(SQL_QUERIES.DELETE_VS_ALL); + expect(mockStatement.run).toHaveBeenCalledWith(vatId); + }); + + it('deleteVatStore handles empty vatId correctly', async () => { + const db = await makeSQLKernelDatabase({}); + db.deleteVatStore(''); + expect(mockStatement.run).toHaveBeenCalledWith(''); + }); + + it("deleteVatStore doesn't affect other vat stores", async () => { + const db = await makeSQLKernelDatabase({}); + db.makeVatStore('vat1'); + const vatStore2 = db.makeVatStore('vat2'); + db.deleteVatStore('vat1'); + mockStatement.iterate.mockReturnValueOnce([ + { key: 'testKey', value: 'testValue' }, + ]); + const data = vatStore2.getKVData(); + expect(data).toStrictEqual([['testKey', 'testValue']]); + expect(mockStatement.iterate).toHaveBeenCalledWith('vat2'); + }); + + it('deleteVatStore handles errors correctly', async () => { + const db = await makeSQLKernelDatabase({}); + mockStatement.run.mockImplementationOnce(() => { + throw new Error('Database error during delete'); + }); + expect(() => db.deleteVatStore('test-vat')).toThrow( + 'Database error during delete', + ); + }); + }); + describe('getDBFilename', () => { it('returns in-memory database path when label starts with ":"', async () => { const result = await getDBFilename(':memory:'); @@ -151,4 +195,53 @@ describe('makeSQLKernelDatabase', () => { }); }); }); + + describe('savepoint functionality', () => { + let execSpy: ReturnType; + + beforeEach(async () => { + execSpy = vi.fn(); + (mockDb.exec as unknown) = execSpy; + }); + + it('creates a savepoint using sanitized name', async () => { + const db = await makeSQLKernelDatabase({}); + db.createSavepoint('valid_name'); + expect(execSpy).toHaveBeenCalledWith( + expect.stringContaining('SAVEPOINT valid_name'), + ); + }); + + it('rejects invalid savepoint names', async () => { + const db = await makeSQLKernelDatabase({}); + expect(() => db.createSavepoint('invalid-name')).toThrow( + 'Invalid identifier', + ); + expect(() => db.createSavepoint('123numeric')).toThrow( + 'Invalid identifier', + ); + expect(() => db.createSavepoint('spaces not allowed')).toThrow( + 'Invalid identifier', + ); + expect(() => db.createSavepoint("point'; DROP TABLE kv--")).toThrow( + 'Invalid identifier', + ); + }); + + it('rolls back to a savepoint', async () => { + const db = await makeSQLKernelDatabase({}); + db.rollbackSavepoint('test_point'); + expect(execSpy).toHaveBeenCalledWith( + expect.stringContaining('ROLLBACK TO SAVEPOINT test_point'), + ); + }); + + it('releases a savepoint', async () => { + const db = await makeSQLKernelDatabase({}); + db.releaseSavepoint('test_point'); + expect(execSpy).toHaveBeenCalledWith( + expect.stringContaining('RELEASE SAVEPOINT test_point'), + ); + }); + }); }); diff --git a/packages/kernel-store/src/sqlite/nodejs.ts b/packages/kernel-store/src/sqlite/nodejs.ts index 456173913..e53aa3eeb 100644 --- a/packages/kernel-store/src/sqlite/nodejs.ts +++ b/packages/kernel-store/src/sqlite/nodejs.ts @@ -6,7 +6,7 @@ import { mkdir } from 'fs/promises'; import { tmpdir } from 'os'; import { join } from 'path'; -import { SQL_QUERIES, DEFAULT_DB_FILENAME } from './common.ts'; +import { SQL_QUERIES, DEFAULT_DB_FILENAME, safeIdentifier } from './common.ts'; import { getDBFolder } from './env.ts'; import type { KVStore, VatStore, KernelDatabase } from '../types.ts'; @@ -223,12 +223,48 @@ export async function makeSQLKernelDatabase({ sqlVatstoreDeleteAll.run(vatId); } + /** + * Create a savepoint in the database. + * + * @param name - The name of the savepoint. + */ + function createSavepoint(name: string): void { + const point = safeIdentifier(name); + const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', point); + db.exec(query); + } + + /** + * Rollback to a savepoint in the database. + * + * @param name - The name of the savepoint. + */ + function rollbackSavepoint(name: string): void { + const point = safeIdentifier(name); + const query = SQL_QUERIES.ROLLBACK_SAVEPOINT.replace('%NAME%', point); + db.exec(query); + } + + /** + * Release a savepoint in the database. + * + * @param name - The name of the savepoint. + */ + function releaseSavepoint(name: string): void { + const point = safeIdentifier(name); + const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', point); + db.exec(query); + } + return { kernelKVStore: kvStore, executeQuery: kvExecuteQuery, clear: db.transaction(kvClear), makeVatStore, deleteVatStore, + createSavepoint, + rollbackSavepoint, + releaseSavepoint, }; } diff --git a/packages/kernel-store/src/sqlite/wasm.test.ts b/packages/kernel-store/src/sqlite/wasm.test.ts index 69a828d12..d3b385443 100644 --- a/packages/kernel-store/src/sqlite/wasm.test.ts +++ b/packages/kernel-store/src/sqlite/wasm.test.ts @@ -364,4 +364,115 @@ describe('makeSQLKernelDatabase', () => { expect(mockStatement.reset).toHaveBeenCalled(); }); }); + + describe('savepoint functionality', () => { + beforeEach(() => { + mockDb.exec.mockClear(); + }); + + it('creates a savepoint using sanitized name', async () => { + const db = await makeSQLKernelDatabase({}); + db.createSavepoint('valid_name'); + + expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT valid_name'); + }); + + it('rejects invalid savepoint names', async () => { + const db = await makeSQLKernelDatabase({}); + expect(() => db.createSavepoint('invalid-name')).toThrow( + 'Invalid identifier', + ); + expect(() => db.createSavepoint('123numeric')).toThrow( + 'Invalid identifier', + ); + expect(() => db.createSavepoint('spaces not allowed')).toThrow( + 'Invalid identifier', + ); + expect(() => db.createSavepoint("point'; DROP TABLE kv--")).toThrow( + 'Invalid identifier', + ); + expect(mockDb.exec).not.toHaveBeenCalledWith( + expect.stringContaining('DROP TABLE'), + ); + }); + + it('rolls back to a savepoint', async () => { + const db = await makeSQLKernelDatabase({}); + db.rollbackSavepoint('test_point'); + expect(mockDb.exec).toHaveBeenCalledWith( + 'ROLLBACK TO SAVEPOINT test_point', + ); + }); + + it('releases a savepoint', async () => { + const db = await makeSQLKernelDatabase({}); + db.releaseSavepoint('test_point'); + expect(mockDb.exec).toHaveBeenCalledWith('RELEASE SAVEPOINT test_point'); + }); + }); + + it('deleteVatStore removes all data for a given vat', async () => { + Object.values(mockStatement).forEach((mock) => { + if (typeof mock === 'function' && mock.mockReset) { + mock.mockReset(); + } + }); + const db = await makeSQLKernelDatabase({}); + const vatId = 'test-vat'; + db.deleteVatStore(vatId); + expect(mockDb.prepare).toHaveBeenCalledWith(SQL_QUERIES.DELETE_VS_ALL); + expect(mockStatement.bind).toHaveBeenCalledWith([vatId]); + expect(mockStatement.step).toHaveBeenCalled(); + expect(mockStatement.reset).toHaveBeenCalled(); + }); + + it('deleteVatStore handles errors correctly', async () => { + Object.values(mockStatement).forEach((mock) => { + if (typeof mock === 'function' && mock.mockReset) { + mock.mockReset(); + } + }); + mockStatement.step.mockImplementationOnce(() => { + throw new Error('Database error'); + }); + const db = await makeSQLKernelDatabase({}); + expect(() => db.deleteVatStore('test-vat')).toThrow('Database error'); + expect(mockStatement.bind).toHaveBeenCalled(); + expect(mockStatement.reset).not.toHaveBeenCalled(); + }); + + it('deleteVatStore handles empty vatId correctly', async () => { + Object.values(mockStatement).forEach((mock) => { + if (typeof mock === 'function' && mock.mockReset) { + mock.mockReset(); + } + }); + + const db = await makeSQLKernelDatabase({}); + db.deleteVatStore(''); + expect(mockStatement.bind).toHaveBeenCalledWith(['']); + expect(mockStatement.step).toHaveBeenCalled(); + expect(mockStatement.reset).toHaveBeenCalled(); + }); + + it("deleteVatStore doesn't affect other vat stores", async () => { + Object.values(mockStatement).forEach((mock) => { + if (typeof mock === 'function' && mock.mockReset) { + mock.mockReset(); + } + }); + + const db = await makeSQLKernelDatabase({}); + db.makeVatStore('vat1'); + const vatStore2 = db.makeVatStore('vat2'); + db.deleteVatStore('vat1'); + mockStatement.step.mockReturnValueOnce(true).mockReturnValueOnce(false); + mockStatement.getString + .mockReturnValueOnce('testKey') + .mockReturnValueOnce('testValue'); + + const data = vatStore2.getKVData(); + expect(mockStatement.bind).toHaveBeenCalledWith(['vat2']); + expect(data).toStrictEqual([['testKey', 'testValue']]); + }); }); diff --git a/packages/kernel-store/src/sqlite/wasm.ts b/packages/kernel-store/src/sqlite/wasm.ts index d407e417a..5a9f704a7 100644 --- a/packages/kernel-store/src/sqlite/wasm.ts +++ b/packages/kernel-store/src/sqlite/wasm.ts @@ -1,8 +1,8 @@ import { Logger } from '@metamask/logger'; -import type { Database, PreparedStatement } from '@sqlite.org/sqlite-wasm'; +import type { Database } from '@sqlite.org/sqlite-wasm'; import sqlite3InitModule from '@sqlite.org/sqlite-wasm'; -import { DEFAULT_DB_FILENAME, SQL_QUERIES } from './common.ts'; +import { DEFAULT_DB_FILENAME, safeIdentifier, SQL_QUERIES } from './common.ts'; import { getDBFolder } from './env.ts'; import type { KVStore, VatStore, KernelDatabase } from '../types.ts'; @@ -29,22 +29,6 @@ export async function initDB(dbFilename: string): Promise { return db; } -/** - * Helper function to paper over SQLite-wasm awfulness. Runs a prepared - * statement as it would be run in a more sensible API. - * - * @param stmt - A prepared statement to run. - * @param bindings - Optional parameters to bind for execution. - */ -// eslint-disable-next-line @typescript-eslint/no-unused-vars -function run(stmt: PreparedStatement, ...bindings: string[]): void { - if (bindings && bindings.length > 0) { - stmt.bind(bindings); - } - stmt.step(); - stmt.reset(); -} - /** * Makes a {@link KVStore} on top of a SQLite database * @@ -300,11 +284,47 @@ export async function makeSQLKernelDatabase({ sqlVatstoreDeleteAll.reset(); } + /** + * Create a savepoint in the database. + * + * @param name - The name of the savepoint. + */ + function createSavepoint(name: string): void { + const point = safeIdentifier(name); + const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', point); + db.exec(query); + } + + /** + * Rollback to a savepoint in the database. + * + * @param name - The name of the savepoint. + */ + function rollbackSavepoint(name: string): void { + const point = safeIdentifier(name); + const query = SQL_QUERIES.ROLLBACK_SAVEPOINT.replace('%NAME%', point); + db.exec(query); + } + + /** + * Release a savepoint in the database. + * + * @param name - The name of the savepoint. + */ + function releaseSavepoint(name: string): void { + const point = safeIdentifier(name); + const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', point); + db.exec(query); + } + return { kernelKVStore: kvStore, clear: kvClear, executeQuery, makeVatStore, deleteVatStore, + createSavepoint, + rollbackSavepoint, + releaseSavepoint, }; } diff --git a/packages/kernel-store/src/types.ts b/packages/kernel-store/src/types.ts index 64fd0c6e7..d37bd524b 100644 --- a/packages/kernel-store/src/types.ts +++ b/packages/kernel-store/src/types.ts @@ -30,4 +30,7 @@ export type KernelDatabase = { clear(): void; makeVatStore(vatID: string): VatStore; deleteVatStore(vatID: string): void; + createSavepoint(name: string): void; + rollbackSavepoint(name: string): void; + releaseSavepoint(name: string): void; }; diff --git a/packages/kernel-test/src/exo.test.ts b/packages/kernel-test/src/exo.test.ts index 4c06f7c79..19ae1faf3 100644 --- a/packages/kernel-test/src/exo.test.ts +++ b/packages/kernel-test/src/exo.test.ts @@ -1,4 +1,3 @@ -import '@metamask/kernel-shims/endoify'; import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; import { waitUntilQuiescent } from '@metamask/kernel-utils'; import type { LogEntry } from '@metamask/logger'; diff --git a/packages/kernel-test/src/garbage-collection.test.ts b/packages/kernel-test/src/garbage-collection.test.ts index d6beb1e28..bd8687d7e 100644 --- a/packages/kernel-test/src/garbage-collection.test.ts +++ b/packages/kernel-test/src/garbage-collection.test.ts @@ -1,4 +1,3 @@ -import '@metamask/kernel-shims/endoify'; import type { KernelDatabase } from '@metamask/kernel-store'; import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; import { waitUntilQuiescent } from '@metamask/kernel-utils'; diff --git a/packages/kernel-test/src/liveslots.test.ts b/packages/kernel-test/src/liveslots.test.ts index 7209bcabd..c8148ea9d 100644 --- a/packages/kernel-test/src/liveslots.test.ts +++ b/packages/kernel-test/src/liveslots.test.ts @@ -1,4 +1,3 @@ -import '@metamask/kernel-shims/endoify'; import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; import { waitUntilQuiescent } from '@metamask/kernel-utils'; import type { LogEntry } from '@metamask/logger'; diff --git a/packages/kernel-test/src/logger.test.ts b/packages/kernel-test/src/logger.test.ts index 19f65fa62..29a154188 100644 --- a/packages/kernel-test/src/logger.test.ts +++ b/packages/kernel-test/src/logger.test.ts @@ -1,4 +1,3 @@ -import '@metamask/kernel-shims/endoify'; import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; import { waitUntilQuiescent } from '@metamask/kernel-utils'; import type { VatId } from '@metamask/ocap-kernel'; diff --git a/packages/kernel-test/src/resume.test.ts b/packages/kernel-test/src/resume.test.ts index 2489e3ec9..ad2b702ff 100644 --- a/packages/kernel-test/src/resume.test.ts +++ b/packages/kernel-test/src/resume.test.ts @@ -1,4 +1,3 @@ -import '@metamask/kernel-shims/endoify'; import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; import { waitUntilQuiescent } from '@metamask/kernel-utils'; import { describe, expect, it } from 'vitest'; diff --git a/packages/kernel-test/src/savepoint.test.ts b/packages/kernel-test/src/savepoint.test.ts new file mode 100644 index 000000000..7ce4c80b1 --- /dev/null +++ b/packages/kernel-test/src/savepoint.test.ts @@ -0,0 +1,82 @@ +import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; +import { describe, it, expect, vi } from 'vitest'; + +/** + * Helper to create a test database with some initial data + * + * @returns A SQLite database instance with initial data + */ +async function setupTestDb() { + const db = await makeSQLKernelDatabase({ + dbFilename: ':memory:', + label: 'savepoint-test', + }); + const { kernelKVStore } = db; + kernelKVStore.set('key1', 'value1'); + kernelKVStore.set('key2', 'value2'); + return db; +} + +describe('Savepoint functionality', () => { + it('allows creating and releasing a savepoint', async () => { + const db = await setupTestDb(); + db.createSavepoint('test_point'); + db.kernelKVStore.set('key3', 'value3'); + db.releaseSavepoint('test_point'); + expect(db.kernelKVStore.get('key3')).toBe('value3'); + }); + + it('can rollback to a savepoint to undo changes', async () => { + const db = await setupTestDb(); + expect(db.kernelKVStore.get('key1')).toBe('value1'); + expect(db.kernelKVStore.get('key2')).toBe('value2'); + db.createSavepoint('test_point'); + db.kernelKVStore.set('key1', 'modified1'); + db.kernelKVStore.set('key3', 'value3'); + db.kernelKVStore.delete('key2'); + expect(db.kernelKVStore.get('key1')).toBe('modified1'); + expect(db.kernelKVStore.get('key2')).toBeUndefined(); + expect(db.kernelKVStore.get('key3')).toBe('value3'); + db.rollbackSavepoint('test_point'); + expect(db.kernelKVStore.get('key1')).toBe('value1'); + expect(db.kernelKVStore.get('key2')).toBe('value2'); + expect(db.kernelKVStore.get('key3')).toBeUndefined(); + }); + + it('supports nested savepoints', async () => { + const db = await setupTestDb(); + db.createSavepoint('outer'); + db.kernelKVStore.set('key3', 'value3'); + db.createSavepoint('inner'); + db.kernelKVStore.set('key4', 'value4'); + expect(db.kernelKVStore.get('key3')).toBe('value3'); + expect(db.kernelKVStore.get('key4')).toBe('value4'); + db.rollbackSavepoint('inner'); + expect(db.kernelKVStore.get('key3')).toBe('value3'); + expect(db.kernelKVStore.get('key4')).toBeUndefined(); + db.releaseSavepoint('outer'); + expect(db.kernelKVStore.get('key3')).toBe('value3'); + }); + + it('rejects invalid savepoint names', async () => { + const db = await setupTestDb(); + expect(() => db.createSavepoint('invalid-name')).toThrow( + 'Invalid identifier', + ); + expect(() => db.createSavepoint('123numeric')).toThrow( + 'Invalid identifier', + ); + expect(() => db.createSavepoint('spaces not allowed')).toThrow( + 'Invalid identifier', + ); + }); + + it('sanitizes savepoint names to prevent SQL injection', async () => { + const db = await setupTestDb(); + const executeQuerySpy = vi.spyOn(db, 'executeQuery'); + expect(() => db.createSavepoint("point'; DROP TABLE kv--")).toThrow( + 'Invalid identifier', + ); + expect(executeQuerySpy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/kernel-test/src/supervisor.test.ts b/packages/kernel-test/src/supervisor.test.ts index 8355b6824..614df1768 100644 --- a/packages/kernel-test/src/supervisor.test.ts +++ b/packages/kernel-test/src/supervisor.test.ts @@ -1,4 +1,3 @@ -import '@metamask/kernel-shims/endoify'; import { delay } from '@metamask/kernel-utils'; import type { JsonRpcMessage } from '@metamask/kernel-utils'; import type { VatConfig } from '@metamask/ocap-kernel'; diff --git a/packages/kernel-test/vitest.config.ts b/packages/kernel-test/vitest.config.ts index 1f60917fc..08608da08 100644 --- a/packages/kernel-test/vitest.config.ts +++ b/packages/kernel-test/vitest.config.ts @@ -1,3 +1,4 @@ +import path from 'path'; import { defineProject, mergeConfig } from 'vitest/config'; import defaultConfig from '../../vitest.config.ts'; @@ -8,6 +9,7 @@ const config = mergeConfig( test: { name: 'kernel-test', pool: 'forks', + setupFiles: path.resolve(__dirname, '../kernel-shims/src/endoify.js'), }, }), ); diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index d0fcbc26b..f83c647d3 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -45,6 +45,9 @@ describe('KernelQueue', () => { resolveKernelPromise: vi.fn(), nextReapAction: vi.fn().mockReturnValue(null), getGCActions: vi.fn().mockReturnValue([]), + startCrank: vi.fn(), + endCrank: vi.fn(), + createCrankSavepoint: vi.fn(), } as unknown as KernelStore; kernelQueue = new KernelQueue(kernelStore); @@ -67,7 +70,10 @@ describe('KernelQueue', () => { const deliver = vi.fn().mockRejectedValue(deliverError); await expect(kernelQueue.run(deliver)).rejects.toBe(deliverError); expect(kernelStore.nextTerminatedVatCleanup).toHaveBeenCalled(); + expect(kernelStore.startCrank).toHaveBeenCalled(); + expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('start'); expect(deliver).toHaveBeenCalledWith(mockItem); + expect(kernelStore.endCrank).toHaveBeenCalled(); }); }); diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index 2a2e9af92..e981ab747 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -58,34 +58,40 @@ export class KernelQueue { */ async *#runQueueItems(): AsyncGenerator { for (;;) { - const gcAction = processGCActionSet(this.#kernelStore); - if (gcAction) { - yield gcAction; - continue; - } + this.#kernelStore.startCrank(); + try { + this.#kernelStore.createCrankSavepoint('start'); + const gcAction = processGCActionSet(this.#kernelStore); + if (gcAction) { + yield gcAction; + continue; + } - const reapAction = this.#kernelStore.nextReapAction(); - if (reapAction) { - yield reapAction; - continue; - } + const reapAction = this.#kernelStore.nextReapAction(); + if (reapAction) { + yield reapAction; + continue; + } - while (this.#kernelStore.runQueueLength() > 0) { - const item = this.#kernelStore.dequeueRun(); - if (item) { - yield item; - } else { - break; + while (this.#kernelStore.runQueueLength() > 0) { + const item = this.#kernelStore.dequeueRun(); + if (item) { + yield item; + } else { + break; + } } - } - if (this.#kernelStore.runQueueLength() === 0) { - const { promise, resolve } = makePromiseKit(); - if (this.#wakeUpTheRunQueue !== null) { - Fail`wakeUpTheRunQueue function already set`; + if (this.#kernelStore.runQueueLength() === 0) { + const { promise, resolve } = makePromiseKit(); + if (this.#wakeUpTheRunQueue !== null) { + Fail`wakeUpTheRunQueue function already set`; + } + this.#wakeUpTheRunQueue = resolve; + await promise; } - this.#wakeUpTheRunQueue = resolve; - await promise; + } finally { + this.#kernelStore.endCrank(); } } } diff --git a/packages/ocap-kernel/src/KernelRouter.test.ts b/packages/ocap-kernel/src/KernelRouter.test.ts index 3216dec40..743179591 100644 --- a/packages/ocap-kernel/src/KernelRouter.test.ts +++ b/packages/ocap-kernel/src/KernelRouter.test.ts @@ -65,6 +65,7 @@ describe('KernelRouter', () => { krefsToExistingErefs: vi.fn((_vatId: string, krefs: string[]) => krefs.map((kref: string) => `translated-${kref}`), ) as unknown as MockInstance, + createCrankSavepoint: vi.fn(), } as unknown as KernelStore; // Mock KernelQueue @@ -77,6 +78,28 @@ describe('KernelRouter', () => { }); describe('deliver', () => { + it('creates a savepoint at the start of delivery', async () => { + // Minimal send item for this test + const sendItem: RunQueueItemSend = { + type: 'send', + target: 'ko123', + message: { + methargs: { body: 'test', slots: [] }, + result: null, + } as unknown as SwingsetMessage, + }; + + // Return a valid vat owner so the delivery proceeds + (kernelStore.getOwner as unknown as MockInstance).mockReturnValueOnce( + 'v1', + ); + + await kernelRouter.deliver(sendItem); + + // Verify savepoint was created + expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('deliver'); + }); + describe('send', () => { it('delivers a send message to a vat with an object target', async () => { // Setup the kernel store to return an owner for the target @@ -99,6 +122,10 @@ describe('KernelRouter', () => { message: message as unknown as SwingsetMessage, }; await kernelRouter.deliver(sendItem); + // Verify savepoint was created + expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith( + 'deliver', + ); // Verify the message was delivered to the vat expect(getVat).toHaveBeenCalledWith(vatId); expect(vatHandle.deliverMessage).toHaveBeenCalledWith( @@ -232,6 +259,10 @@ describe('KernelRouter', () => { }); // Deliver the notify await kernelRouter.deliver(notifyItem); + // Verify savepoint was created + expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith( + 'deliver', + ); // Verify the notification was delivered to the vat expect(getVat).toHaveBeenCalledWith(vatId); expect(vatHandle.deliverNotify).toHaveBeenCalledWith(expect.any(Array)); @@ -282,6 +313,10 @@ describe('KernelRouter', () => { }; // Deliver the GC action await kernelRouter.deliver(gcAction); + // Verify savepoint was created + expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith( + 'deliver', + ); // Verify the action was delivered to the vat expect(getVat).toHaveBeenCalledWith(vatId); expect( @@ -299,6 +334,10 @@ describe('KernelRouter', () => { }; // Deliver the bringOutYourDead action await kernelRouter.deliver(bringOutYourDeadItem); + // Verify savepoint was created + expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith( + 'deliver', + ); // Verify the action was delivered to the vat expect(getVat).toHaveBeenCalledWith(vatId); expect(vatHandle.deliverBringOutYourDead).toHaveBeenCalled(); diff --git a/packages/ocap-kernel/src/KernelRouter.ts b/packages/ocap-kernel/src/KernelRouter.ts index 9be7c723b..9de622387 100644 --- a/packages/ocap-kernel/src/KernelRouter.ts +++ b/packages/ocap-kernel/src/KernelRouter.ts @@ -76,6 +76,7 @@ export class KernelRouter { * @param item - The message/notification to deliver. */ async deliver(item: RunQueueItem): Promise { + this.#kernelStore.createCrankSavepoint('deliver'); switch (item.type) { case 'send': await this.#deliverSend(item); diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 25a2be270..796dae262 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -48,6 +48,7 @@ describe('kernel store', () => { 'clear', 'clearReachableFlag', 'collectGarbage', + 'createCrankSavepoint', 'decRefCount', 'decrementRefCount', 'deleteCListEntry', @@ -57,6 +58,7 @@ describe('kernel store', () => { 'deleteVat', 'deleteVatConfig', 'dequeueRun', + 'endCrank', 'enqueuePromiseMessage', 'enqueueRun', 'erefToKref', @@ -108,12 +110,14 @@ describe('kernel store', () => { 'reset', 'resolveKernelPromise', 'retireKernelObjects', + 'rollbackCrank', 'runQueueLength', 'scheduleReap', 'setGCActions', 'setObjectRefCount', 'setPromiseDecider', 'setVatConfig', + 'startCrank', 'translateCapDataKtoV', 'translateMessageKtoV', 'translateRefKtoV', diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 5e01fc2f4..370f7b9f4 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -60,6 +60,7 @@ import type { KernelDatabase, KVStore, VatStore } from '@metamask/kernel-store'; import type { KRef, VatId } from '../types.ts'; import { getBaseMethods } from './methods/base.ts'; import { getCListMethods } from './methods/clist.ts'; +import { getCrankMethods } from './methods/crank.ts'; import { getGCMethods } from './methods/gc.ts'; import { getIdMethods } from './methods/id.ts'; import { getObjectMethods } from './methods/object.ts'; @@ -124,6 +125,8 @@ export function makeKernelStore(kdb: KernelDatabase) { gcActions: provideCachedStoredValue('gcActions', '[]'), reapQueue: provideCachedStoredValue('reapQueue', '[]'), terminatedVats: provideCachedStoredValue('vats.terminated', '[]'), + inCrank: false, + savepoints: [], }; const id = getIdMethods(context); @@ -137,6 +140,7 @@ export function makeKernelStore(kdb: KernelDatabase) { const reachable = getReachableMethods(context); const translators = getTranslators(context); const pinned = getPinMethods(context); + const crank = getCrankMethods(context, kdb); /** * Create a new VatStore for a vat. @@ -173,6 +177,11 @@ export function makeKernelStore(kdb: KernelDatabase) { context.nextPromiseId = provideCachedStoredValue('nextPromiseId', '1'); context.nextVatId = provideCachedStoredValue('nextVatId', '1'); context.nextRemoteId = provideCachedStoredValue('nextRemoteId', '1'); + context.inCrank = false; + if (context.savepoints.length > 0) { + kdb.releaseSavepoint('t0'); + context.savepoints.length = 0; + } } /** @@ -194,6 +203,7 @@ export function makeKernelStore(kdb: KernelDatabase) { ...vat, ...translators, ...pinned, + ...crank, makeVatStore, deleteVat, clear, diff --git a/packages/ocap-kernel/src/store/methods/crank.test.ts b/packages/ocap-kernel/src/store/methods/crank.test.ts new file mode 100644 index 000000000..334e1bff4 --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/crank.test.ts @@ -0,0 +1,126 @@ +import type { KernelDatabase } from '@metamask/kernel-store'; +import { expect, describe, it, vi, beforeEach } from 'vitest'; + +import { getCrankMethods } from './crank.ts'; +import type { StoreContext } from '../types.ts'; + +describe('crank methods', () => { + let context: StoreContext; + let kdb: KernelDatabase; + let crankMethods: ReturnType; + + beforeEach(() => { + context = { + inCrank: false, + savepoints: [], + } as unknown as StoreContext; + + kdb = { + createSavepoint: vi.fn(), + rollbackSavepoint: vi.fn(), + releaseSavepoint: vi.fn(), + } as unknown as KernelDatabase; + + crankMethods = getCrankMethods(context, kdb); + }); + + describe('startCrank', () => { + it('should set inCrank to true', () => { + crankMethods.startCrank(); + expect(context.inCrank).toBe(true); + }); + + it('should throw when already in a crank', () => { + context.inCrank = true; + expect(() => crankMethods.startCrank()).toThrow( + 'startCrank while already in a crank', + ); + }); + }); + + describe('createCrankSavepoint', () => { + it('should create a savepoint when in a crank', () => { + context.inCrank = true; + crankMethods.createCrankSavepoint('test'); + + expect(context.savepoints).toStrictEqual(['test']); + expect(kdb.createSavepoint).toHaveBeenCalledWith('t0'); + }); + + it('should create multiple savepoints sequentially', () => { + context.inCrank = true; + crankMethods.createCrankSavepoint('first'); + crankMethods.createCrankSavepoint('second'); + + expect(context.savepoints).toStrictEqual(['first', 'second']); + expect(kdb.createSavepoint).toHaveBeenCalledWith('t0'); + expect(kdb.createSavepoint).toHaveBeenCalledWith('t1'); + }); + + it('should throw when not in a crank', () => { + expect(() => crankMethods.createCrankSavepoint('test')).toThrow( + 'createCrankSavepoint outside of crank', + ); + }); + }); + + describe('rollbackCrank', () => { + it('should rollback to specified savepoint', () => { + context.inCrank = true; + context.savepoints = ['first', 'second', 'third']; + + crankMethods.rollbackCrank('second'); + + expect(kdb.rollbackSavepoint).toHaveBeenCalledWith('t1'); + expect(context.savepoints).toStrictEqual(['first']); + }); + + it('should throw when savepoint does not exist', () => { + context.inCrank = true; + context.savepoints = ['first', 'second']; + + expect(() => crankMethods.rollbackCrank('nonexistent')).toThrow( + 'no such savepoint as ""nonexistent""', + ); + }); + + it('should throw when not in a crank', () => { + expect(() => crankMethods.rollbackCrank('test')).toThrow( + 'rollbackCrank outside of crank', + ); + }); + }); + + describe('endCrank', () => { + it('should set inCrank to false', () => { + context.inCrank = true; + crankMethods.endCrank(); + + expect(context.inCrank).toBe(false); + }); + + it('should release savepoints if they exist', () => { + context.inCrank = true; + context.savepoints = ['test']; + + crankMethods.endCrank(); + + expect(kdb.releaseSavepoint).toHaveBeenCalledWith('t0'); + expect(context.savepoints).toStrictEqual([]); + }); + + it('should not call releaseSavepoint if no savepoints exist', () => { + context.inCrank = true; + + crankMethods.endCrank(); + + expect(kdb.releaseSavepoint).not.toHaveBeenCalled(); + }); + + it('should throw when not in a crank', () => { + expect(() => crankMethods.endCrank()).toThrow( + 'endCrank outside of crank', + ); + }); + }); +}); diff --git a/packages/ocap-kernel/src/store/methods/crank.ts b/packages/ocap-kernel/src/store/methods/crank.ts new file mode 100644 index 000000000..be634b557 --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/crank.ts @@ -0,0 +1,70 @@ +import { Fail, q } from '@endo/errors'; +import type { KernelDatabase } from '@metamask/kernel-store'; + +import type { StoreContext } from '../types.ts'; + +/** + * Get the crank methods. + * + * @param ctx - The store context. + * @param kdb - The kernel database. + * @returns The crank methods. + */ +// eslint-disable-next-line @typescript-eslint/explicit-function-return-type +export function getCrankMethods(ctx: StoreContext, kdb: KernelDatabase) { + /** + * Start a crank. + */ + function startCrank(): void { + !ctx.inCrank || Fail`startCrank while already in a crank`; + ctx.inCrank = true; + } + + /** + * Create a savepoint in the crank. + * + * @param name - The savepoint name. + */ + function createCrankSavepoint(name: string): void { + ctx.inCrank || Fail`createCrankSavepoint outside of crank`; + const ordinal = ctx.savepoints.length; + ctx.savepoints.push(name); + kdb.createSavepoint(`t${ordinal}`); + } + + /** + * Rollback a crank. + * + * @param savepoint - The savepoint name. + */ + function rollbackCrank(savepoint: string): void { + ctx.inCrank || Fail`rollbackCrank outside of crank`; + for (const ordinal of ctx.savepoints.keys()) { + if (ctx.savepoints[ordinal] === savepoint) { + kdb.rollbackSavepoint(`t${ordinal}`); + ctx.savepoints.length = ordinal; + return; + } + } + Fail`no such savepoint as "${q(savepoint)}"`; + } + + /** + * End a crank. + */ + function endCrank(): void { + ctx.inCrank || Fail`endCrank outside of crank`; + if (ctx.savepoints.length > 0) { + kdb.releaseSavepoint('t0'); + ctx.savepoints.length = 0; + } + ctx.inCrank = false; + } + + return { + startCrank, + createCrankSavepoint, + rollbackCrank, + endCrank, + }; +} diff --git a/packages/ocap-kernel/src/store/types.ts b/packages/ocap-kernel/src/store/types.ts index a2ce57e7c..cf382bf89 100644 --- a/packages/ocap-kernel/src/store/types.ts +++ b/packages/ocap-kernel/src/store/types.ts @@ -14,6 +14,8 @@ export type StoreContext = { gcActions: StoredValue; reapQueue: StoredValue; terminatedVats: StoredValue; + inCrank: boolean; + savepoints: string[]; }; export type StoredValue = { diff --git a/packages/ocap-kernel/test/storage.ts b/packages/ocap-kernel/test/storage.ts index 9d2fc04db..41f9abd2e 100644 --- a/packages/ocap-kernel/test/storage.ts +++ b/packages/ocap-kernel/test/storage.ts @@ -127,5 +127,14 @@ export function makeMapKernelDatabase(): KernelDatabase { deleteVatStore: (vatID: string) => { vatStores.delete(vatID); }, + createSavepoint: () => { + // noop + }, + rollbackSavepoint: () => { + // noop + }, + releaseSavepoint: () => { + // noop + }, }; } diff --git a/vitest.config.ts b/vitest.config.ts index 68d090729..77168ebe4 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -110,10 +110,10 @@ export default defineConfig({ lines: 0, }, 'packages/kernel-store/**': { - statements: 92.44, - functions: 91.17, - branches: 84.78, - lines: 92.39, + statements: 97.35, + functions: 100, + branches: 93.18, + lines: 97.34, }, 'packages/kernel-utils/**': { statements: 100, From 2900e52cab2abf312663fb8933f6238c8a149c11 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 13 May 2025 12:08:10 +0200 Subject: [PATCH 02/25] move delivery savepoint --- packages/ocap-kernel/src/KernelQueue.test.ts | 7 +++- packages/ocap-kernel/src/KernelQueue.ts | 1 + packages/ocap-kernel/src/KernelRouter.test.ts | 38 ------------------- packages/ocap-kernel/src/KernelRouter.ts | 1 - 4 files changed, 7 insertions(+), 40 deletions(-) diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index f83c647d3..abf4f7d5a 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -5,6 +5,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import type { MockInstance } from 'vitest'; import { KernelQueue } from './KernelQueue.ts'; +import * as gc from './services/garbage-collection.ts'; import type { KernelStore } from './store/index.ts'; import type { KRef, @@ -66,12 +67,16 @@ describe('KernelQueue', () => { (kernelStore.dequeueRun as unknown as MockInstance).mockReturnValue( mockItem, ); + const processGCActionSetSpy = vi.spyOn(gc, 'processGCActionSet'); const deliverError = new Error('stop'); const deliver = vi.fn().mockRejectedValue(deliverError); await expect(kernelQueue.run(deliver)).rejects.toBe(deliverError); - expect(kernelStore.nextTerminatedVatCleanup).toHaveBeenCalled(); expect(kernelStore.startCrank).toHaveBeenCalled(); expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('start'); + expect(processGCActionSetSpy).toHaveBeenCalled(); + expect(kernelStore.nextReapAction).toHaveBeenCalled(); + expect(kernelStore.nextTerminatedVatCleanup).toHaveBeenCalled(); + expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('deliver'); expect(deliver).toHaveBeenCalledWith(mockItem); expect(kernelStore.endCrank).toHaveBeenCalled(); }); diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index e981ab747..339f67f4e 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -46,6 +46,7 @@ export class KernelQueue { async run(deliver: (item: RunQueueItem) => Promise): Promise { for await (const item of this.#runQueueItems()) { this.#kernelStore.nextTerminatedVatCleanup(); + this.#kernelStore.createCrankSavepoint('deliver'); await deliver(item); this.#kernelStore.collectGarbage(); } diff --git a/packages/ocap-kernel/src/KernelRouter.test.ts b/packages/ocap-kernel/src/KernelRouter.test.ts index 743179591..67984e3f7 100644 --- a/packages/ocap-kernel/src/KernelRouter.test.ts +++ b/packages/ocap-kernel/src/KernelRouter.test.ts @@ -78,28 +78,6 @@ describe('KernelRouter', () => { }); describe('deliver', () => { - it('creates a savepoint at the start of delivery', async () => { - // Minimal send item for this test - const sendItem: RunQueueItemSend = { - type: 'send', - target: 'ko123', - message: { - methargs: { body: 'test', slots: [] }, - result: null, - } as unknown as SwingsetMessage, - }; - - // Return a valid vat owner so the delivery proceeds - (kernelStore.getOwner as unknown as MockInstance).mockReturnValueOnce( - 'v1', - ); - - await kernelRouter.deliver(sendItem); - - // Verify savepoint was created - expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('deliver'); - }); - describe('send', () => { it('delivers a send message to a vat with an object target', async () => { // Setup the kernel store to return an owner for the target @@ -122,10 +100,6 @@ describe('KernelRouter', () => { message: message as unknown as SwingsetMessage, }; await kernelRouter.deliver(sendItem); - // Verify savepoint was created - expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith( - 'deliver', - ); // Verify the message was delivered to the vat expect(getVat).toHaveBeenCalledWith(vatId); expect(vatHandle.deliverMessage).toHaveBeenCalledWith( @@ -259,10 +233,6 @@ describe('KernelRouter', () => { }); // Deliver the notify await kernelRouter.deliver(notifyItem); - // Verify savepoint was created - expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith( - 'deliver', - ); // Verify the notification was delivered to the vat expect(getVat).toHaveBeenCalledWith(vatId); expect(vatHandle.deliverNotify).toHaveBeenCalledWith(expect.any(Array)); @@ -313,10 +283,6 @@ describe('KernelRouter', () => { }; // Deliver the GC action await kernelRouter.deliver(gcAction); - // Verify savepoint was created - expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith( - 'deliver', - ); // Verify the action was delivered to the vat expect(getVat).toHaveBeenCalledWith(vatId); expect( @@ -334,10 +300,6 @@ describe('KernelRouter', () => { }; // Deliver the bringOutYourDead action await kernelRouter.deliver(bringOutYourDeadItem); - // Verify savepoint was created - expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith( - 'deliver', - ); // Verify the action was delivered to the vat expect(getVat).toHaveBeenCalledWith(vatId); expect(vatHandle.deliverBringOutYourDead).toHaveBeenCalled(); diff --git a/packages/ocap-kernel/src/KernelRouter.ts b/packages/ocap-kernel/src/KernelRouter.ts index 9de622387..9be7c723b 100644 --- a/packages/ocap-kernel/src/KernelRouter.ts +++ b/packages/ocap-kernel/src/KernelRouter.ts @@ -76,7 +76,6 @@ export class KernelRouter { * @param item - The message/notification to deliver. */ async deliver(item: RunQueueItem): Promise { - this.#kernelStore.createCrankSavepoint('deliver'); switch (item.type) { case 'send': await this.#deliverSend(item); From d8f1550a1054413b1df1ad038ee45fddca7dc814 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 13 May 2025 19:27:10 +0200 Subject: [PATCH 03/25] feat(kernel-store): improve SQLite wasm transaction management with savepoint --- .../kernel-store/src/sqlite/common.test.ts | 1 + packages/kernel-store/src/sqlite/common.ts | 1 + packages/kernel-store/src/sqlite/wasm.test.ts | 130 ++++++++++++++++++ packages/kernel-store/src/sqlite/wasm.ts | 119 +++++++++++++--- vitest.config.ts | 6 +- 5 files changed, 233 insertions(+), 24 deletions(-) diff --git a/packages/kernel-store/src/sqlite/common.test.ts b/packages/kernel-store/src/sqlite/common.test.ts index 4a05ef1ad..c9bee1808 100644 --- a/packages/kernel-store/src/sqlite/common.test.ts +++ b/packages/kernel-store/src/sqlite/common.test.ts @@ -40,6 +40,7 @@ describe('SQL_QUERIES', () => { it('has all expected query properties', () => { expect(Object.keys(SQL_QUERIES).sort()).toStrictEqual([ 'ABORT_TRANSACTION', + 'BEGIN_IMMEDIATE_TRANSACTION', 'BEGIN_TRANSACTION', 'CLEAR', 'CLEAR_VS', diff --git a/packages/kernel-store/src/sqlite/common.ts b/packages/kernel-store/src/sqlite/common.ts index 9803b3345..45e45cd26 100644 --- a/packages/kernel-store/src/sqlite/common.ts +++ b/packages/kernel-store/src/sqlite/common.ts @@ -57,6 +57,7 @@ export const SQL_QUERIES = { DROP: `DROP TABLE kv`, DROP_VS: `DROP TABLE kv_vatstore`, BEGIN_TRANSACTION: `BEGIN TRANSACTION`, + BEGIN_IMMEDIATE_TRANSACTION: `BEGIN IMMEDIATE TRANSACTION`, COMMIT_TRANSACTION: `COMMIT TRANSACTION`, ABORT_TRANSACTION: `ROLLBACK TRANSACTION`, // SQLite’s parameter markers (?, ?NNN, :name, @name, $name) can only be used diff --git a/packages/kernel-store/src/sqlite/wasm.test.ts b/packages/kernel-store/src/sqlite/wasm.test.ts index d3b385443..21eac4268 100644 --- a/packages/kernel-store/src/sqlite/wasm.test.ts +++ b/packages/kernel-store/src/sqlite/wasm.test.ts @@ -28,6 +28,10 @@ const mockStatement = { const mockDb = { exec: vi.fn(), prepare: vi.fn(() => mockStatement), + // eslint-disable-next-line @typescript-eslint/naming-convention + _inTx: false, + // eslint-disable-next-line @typescript-eslint/naming-convention + _spStack: [] as string[], }; const OpfsDbMock = vi.fn(() => mockDb); const DBMock = vi.fn(() => mockDb); @@ -398,6 +402,7 @@ describe('makeSQLKernelDatabase', () => { it('rolls back to a savepoint', async () => { const db = await makeSQLKernelDatabase({}); + db.createSavepoint('test_point'); db.rollbackSavepoint('test_point'); expect(mockDb.exec).toHaveBeenCalledWith( 'ROLLBACK TO SAVEPOINT test_point', @@ -406,6 +411,7 @@ describe('makeSQLKernelDatabase', () => { it('releases a savepoint', async () => { const db = await makeSQLKernelDatabase({}); + db.createSavepoint('test_point'); db.releaseSavepoint('test_point'); expect(mockDb.exec).toHaveBeenCalledWith('RELEASE SAVEPOINT test_point'); }); @@ -476,3 +482,127 @@ describe('makeSQLKernelDatabase', () => { expect(data).toStrictEqual([['testKey', 'testValue']]); }); }); + +describe('transaction management', () => { + beforeEach(() => { + Object.values(mockStatement).forEach((mock) => { + if (typeof mock === 'function' && mock.mockReset) { + mock.mockReset(); + } + }); + mockDb.exec.mockReset(); + mockDb._inTx = false; + mockDb._spStack = []; + }); + + it('safeMutate rollbacks transaction on error', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = false; + mockDb._spStack = []; + mockStatement.step.mockImplementationOnce(() => { + throw new Error('Database error'); + }); + const vatStore = db.makeVatStore('test-vat'); + expect(() => vatStore.updateKVData([['key', 'value']], [])).toThrow( + 'Database error', + ); + expect(mockStatement.step).toHaveBeenCalled(); + }); + + it('safeMutate does not commit if already in transaction', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = []; + const vatStore = db.makeVatStore('test-vat'); + vatStore.updateKVData([['key', 'value']], []); + expect(mockStatement.step).not.toHaveBeenCalledWith( + expect.objectContaining({ sql: 'BEGIN TRANSACTION' }), + ); + expect(mockStatement.step).not.toHaveBeenCalledWith( + expect.objectContaining({ sql: 'COMMIT TRANSACTION' }), + ); + }); +}); + +describe('savepoint functionality with transaction management', () => { + beforeEach(() => { + mockDb.exec.mockReset(); + mockDb._inTx = false; + mockDb._spStack = []; + }); + + it('createSavepoint begins transaction if needed', async () => { + const db = await makeSQLKernelDatabase({}); + db.createSavepoint('test_point'); + expect(mockDb._inTx).toBe(true); + expect(mockDb._spStack).toContain('test_point'); + expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT test_point'); + }); + + it('rollbackSavepoint validates savepoint exists', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['existing_point']; + expect(() => db.rollbackSavepoint('nonexistent_point')).toThrow( + 'No such savepoint: nonexistent_point', + ); + }); + + it('rollbackSavepoint removes all points after target', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1', 'point2', 'point3']; + db.rollbackSavepoint('point2'); + expect(mockDb._spStack).toStrictEqual(['point1']); + expect(mockDb.exec).toHaveBeenCalledWith('ROLLBACK TO SAVEPOINT point2'); + }); + + it('rollbackSavepoint closes transaction if no savepoints remain', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + db.rollbackSavepoint('point1'); + expect(mockDb._spStack).toStrictEqual([]); + expect(mockDb._inTx).toBe(false); + }); + + it('releaseSavepoint validates savepoint exists', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['existing_point']; + expect(() => db.releaseSavepoint('nonexistent_point')).toThrow( + 'No such savepoint: nonexistent_point', + ); + }); + + it('releaseSavepoint removes all points after target', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1', 'point2', 'point3']; + db.releaseSavepoint('point2'); + expect(mockDb._spStack).toStrictEqual(['point1']); + expect(mockDb.exec).toHaveBeenCalledWith('RELEASE SAVEPOINT point2'); + }); + + it('releaseSavepoint commits transaction if no savepoints remain', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + db.releaseSavepoint('point1'); + expect(mockDb._spStack).toStrictEqual([]); + expect(mockDb._inTx).toBe(false); + }); + + it('supports nested savepoints', async () => { + const db = await makeSQLKernelDatabase({}); + db.createSavepoint('outer'); + db.createSavepoint('inner'); + expect(mockDb._spStack).toStrictEqual(['outer', 'inner']); + db.rollbackSavepoint('inner'); + expect(mockDb._spStack).toStrictEqual(['outer']); + expect(mockDb._inTx).toBe(true); + db.releaseSavepoint('outer'); + expect(mockDb._spStack).toStrictEqual([]); + expect(mockDb._inTx).toBe(false); + }); +}); diff --git a/packages/kernel-store/src/sqlite/wasm.ts b/packages/kernel-store/src/sqlite/wasm.ts index 5a9f704a7..127a4c915 100644 --- a/packages/kernel-store/src/sqlite/wasm.ts +++ b/packages/kernel-store/src/sqlite/wasm.ts @@ -1,11 +1,17 @@ import { Logger } from '@metamask/logger'; -import type { Database } from '@sqlite.org/sqlite-wasm'; +import type { Database as SqliteDatabase } from '@sqlite.org/sqlite-wasm'; import sqlite3InitModule from '@sqlite.org/sqlite-wasm'; import { DEFAULT_DB_FILENAME, safeIdentifier, SQL_QUERIES } from './common.ts'; import { getDBFolder } from './env.ts'; import type { KVStore, VatStore, KernelDatabase } from '../types.ts'; +export type Database = SqliteDatabase & { + _inTx: boolean; + // stack of active savepoint names + _spStack: string[]; +}; + /** * Ensure that SQLite is initialized. * @@ -14,7 +20,7 @@ import type { KVStore, VatStore, KernelDatabase } from '../types.ts'; */ export async function initDB(dbFilename: string): Promise { const sqlite3 = await sqlite3InitModule(); - let db: Database; + let db: SqliteDatabase; if (sqlite3.oo1.OpfsDb) { const dbName = dbFilename.startsWith(':') @@ -26,7 +32,11 @@ export async function initDB(dbFilename: string): Promise { db = new sqlite3.oo1.DB(`:memory:`, 'cw'); } - return db; + const dbWithTx = db as Database; + dbWithTx._inTx = false; + dbWithTx._spStack = []; + + return dbWithTx; } /** @@ -161,6 +171,71 @@ export async function makeSQLKernelDatabase({ const sqlKVClear = db.prepare(SQL_QUERIES.CLEAR); const sqlKVClearVS = db.prepare(SQL_QUERIES.CLEAR_VS); + const sqlVatstoreGetAll = db.prepare(SQL_QUERIES.GET_ALL_VS); + const sqlVatstoreSet = db.prepare(SQL_QUERIES.SET_VS); + const sqlVatstoreDelete = db.prepare(SQL_QUERIES.DELETE_VS); + const sqlVatstoreDeleteAll = db.prepare(SQL_QUERIES.DELETE_VS_ALL); + const sqlBeginTransaction = db.prepare(SQL_QUERIES.BEGIN_TRANSACTION); + const sqlCommitTransaction = db.prepare(SQL_QUERIES.COMMIT_TRANSACTION); + const sqlAbortTransaction = db.prepare(SQL_QUERIES.ABORT_TRANSACTION); + + /** + * Issue BEGIN IMMEDIATE if we're not already inside one. + */ + function beginIfNeeded(): void { + if (!db._inTx) { + sqlBeginTransaction.step(); + sqlBeginTransaction.reset(); + db._inTx = true; + } + } + + /** + * COMMIT only if we ourselves opened the tx. + */ + function commitIfNeeded(): void { + if (db._inTx && db._spStack.length === 0) { + sqlCommitTransaction.step(); + sqlCommitTransaction.reset(); + db._inTx = false; + } + } + + /** + * ROLLBACK only if we ourselves opened the tx. + */ + function rollbackIfNeeded(): void { + if (db._inTx) { + sqlAbortTransaction.step(); + sqlAbortTransaction.reset(); + db._inTx = false; + db._spStack.length = 0; + } + } + + /** + * Wrap any helper that mutates data so that it only + * calls BEGIN/COMMIT if it wasn’t already inside one. + * + * @param fn - The function to wrap. + */ + function safeMutate(fn: () => void): void { + const opened = !db._inTx; + if (opened) { + beginIfNeeded(); + } + try { + fn(); + if (opened) { + commitIfNeeded(); + } + } catch (error) { + if (opened) { + rollbackIfNeeded(); + } + throw error; + } + } /** * Delete everything from the database. @@ -202,14 +277,6 @@ export async function makeSQLKernelDatabase({ return results; } - const sqlVatstoreGetAll = db.prepare(SQL_QUERIES.GET_ALL_VS); - const sqlVatstoreSet = db.prepare(SQL_QUERIES.SET_VS); - const sqlVatstoreDelete = db.prepare(SQL_QUERIES.DELETE_VS); - const sqlVatstoreDeleteAll = db.prepare(SQL_QUERIES.DELETE_VS_ALL); - const sqlBeginTransaction = db.prepare(SQL_QUERIES.BEGIN_TRANSACTION); - const sqlCommitTransaction = db.prepare(SQL_QUERIES.COMMIT_TRANSACTION); - const sqlAbortTransaction = db.prepare(SQL_QUERIES.ABORT_TRANSACTION); - /** * Create a new VatStore for a vat. * @@ -245,9 +312,7 @@ export async function makeSQLKernelDatabase({ * @param deletes - A set of keys that have been deleted. */ function updateKVData(sets: [string, string][], deletes: string[]): void { - try { - sqlBeginTransaction.step(); - sqlBeginTransaction.reset(); + safeMutate(() => { for (const [key, value] of sets) { sqlVatstoreSet.bind([vatID, key, value]); sqlVatstoreSet.step(); @@ -258,13 +323,7 @@ export async function makeSQLKernelDatabase({ sqlVatstoreDelete.step(); sqlVatstoreDelete.reset(); } - sqlCommitTransaction.step(); - sqlCommitTransaction.reset(); - } catch (problem) { - sqlAbortTransaction.step(); - sqlAbortTransaction.reset(); - throw problem; - } + }); } return { @@ -290,9 +349,11 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function createSavepoint(name: string): void { + beginIfNeeded(); const point = safeIdentifier(name); const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', point); db.exec(query); + db._spStack.push(point); } /** @@ -302,8 +363,16 @@ export async function makeSQLKernelDatabase({ */ function rollbackSavepoint(name: string): void { const point = safeIdentifier(name); + const idx = db._spStack.lastIndexOf(point); + if (idx < 0) { + throw new Error(`No such savepoint: ${point}`); + } const query = SQL_QUERIES.ROLLBACK_SAVEPOINT.replace('%NAME%', point); db.exec(query); + db._spStack.splice(idx); + if (db._spStack.length === 0) { + rollbackIfNeeded(); + } } /** @@ -313,8 +382,16 @@ export async function makeSQLKernelDatabase({ */ function releaseSavepoint(name: string): void { const point = safeIdentifier(name); + const idx = db._spStack.lastIndexOf(point); + if (idx < 0) { + throw new Error(`No such savepoint: ${point}`); + } const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', point); db.exec(query); + db._spStack.splice(idx); + if (db._spStack.length === 0) { + commitIfNeeded(); + } } return { diff --git a/vitest.config.ts b/vitest.config.ts index 77168ebe4..80c325f14 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -110,10 +110,10 @@ export default defineConfig({ lines: 0, }, 'packages/kernel-store/**': { - statements: 97.35, + statements: 97.74, functions: 100, - branches: 93.18, - lines: 97.34, + branches: 89.39, + lines: 97.73, }, 'packages/kernel-utils/**': { statements: 100, From cbb0c800809221dcf93626e3298b70d403c3d428 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 14 May 2025 13:25:52 +0200 Subject: [PATCH 04/25] feat(kernel-store): prevent SQLite nodejs savepoint autocommit bug --- .../kernel-store/src/sqlite/nodejs.test.ts | 98 +++++++++-- packages/kernel-store/src/sqlite/nodejs.ts | 75 +++++++- packages/kernel-store/src/sqlite/wasm.test.ts | 160 +++++++++--------- packages/kernel-store/src/sqlite/wasm.ts | 38 ++--- vitest.config.ts | 6 +- 5 files changed, 252 insertions(+), 125 deletions(-) diff --git a/packages/kernel-store/src/sqlite/nodejs.test.ts b/packages/kernel-store/src/sqlite/nodejs.test.ts index 6639e5927..afe27845a 100644 --- a/packages/kernel-store/src/sqlite/nodejs.test.ts +++ b/packages/kernel-store/src/sqlite/nodejs.test.ts @@ -26,6 +26,9 @@ const mockDb = { prepare: vi.fn(() => mockStatement), transaction: vi.fn((fn) => fn), exec: vi.fn(), + inTransaction: false, + // eslint-disable-next-line @typescript-eslint/naming-convention + _spStack: [] as string[], }; vi.mock('better-sqlite3', () => ({ @@ -197,19 +200,17 @@ describe('makeSQLKernelDatabase', () => { }); describe('savepoint functionality', () => { - let execSpy: ReturnType; - - beforeEach(async () => { - execSpy = vi.fn(); - (mockDb.exec as unknown) = execSpy; + beforeEach(() => { + mockDb.exec.mockClear(); + mockDb.inTransaction = false; + mockDb._spStack = []; }); it('creates a savepoint using sanitized name', async () => { const db = await makeSQLKernelDatabase({}); db.createSavepoint('valid_name'); - expect(execSpy).toHaveBeenCalledWith( - expect.stringContaining('SAVEPOINT valid_name'), - ); + + expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT valid_name'); }); it('rejects invalid savepoint names', async () => { @@ -226,22 +227,95 @@ describe('makeSQLKernelDatabase', () => { expect(() => db.createSavepoint("point'; DROP TABLE kv--")).toThrow( 'Invalid identifier', ); + expect(mockDb.exec).not.toHaveBeenCalledWith( + expect.stringContaining('DROP TABLE'), + ); }); it('rolls back to a savepoint', async () => { const db = await makeSQLKernelDatabase({}); + db.createSavepoint('test_point'); db.rollbackSavepoint('test_point'); - expect(execSpy).toHaveBeenCalledWith( - expect.stringContaining('ROLLBACK TO SAVEPOINT test_point'), + expect(mockDb.exec).toHaveBeenCalledWith( + 'ROLLBACK TO SAVEPOINT test_point', ); }); it('releases a savepoint', async () => { const db = await makeSQLKernelDatabase({}); + db.createSavepoint('test_point'); db.releaseSavepoint('test_point'); - expect(execSpy).toHaveBeenCalledWith( - expect.stringContaining('RELEASE SAVEPOINT test_point'), + expect(mockDb.exec).toHaveBeenCalledWith('RELEASE SAVEPOINT test_point'); + }); + + it('createSavepoint begins transaction if needed', async () => { + const db = await makeSQLKernelDatabase({}); + db.createSavepoint('test_point'); + expect(mockDb._spStack).toContain('test_point'); + expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT test_point'); + }); + + it('rollbackSavepoint validates savepoint exists', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb.inTransaction = true; + mockDb._spStack = ['existing_point']; + expect(() => db.rollbackSavepoint('nonexistent_point')).toThrow( + 'No such savepoint: nonexistent_point', + ); + }); + + it('rollbackSavepoint removes all points after target', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb.inTransaction = true; + mockDb._spStack = ['point1', 'point2', 'point3']; + db.rollbackSavepoint('point2'); + expect(mockDb._spStack).toStrictEqual(['point1']); + expect(mockDb.exec).toHaveBeenCalledWith('ROLLBACK TO SAVEPOINT point2'); + }); + + it('rollbackSavepoint closes transaction if no savepoints remain', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb.inTransaction = true; + mockDb._spStack = ['point1']; + db.rollbackSavepoint('point1'); + expect(mockDb._spStack).toStrictEqual([]); + }); + + it('releaseSavepoint validates savepoint exists', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb.inTransaction = true; + mockDb._spStack = ['existing_point']; + expect(() => db.releaseSavepoint('nonexistent_point')).toThrow( + 'No such savepoint: nonexistent_point', ); }); + + it('releaseSavepoint removes all points after target', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb.inTransaction = true; + mockDb._spStack = ['point1', 'point2', 'point3']; + db.releaseSavepoint('point2'); + expect(mockDb._spStack).toStrictEqual(['point1']); + expect(mockDb.exec).toHaveBeenCalledWith('RELEASE SAVEPOINT point2'); + }); + + it('releaseSavepoint commits transaction if no savepoints remain', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb.inTransaction = true; + mockDb._spStack = ['point1']; + db.releaseSavepoint('point1'); + expect(mockDb._spStack).toStrictEqual([]); + }); + + it('supports nested savepoints', async () => { + const db = await makeSQLKernelDatabase({}); + db.createSavepoint('outer'); + db.createSavepoint('inner'); + expect(mockDb._spStack).toStrictEqual(['outer', 'inner']); + db.rollbackSavepoint('inner'); + expect(mockDb._spStack).toStrictEqual(['outer']); + db.releaseSavepoint('outer'); + expect(mockDb._spStack).toStrictEqual([]); + }); }); }); diff --git a/packages/kernel-store/src/sqlite/nodejs.ts b/packages/kernel-store/src/sqlite/nodejs.ts index e53aa3eeb..de3a47872 100644 --- a/packages/kernel-store/src/sqlite/nodejs.ts +++ b/packages/kernel-store/src/sqlite/nodejs.ts @@ -1,5 +1,5 @@ import { Logger } from '@metamask/logger'; -import type { Database } from 'better-sqlite3'; +import type { Database as SqliteDatabase } from 'better-sqlite3'; // eslint-disable-next-line @typescript-eslint/naming-convention import Sqlite from 'better-sqlite3'; import { mkdir } from 'fs/promises'; @@ -10,6 +10,11 @@ import { SQL_QUERIES, DEFAULT_DB_FILENAME, safeIdentifier } from './common.ts'; import { getDBFolder } from './env.ts'; import type { KVStore, VatStore, KernelDatabase } from '../types.ts'; +export type Database = SqliteDatabase & { + // stack of active savepoint names + _spStack: string[]; +}; + /** * Ensure that SQLite is initialized. * @@ -25,11 +30,13 @@ async function initDB( ): Promise { const dbPath = await getDBFilename(dbFilename); logger.debug('dbPath:', dbPath); - return new Sqlite(dbPath, { + const db = new Sqlite(dbPath, { verbose: (verbose ? logger.info.bind(logger) : undefined) as | ((...args: unknown[]) => void) | undefined, - }); + }) as Database; + db._spStack = []; + return db; } /** @@ -140,6 +147,45 @@ export async function makeSQLKernelDatabase({ const sqlKVClear = db.prepare(SQL_QUERIES.CLEAR); const sqlKVClearVS = db.prepare(SQL_QUERIES.CLEAR_VS); + const sqlVatstoreGetAll = db.prepare(SQL_QUERIES.GET_ALL_VS); + const sqlVatstoreSet = db.prepare(SQL_QUERIES.SET_VS); + const sqlVatstoreDelete = db.prepare(SQL_QUERIES.DELETE_VS); + const sqlVatstoreDeleteAll = db.prepare(SQL_QUERIES.DELETE_VS_ALL); + const sqlBeginTransaction = db.prepare(SQL_QUERIES.BEGIN_TRANSACTION); + const sqlCommitTransaction = db.prepare(SQL_QUERIES.COMMIT_TRANSACTION); + const sqlAbortTransaction = db.prepare(SQL_QUERIES.ABORT_TRANSACTION); + + /** + * Begin a transaction if not already in one + * + * @returns True if a new transaction was started, false if already in one + */ + function beginIfNeeded(): boolean { + if (db.inTransaction) { + return false; + } + sqlBeginTransaction.run(); + return true; + } + + /** + * Commit a transaction if one is active and no savepoints remain + */ + function commitIfNeeded(): void { + if (db.inTransaction && db._spStack.length === 0) { + sqlCommitTransaction.run(); + } + } + + /** + * Rollback a transaction + */ + function rollbackIfNeeded(): void { + if (db.inTransaction) { + sqlAbortTransaction.run(); + db._spStack.length = 0; + } + } /** * Delete everything from the database. @@ -160,11 +206,6 @@ export async function makeSQLKernelDatabase({ return query.all() as Record[]; } - const sqlVatstoreGetAll = db.prepare(SQL_QUERIES.GET_ALL_VS); - const sqlVatstoreSet = db.prepare(SQL_QUERIES.SET_VS); - const sqlVatstoreDelete = db.prepare(SQL_QUERIES.DELETE_VS); - const sqlVatstoreDeleteAll = db.prepare(SQL_QUERIES.DELETE_VS_ALL); - /** * Create a new VatStore for a vat. * @@ -229,9 +270,11 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function createSavepoint(name: string): void { + beginIfNeeded(); const point = safeIdentifier(name); const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', point); db.exec(query); + db._spStack.push(point); } /** @@ -241,8 +284,16 @@ export async function makeSQLKernelDatabase({ */ function rollbackSavepoint(name: string): void { const point = safeIdentifier(name); + const idx = db._spStack.lastIndexOf(point); + if (idx < 0) { + throw new Error(`No such savepoint: ${point}`); + } const query = SQL_QUERIES.ROLLBACK_SAVEPOINT.replace('%NAME%', point); db.exec(query); + db._spStack.splice(idx); + if (db._spStack.length === 0) { + rollbackIfNeeded(); + } } /** @@ -252,8 +303,16 @@ export async function makeSQLKernelDatabase({ */ function releaseSavepoint(name: string): void { const point = safeIdentifier(name); + const idx = db._spStack.lastIndexOf(point); + if (idx < 0) { + throw new Error(`No such savepoint: ${point}`); + } const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', point); db.exec(query); + db._spStack.splice(idx); + if (db._spStack.length === 0) { + commitIfNeeded(); + } } return { diff --git a/packages/kernel-store/src/sqlite/wasm.test.ts b/packages/kernel-store/src/sqlite/wasm.test.ts index 21eac4268..64f167c46 100644 --- a/packages/kernel-store/src/sqlite/wasm.test.ts +++ b/packages/kernel-store/src/sqlite/wasm.test.ts @@ -372,6 +372,8 @@ describe('makeSQLKernelDatabase', () => { describe('savepoint functionality', () => { beforeEach(() => { mockDb.exec.mockClear(); + mockDb._inTx = false; + mockDb._spStack = []; }); it('creates a savepoint using sanitized name', async () => { @@ -415,6 +417,81 @@ describe('makeSQLKernelDatabase', () => { db.releaseSavepoint('test_point'); expect(mockDb.exec).toHaveBeenCalledWith('RELEASE SAVEPOINT test_point'); }); + + it('createSavepoint begins transaction if needed', async () => { + const db = await makeSQLKernelDatabase({}); + db.createSavepoint('test_point'); + expect(mockDb._inTx).toBe(true); + expect(mockDb._spStack).toContain('test_point'); + expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT test_point'); + }); + + it('rollbackSavepoint validates savepoint exists', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['existing_point']; + expect(() => db.rollbackSavepoint('nonexistent_point')).toThrow( + 'No such savepoint: nonexistent_point', + ); + }); + + it('rollbackSavepoint removes all points after target', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1', 'point2', 'point3']; + db.rollbackSavepoint('point2'); + expect(mockDb._spStack).toStrictEqual(['point1']); + expect(mockDb.exec).toHaveBeenCalledWith('ROLLBACK TO SAVEPOINT point2'); + }); + + it('rollbackSavepoint closes transaction if no savepoints remain', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + db.rollbackSavepoint('point1'); + expect(mockDb._spStack).toStrictEqual([]); + expect(mockDb._inTx).toBe(false); + }); + + it('releaseSavepoint validates savepoint exists', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['existing_point']; + expect(() => db.releaseSavepoint('nonexistent_point')).toThrow( + 'No such savepoint: nonexistent_point', + ); + }); + + it('releaseSavepoint removes all points after target', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1', 'point2', 'point3']; + db.releaseSavepoint('point2'); + expect(mockDb._spStack).toStrictEqual(['point1']); + expect(mockDb.exec).toHaveBeenCalledWith('RELEASE SAVEPOINT point2'); + }); + + it('releaseSavepoint commits transaction if no savepoints remain', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + db.releaseSavepoint('point1'); + expect(mockDb._spStack).toStrictEqual([]); + expect(mockDb._inTx).toBe(false); + }); + + it('supports nested savepoints', async () => { + const db = await makeSQLKernelDatabase({}); + db.createSavepoint('outer'); + db.createSavepoint('inner'); + expect(mockDb._spStack).toStrictEqual(['outer', 'inner']); + db.rollbackSavepoint('inner'); + expect(mockDb._spStack).toStrictEqual(['outer']); + expect(mockDb._inTx).toBe(true); + db.releaseSavepoint('outer'); + expect(mockDb._spStack).toStrictEqual([]); + expect(mockDb._inTx).toBe(false); + }); }); it('deleteVatStore removes all data for a given vat', async () => { @@ -523,86 +600,3 @@ describe('transaction management', () => { ); }); }); - -describe('savepoint functionality with transaction management', () => { - beforeEach(() => { - mockDb.exec.mockReset(); - mockDb._inTx = false; - mockDb._spStack = []; - }); - - it('createSavepoint begins transaction if needed', async () => { - const db = await makeSQLKernelDatabase({}); - db.createSavepoint('test_point'); - expect(mockDb._inTx).toBe(true); - expect(mockDb._spStack).toContain('test_point'); - expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT test_point'); - }); - - it('rollbackSavepoint validates savepoint exists', async () => { - const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; - mockDb._spStack = ['existing_point']; - expect(() => db.rollbackSavepoint('nonexistent_point')).toThrow( - 'No such savepoint: nonexistent_point', - ); - }); - - it('rollbackSavepoint removes all points after target', async () => { - const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; - mockDb._spStack = ['point1', 'point2', 'point3']; - db.rollbackSavepoint('point2'); - expect(mockDb._spStack).toStrictEqual(['point1']); - expect(mockDb.exec).toHaveBeenCalledWith('ROLLBACK TO SAVEPOINT point2'); - }); - - it('rollbackSavepoint closes transaction if no savepoints remain', async () => { - const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; - mockDb._spStack = ['point1']; - db.rollbackSavepoint('point1'); - expect(mockDb._spStack).toStrictEqual([]); - expect(mockDb._inTx).toBe(false); - }); - - it('releaseSavepoint validates savepoint exists', async () => { - const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; - mockDb._spStack = ['existing_point']; - expect(() => db.releaseSavepoint('nonexistent_point')).toThrow( - 'No such savepoint: nonexistent_point', - ); - }); - - it('releaseSavepoint removes all points after target', async () => { - const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; - mockDb._spStack = ['point1', 'point2', 'point3']; - db.releaseSavepoint('point2'); - expect(mockDb._spStack).toStrictEqual(['point1']); - expect(mockDb.exec).toHaveBeenCalledWith('RELEASE SAVEPOINT point2'); - }); - - it('releaseSavepoint commits transaction if no savepoints remain', async () => { - const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; - mockDb._spStack = ['point1']; - db.releaseSavepoint('point1'); - expect(mockDb._spStack).toStrictEqual([]); - expect(mockDb._inTx).toBe(false); - }); - - it('supports nested savepoints', async () => { - const db = await makeSQLKernelDatabase({}); - db.createSavepoint('outer'); - db.createSavepoint('inner'); - expect(mockDb._spStack).toStrictEqual(['outer', 'inner']); - db.rollbackSavepoint('inner'); - expect(mockDb._spStack).toStrictEqual(['outer']); - expect(mockDb._inTx).toBe(true); - db.releaseSavepoint('outer'); - expect(mockDb._spStack).toStrictEqual([]); - expect(mockDb._inTx).toBe(false); - }); -}); diff --git a/packages/kernel-store/src/sqlite/wasm.ts b/packages/kernel-store/src/sqlite/wasm.ts index 127a4c915..0d1044fbc 100644 --- a/packages/kernel-store/src/sqlite/wasm.ts +++ b/packages/kernel-store/src/sqlite/wasm.ts @@ -180,18 +180,22 @@ export async function makeSQLKernelDatabase({ const sqlAbortTransaction = db.prepare(SQL_QUERIES.ABORT_TRANSACTION); /** - * Issue BEGIN IMMEDIATE if we're not already inside one. + * Begin a transaction if not already in one + * + * @returns True if a new transaction was started, false if already in one */ - function beginIfNeeded(): void { - if (!db._inTx) { - sqlBeginTransaction.step(); - sqlBeginTransaction.reset(); - db._inTx = true; + function beginIfNeeded(): boolean { + if (db._inTx) { + return false; } + sqlBeginTransaction.step(); + sqlBeginTransaction.reset(); + db._inTx = true; + return true; } /** - * COMMIT only if we ourselves opened the tx. + * Commit a transaction if one is active and no savepoints remain */ function commitIfNeeded(): void { if (db._inTx && db._spStack.length === 0) { @@ -202,7 +206,7 @@ export async function makeSQLKernelDatabase({ } /** - * ROLLBACK only if we ourselves opened the tx. + * Rollback a transaction */ function rollbackIfNeeded(): void { if (db._inTx) { @@ -214,23 +218,19 @@ export async function makeSQLKernelDatabase({ } /** - * Wrap any helper that mutates data so that it only - * calls BEGIN/COMMIT if it wasn’t already inside one. + * Safely mutate the database with proper transaction management * - * @param fn - The function to wrap. + * @param mutator - Function that performs the database mutations */ - function safeMutate(fn: () => void): void { - const opened = !db._inTx; - if (opened) { - beginIfNeeded(); - } + function safeMutate(mutator: () => void): void { + const startedTx = beginIfNeeded(); try { - fn(); - if (opened) { + mutator(); + if (startedTx) { commitIfNeeded(); } } catch (error) { - if (opened) { + if (startedTx) { rollbackIfNeeded(); } throw error; diff --git a/vitest.config.ts b/vitest.config.ts index 80c325f14..d70bde553 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -110,10 +110,10 @@ export default defineConfig({ lines: 0, }, 'packages/kernel-store/**': { - statements: 97.74, + statements: 98, functions: 100, - branches: 89.39, - lines: 97.73, + branches: 91.25, + lines: 97.99, }, 'packages/kernel-utils/**': { statements: 100, From f14272e68d750dbbd3a1a047d3ee33b4fa7a9d9a Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 19 May 2025 20:35:30 +0200 Subject: [PATCH 05/25] detect syscall failures --- packages/kernel-store/src/sqlite/nodejs.ts | 3 + packages/kernel-store/src/sqlite/wasm.ts | 3 + packages/ocap-kernel/src/VatSyscall.ts | 195 +++++++++++++-------- 3 files changed, 126 insertions(+), 75 deletions(-) diff --git a/packages/kernel-store/src/sqlite/nodejs.ts b/packages/kernel-store/src/sqlite/nodejs.ts index de3a47872..40b24ee59 100644 --- a/packages/kernel-store/src/sqlite/nodejs.ts +++ b/packages/kernel-store/src/sqlite/nodejs.ts @@ -270,6 +270,9 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function createSavepoint(name: string): void { + // We must be in a transaction when creating the savepoint or releasing it + // later will cause an autocommit. + // See https://github.com/Agoric/agoric-sdk/issues/8423 beginIfNeeded(); const point = safeIdentifier(name); const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', point); diff --git a/packages/kernel-store/src/sqlite/wasm.ts b/packages/kernel-store/src/sqlite/wasm.ts index 0d1044fbc..ca7cbe597 100644 --- a/packages/kernel-store/src/sqlite/wasm.ts +++ b/packages/kernel-store/src/sqlite/wasm.ts @@ -349,6 +349,9 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function createSavepoint(name: string): void { + // We must be in a transaction when creating the savepoint or releasing it + // later will cause an autocommit. + // See https://github.com/Agoric/agoric-sdk/issues/8423 beginIfNeeded(); const point = safeIdentifier(name); const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', point); diff --git a/packages/ocap-kernel/src/VatSyscall.ts b/packages/ocap-kernel/src/VatSyscall.ts index c8a4bf407..0c229a65d 100644 --- a/packages/ocap-kernel/src/VatSyscall.ts +++ b/packages/ocap-kernel/src/VatSyscall.ts @@ -1,10 +1,13 @@ import type { VatOneResolution, VatSyscallObject, + VatSyscallResult, } from '@agoric/swingset-liveslots'; +import type { CapData } from '@endo/marshal'; import { Logger } from '@metamask/logger'; import type { KernelQueue } from './KernelQueue.ts'; +import { makeError } from './services/kernel-marshal.ts'; import type { KernelStore } from './store/index.ts'; import { parseRef } from './store/utils/parse-ref.ts'; import { coerceMessage } from './types.ts'; @@ -36,6 +39,14 @@ export class VatSyscall { /** Logger for outputting messages (such as errors) to the console */ readonly #logger: Logger; + /** The illegal syscall that was received */ + illegalSyscall: { vatId: VatId; info: CapData } | undefined; + + /** The termination request that was received from the vat with syscall.exit() */ + vatRequestedTermination: + | { reject: boolean; info: CapData } + | undefined; + /** * Construct a new VatSyscall instance. * @@ -159,84 +170,118 @@ export class VatSyscall { * Handle a syscall from the vat. * * @param vso - The syscall that was received. + * @returns The result of the syscall. */ - async handleSyscall(vso: VatSyscallObject): Promise { - const kso: VatSyscallObject = this.#kernelStore.translateSyscallVtoK( - this.vatId, - vso, - ); - const [op] = kso; - const { vatId } = this; - const { log } = console; - switch (op) { - case 'send': { - // [KRef, Message]; - const [, target, message] = kso; - log(`@@@@ ${vatId} syscall send ${target}<-${JSON.stringify(message)}`); - this.#handleSyscallSend(target, coerceMessage(message)); - break; - } - case 'subscribe': { - // [KRef]; - const [, promise] = kso; - log(`@@@@ ${vatId} syscall subscribe ${promise}`); - this.#handleSyscallSubscribe(promise); - break; - } - case 'resolve': { - // [VatOneResolution[]]; - const [, resolutions] = kso; - log(`@@@@ ${vatId} syscall resolve ${JSON.stringify(resolutions)}`); - this.#handleSyscallResolve(resolutions as VatOneResolution[]); - break; - } - case 'exit': { - // [boolean, SwingSetCapData]; - const [, fail, info] = kso; - log(`@@@@ ${vatId} syscall exit fail=${fail} ${JSON.stringify(info)}`); - break; - } - case 'dropImports': { - // [KRef[]]; - const [, refs] = kso; - log(`@@@@ ${vatId} syscall dropImports ${JSON.stringify(refs)}`); - this.#handleSyscallDropImports(refs); - break; - } - case 'retireImports': { - // [KRef[]]; - const [, refs] = kso; - log(`@@@@ ${vatId} syscall retireImports ${JSON.stringify(refs)}`); - this.#handleSyscallRetireImports(refs); - break; - } - case 'retireExports': { - // [KRef[]]; - const [, refs] = kso; - log(`@@@@ ${vatId} syscall retireExports ${JSON.stringify(refs)}`); - this.#handleSyscallExportCleanup(refs, true); - break; - } - case 'abandonExports': { - // [KRef[]]; - const [, refs] = kso; - log(`@@@@ ${vatId} syscall abandonExports ${JSON.stringify(refs)}`); - this.#handleSyscallExportCleanup(refs, false); - break; + async handleSyscall(vso: VatSyscallObject): Promise { + try { + this.illegalSyscall = undefined; + this.vatRequestedTermination = undefined; + + // This is a safety check - this case should never happen + if (!this.#kernelStore.getVatConfig(this.vatId)) { + this.#vatFatalSyscall('vat not found'); + return harden(['error', 'vat not found']); } - case 'callNow': - case 'vatstoreGet': - case 'vatstoreGetNextKey': - case 'vatstoreSet': - case 'vatstoreDelete': { - console.warn(`vat ${vatId} issued invalid syscall ${op} `, vso); - break; + + const kso: VatSyscallObject = this.#kernelStore.translateSyscallVtoK( + this.vatId, + vso, + ); + const [op] = kso; + const { vatId } = this; + const { log } = console; + switch (op) { + case 'send': { + // [KRef, Message]; + const [, target, message] = kso; + log( + `@@@@ ${vatId} syscall send ${target}<-${JSON.stringify(message)}`, + ); + this.#handleSyscallSend(target, coerceMessage(message)); + break; + } + case 'subscribe': { + // [KRef]; + const [, promise] = kso; + log(`@@@@ ${vatId} syscall subscribe ${promise}`); + this.#handleSyscallSubscribe(promise); + break; + } + case 'resolve': { + // [VatOneResolution[]]; + const [, resolutions] = kso; + log(`@@@@ ${vatId} syscall resolve ${JSON.stringify(resolutions)}`); + this.#handleSyscallResolve(resolutions as VatOneResolution[]); + break; + } + case 'exit': { + // [boolean, SwingSetCapData]; + const [, isFailure, info] = kso; + log( + `@@@@ ${vatId} syscall exit fail=${isFailure} ${JSON.stringify(info)}`, + ); + this.vatRequestedTermination = { reject: isFailure, info }; + break; + } + case 'dropImports': { + // [KRef[]]; + const [, refs] = kso; + log(`@@@@ ${vatId} syscall dropImports ${JSON.stringify(refs)}`); + this.#handleSyscallDropImports(refs); + break; + } + case 'retireImports': { + // [KRef[]]; + const [, refs] = kso; + log(`@@@@ ${vatId} syscall retireImports ${JSON.stringify(refs)}`); + this.#handleSyscallRetireImports(refs); + break; + } + case 'retireExports': { + // [KRef[]]; + const [, refs] = kso; + log(`@@@@ ${vatId} syscall retireExports ${JSON.stringify(refs)}`); + this.#handleSyscallExportCleanup(refs, true); + break; + } + case 'abandonExports': { + // [KRef[]]; + const [, refs] = kso; + log(`@@@@ ${vatId} syscall abandonExports ${JSON.stringify(refs)}`); + this.#handleSyscallExportCleanup(refs, false); + break; + } + case 'callNow': + case 'vatstoreGet': + case 'vatstoreGetNextKey': + case 'vatstoreSet': + case 'vatstoreDelete': { + console.warn(`vat ${vatId} issued invalid syscall ${op} `, vso); + break; + } + default: + // Compile-time exhaustiveness check + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + console.warn(`vat ${vatId} issued unknown syscall ${op} `, vso); + break; } - default: - // Compile-time exhaustiveness check - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - console.warn(`vat ${vatId} issued unknown syscall ${op} `, vso); - break; + return harden(['ok', null]); + } catch (error) { + this.#logger.error(`Fatal syscall error in vat ${this.vatId}`, error); + this.#vatFatalSyscall('syscall translation error: prepare to die'); + return harden([ + 'error', + error instanceof Error ? error.message : String(error), + ]); } } + + /** + * Log a fatal syscall error and set the illegalSyscall property. + * + * @param error - The error message to log. + */ + #vatFatalSyscall(error: string): void { + this.illegalSyscall = { vatId: this.vatId, info: makeError(error) }; + } } From a93ef166e60cb62f48787339c02c9123c65130d6 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 19 May 2025 23:34:35 +0200 Subject: [PATCH 06/25] handle syscall failures --- packages/ocap-kernel/src/Kernel.ts | 34 +++++-- packages/ocap-kernel/src/KernelQueue.ts | 37 +++++++- packages/ocap-kernel/src/KernelRouter.ts | 110 ++++++++++++----------- packages/ocap-kernel/src/VatHandle.ts | 77 +++++++++++++--- packages/ocap-kernel/src/VatSyscall.ts | 6 +- packages/ocap-kernel/src/types.ts | 11 ++- 6 files changed, 198 insertions(+), 77 deletions(-) diff --git a/packages/ocap-kernel/src/Kernel.ts b/packages/ocap-kernel/src/Kernel.ts index 8b84b69af..78bb727ad 100644 --- a/packages/ocap-kernel/src/Kernel.ts +++ b/packages/ocap-kernel/src/Kernel.ts @@ -2,6 +2,7 @@ import type { CapData } from '@endo/marshal'; import { StreamReadError, VatAlreadyExistsError, + VatDeletedError, VatNotFoundError, } from '@metamask/kernel-errors'; import { RpcService } from '@metamask/kernel-rpc-methods'; @@ -91,7 +92,10 @@ export class Kernel { if (options.resetStorage) { this.#resetKernelState(); } - this.#kernelQueue = new KernelQueue(this.#kernelStore); + this.#kernelQueue = new KernelQueue( + this.#kernelStore, + this.terminateVat.bind(this), + ); this.#kernelRouter = new KernelRouter( this.#kernelStore, this.#kernelQueue, @@ -302,14 +306,31 @@ export class Kernel { * @param vatId - The ID of the vat. * @param terminating - If true, the vat is being killed, if false, it's being * restarted. + * @param reason - If the vat is being terminated, the reason for the termination. */ - async #stopVat(vatId: VatId, terminating: boolean): Promise { + async #stopVat( + vatId: VatId, + terminating: boolean, + reason?: CapData, + ): Promise { const vat = this.#getVat(vatId); if (!vat) { throw new VatNotFoundError(vatId); } - await vat.terminate(terminating); - await this.#vatWorkerService.terminate(vatId).catch(this.#logger.error); + + // Construct an error object for vat.terminate and vatWorkerService.terminate + let terminationError: Error | undefined; + if (reason) { + // You might want a more sophisticated way to turn CapData into an Error + terminationError = new Error(`Vat termination: ${reason.body}`); + } else if (terminating) { + terminationError = new VatDeletedError(vatId); + } + + await vat.terminate(terminating, terminationError); + await this.#vatWorkerService + .terminate(vatId, terminationError) + .catch(this.#logger.error); this.#vats.delete(vatId); } @@ -317,9 +338,10 @@ export class Kernel { * Terminate a vat with extreme prejudice. * * @param vatId - The ID of the vat. + * @param reason - If the vat is being terminated, the reason for the termination. */ - async terminateVat(vatId: VatId): Promise { - await this.#stopVat(vatId, true); + async terminateVat(vatId: VatId, reason?: CapData): Promise { + await this.#stopVat(vatId, true, reason); this.#kernelStore.deleteVatConfig(vatId); // Mark for deletion (which will happen later, in vat-cleanup events) this.#kernelStore.markVatAsTerminated(vatId); diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index 339f67f4e..100b06068 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -7,6 +7,7 @@ import { kser } from './services/kernel-marshal.ts'; import type { KernelStore } from './store/index.ts'; import { insistVatId } from './types.ts'; import type { + CrankResults, KRef, Message, RunQueueItem, @@ -26,14 +27,24 @@ export class KernelQueue { /** Storage holding the kernel's own persistent state */ readonly #kernelStore: KernelStore; + /** A function that terminates a vat. */ + readonly #terminateVat: ( + vatId: VatId, + reason?: CapData, + ) => Promise; + /** Message results that the kernel itself has subscribed to */ readonly subscriptions: Map) => void> = new Map(); /** Thunk to signal run queue transition from empty to non-empty */ #wakeUpTheRunQueue: (() => void) | null; - constructor(kernelStore: KernelStore) { + constructor( + kernelStore: KernelStore, + terminateVat: (vatId: VatId, reason?: CapData) => Promise, + ) { this.#kernelStore = kernelStore; + this.#terminateVat = terminateVat; this.#wakeUpTheRunQueue = null; } @@ -43,11 +54,31 @@ export class KernelQueue { * * @param deliver - A function that delivers an item to the kernel. */ - async run(deliver: (item: RunQueueItem) => Promise): Promise { + async run( + deliver: (item: RunQueueItem) => Promise, + ): Promise { for await (const item of this.#runQueueItems()) { this.#kernelStore.nextTerminatedVatCleanup(); this.#kernelStore.createCrankSavepoint('deliver'); - await deliver(item); + const crankResults = await deliver(item); + if (crankResults?.abort) { + // Errors unwind any changes the vat made, by rolling back to the + // "deliver" savepoint In addition, the crankResults will either ask for + // the message to be consumed (without redelivery), or they'll ask for it + // to be attempted again (so it can go "splat" against a terminated vat, + // and give the sender the right error). In the latter case, we roll back + // to the "start" savepoint before the delivery was pulled off the run-queue, + // undoing the dequeueing. + this.#kernelStore.rollbackCrank( + crankResults.consumeMessage ? 'deliver' : 'start', + ); + } + // Vat termination during delivery is triggered by + // an illegal syscall or by syscall.exit() + if (crankResults?.terminate) { + const { vatId, info } = crankResults.terminate; + await this.#terminateVat(vatId, info); + } this.#kernelStore.collectGarbage(); } } diff --git a/packages/ocap-kernel/src/KernelRouter.ts b/packages/ocap-kernel/src/KernelRouter.ts index 9be7c723b..442593785 100644 --- a/packages/ocap-kernel/src/KernelRouter.ts +++ b/packages/ocap-kernel/src/KernelRouter.ts @@ -15,6 +15,7 @@ import type { RunQueueItemBringOutYourDead, RunQueueItemNotify, RunQueueItemGCAction, + CrankResults, } from './types.ts'; import { insistVatId, insistMessage } from './types.ts'; import { assert, Fail } from './utils/assert.ts'; @@ -74,27 +75,25 @@ export class KernelRouter { * is also forwarded to all of the promise's registered subscribers. * * @param item - The message/notification to deliver. + * @returns The crank outcome. */ - async deliver(item: RunQueueItem): Promise { + async deliver(item: RunQueueItem): Promise { switch (item.type) { case 'send': - await this.#deliverSend(item); - break; + return await this.#deliverSend(item); case 'notify': - await this.#deliverNotify(item); - break; + return await this.#deliverNotify(item); case 'dropExports': case 'retireExports': case 'retireImports': - await this.#deliverGCAction(item); - break; + return await this.#deliverGCAction(item); case 'bringOutYourDead': - await this.#deliverBringOutYourDead(item); - break; + return await this.#deliverBringOutYourDead(item); default: // @ts-expect-error Runtime does not respect "never". Fail`unsupported or unknown run queue item type ${item.type}`; } + return undefined; } /** @@ -167,11 +166,14 @@ export class KernelRouter { * Deliver a 'send' run queue item. * * @param item - The send item to deliver. + * @returns The crank outcome. */ - async #deliverSend(item: RunQueueItemSend): Promise { + async #deliverSend( + item: RunQueueItemSend, + ): Promise { const route = this.#routeMessage(item); - // Message went splat + // Message went splat, no longer the kernel's responsibility if (!route) { this.#kernelStore.decrementRefCount(item.target, 'deliver|splat|target'); if (item.message.result) { @@ -186,7 +188,7 @@ export class KernelRouter { console.log( `@@@@ message went splat ${item.target}<-${JSON.stringify(item.message)}`, ); - return; + return undefined; } const { vatId, target } = route; @@ -194,50 +196,51 @@ export class KernelRouter { console.log( `@@@@ deliver ${vatId} send ${target}<-${JSON.stringify(message)}`, ); - if (vatId) { - const vat = this.#getVat(vatId); - if (vat) { - if (message.result) { - if (typeof message.result !== 'string') { - throw TypeError('message result must be a string'); - } - this.#kernelStore.setPromiseDecider(message.result, vatId); - this.#kernelStore.decrementRefCount( - message.result, - 'deliver|send|result', - ); - } - const vatTarget = this.#kernelStore.translateRefKtoV( - vatId, - target, - false, - ); - const vatMessage = this.#kernelStore.translateMessageKtoV( - vatId, - message, - ); - await vat.deliverMessage(vatTarget, vatMessage); - this.#kernelStore.decrementRefCount(target, 'deliver|send|target'); - for (const slot of message.methargs.slots) { - this.#kernelStore.decrementRefCount(slot, 'deliver|send|slot'); - } - } else { - Fail`no owner for kernel object ${target}`; - } - } else { + if (!vatId) { + throw Fail`no owner for kernel object ${target}`; + } + + const vat = this.#getVat(vatId); + + if (!vat) { + // Requeue the message for later delivery. this.#kernelStore.enqueuePromiseMessage(target, message); + // For requeued messages, there's no immediate crank outcome from a vat. + return undefined; } + + if (message.result) { + if (typeof message.result !== 'string') { + throw TypeError('message result must be a string'); + } + this.#kernelStore.setPromiseDecider(message.result, vatId); + this.#kernelStore.decrementRefCount( + message.result, + 'deliver|send|result', + ); + } + const vatTarget = this.#kernelStore.translateRefKtoV(vatId, target, false); + const vatMessage = this.#kernelStore.translateMessageKtoV(vatId, message); + const crankResults = await vat.deliverMessage(vatTarget, vatMessage); + this.#kernelStore.decrementRefCount(target, 'deliver|send|target'); + for (const slot of message.methargs.slots) { + this.#kernelStore.decrementRefCount(slot, 'deliver|send|slot'); + } + console.log( `@@@@ done ${vatId} send ${target}<-${JSON.stringify(message)}`, ); + + return crankResults; } /** * Deliver a 'notify' run queue item. * * @param item - The notify item to deliver. + * @returns The crank outcome. */ - async #deliverNotify(item: RunQueueItemNotify): Promise { + async #deliverNotify(item: RunQueueItemNotify): Promise { const { vatId, kpid } = item; insistVatId(vatId); const { context, isPromise } = parseRef(kpid); @@ -254,12 +257,12 @@ export class KernelRouter { } if (!this.#kernelStore.krefToEref(vatId, kpid)) { // no c-list entry, already done - return; + return { didDelivery: vatId }; } const targets = this.#kernelStore.getKpidsToRetire(kpid, value); if (targets.length === 0) { // no kpids to retire, already done - return; + return { didDelivery: vatId }; } const resolutions: VatOneResolution[] = []; for (const toResolve of targets) { @@ -281,18 +284,20 @@ export class KernelRouter { } } const vat = this.#getVat(vatId); - await vat.deliverNotify(resolutions); + const crankResults = await vat.deliverNotify(resolutions); // Decrement reference count for processed 'notify' item this.#kernelStore.decrementRefCount(kpid, 'deliver|notify'); console.log(`@@@@ done ${vatId} notify ${vatId} ${kpid}`); + return crankResults; } /** * Deliver a Garbage Collection action run queue item. * * @param item - The dropExports | retireExports | retireImports item to deliver. + * @returns The crank outcome. */ - async #deliverGCAction(item: RunQueueItemGCAction): Promise { + async #deliverGCAction(item: RunQueueItemGCAction): Promise { const { type, vatId, krefs } = item; console.log(`@@@@ deliver ${vatId} ${type}`, krefs); const vat = this.#getVat(vatId); @@ -302,22 +307,25 @@ export class KernelRouter { | 'deliverDropExports' | 'deliverRetireExports' | 'deliverRetireImports'; - await vat[method](vrefs); + const crankResults = await vat[method](vrefs); console.log(`@@@@ done ${vatId} ${type}`, krefs); + return crankResults; } /** * Deliver a 'bringOutYourDead' run queue item. * * @param item - The bringOutYourDead item to deliver. + * @returns The crank outcome. */ async #deliverBringOutYourDead( item: RunQueueItemBringOutYourDead, - ): Promise { + ): Promise { const { vatId } = item; console.log(`@@@@ deliver ${vatId} bringOutYourDead`); const vat = this.#getVat(vatId); - await vat.deliverBringOutYourDead(); + const crankResults = await vat.deliverBringOutYourDead(); console.log(`@@@@ done ${vatId} bringOutYourDead`); + return crankResults; } } diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index 641133d79..ddf6fff48 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -20,7 +20,7 @@ import { vatMethodSpecs, vatSyscallHandlers } from './rpc/index.ts'; import type { PingVatResult, VatMethod } from './rpc/index.ts'; import { kser } from './services/kernel-marshal.ts'; import type { KernelStore } from './store/index.ts'; -import type { Message, VatId, VatConfig, VRef } from './types.ts'; +import type { Message, VatId, VatConfig, VRef, CrankResults } from './types.ts'; import { VatSyscall } from './VatSyscall.ts'; type VatConstructorProps = { @@ -104,8 +104,7 @@ export class VatHandle { ); this.#rpcService = new RpcService(vatSyscallHandlers, { handleSyscall: async (params) => { - await this.#vatSyscall.handleSyscall(params as VatSyscallObject); - return ['ok', null]; // XXX TODO: Return actual results from syscalls + return await this.#vatSyscall.handleSyscall(params as VatSyscallObject); }, }); } @@ -202,70 +201,83 @@ export class VatHandle { * * @param target - The VRef of the object to which the message is addressed. * @param message - The message to deliver. + * @returns The crank results. */ - async deliverMessage(target: VRef, message: Message): Promise { + async deliverMessage(target: VRef, message: Message): Promise { await this.sendVatCommand({ method: 'deliver', params: ['message', target, message], }); + return this.#deliveryCrankResults(); } /** * Make a 'notify' delivery to the vat. * * @param resolutions - One or more promise resolutions to deliver. + * @returns The crank results. */ - async deliverNotify(resolutions: VatOneResolution[]): Promise { + async deliverNotify(resolutions: VatOneResolution[]): Promise { await this.sendVatCommand({ method: 'deliver', params: ['notify', resolutions], }); + return this.#deliveryCrankResults(); } /** * Make a 'dropExports' delivery to the vat. * * @param vrefs - The VRefs of the exports to be dropped. + * @returns The crank results. */ - async deliverDropExports(vrefs: VRef[]): Promise { + async deliverDropExports(vrefs: VRef[]): Promise { await this.sendVatCommand({ method: 'deliver', params: ['dropExports', vrefs], }); + return this.#deliveryCrankResults(); } /** * Make a 'retireExports' delivery to the vat. * * @param vrefs - The VRefs of the exports to be retired. + * @returns The crank results. */ - async deliverRetireExports(vrefs: VRef[]): Promise { + async deliverRetireExports(vrefs: VRef[]): Promise { await this.sendVatCommand({ method: 'deliver', params: ['retireExports', vrefs], }); + return this.#deliveryCrankResults(); } /** * Make a 'retireImports' delivery to the vat. * * @param vrefs - The VRefs of the imports to be retired. + * @returns The crank results. */ - async deliverRetireImports(vrefs: VRef[]): Promise { + async deliverRetireImports(vrefs: VRef[]): Promise { await this.sendVatCommand({ method: 'deliver', params: ['retireImports', vrefs], }); + return this.#deliveryCrankResults(); } /** * Make a 'bringOutYourDead' delivery to the vat. + * + * @returns The crank results. */ - async deliverBringOutYourDead(): Promise { + async deliverBringOutYourDead(): Promise { await this.sendVatCommand({ method: 'deliver', params: ['bringOutYourDead'], }); + return this.#deliveryCrankResults(); } /** @@ -277,15 +289,14 @@ export class VatHandle { */ async terminate(terminating: boolean, error?: Error): Promise { await this.#vatStream.end(error); - + const terminationError = error ?? new VatDeletedError(this.vatId); if (terminating) { // Reject promises exported to other vats for which this vat is the decider - const failure = kser(new VatDeletedError(this.vatId)); + const failure = kser(terminationError); for (const kpid of this.#kernelStore.getPromisesByDecider(this.vatId)) { this.#kernelQueue.resolvePromises(this.vatId, [[kpid, true, failure]]); } - - this.#rpcClient.rejectAll(error ?? new VatDeletedError(this.vatId)); + this.#rpcClient.rejectAll(terminationError); this.#kernelStore.deleteVat(this.vatId); } } @@ -313,4 +324,44 @@ export class VatHandle { } return result; } + + /** + * Get the crank outcome for a given checkpoint result. + * + * @returns The crank outcome. + */ + #deliveryCrankResults(): CrankResults { + const results: CrankResults = { + didDelivery: this.vatId, + }; + + // note: these conditionals express a priority order: the + // consequences of an illegal syscall take precedence over a vat + // requesting termination, etc + if (this.#vatSyscall.illegalSyscall) { + // For now, vat errors both rewind changes and terminate the vat. + // Some day, they might rewind changes but then suspend the vat. + results.abort = true; + const { info } = this.#vatSyscall.illegalSyscall; + results.terminate = { vatId: this.vatId, reject: true, info }; + // } else if (this.#vatSyscall.deliveryError) { + // results.abort = true; + // const info = makeError(this.#vatSyscall.deliveryError); + // results.terminate = { vatId: this.vatId, reject: true, info }; + } else if (this.#vatSyscall.vatRequestedTermination) { + if (this.#vatSyscall.vatRequestedTermination.reject) { + results.abort = true; // vatPowers.exitWithFailure wants rewind + } + results.terminate = { + vatId: this.vatId, + ...this.#vatSyscall.vatRequestedTermination, + }; + } + // We leave results.consumeMessage up to the caller. Send failures + // never set results.consumeMessage (we allow the delivery to be + // re-attempted and it splats against the now-dead vat, or doesn't + // get delivered until the vat is unsuspended). But failures in + // e.g. startVat will want to set consumeMessage. + return harden(results); + } } diff --git a/packages/ocap-kernel/src/VatSyscall.ts b/packages/ocap-kernel/src/VatSyscall.ts index 0c229a65d..a7b120a7f 100644 --- a/packages/ocap-kernel/src/VatSyscall.ts +++ b/packages/ocap-kernel/src/VatSyscall.ts @@ -1,9 +1,9 @@ import type { + SwingSetCapData, VatOneResolution, VatSyscallObject, VatSyscallResult, } from '@agoric/swingset-liveslots'; -import type { CapData } from '@endo/marshal'; import { Logger } from '@metamask/logger'; import type { KernelQueue } from './KernelQueue.ts'; @@ -40,11 +40,11 @@ export class VatSyscall { readonly #logger: Logger; /** The illegal syscall that was received */ - illegalSyscall: { vatId: VatId; info: CapData } | undefined; + illegalSyscall: { vatId: VatId; info: SwingSetCapData } | undefined; /** The termination request that was received from the vat with syscall.exit() */ vatRequestedTermination: - | { reject: boolean; info: CapData } + | { reject: boolean; info: SwingSetCapData } | undefined; /** diff --git a/packages/ocap-kernel/src/types.ts b/packages/ocap-kernel/src/types.ts index c2adef5c0..e3a42e02e 100644 --- a/packages/ocap-kernel/src/types.ts +++ b/packages/ocap-kernel/src/types.ts @@ -1,4 +1,5 @@ import type { + SwingSetCapData, Message as SwingsetMessage, VatSyscallObject, VatSyscallSend, @@ -249,10 +250,11 @@ export type VatWorkerService = { * Terminate a worker identified by its vat id. * * @param vatId - The vat id of the worker to terminate. + * @param error - An optional error to terminate the worker with. * @returns A promise that resolves when the worker has terminated * or rejects if that worker does not exist. */ - terminate: (vatId: VatId) => Promise; + terminate: (vatId: VatId, error?: Error) => Promise; /** * Terminate all workers managed by the service. * @@ -377,3 +379,10 @@ export const GCActionStruct = define('GCAction', (value: unknown) => { export const isGCAction = (value: unknown): value is GCAction => is(value, GCActionStruct); + +export type CrankResults = { + didDelivery?: VatId; // the vat on which we made a delivery + abort?: boolean; // changes should be discarded, not committed + consumeMessage?: boolean; // discard the aborted delivery + terminate?: { vatId: VatId; reject: boolean; info: SwingSetCapData }; +}; From 880acd34690d6394adbbd5cb052c3eac5bd3c073 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 21 May 2025 10:28:30 +0200 Subject: [PATCH 07/25] retrun dispatch delivery errors from VatSupervisor --- packages/ocap-kernel/src/Kernel.ts | 2 - packages/ocap-kernel/src/KernelQueue.ts | 15 ++-- packages/ocap-kernel/src/VatHandle.ts | 57 ++++++++------- packages/ocap-kernel/src/VatSupervisor.ts | 53 ++++++++++---- packages/ocap-kernel/src/VatSyscall.ts | 3 + packages/ocap-kernel/src/rpc/vat/deliver.ts | 12 ++-- packages/ocap-kernel/src/rpc/vat/initVat.ts | 17 +++-- packages/ocap-kernel/src/rpc/vat/shared.ts | 9 ++- packages/ocap-kernel/src/types.ts | 3 + vitest.config.ts | 79 +++++++++++---------- 10 files changed, 148 insertions(+), 102 deletions(-) diff --git a/packages/ocap-kernel/src/Kernel.ts b/packages/ocap-kernel/src/Kernel.ts index 78bb727ad..2694987cb 100644 --- a/packages/ocap-kernel/src/Kernel.ts +++ b/packages/ocap-kernel/src/Kernel.ts @@ -318,10 +318,8 @@ export class Kernel { throw new VatNotFoundError(vatId); } - // Construct an error object for vat.terminate and vatWorkerService.terminate let terminationError: Error | undefined; if (reason) { - // You might want a more sophisticated way to turn CapData into an Error terminationError = new Error(`Vat termination: ${reason.body}`); } else if (terminating) { terminationError = new VatDeletedError(vatId); diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index 100b06068..9b0df75be 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -62,19 +62,16 @@ export class KernelQueue { this.#kernelStore.createCrankSavepoint('deliver'); const crankResults = await deliver(item); if (crankResults?.abort) { - // Errors unwind any changes the vat made, by rolling back to the - // "deliver" savepoint In addition, the crankResults will either ask for - // the message to be consumed (without redelivery), or they'll ask for it - // to be attempted again (so it can go "splat" against a terminated vat, - // and give the sender the right error). In the latter case, we roll back - // to the "start" savepoint before the delivery was pulled off the run-queue, - // undoing the dequeueing. + // - consumeMessage=true (for hypothetical one-shot lifecycle ops): + // rolls back to 'deliver', discarding the message. + // - consumeMessage=false (default for sends/notifies): + // rolls back to 'start', re-queuing the message (e.g., to "splat" against a now-terminated vat, rejecting its result to the sender). + // Currently, consumeMessage defaults to false as such lifecycle ops are not distinct run-queue item types here. this.#kernelStore.rollbackCrank( crankResults.consumeMessage ? 'deliver' : 'start', ); } - // Vat termination during delivery is triggered by - // an illegal syscall or by syscall.exit() + // Vat termination during delivery is triggered by an illegal syscall or by syscall.exit() if (crankResults?.terminate) { const { vatId, info } = crankResults.terminate; await this.#terminateVat(vatId, info); diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index ddf6fff48..4ad795244 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -8,7 +8,7 @@ import type { ExtractParams, ExtractResult, } from '@metamask/kernel-rpc-methods'; -import type { VatStore, VatCheckpoint } from '@metamask/kernel-store'; +import type { VatStore } from '@metamask/kernel-store'; import type { JsonRpcMessage } from '@metamask/kernel-utils'; import { Logger } from '@metamask/logger'; import { serializeError } from '@metamask/rpc-errors'; @@ -18,9 +18,16 @@ import { isJsonRpcRequest, isJsonRpcResponse } from '@metamask/utils'; import type { KernelQueue } from './KernelQueue.ts'; import { vatMethodSpecs, vatSyscallHandlers } from './rpc/index.ts'; import type { PingVatResult, VatMethod } from './rpc/index.ts'; -import { kser } from './services/kernel-marshal.ts'; +import { kser, makeError } from './services/kernel-marshal.ts'; import type { KernelStore } from './store/index.ts'; -import type { Message, VatId, VatConfig, VRef, CrankResults } from './types.ts'; +import type { + Message, + VatId, + VatConfig, + VRef, + CrankResults, + VatDeliveryResult, +} from './types.ts'; import { VatSyscall } from './VatSyscall.ts'; type VatConstructorProps = { @@ -143,7 +150,7 @@ export class VatHandle { }, ); - await this.sendVatCommand({ + await this.#sendVatCommand({ method: 'initVat', params: { vatConfig: this.config, @@ -158,7 +165,7 @@ export class VatHandle { * @returns A promise that resolves to the result of the ping. */ async ping(): Promise { - return await this.sendVatCommand({ + return await this.#sendVatCommand({ method: 'ping', params: [], }); @@ -204,10 +211,11 @@ export class VatHandle { * @returns The crank results. */ async deliverMessage(target: VRef, message: Message): Promise { - await this.sendVatCommand({ + const result = await this.#sendVatCommand({ method: 'deliver', params: ['message', target, message], }); + console.log('deliverMessage result', result); return this.#deliveryCrankResults(); } @@ -218,7 +226,7 @@ export class VatHandle { * @returns The crank results. */ async deliverNotify(resolutions: VatOneResolution[]): Promise { - await this.sendVatCommand({ + await this.#sendVatCommand({ method: 'deliver', params: ['notify', resolutions], }); @@ -232,7 +240,7 @@ export class VatHandle { * @returns The crank results. */ async deliverDropExports(vrefs: VRef[]): Promise { - await this.sendVatCommand({ + await this.#sendVatCommand({ method: 'deliver', params: ['dropExports', vrefs], }); @@ -246,7 +254,7 @@ export class VatHandle { * @returns The crank results. */ async deliverRetireExports(vrefs: VRef[]): Promise { - await this.sendVatCommand({ + await this.#sendVatCommand({ method: 'deliver', params: ['retireExports', vrefs], }); @@ -260,7 +268,7 @@ export class VatHandle { * @returns The crank results. */ async deliverRetireImports(vrefs: VRef[]): Promise { - await this.sendVatCommand({ + await this.#sendVatCommand({ method: 'deliver', params: ['retireImports', vrefs], }); @@ -273,7 +281,7 @@ export class VatHandle { * @returns The crank results. */ async deliverBringOutYourDead(): Promise { - await this.sendVatCommand({ + await this.#sendVatCommand({ method: 'deliver', params: ['bringOutYourDead'], }); @@ -309,7 +317,7 @@ export class VatHandle { * @param payload.params - The parameters to pass to the method. * @returns A promise that resolves the response to the command. */ - async sendVatCommand({ + async #sendVatCommand({ method, params, }: { @@ -317,10 +325,10 @@ export class VatHandle { params: ExtractParams; }): Promise> { const result = await this.#rpcClient.call(method, params); - if (method === 'deliver' || method === 'initVat') { - // TypeScript fails to narrow the result type on its own - const [sets, deletes] = result as VatCheckpoint; + if (method === 'initVat' || method === 'deliver') { + const [[sets, deletes], deliveryError] = result as VatDeliveryResult; this.#vatStore.updateKVData(sets, deletes); + this.#vatSyscall.deliveryError = deliveryError ?? undefined; } return result; } @@ -335,19 +343,18 @@ export class VatHandle { didDelivery: this.vatId, }; - // note: these conditionals express a priority order: the - // consequences of an illegal syscall take precedence over a vat - // requesting termination, etc + // These conditionals express a priority order: the consequences of an + // illegal syscall take precedence over a vat requesting termination, etc. if (this.#vatSyscall.illegalSyscall) { // For now, vat errors both rewind changes and terminate the vat. // Some day, they might rewind changes but then suspend the vat. results.abort = true; const { info } = this.#vatSyscall.illegalSyscall; results.terminate = { vatId: this.vatId, reject: true, info }; - // } else if (this.#vatSyscall.deliveryError) { - // results.abort = true; - // const info = makeError(this.#vatSyscall.deliveryError); - // results.terminate = { vatId: this.vatId, reject: true, info }; + } else if (this.#vatSyscall.deliveryError) { + results.abort = true; + const info = makeError(this.#vatSyscall.deliveryError); + results.terminate = { vatId: this.vatId, reject: true, info }; } else if (this.#vatSyscall.vatRequestedTermination) { if (this.#vatSyscall.vatRequestedTermination.reject) { results.abort = true; // vatPowers.exitWithFailure wants rewind @@ -357,11 +364,7 @@ export class VatHandle { ...this.#vatSyscall.vatRequestedTermination, }; } - // We leave results.consumeMessage up to the caller. Send failures - // never set results.consumeMessage (we allow the delivery to be - // re-attempted and it splats against the now-dead vat, or doesn't - // get delivered until the vat is unsuspended). But failures in - // e.g. startVat will want to set consumeMessage. + return harden(results); } } diff --git a/packages/ocap-kernel/src/VatSupervisor.ts b/packages/ocap-kernel/src/VatSupervisor.ts index 59a65cc29..8d4252fec 100644 --- a/packages/ocap-kernel/src/VatSupervisor.ts +++ b/packages/ocap-kernel/src/VatSupervisor.ts @@ -9,7 +9,7 @@ import { makeMarshal } from '@endo/marshal'; import type { CapData } from '@endo/marshal'; import { StreamReadError } from '@metamask/kernel-errors'; import { RpcClient, RpcService } from '@metamask/kernel-rpc-methods'; -import type { VatKVStore, VatCheckpoint } from '@metamask/kernel-store'; +import type { VatKVStore } from '@metamask/kernel-store'; import { waitUntilQuiescent } from '@metamask/kernel-utils'; import type { JsonRpcMessage } from '@metamask/kernel-utils'; import type { Logger } from '@metamask/logger'; @@ -18,13 +18,12 @@ import type { DuplexStream } from '@metamask/streams'; import { isJsonRpcRequest, isJsonRpcResponse } from '@metamask/utils'; import { vatSyscallMethodSpecs, vatHandlers } from './rpc/index.ts'; -import type { InitVat } from './rpc/vat/initVat.ts'; import { makeGCAndFinalize } from './services/gc-finalize.ts'; import { makeDummyMeterControl } from './services/meter-control.ts'; import { makeSupervisorSyscall } from './services/syscall.ts'; import type { DispatchFn, MakeLiveSlotsFn, GCTools } from './services/types.ts'; import { makeVatKVStore } from './store/vat-kv-store.ts'; -import type { VatId } from './types.ts'; +import type { VatConfig, VatDeliveryResult, VatId } from './types.ts'; import { isVatConfig, coerceVatSyscallObject } from './types.ts'; const makeLiveSlots: MakeLiveSlotsFn = localMakeLiveSlots; @@ -179,7 +178,17 @@ export class VatSupervisor { */ executeSyscall(vso: VatSyscallObject): VatSyscallResult { this.#syscallsInFlight.push( - // XXX TODO: These all get rejected, so we have to catch them. See #deliver. + // IMPORTANT: Syscall architecture design explanation: + // 1. Vats operate on an "optimistic execution" model - they send syscalls and continue execution + // without waiting for responses, assuming success. + // 2. The Kernel processes syscalls asynchronously, and responses often arrive after the current crank is complete. + // 3. At the end of each crank (in #deliver), we explicitly reject all in-flight syscalls using + // this.#rpcClient.rejectAll() because: + // - The vat model requires synchronous syscall returns but asynchronous actual execution + // - Late-arriving responses would cause unexpected state changes if handled mid-crank + // - This gives us a clean slate for the next crank + // 4. We catch these rejections here to prevent unhandled promise rejections, as they're an + // expected part of the architecture, not errors this.#rpcClient .call('syscall', coerceVatSyscallObject(vso)) .catch(() => undefined), @@ -187,18 +196,32 @@ export class VatSupervisor { return ['ok', null]; } - async #deliver(params: VatDeliveryObject): Promise { + async #deliver(params: VatDeliveryObject): Promise { if (!this.#dispatch) { throw new Error(`cannot deliver before vat is loaded`); } - await this.#dispatch(harden(params)); - // XXX TODO: Actually handle the syscall results - this.#syscallsInFlight.length = 0; - this.#rpcClient.rejectAll(new Error('end of crank')); + let deliveryError: string | null = null; + + try { + await this.#dispatch(harden(params)); + } catch (error) { + // Handle delivery errors + deliveryError = error instanceof Error ? error.message : String(error); + this.#logger.error(`Delivery error in vat ${this.id}:`, deliveryError); + } finally { + // Clean up at the end of a crank: + // 1. Clear the syscallsInFlight array to prevent memory leaks + this.#syscallsInFlight.length = 0; + // 2. Reject all pending RPC requests to maintain the optimistic execution model + // This prevents late responses from affecting the vat in unexpected ways + // between cranks. Any actual responses that arrive later will be ignored + // since their message IDs will no longer be in the unresolvedMessages map. + this.#rpcClient.rejectAll(new Error('end of crank')); + } // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return this.#vatKVStore!.checkpoint(); + return [this.#vatKVStore!.checkpoint(), deliveryError]; } /** @@ -210,7 +233,10 @@ export class VatSupervisor { * * @returns a promise for a checkpoint of the new vat. */ - readonly #initVat: InitVat = async (vatConfig, state) => { + async #initVat( + vatConfig: VatConfig, + state: Map, + ): Promise { if (this.#loaded) { throw Error( 'VatSupervisor received initVat after user code already loaded', @@ -275,8 +301,7 @@ export class VatSupervisor { this.#dispatch = liveslots.dispatch; const serParam = marshal.toCapData(harden(parameters)) as CapData; - await this.#dispatch(harden(['startVat', serParam])); - return this.#vatKVStore.checkpoint(); - }; + return await this.#deliver(harden(['startVat', serParam])); + } } diff --git a/packages/ocap-kernel/src/VatSyscall.ts b/packages/ocap-kernel/src/VatSyscall.ts index a7b120a7f..edb193b5e 100644 --- a/packages/ocap-kernel/src/VatSyscall.ts +++ b/packages/ocap-kernel/src/VatSyscall.ts @@ -42,6 +42,9 @@ export class VatSyscall { /** The illegal syscall that was received */ illegalSyscall: { vatId: VatId; info: SwingSetCapData } | undefined; + /** The error when delivery failed */ + deliveryError: string | undefined; + /** The termination request that was received from the vat with syscall.exit() */ vatRequestedTermination: | { reject: boolean; info: SwingSetCapData } diff --git a/packages/ocap-kernel/src/rpc/vat/deliver.ts b/packages/ocap-kernel/src/rpc/vat/deliver.ts index 1893430ff..d4fcfe83e 100644 --- a/packages/ocap-kernel/src/rpc/vat/deliver.ts +++ b/packages/ocap-kernel/src/rpc/vat/deliver.ts @@ -1,5 +1,4 @@ import type { Handler, MethodSpec } from '@metamask/kernel-rpc-methods'; -import type { VatCheckpoint } from '@metamask/kernel-store'; import { tuple, literal, @@ -11,12 +10,13 @@ import { import type { Infer } from '@metamask/superstruct'; import { UnsafeJsonStruct } from '@metamask/utils'; -import { VatCheckpointStruct } from './shared.ts'; +import { VatDeliveryResultStruct } from './shared.ts'; import { CapDataStruct, MessageStruct, VatOneResolutionStruct, } from '../../types.ts'; +import type { VatDeliveryResult } from '../../types.ts'; const MessageDeliveryStruct = tuple([ literal('message'), @@ -72,18 +72,18 @@ type VatDeliveryParams = Infer; export type DeliverSpec = MethodSpec< 'deliver', VatDeliveryParams, - Promise + Promise >; export const deliverSpec: DeliverSpec = { method: 'deliver', params: VatDeliveryParamsStruct, - result: VatCheckpointStruct, + result: VatDeliveryResultStruct, } as const; export type HandleDelivery = ( params: VatDeliveryParams, -) => Promise; +) => Promise; type DeliverHooks = { handleDelivery: HandleDelivery; @@ -92,7 +92,7 @@ type DeliverHooks = { export type DeliverHandler = Handler< 'deliver', VatDeliveryParams, - Promise, + Promise, DeliverHooks >; diff --git a/packages/ocap-kernel/src/rpc/vat/initVat.ts b/packages/ocap-kernel/src/rpc/vat/initVat.ts index 2e76f02fd..e46a124c7 100644 --- a/packages/ocap-kernel/src/rpc/vat/initVat.ts +++ b/packages/ocap-kernel/src/rpc/vat/initVat.ts @@ -1,11 +1,10 @@ import type { MethodSpec, Handler } from '@metamask/kernel-rpc-methods'; -import type { VatCheckpoint } from '@metamask/kernel-store'; import { array, object, string, tuple } from '@metamask/superstruct'; import type { Infer } from '@metamask/superstruct'; -import { VatCheckpointStruct } from './shared.ts'; +import { VatDeliveryResultStruct } from './shared.ts'; import { VatConfigStruct } from '../../types.ts'; -import type { VatConfig } from '../../types.ts'; +import type { VatConfig, VatDeliveryResult } from '../../types.ts'; const paramsStruct = object({ vatConfig: VatConfigStruct, @@ -14,18 +13,22 @@ const paramsStruct = object({ type Params = Infer; -export type InitVatSpec = MethodSpec<'initVat', Params, Promise>; +export type InitVatSpec = MethodSpec< + 'initVat', + Params, + Promise +>; export const initVatSpec: InitVatSpec = { method: 'initVat', params: paramsStruct, - result: VatCheckpointStruct, + result: VatDeliveryResultStruct, }; export type InitVat = ( vatConfig: VatConfig, state: Map, -) => Promise; +) => Promise; type InitVatHooks = { initVat: InitVat; @@ -34,7 +37,7 @@ type InitVatHooks = { export type InitVatHandler = Handler< 'initVat', Params, - Promise, + Promise, InitVatHooks >; diff --git a/packages/ocap-kernel/src/rpc/vat/shared.ts b/packages/ocap-kernel/src/rpc/vat/shared.ts index a0aa21425..0a4b313ea 100644 --- a/packages/ocap-kernel/src/rpc/vat/shared.ts +++ b/packages/ocap-kernel/src/rpc/vat/shared.ts @@ -1,8 +1,15 @@ import type { VatCheckpoint } from '@metamask/kernel-store'; import type { Struct } from '@metamask/superstruct'; -import { tuple, array, string } from '@metamask/superstruct'; +import { tuple, array, string, union, literal } from '@metamask/superstruct'; + +import type { VatDeliveryResult } from '../../types.ts'; export const VatCheckpointStruct: Struct = tuple([ array(tuple([string(), string()])), array(string()), ]); + +export const VatDeliveryResultStruct: Struct = tuple([ + VatCheckpointStruct, + union([string(), literal(null)]), +]); diff --git a/packages/ocap-kernel/src/types.ts b/packages/ocap-kernel/src/types.ts index e3a42e02e..3af503a64 100644 --- a/packages/ocap-kernel/src/types.ts +++ b/packages/ocap-kernel/src/types.ts @@ -5,6 +5,7 @@ import type { VatSyscallSend, } from '@agoric/swingset-liveslots'; import type { CapData } from '@endo/marshal'; +import type { VatCheckpoint } from '@metamask/kernel-store'; import type { JsonRpcMessage } from '@metamask/kernel-utils'; import type { DuplexStream } from '@metamask/streams'; import { @@ -386,3 +387,5 @@ export type CrankResults = { consumeMessage?: boolean; // discard the aborted delivery terminate?: { vatId: VatId; reject: boolean; info: SwingSetCapData }; }; + +export type VatDeliveryResult = [VatCheckpoint, string | null]; diff --git a/vitest.config.ts b/vitest.config.ts index d70bde553..f9034e5c6 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -68,18 +68,19 @@ export default defineConfig({ thresholds: { autoUpdate: true, 'packages/cli/**': { - statements: 70, - functions: 66.66, - branches: 88.57, - lines: 70, + statements: 0, + functions: 0, + branches: 0, + lines: 0, }, 'packages/create-package/**': { - statements: 100, - functions: 100, - branches: 100, - lines: 100, + statements: 0, + functions: 0, + branches: 0, + lines: 0, }, 'packages/extension/**': { +<<<<<<< HEAD statements: 87.65, functions: 87.79, branches: 82.25, @@ -90,18 +91,24 @@ export default defineConfig({ functions: 78.94, branches: 66.66, lines: 80.45, +======= + statements: 0, + functions: 0, + branches: 0, + lines: 0, +>>>>>>> 5ffe4069 (retrun dispatch delivery errors from VatSupervisor) }, 'packages/kernel-errors/**': { - statements: 98.63, - functions: 95.23, - branches: 92, - lines: 98.63, + statements: 24.65, + functions: 4.76, + branches: 0, + lines: 24.65, }, 'packages/kernel-rpc-methods/**': { - statements: 100, - functions: 100, - branches: 100, - lines: 100, + statements: 64.7, + functions: 60, + branches: 44.44, + lines: 64.7, }, 'packages/kernel-shims/**': { statements: 0, @@ -110,28 +117,28 @@ export default defineConfig({ lines: 0, }, 'packages/kernel-store/**': { - statements: 98, - functions: 100, - branches: 91.25, - lines: 97.99, + statements: 29.2, + functions: 41.66, + branches: 22.5, + lines: 29.31, }, 'packages/kernel-utils/**': { - statements: 100, - functions: 100, - branches: 100, - lines: 100, + statements: 43.63, + functions: 35.29, + branches: 15.38, + lines: 44, }, 'packages/logger/**': { - statements: 98.55, - functions: 95.83, - branches: 97.29, - lines: 100, + statements: 88.4, + functions: 79.16, + branches: 54.05, + lines: 89.39, }, 'packages/nodejs/**': { - statements: 72.22, - functions: 76.92, - branches: 69.23, - lines: 73.58, + statements: 29.62, + functions: 30.76, + branches: 23.07, + lines: 30.18, }, 'packages/ocap-kernel/**': { statements: 91.58, @@ -140,10 +147,10 @@ export default defineConfig({ lines: 91.56, }, 'packages/streams/**': { - statements: 100, - functions: 100, - branches: 100, - lines: 100, + statements: 39.66, + functions: 35.77, + branches: 31.29, + lines: 40.19, }, }, }, From 125f8d5ddfdb4ebb9dc057bd300dfa624636120c Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 21 May 2025 11:06:19 +0200 Subject: [PATCH 08/25] Terminate compromised vats --- packages/ocap-kernel/src/KernelQueue.ts | 39 +++++++++++- packages/ocap-kernel/src/VatSyscall.ts | 4 ++ packages/ocap-kernel/src/store/index.ts | 6 ++ .../src/store/methods/compromised.ts | 61 +++++++++++++++++++ .../ocap-kernel/src/store/methods/queue.ts | 30 ++++++++- packages/ocap-kernel/src/store/types.ts | 1 + vitest.config.ts | 10 +-- 7 files changed, 143 insertions(+), 8 deletions(-) create mode 100644 packages/ocap-kernel/src/store/methods/compromised.ts diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index 9b0df75be..d2f851180 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -3,7 +3,7 @@ import type { CapData } from '@endo/marshal'; import { makePromiseKit } from '@endo/promise-kit'; import { processGCActionSet } from './services/garbage-collection.ts'; -import { kser } from './services/kernel-marshal.ts'; +import { kser, makeError } from './services/kernel-marshal.ts'; import type { KernelStore } from './store/index.ts'; import { insistVatId } from './types.ts'; import type { @@ -59,6 +59,15 @@ export class KernelQueue { ): Promise { for await (const item of this.#runQueueItems()) { this.#kernelStore.nextTerminatedVatCleanup(); + + // Check if the target vat is compromised before attempting delivery + const targetVatId = this.#kernelStore.getRunQueueItemTargetVatId(item); + if (targetVatId && this.#kernelStore.isVatCompromised(targetVatId)) { + await this.#terminateCompromisedVat(item, targetVatId); + this.#kernelStore.collectGarbage(); + continue; + } + this.#kernelStore.createCrankSavepoint('deliver'); const crankResults = await deliver(item); if (crankResults?.abort) { @@ -244,4 +253,32 @@ export class KernelQueue { } } } + + /** + * Terminate a compromised vat and reject any pending promises. + * + * @param item - The run queue item that triggered the termination. + * @param vatId - The ID of the compromised vat. + */ + async #terminateCompromisedVat( + item: RunQueueItem, + vatId: VatId, + ): Promise { + // Ensure the vat is fully terminated if not already done + await this.#terminateVat( + vatId, + makeError(`Vat ${vatId} terminated due to prior syscall error.`), + ); + + // If the item was a 'send' with a result promise, reject that promise + if (item.type === 'send' && item.message.result) { + this.resolvePromises(undefined, [ + [ + item.message.result, + true, // rejected + makeError(`Vat ${vatId} terminated.`), + ], + ]); + } + } } diff --git a/packages/ocap-kernel/src/VatSyscall.ts b/packages/ocap-kernel/src/VatSyscall.ts index edb193b5e..557145969 100644 --- a/packages/ocap-kernel/src/VatSyscall.ts +++ b/packages/ocap-kernel/src/VatSyscall.ts @@ -286,5 +286,9 @@ export class VatSyscall { */ #vatFatalSyscall(error: string): void { this.illegalSyscall = { vatId: this.vatId, info: makeError(error) }; + this.#kernelStore.markVatAsCompromised(this.vatId); + this.#logger.error( + `Vat ${this.vatId} marked as compromised due to fatal syscall error: ${error}`, + ); } } diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 370f7b9f4..fa2b383ef 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -60,6 +60,7 @@ import type { KernelDatabase, KVStore, VatStore } from '@metamask/kernel-store'; import type { KRef, VatId } from '../types.ts'; import { getBaseMethods } from './methods/base.ts'; import { getCListMethods } from './methods/clist.ts'; +import { getCompromisedMethods } from './methods/compromised.ts'; import { getCrankMethods } from './methods/crank.ts'; import { getGCMethods } from './methods/gc.ts'; import { getIdMethods } from './methods/id.ts'; @@ -127,6 +128,7 @@ export function makeKernelStore(kdb: KernelDatabase) { terminatedVats: provideCachedStoredValue('vats.terminated', '[]'), inCrank: false, savepoints: [], + compromisedVats: provideCachedStoredValue('compromisedVats', '[]'), }; const id = getIdMethods(context); @@ -141,6 +143,7 @@ export function makeKernelStore(kdb: KernelDatabase) { const translators = getTranslators(context); const pinned = getPinMethods(context); const crank = getCrankMethods(context, kdb); + const compromised = getCompromisedMethods(context); /** * Create a new VatStore for a vat. @@ -160,6 +163,7 @@ export function makeKernelStore(kdb: KernelDatabase) { */ function deleteVat(vatId: VatId): void { vat.deleteVatConfig(vatId); + compromised.clearVatCompromisedStatus(vatId); kdb.deleteVatStore(vatId); } @@ -177,6 +181,7 @@ export function makeKernelStore(kdb: KernelDatabase) { context.nextPromiseId = provideCachedStoredValue('nextPromiseId', '1'); context.nextVatId = provideCachedStoredValue('nextVatId', '1'); context.nextRemoteId = provideCachedStoredValue('nextRemoteId', '1'); + context.compromisedVats = provideCachedStoredValue('compromisedVats', '[]'); context.inCrank = false; if (context.savepoints.length > 0) { kdb.releaseSavepoint('t0'); @@ -204,6 +209,7 @@ export function makeKernelStore(kdb: KernelDatabase) { ...translators, ...pinned, ...crank, + ...compromised, makeVatStore, deleteVat, clear, diff --git a/packages/ocap-kernel/src/store/methods/compromised.ts b/packages/ocap-kernel/src/store/methods/compromised.ts new file mode 100644 index 000000000..95ee44522 --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/compromised.ts @@ -0,0 +1,61 @@ +import type { VatId } from '../../types.ts'; +import type { StoreContext } from '../types.ts'; + +/** + * Get methods for tracking vats that have been compromised by syscall failures. + * + * @param ctx - The store context. + * @returns An object with methods for tracking compromised vats. + */ +// eslint-disable-next-line @typescript-eslint/explicit-function-return-type +export function getCompromisedMethods(ctx: StoreContext) { + /** + * Get the list of compromised vats. + * + * @returns An array of compromised vat IDs. + */ + function getCompromisedVats(): VatId[] { + return JSON.parse(ctx.compromisedVats.get() ?? '[]'); + } + + /** + * Check if a vat is compromised. + * + * @param vatId - The ID of the vat to check. + * @returns True if the vat is compromised, false otherwise. + */ + function isVatCompromised(vatId: VatId): boolean { + return getCompromisedVats().includes(vatId); + } + + /** + * Mark a vat as compromised. + * + * @param vatId - The ID of the vat to mark as compromised. + */ + function markVatAsCompromised(vatId: VatId): void { + const compromisedVats = getCompromisedVats(); + if (!compromisedVats.includes(vatId)) { + compromisedVats.push(vatId); + ctx.compromisedVats.set(JSON.stringify(compromisedVats)); + } + } + + /** + * Clear the compromised status for a vat. This is typically used + * when a vat is fully terminated and its state is cleaned up. + * + * @param vatId - The ID of the vat to clear. + */ + function clearVatCompromisedStatus(vatId: VatId): void { + const compromisedVats = getCompromisedVats().filter((id) => id !== vatId); + ctx.compromisedVats.set(JSON.stringify(compromisedVats)); + } + + return { + getCompromisedVats, + isVatCompromised, + markVatAsCompromised, + clearVatCompromisedStatus, + }; +} diff --git a/packages/ocap-kernel/src/store/methods/queue.ts b/packages/ocap-kernel/src/store/methods/queue.ts index 54693580e..9650d481b 100644 --- a/packages/ocap-kernel/src/store/methods/queue.ts +++ b/packages/ocap-kernel/src/store/methods/queue.ts @@ -1,6 +1,6 @@ -import type { RunQueueItem } from '../../types.ts'; +import type { RunQueueItem, VatId } from '../../types.ts'; import type { StoreContext } from '../types.ts'; - +import { getObjectMethods } from './object.ts'; /** * Get a queue store object that provides functionality for managing queues. * @@ -10,6 +10,8 @@ import type { StoreContext } from '../types.ts'; */ // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getQueueMethods(ctx: StoreContext) { + const { getOwner } = getObjectMethods(ctx); + /** * Find out how long some queue is. * @@ -60,10 +62,34 @@ export function getQueueMethods(ctx: StoreContext) { return ctx.runQueueLengthCache; } + /** + * Get the target VatId from a RunQueueItem. + * + * @param item - The RunQueueItem to get the target VatId from. + * @returns The target VatId, or undefined if the item is not a send. + */ + function getRunQueueItemTargetVatId(item: RunQueueItem): VatId | undefined { + switch (item.type) { + case 'send': { + return getOwner(item.target); + } + case 'notify': + case 'dropExports': + case 'retireExports': + case 'retireImports': + case 'bringOutYourDead': { + return item.vatId; + } + default: + return undefined; + } + } + return { getQueueLength, enqueueRun, dequeueRun, runQueueLength, + getRunQueueItemTargetVatId, }; } diff --git a/packages/ocap-kernel/src/store/types.ts b/packages/ocap-kernel/src/store/types.ts index cf382bf89..8f8c8f722 100644 --- a/packages/ocap-kernel/src/store/types.ts +++ b/packages/ocap-kernel/src/store/types.ts @@ -16,6 +16,7 @@ export type StoreContext = { terminatedVats: StoredValue; inCrank: boolean; savepoints: string[]; + compromisedVats: StoredValue; }; export type StoredValue = { diff --git a/vitest.config.ts b/vitest.config.ts index f9034e5c6..8751ab962 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -105,10 +105,10 @@ export default defineConfig({ lines: 24.65, }, 'packages/kernel-rpc-methods/**': { - statements: 64.7, - functions: 60, + statements: 72.54, + functions: 69.23, branches: 44.44, - lines: 64.7, + lines: 72.54, }, 'packages/kernel-shims/**': { statements: 0, @@ -129,8 +129,8 @@ export default defineConfig({ lines: 44, }, 'packages/logger/**': { - statements: 88.4, - functions: 79.16, + statements: 86.95, + functions: 75, branches: 54.05, lines: 89.39, }, From 80748d40e54b1905d2b4d44bf74b187c24374b9c Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 21 May 2025 19:28:16 +0200 Subject: [PATCH 09/25] Fix deliver unresolved promises bug and add tests --- packages/ocap-kernel/src/KernelQueue.test.ts | 9 +- packages/ocap-kernel/src/KernelRouter.test.ts | 303 +++++++++++++++--- packages/ocap-kernel/src/KernelRouter.ts | 65 ++-- packages/ocap-kernel/src/VatHandle.test.ts | 27 +- packages/ocap-kernel/src/VatHandle.ts | 19 +- packages/ocap-kernel/src/VatSyscall.test.ts | 116 ++++++- packages/ocap-kernel/src/store/index.test.ts | 5 + .../src/store/methods/compromised.test.ts | 107 +++++++ .../ocap-kernel/src/store/methods/queue.ts | 2 +- vitest.config.ts | 87 +++-- 10 files changed, 578 insertions(+), 162 deletions(-) create mode 100644 packages/ocap-kernel/src/store/methods/compromised.test.ts diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index abf4f7d5a..13707ee6f 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -26,6 +26,7 @@ describe('KernelQueue', () => { let kernelStore: KernelStore; let kernelQueue: KernelQueue; let mockPromiseKit: ReturnType; + let terminateVat: (vatId: string, reason?: CapData) => Promise; beforeEach(() => { mockPromiseKit = { @@ -34,6 +35,9 @@ describe('KernelQueue', () => { reject: vi.fn(), }; (makePromiseKit as unknown as MockInstance).mockReturnValue(mockPromiseKit); + + terminateVat = vi.fn().mockResolvedValue(undefined); + kernelStore = { nextTerminatedVatCleanup: vi.fn(), collectGarbage: vi.fn(), @@ -49,9 +53,12 @@ describe('KernelQueue', () => { startCrank: vi.fn(), endCrank: vi.fn(), createCrankSavepoint: vi.fn(), + rollbackCrank: vi.fn(), + getRunQueueItemTargetVatId: vi.fn(), + isVatCompromised: vi.fn().mockReturnValue(false), } as unknown as KernelStore; - kernelQueue = new KernelQueue(kernelStore); + kernelQueue = new KernelQueue(kernelStore, terminateVat); }); describe('run', () => { diff --git a/packages/ocap-kernel/src/KernelRouter.test.ts b/packages/ocap-kernel/src/KernelRouter.test.ts index 67984e3f7..2d3fc63dc 100644 --- a/packages/ocap-kernel/src/KernelRouter.test.ts +++ b/packages/ocap-kernel/src/KernelRouter.test.ts @@ -13,6 +13,7 @@ import type { RunQueueItemBringOutYourDead, VatId, GCRunQueueType, + CrankResults, } from './types.ts'; import type { VatHandle } from './VatHandle.ts'; @@ -31,14 +32,16 @@ describe('KernelRouter', () => { let kernelRouter: KernelRouter; beforeEach(() => { - // Mock VatHandle + // Mock VatHandle with more detailed return values + const mockCrankResults: CrankResults = { didDelivery: 'v1' }; + vatHandle = { - deliverMessage: vi.fn().mockResolvedValue(undefined), - deliverNotify: vi.fn().mockResolvedValue(undefined), - deliverDropExports: vi.fn().mockResolvedValue(undefined), - deliverRetireExports: vi.fn().mockResolvedValue(undefined), - deliverRetireImports: vi.fn().mockResolvedValue(undefined), - deliverBringOutYourDead: vi.fn().mockResolvedValue(undefined), + deliverMessage: vi.fn().mockResolvedValue(mockCrankResults), + deliverNotify: vi.fn().mockResolvedValue(mockCrankResults), + deliverDropExports: vi.fn().mockResolvedValue(mockCrankResults), + deliverRetireExports: vi.fn().mockResolvedValue(mockCrankResults), + deliverRetireImports: vi.fn().mockResolvedValue(mockCrankResults), + deliverBringOutYourDead: vi.fn().mockResolvedValue(mockCrankResults), } as unknown as VatHandle; // Mock getVat function @@ -79,16 +82,23 @@ describe('KernelRouter', () => { describe('deliver', () => { describe('send', () => { - it('delivers a send message to a vat with an object target', async () => { + it('delivers a send message to a vat with an object target and returns crank results', async () => { // Setup the kernel store to return an owner for the target const vatId = 'v1'; const target = 'ko123'; (kernelStore.getOwner as unknown as MockInstance).mockReturnValueOnce( vatId, ); - (kernelStore.erefToKref as unknown as MockInstance).mockReturnValueOnce( - 'not-the-target', - ); + + // Create a mock crank result that the vat will return + const mockCrankResults: CrankResults = { + didDelivery: vatId, + abort: false, + }; + ( + vatHandle.deliverMessage as unknown as MockInstance + ).mockResolvedValueOnce(mockCrankResults); + // Create a send message const message: Message = { methargs: { body: 'method args', slots: ['slot1', 'slot2'] }, @@ -99,13 +109,16 @@ describe('KernelRouter', () => { target, message: message as unknown as SwingsetMessage, }; - await kernelRouter.deliver(sendItem); - // Verify the message was delivered to the vat + + const result = await kernelRouter.deliver(sendItem); + + // Verify the message was delivered to the vat and results returned expect(getVat).toHaveBeenCalledWith(vatId); expect(vatHandle.deliverMessage).toHaveBeenCalledWith( `translated-${target}`, message, ); + expect(result).toStrictEqual(mockCrankResults); expect(kernelStore.decrementRefCount).toHaveBeenCalledWith( 'slot1', 'deliver|send|slot', @@ -124,7 +137,7 @@ describe('KernelRouter', () => { ); }); - it('splats a message when target has no owner', async () => { + it('splats a message when target has no owner and returns undefined', async () => { // Setup the kernel store to return no owner for the target (kernelStore.getOwner as unknown as MockInstance).mockReturnValueOnce( null, @@ -141,10 +154,13 @@ describe('KernelRouter', () => { target, message: message as unknown as SwingsetMessage, }; - await kernelRouter.deliver(sendItem); + const result = await kernelRouter.deliver(sendItem); + // Verify the message was not delivered to any vat and resources were cleaned up expect(getVat).not.toHaveBeenCalled(); expect(vatHandle.deliverMessage).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + // Verify refcounts were decremented expect(kernelStore.decrementRefCount).toHaveBeenCalledWith( target, @@ -171,13 +187,14 @@ describe('KernelRouter', () => { ); }); - it('enqueues a message on an unresolved promise', async () => { + it('enqueues a message on an unresolved promise and returns undefined', async () => { // Setup a promise reference and unresolved promise in the kernel store const target = 'kp123'; ( kernelStore.getKernelPromise as unknown as MockInstance ).mockReturnValueOnce({ state: 'unresolved', + value: { body: JSON.stringify({ status: 'unresolved' }), slots: [] }, }); // Create a send message const message: Message = { @@ -189,19 +206,108 @@ describe('KernelRouter', () => { target, message: message as unknown as SwingsetMessage, }; - await kernelRouter.deliver(sendItem); + const result = await kernelRouter.deliver(sendItem); + // Verify the message was enqueued on the promise expect(kernelStore.enqueuePromiseMessage).toHaveBeenCalledWith( target, message, ); + // Verify no vat interaction occurred + expect(getVat).not.toHaveBeenCalled(); + expect(vatHandle.deliverMessage).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + + // Verify that no refcount decrementation happened since we're requeuing + expect(kernelStore.decrementRefCount).not.toHaveBeenCalled(); + }); + + it('splats message when promise resolves to a non-object', async () => { + // Setup a fulfilled promise that doesn't resolve to an object + const promiseId = 'kp123'; + + ( + kernelStore.getKernelPromise as unknown as MockInstance + ).mockReturnValueOnce({ + state: 'fulfilled', + value: { + body: JSON.stringify({ value: 'not an object' }), + slots: [], + }, + }); + + // Create a send message to the promise + const message: Message = { + methargs: { body: 'method args', slots: [] }, + result: 'kp2', + }; + const sendItem: RunQueueItemSend = { + type: 'send', + target: promiseId, + message: message as unknown as SwingsetMessage, + }; + + const result = await kernelRouter.deliver(sendItem); + + // Message should be splatted, not delivered + expect(getVat).not.toHaveBeenCalled(); + expect(vatHandle.deliverMessage).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + + // Verify the result promise was rejected + expect(kernelQueue.resolvePromises).toHaveBeenCalledWith( + undefined, + expect.arrayContaining([ + expect.arrayContaining(['kp2', true, expect.anything()]), + ]), + ); + }); + + it('splats message when promise is rejected', async () => { + // Setup a rejected promise + const promiseId = 'kp123'; + const rejection = { + body: JSON.stringify({ error: 'rejection reason' }), + slots: [], + }; + + ( + kernelStore.getKernelPromise as unknown as MockInstance + ).mockReturnValueOnce({ + state: 'rejected', + value: rejection, + }); + + // Create a send message to the promise + const message: Message = { + methargs: { body: 'method args', slots: [] }, + result: 'kp2', + }; + const sendItem: RunQueueItemSend = { + type: 'send', + target: promiseId, + message: message as unknown as SwingsetMessage, + }; + + const result = await kernelRouter.deliver(sendItem); + + // Message should be splatted, not delivered expect(getVat).not.toHaveBeenCalled(); expect(vatHandle.deliverMessage).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + + // Verify the result promise was rejected with the same reason + expect(kernelQueue.resolvePromises).toHaveBeenCalledWith( + undefined, + expect.arrayContaining([ + expect.arrayContaining(['kp2', true, rejection]), + ]), + ); }); }); describe('notify', () => { - it('delivers a notify to a vat', async () => { + it('delivers a notify to a vat and returns crank results', async () => { const vatId = 'v1'; const kpid = 'kp123'; const notifyItem: RunQueueItemNotify = { @@ -209,30 +315,48 @@ describe('KernelRouter', () => { vatId, kpid, }; + // Mock a resolved promise ( kernelStore.getKernelPromise as unknown as MockInstance ).mockReturnValueOnce({ state: 'fulfilled', - value: { body: 'resolved value', slots: [] }, + value: { + body: JSON.stringify({ value: 'resolved value' }), + slots: [], + }, }); + // Mock that this promise is in the vat's clist (kernelStore.krefToEref as unknown as MockInstance).mockReturnValueOnce( 'p+123', ); + // Mock that there's a promise to retire ( kernelStore.getKpidsToRetire as unknown as MockInstance ).mockReturnValueOnce([kpid]); + // Mock the getKernelPromise for the target promise ( kernelStore.getKernelPromise as unknown as MockInstance ).mockReturnValueOnce({ state: 'fulfilled', - value: { body: 'target promise value', slots: [] }, + value: { + body: JSON.stringify({ value: 'target promise value' }), + slots: [], + }, }); + + // Mock crank results + const mockCrankResults: CrankResults = { didDelivery: vatId }; + ( + vatHandle.deliverNotify as unknown as MockInstance + ).mockResolvedValueOnce(mockCrankResults); + // Deliver the notify - await kernelRouter.deliver(notifyItem); + const result = await kernelRouter.deliver(notifyItem); + // Verify the notification was delivered to the vat expect(getVat).toHaveBeenCalledWith(vatId); expect(vatHandle.deliverNotify).toHaveBeenCalledWith(expect.any(Array)); @@ -240,9 +364,10 @@ describe('KernelRouter', () => { kpid, 'deliver|notify', ); + expect(result).toStrictEqual(mockCrankResults); }); - it('does nothing if the promise is not in vat clist', async () => { + it('returns didDelivery when promise is not in vat clist', async () => { const vatId = 'v1'; const kpid = 'kp123'; const notifyItem: RunQueueItemNotify = { @@ -250,21 +375,90 @@ describe('KernelRouter', () => { vatId, kpid, }; + // Mock a resolved promise ( kernelStore.getKernelPromise as unknown as MockInstance ).mockReturnValueOnce({ state: 'fulfilled', - value: { body: 'resolved value', slots: [] }, + value: { + body: JSON.stringify({ value: 'resolved value' }), + slots: [], + }, }); + // Mock that this promise is NOT in the vat's clist (kernelStore.krefToEref as unknown as MockInstance).mockReturnValueOnce( null, ); + + // Deliver the notify + const result = await kernelRouter.deliver(notifyItem); + + // Verify no notification was delivered to the vat + expect(vatHandle.deliverNotify).not.toHaveBeenCalled(); + expect(result).toStrictEqual({ didDelivery: vatId }); + }); + + it('returns didDelivery when no kpids to retire', async () => { + const vatId = 'v1'; + const kpid = 'kp123'; + const notifyItem: RunQueueItemNotify = { + type: 'notify', + vatId, + kpid, + }; + + // Mock a resolved promise + ( + kernelStore.getKernelPromise as unknown as MockInstance + ).mockReturnValueOnce({ + state: 'fulfilled', + value: { + body: JSON.stringify({ value: 'resolved value' }), + slots: [], + }, + }); + + // Mock that this promise is in the vat's clist + (kernelStore.krefToEref as unknown as MockInstance).mockReturnValueOnce( + 'p+123', + ); + + // Mock that there are no promises to retire + ( + kernelStore.getKpidsToRetire as unknown as MockInstance + ).mockReturnValueOnce([]); + // Deliver the notify - await kernelRouter.deliver(notifyItem); + const result = await kernelRouter.deliver(notifyItem); + // Verify no notification was delivered to the vat expect(vatHandle.deliverNotify).not.toHaveBeenCalled(); + expect(result).toStrictEqual({ didDelivery: vatId }); + }); + + it('throws if notification is for an unresolved promise', async () => { + const vatId = 'v1'; + const kpid = 'kp123'; + const notifyItem: RunQueueItemNotify = { + type: 'notify', + vatId, + kpid, + }; + + // Mock an unresolved promise with no value + ( + kernelStore.getKernelPromise as unknown as MockInstance + ).mockReturnValueOnce({ + state: 'unresolved', + value: null, + }); + + // Deliver the notify should throw with the expected error message + await expect(kernelRouter.deliver(notifyItem)).rejects.toThrow( + 'no value for promise kp123', + ); }); }); @@ -273,36 +467,59 @@ describe('KernelRouter', () => { ['dropExports', 'deliverDropExports'], ['retireExports', 'deliverRetireExports'], ['retireImports', 'deliverRetireImports'], - ])('delivers %s to a vat', async (actionType, deliverMethod) => { - const vatId = 'v1'; - const krefs = ['ko1', 'ko2']; - const gcAction: RunQueueItemGCAction = { - type: actionType as GCRunQueueType, - vatId, - krefs, - }; - // Deliver the GC action - await kernelRouter.deliver(gcAction); - // Verify the action was delivered to the vat - expect(getVat).toHaveBeenCalledWith(vatId); - expect( - vatHandle[deliverMethod as keyof VatHandle], - ).toHaveBeenCalledWith(krefs.map((kref) => `translated-${kref}`)); - }); + ])( + 'delivers %s to a vat and returns crank results', + async (actionType, deliverMethod) => { + const vatId = 'v1'; + const krefs = ['ko1', 'ko2']; + const gcAction: RunQueueItemGCAction = { + type: actionType as GCRunQueueType, + vatId, + krefs, + }; + + // Mock crank results + const mockCrankResults: CrankResults = { didDelivery: vatId }; + ( + vatHandle[ + deliverMethod as keyof VatHandle + ] as unknown as MockInstance + ).mockResolvedValueOnce(mockCrankResults); + + // Deliver the GC action + const result = await kernelRouter.deliver(gcAction); + + // Verify the action was delivered to the vat + expect(getVat).toHaveBeenCalledWith(vatId); + expect( + vatHandle[deliverMethod as keyof VatHandle], + ).toHaveBeenCalledWith(krefs.map((kref) => `translated-${kref}`)); + expect(result).toStrictEqual(mockCrankResults); + }, + ); }); describe('bringOutYourDead', () => { - it('delivers bringOutYourDead to a vat', async () => { + it('delivers bringOutYourDead to a vat and returns crank results', async () => { const vatId = 'v1'; const bringOutYourDeadItem: RunQueueItemBringOutYourDead = { type: 'bringOutYourDead', vatId, }; + + // Mock crank results + const mockCrankResults: CrankResults = { didDelivery: vatId }; + ( + vatHandle.deliverBringOutYourDead as unknown as MockInstance + ).mockResolvedValueOnce(mockCrankResults); + // Deliver the bringOutYourDead action - await kernelRouter.deliver(bringOutYourDeadItem); + const result = await kernelRouter.deliver(bringOutYourDeadItem); + // Verify the action was delivered to the vat expect(getVat).toHaveBeenCalledWith(vatId); expect(vatHandle.deliverBringOutYourDead).toHaveBeenCalled(); + expect(result).toStrictEqual(mockCrankResults); }); }); @@ -310,7 +527,7 @@ describe('KernelRouter', () => { // @ts-expect-error - deliberately using an invalid type const invalidItem: RunQueueItem = { type: 'invalid' }; await expect(kernelRouter.deliver(invalidItem)).rejects.toThrow( - 'unknown run queue item type', + 'unsupported or unknown run queue item type', ); }); }); diff --git a/packages/ocap-kernel/src/KernelRouter.ts b/packages/ocap-kernel/src/KernelRouter.ts index 442593785..b0f4d9180 100644 --- a/packages/ocap-kernel/src/KernelRouter.ts +++ b/packages/ocap-kernel/src/KernelRouter.ts @@ -172,8 +172,9 @@ export class KernelRouter { item: RunQueueItemSend, ): Promise { const route = this.#routeMessage(item); + let crankResults: CrankResults | undefined; - // Message went splat, no longer the kernel's responsibility + // Message went splat if (!route) { this.#kernelStore.decrementRefCount(item.target, 'deliver|splat|target'); if (item.message.result) { @@ -188,7 +189,7 @@ export class KernelRouter { console.log( `@@@@ message went splat ${item.target}<-${JSON.stringify(item.message)}`, ); - return undefined; + return crankResults; } const { vatId, target } = route; @@ -196,37 +197,39 @@ export class KernelRouter { console.log( `@@@@ deliver ${vatId} send ${target}<-${JSON.stringify(message)}`, ); - if (!vatId) { - throw Fail`no owner for kernel object ${target}`; - } - - const vat = this.#getVat(vatId); - - if (!vat) { - // Requeue the message for later delivery. - this.#kernelStore.enqueuePromiseMessage(target, message); - // For requeued messages, there's no immediate crank outcome from a vat. - return undefined; - } - - if (message.result) { - if (typeof message.result !== 'string') { - throw TypeError('message result must be a string'); + if (vatId) { + const vat = this.#getVat(vatId); + if (vat) { + if (message.result) { + if (typeof message.result !== 'string') { + throw TypeError('message result must be a string'); + } + this.#kernelStore.setPromiseDecider(message.result, vatId); + this.#kernelStore.decrementRefCount( + message.result, + 'deliver|send|result', + ); + } + const vatTarget = this.#kernelStore.translateRefKtoV( + vatId, + target, + false, + ); + const vatMessage = this.#kernelStore.translateMessageKtoV( + vatId, + message, + ); + crankResults = await vat.deliverMessage(vatTarget, vatMessage); + this.#kernelStore.decrementRefCount(target, 'deliver|send|target'); + for (const slot of message.methargs.slots) { + this.#kernelStore.decrementRefCount(slot, 'deliver|send|slot'); + } + } else { + Fail`no owner for kernel object ${target}`; } - this.#kernelStore.setPromiseDecider(message.result, vatId); - this.#kernelStore.decrementRefCount( - message.result, - 'deliver|send|result', - ); - } - const vatTarget = this.#kernelStore.translateRefKtoV(vatId, target, false); - const vatMessage = this.#kernelStore.translateMessageKtoV(vatId, message); - const crankResults = await vat.deliverMessage(vatTarget, vatMessage); - this.#kernelStore.decrementRefCount(target, 'deliver|send|target'); - for (const slot of message.methargs.slots) { - this.#kernelStore.decrementRefCount(slot, 'deliver|send|slot'); + } else { + this.#kernelStore.enqueuePromiseMessage(target, message); } - console.log( `@@@@ done ${vatId} send ${target}<-${JSON.stringify(message)}`, ); diff --git a/packages/ocap-kernel/src/VatHandle.test.ts b/packages/ocap-kernel/src/VatHandle.test.ts index d87fed225..342c890aa 100644 --- a/packages/ocap-kernel/src/VatHandle.test.ts +++ b/packages/ocap-kernel/src/VatHandle.test.ts @@ -1,5 +1,4 @@ import type { VatOneResolution } from '@agoric/swingset-liveslots'; -import type { VatCheckpoint } from '@metamask/kernel-store'; import type { JsonRpcMessage } from '@metamask/kernel-utils'; import { isJsonRpcMessage } from '@metamask/kernel-utils'; import type { Logger } from '@metamask/logger'; @@ -12,7 +11,7 @@ import type { MockInstance } from 'vitest'; import type { KernelQueue } from './KernelQueue.ts'; import { makeKernelStore } from './store/index.ts'; import type { KernelStore } from './store/index.ts'; -import type { VRef, Message } from './types.ts'; +import type { VRef, Message, VatDeliveryResult } from './types.ts'; import { VatHandle } from './VatHandle.ts'; import { makeMapKernelDatabase } from '../test/storage.ts'; @@ -123,8 +122,8 @@ describe('VatHandle', () => { it('calls sendVatCommand with the correct method and params', async () => { const { vat } = await makeVat(); sendVatCommandMock.mockReset(); - const mockCheckpoint: VatCheckpoint = [[], []]; - sendVatCommandMock.mockResolvedValueOnce(mockCheckpoint); + const mockResult: VatDeliveryResult = [[[], []], null]; + sendVatCommandMock.mockResolvedValueOnce(mockResult); const target = 'kp1' as VRef; const message: Message = { methargs: { body: '["arg1","arg2"]', slots: [] }, @@ -143,8 +142,8 @@ describe('VatHandle', () => { it('calls sendVatCommand with the correct method and params', async () => { const { vat } = await makeVat(); sendVatCommandMock.mockReset(); - const mockCheckpoint: VatCheckpoint = [[], []]; - sendVatCommandMock.mockResolvedValueOnce(mockCheckpoint); + const mockResult: VatDeliveryResult = [[[], []], null]; + sendVatCommandMock.mockResolvedValueOnce(mockResult); const resolutions: VatOneResolution[] = [ ['vp123', false, { body: '"resolved value"', slots: [] }], ]; @@ -161,8 +160,8 @@ describe('VatHandle', () => { it('calls sendVatCommand with the correct method and params', async () => { const { vat } = await makeVat(); sendVatCommandMock.mockReset(); - const mockCheckpoint: VatCheckpoint = [[], []]; - sendVatCommandMock.mockResolvedValueOnce(mockCheckpoint); + const mockResult: VatDeliveryResult = [[[], []], null]; + sendVatCommandMock.mockResolvedValueOnce(mockResult); const vrefs: VRef[] = ['kp123', 'kp456']; await vat.deliverDropExports(vrefs); expect(sendVatCommandMock).toHaveBeenCalledTimes(1); @@ -177,8 +176,8 @@ describe('VatHandle', () => { it('calls sendVatCommand with the correct method and params', async () => { const { vat } = await makeVat(); sendVatCommandMock.mockReset(); - const mockCheckpoint: VatCheckpoint = [[], []]; - sendVatCommandMock.mockResolvedValueOnce(mockCheckpoint); + const mockResult: VatDeliveryResult = [[[], []], null]; + sendVatCommandMock.mockResolvedValueOnce(mockResult); const vrefs: VRef[] = ['kp123', 'kp456']; await vat.deliverRetireExports(vrefs); expect(sendVatCommandMock).toHaveBeenCalledTimes(1); @@ -193,8 +192,8 @@ describe('VatHandle', () => { it('calls sendVatCommand with the correct method and params', async () => { const { vat } = await makeVat(); sendVatCommandMock.mockReset(); - const mockCheckpoint: VatCheckpoint = [[], []]; - sendVatCommandMock.mockResolvedValueOnce(mockCheckpoint); + const mockResult: VatDeliveryResult = [[[], []], null]; + sendVatCommandMock.mockResolvedValueOnce(mockResult); const vrefs: VRef[] = ['kp123', 'kp456']; await vat.deliverRetireImports(vrefs); expect(sendVatCommandMock).toHaveBeenCalledTimes(1); @@ -209,8 +208,8 @@ describe('VatHandle', () => { it('calls sendVatCommand with the correct method and params', async () => { const { vat } = await makeVat(); sendVatCommandMock.mockReset(); - const mockCheckpoint: VatCheckpoint = [[], []]; - sendVatCommandMock.mockResolvedValueOnce(mockCheckpoint); + const mockResult: VatDeliveryResult = [[[], []], null]; + sendVatCommandMock.mockResolvedValueOnce(mockResult); await vat.deliverBringOutYourDead(); expect(sendVatCommandMock).toHaveBeenCalledTimes(1); expect(sendVatCommandMock).toHaveBeenCalledWith({ diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index 4ad795244..a15b1930a 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -150,7 +150,7 @@ export class VatHandle { }, ); - await this.#sendVatCommand({ + await this.sendVatCommand({ method: 'initVat', params: { vatConfig: this.config, @@ -165,7 +165,7 @@ export class VatHandle { * @returns A promise that resolves to the result of the ping. */ async ping(): Promise { - return await this.#sendVatCommand({ + return await this.sendVatCommand({ method: 'ping', params: [], }); @@ -211,11 +211,10 @@ export class VatHandle { * @returns The crank results. */ async deliverMessage(target: VRef, message: Message): Promise { - const result = await this.#sendVatCommand({ + await this.sendVatCommand({ method: 'deliver', params: ['message', target, message], }); - console.log('deliverMessage result', result); return this.#deliveryCrankResults(); } @@ -226,7 +225,7 @@ export class VatHandle { * @returns The crank results. */ async deliverNotify(resolutions: VatOneResolution[]): Promise { - await this.#sendVatCommand({ + await this.sendVatCommand({ method: 'deliver', params: ['notify', resolutions], }); @@ -240,7 +239,7 @@ export class VatHandle { * @returns The crank results. */ async deliverDropExports(vrefs: VRef[]): Promise { - await this.#sendVatCommand({ + await this.sendVatCommand({ method: 'deliver', params: ['dropExports', vrefs], }); @@ -254,7 +253,7 @@ export class VatHandle { * @returns The crank results. */ async deliverRetireExports(vrefs: VRef[]): Promise { - await this.#sendVatCommand({ + await this.sendVatCommand({ method: 'deliver', params: ['retireExports', vrefs], }); @@ -268,7 +267,7 @@ export class VatHandle { * @returns The crank results. */ async deliverRetireImports(vrefs: VRef[]): Promise { - await this.#sendVatCommand({ + await this.sendVatCommand({ method: 'deliver', params: ['retireImports', vrefs], }); @@ -281,7 +280,7 @@ export class VatHandle { * @returns The crank results. */ async deliverBringOutYourDead(): Promise { - await this.#sendVatCommand({ + await this.sendVatCommand({ method: 'deliver', params: ['bringOutYourDead'], }); @@ -317,7 +316,7 @@ export class VatHandle { * @param payload.params - The parameters to pass to the method. * @returns A promise that resolves the response to the command. */ - async #sendVatCommand({ + async sendVatCommand({ method, params, }: { diff --git a/packages/ocap-kernel/src/VatSyscall.test.ts b/packages/ocap-kernel/src/VatSyscall.test.ts index 652dc3a5d..c2a6253c6 100644 --- a/packages/ocap-kernel/src/VatSyscall.test.ts +++ b/packages/ocap-kernel/src/VatSyscall.test.ts @@ -30,8 +30,13 @@ describe('VatSyscall', () => { clearReachableFlag: vi.fn(), getReachableFlag: vi.fn(), forgetKref: vi.fn(), + getVatConfig: vi.fn(() => ({})), + markVatAsCompromised: vi.fn(), } as unknown as KernelStore; - logger = { debug: vi.fn() } as unknown as Logger; + logger = { + debug: vi.fn(), + error: vi.fn(), + } as unknown as Logger; vatSys = new VatSyscall({ vatId: 'v1', kernelQueue, kernelStore, logger }); }); @@ -93,9 +98,15 @@ describe('VatSyscall', () => { it.each([ ['o+1', 'vat v1 issued invalid syscall dropImports for o+1'], ['p-1', 'vat v1 issued invalid syscall dropImports for p-1'], - ])('throws for invalid ref %s', async (ref, errMsg) => { + ])('returns error for invalid ref %s', async (ref, errMsg) => { + ( + kernelStore.translateSyscallVtoK as unknown as MockInstance + ).mockImplementationOnce(() => { + throw new Error(errMsg); + }); const vso = ['dropImports', [ref]] as unknown as VatSyscallObject; - await expect(vatSys.handleSyscall(vso)).rejects.toThrow(errMsg); + const result = await vatSys.handleSyscall(vso); + expect(result).toStrictEqual(['error', errMsg]); }); }); @@ -109,14 +120,21 @@ describe('VatSyscall', () => { expect(kernelStore.forgetKref).toHaveBeenCalledWith('v1', 'o-1'); }); - it('throws if still reachable', async () => { + it('returns error if still reachable', async () => { ( - kernelStore.getReachableFlag as unknown as MockInstance - ).mockReturnValueOnce(true); + kernelStore.translateSyscallVtoK as unknown as MockInstance + ).mockImplementationOnce(() => { + ( + kernelStore.getReachableFlag as unknown as MockInstance + ).mockReturnValueOnce(true); + throw new Error('syscall.retireImports but o-1 is still reachable'); + }); const vso = ['retireImports', ['o-1']] as unknown as VatSyscallObject; - await expect(vatSys.handleSyscall(vso)).rejects.toThrow( + const result = await vatSys.handleSyscall(vso); + expect(result).toStrictEqual([ + 'error', 'syscall.retireImports but o-1 is still reachable', - ); + ]); }); }); @@ -133,14 +151,21 @@ describe('VatSyscall', () => { ); }); - it('throws for reachable exports', async () => { + it('returns error for reachable exports', async () => { ( - kernelStore.getReachableFlag as unknown as MockInstance - ).mockReturnValueOnce(true); + kernelStore.translateSyscallVtoK as unknown as MockInstance + ).mockImplementationOnce(() => { + ( + kernelStore.getReachableFlag as unknown as MockInstance + ).mockReturnValueOnce(true); + throw new Error('syscall.retireExports but o+1 is still reachable'); + }); const vso = ['retireExports', ['o+1']] as unknown as VatSyscallObject; - await expect(vatSys.handleSyscall(vso)).rejects.toThrow( + const result = await vatSys.handleSyscall(vso); + expect(result).toStrictEqual([ + 'error', 'syscall.retireExports but o+1 is still reachable', - ); + ]); }); it('abandons exports without reachability check', async () => { @@ -152,10 +177,67 @@ describe('VatSyscall', () => { ); }); - it('throws for invalid abandonExports refs', async () => { + it('returns error for invalid abandonExports refs', async () => { + ( + kernelStore.translateSyscallVtoK as unknown as MockInstance + ).mockImplementationOnce(() => { + throw new Error('vat v1 issued invalid syscall abandonExports for o-1'); + }); const vso = ['abandonExports', ['o-1']] as unknown as VatSyscallObject; - await expect(vatSys.handleSyscall(vso)).rejects.toThrow( + const result = await vatSys.handleSyscall(vso); + expect(result).toStrictEqual([ + 'error', 'vat v1 issued invalid syscall abandonExports for o-1', + ]); + }); + }); + + describe('exit syscall', () => { + it('records vat termination request', async () => { + const vso = [ + 'exit', + true, + { message: 'error' }, + ] as unknown as VatSyscallObject; + await vatSys.handleSyscall(vso); + expect(vatSys.vatRequestedTermination).toStrictEqual({ + reject: true, + info: { message: 'error' }, + }); + }); + }); + + describe('error handling', () => { + it('handles vat not found error', async () => { + (kernelStore.getVatConfig as unknown as MockInstance).mockReturnValueOnce( + undefined, + ); + const vso = ['send', 'o+1', {}] as unknown as VatSyscallObject; + const result = await vatSys.handleSyscall(vso); + + expect(result).toStrictEqual(['error', 'vat not found']); + expect(vatSys.illegalSyscall).toBeDefined(); + expect(kernelStore.markVatAsCompromised).toHaveBeenCalledWith('v1'); + expect(logger.error).toHaveBeenCalledWith( + 'Vat v1 marked as compromised due to fatal syscall error: vat not found', + ); + }); + + it('handles general syscall errors', async () => { + const error = new Error('test error'); + ( + kernelStore.translateSyscallVtoK as unknown as MockInstance + ).mockImplementationOnce(() => { + throw error; + }); + + const vso = ['send', 'o+1', {}] as unknown as VatSyscallObject; + const result = await vatSys.handleSyscall(vso); + + expect(result).toStrictEqual(['error', 'test error']); + expect(logger.error).toHaveBeenCalledWith( + 'Fatal syscall error in vat v1', + error, ); }); }); @@ -163,6 +245,10 @@ describe('VatSyscall', () => { describe('invalid or unknown syscalls', () => { it.each([ ['vatstoreGet', 'invalid syscall vatstoreGet'], + ['vatstoreGetNextKey', 'invalid syscall vatstoreGetNextKey'], + ['vatstoreSet', 'invalid syscall vatstoreSet'], + ['vatstoreDelete', 'invalid syscall vatstoreDelete'], + ['callNow', 'invalid syscall callNow'], ['unknownOp', 'unknown syscall unknownOp'], ])('%s should warn', async (op, message) => { const spy = vi.spyOn(console, 'warn').mockImplementation(() => { diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 796dae262..769daad3e 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -47,6 +47,7 @@ describe('kernel store', () => { 'cleanupTerminatedVat', 'clear', 'clearReachableFlag', + 'clearVatCompromisedStatus', 'collectGarbage', 'createCrankSavepoint', 'decRefCount', @@ -67,6 +68,7 @@ describe('kernel store', () => { 'forgetKref', 'forgetTerminatedVat', 'getAllVatRecords', + 'getCompromisedVats', 'getGCActions', 'getImporters', 'getKernelPromise', @@ -85,6 +87,7 @@ describe('kernel store', () => { 'getReachableFlag', 'getRefCount', 'getRootObject', + 'getRunQueueItemTargetVatId', 'getTerminatedVats', 'getVatConfig', 'getVatIDs', @@ -97,12 +100,14 @@ describe('kernel store', () => { 'initKernelPromise', 'isObjectPinned', 'isRootObject', + 'isVatCompromised', 'isVatTerminated', 'kernelRefExists', 'krefToEref', 'krefsToExistingErefs', 'kv', 'makeVatStore', + 'markVatAsCompromised', 'markVatAsTerminated', 'nextReapAction', 'nextTerminatedVatCleanup', diff --git a/packages/ocap-kernel/src/store/methods/compromised.test.ts b/packages/ocap-kernel/src/store/methods/compromised.test.ts new file mode 100644 index 000000000..b000ff9f3 --- /dev/null +++ b/packages/ocap-kernel/src/store/methods/compromised.test.ts @@ -0,0 +1,107 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import type { MockInstance } from 'vitest'; + +import type { StoreContext } from '../types.ts'; +import { getCompromisedMethods } from './compromised.ts'; + +describe('compromised vat methods', () => { + let ctx: StoreContext; + let compromisedVatsMock: { + get: MockInstance; + set: MockInstance; + }; + let methods: ReturnType; + + beforeEach(() => { + // Create mock for the context's compromisedVats + compromisedVatsMock = { + get: vi.fn(), + set: vi.fn(), + }; + + // Initialize the mock context + ctx = { + compromisedVats: compromisedVatsMock, + } as unknown as StoreContext; + + // Initialize with empty compromised vats array + compromisedVatsMock.get.mockReturnValue('[]'); + + // Get the methods to test + methods = getCompromisedMethods(ctx); + }); + + describe('getCompromisedVats', () => { + it('returns an empty array when no vats are compromised', () => { + const result = methods.getCompromisedVats(); + expect(result).toStrictEqual([]); + expect(compromisedVatsMock.get).toHaveBeenCalled(); + }); + + it('returns array of compromised vat IDs', () => { + compromisedVatsMock.get.mockReturnValueOnce('["v1", "v3"]'); + const result = methods.getCompromisedVats(); + expect(result).toStrictEqual(['v1', 'v3']); + }); + + it('handles null return from store by returning empty array', () => { + compromisedVatsMock.get.mockReturnValueOnce(null); + const result = methods.getCompromisedVats(); + expect(result).toStrictEqual([]); + }); + }); + + describe('isVatCompromised', () => { + it('returns false for uncompromised vat', () => { + compromisedVatsMock.get.mockReturnValueOnce('["v1", "v3"]'); + const result = methods.isVatCompromised('v2'); + expect(result).toBe(false); + }); + + it('returns true for compromised vat', () => { + compromisedVatsMock.get.mockReturnValueOnce('["v1", "v3"]'); + const result = methods.isVatCompromised('v3'); + expect(result).toBe(true); + }); + }); + + describe('markVatAsCompromised', () => { + it('adds vat to compromised list when not already marked', () => { + compromisedVatsMock.get.mockReturnValueOnce('["v1"]'); + methods.markVatAsCompromised('v2'); + expect(compromisedVatsMock.set).toHaveBeenCalledWith('["v1","v2"]'); + }); + + it('does not modify list when vat is already marked as compromised', () => { + compromisedVatsMock.get.mockReturnValueOnce('["v1", "v2"]'); + methods.markVatAsCompromised('v2'); + expect(compromisedVatsMock.set).not.toHaveBeenCalled(); + }); + + it('initializes list when none exists', () => { + compromisedVatsMock.get.mockReturnValueOnce(null); + methods.markVatAsCompromised('v1'); + expect(compromisedVatsMock.set).toHaveBeenCalledWith('["v1"]'); + }); + }); + + describe('clearVatCompromisedStatus', () => { + it('removes vat from compromised list', () => { + compromisedVatsMock.get.mockReturnValueOnce('["v1","v2","v3"]'); + methods.clearVatCompromisedStatus('v2'); + expect(compromisedVatsMock.set).toHaveBeenCalledWith('["v1","v3"]'); + }); + + it('does nothing if vat is not in compromised list', () => { + compromisedVatsMock.get.mockReturnValueOnce('["v1","v3"]'); + methods.clearVatCompromisedStatus('v2'); + expect(compromisedVatsMock.set).toHaveBeenCalledWith('["v1","v3"]'); + }); + + it('clears empty list when last vat is removed', () => { + compromisedVatsMock.get.mockReturnValueOnce('["v1"]'); + methods.clearVatCompromisedStatus('v1'); + expect(compromisedVatsMock.set).toHaveBeenCalledWith('[]'); + }); + }); +}); diff --git a/packages/ocap-kernel/src/store/methods/queue.ts b/packages/ocap-kernel/src/store/methods/queue.ts index 9650d481b..cb0c7780f 100644 --- a/packages/ocap-kernel/src/store/methods/queue.ts +++ b/packages/ocap-kernel/src/store/methods/queue.ts @@ -71,7 +71,7 @@ export function getQueueMethods(ctx: StoreContext) { function getRunQueueItemTargetVatId(item: RunQueueItem): VatId | undefined { switch (item.type) { case 'send': { - return getOwner(item.target); + return getOwner(item.target, false); } case 'notify': case 'dropExports': diff --git a/vitest.config.ts b/vitest.config.ts index 8751ab962..6baad30cb 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -68,19 +68,18 @@ export default defineConfig({ thresholds: { autoUpdate: true, 'packages/cli/**': { - statements: 0, - functions: 0, - branches: 0, - lines: 0, + statements: 70, + functions: 66.66, + branches: 88.57, + lines: 70, }, 'packages/create-package/**': { - statements: 0, - functions: 0, - branches: 0, - lines: 0, + statements: 100, + functions: 100, + branches: 100, + lines: 100, }, 'packages/extension/**': { -<<<<<<< HEAD statements: 87.65, functions: 87.79, branches: 82.25, @@ -91,24 +90,18 @@ export default defineConfig({ functions: 78.94, branches: 66.66, lines: 80.45, -======= - statements: 0, - functions: 0, - branches: 0, - lines: 0, ->>>>>>> 5ffe4069 (retrun dispatch delivery errors from VatSupervisor) }, 'packages/kernel-errors/**': { - statements: 24.65, - functions: 4.76, - branches: 0, - lines: 24.65, + statements: 98.63, + functions: 95.23, + branches: 92, + lines: 98.63, }, 'packages/kernel-rpc-methods/**': { - statements: 72.54, - functions: 69.23, - branches: 44.44, - lines: 72.54, + statements: 100, + functions: 100, + branches: 100, + lines: 100, }, 'packages/kernel-shims/**': { statements: 0, @@ -117,40 +110,40 @@ export default defineConfig({ lines: 0, }, 'packages/kernel-store/**': { - statements: 29.2, - functions: 41.66, - branches: 22.5, - lines: 29.31, + statements: 98, + functions: 100, + branches: 91.25, + lines: 97.99, }, 'packages/kernel-utils/**': { - statements: 43.63, - functions: 35.29, - branches: 15.38, - lines: 44, + statements: 100, + functions: 100, + branches: 100, + lines: 100, }, 'packages/logger/**': { - statements: 86.95, - functions: 75, - branches: 54.05, - lines: 89.39, + statements: 98.55, + functions: 95.83, + branches: 97.29, + lines: 100, }, 'packages/nodejs/**': { - statements: 29.62, - functions: 30.76, - branches: 23.07, - lines: 30.18, + statements: 72.22, + functions: 76.92, + branches: 69.23, + lines: 73.58, }, 'packages/ocap-kernel/**': { - statements: 91.58, - functions: 94.96, - branches: 81.89, - lines: 91.56, + statements: 90.66, + functions: 95.16, + branches: 80.57, + lines: 90.63, }, 'packages/streams/**': { - statements: 39.66, - functions: 35.77, - branches: 31.29, - lines: 40.19, + statements: 100, + functions: 100, + branches: 100, + lines: 100, }, }, }, From f5ad6db843efd120e4f6da804d2913326f39c5ae Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 22 May 2025 00:27:34 +0200 Subject: [PATCH 10/25] Fix tests --- .../src/ui/components/ObjectRegistry.test.tsx | 3 + .../src/ui/components/ObjectRegistry.tsx | 11 +- .../ui/components/SendMessageForm.test.tsx | 1 + .../src/ui/hooks/useDatabase.test.ts | 1 + .../src/ui/services/db-parser.test.ts | 2 + .../extension/src/ui/services/db-parser.ts | 6 + packages/extension/src/ui/types.ts | 1 + .../extension/test/e2e/control-panel.test.ts | 9 +- packages/ocap-kernel/src/Kernel.ts | 2 +- packages/ocap-kernel/src/KernelQueue.ts | 10 +- packages/ocap-kernel/src/store/index.ts | 8 +- .../src/store/methods/queue.test.ts | 112 ++++++++++++++++++ vitest.config.ts | 12 +- 13 files changed, 159 insertions(+), 19 deletions(-) diff --git a/packages/extension/src/ui/components/ObjectRegistry.test.tsx b/packages/extension/src/ui/components/ObjectRegistry.test.tsx index f4e5fa41b..89746ffa6 100644 --- a/packages/extension/src/ui/components/ObjectRegistry.test.tsx +++ b/packages/extension/src/ui/components/ObjectRegistry.test.tsx @@ -34,6 +34,7 @@ describe('ObjectRegistry Component', () => { gcActions: 'test gc actions', reapQueue: 'test reap queue', terminatedVats: 'test terminated vats', + compromisedVats: 'test compromised vats', vats: { vat1: { overview: { name: 'TestVat1', bundleSpec: '' }, @@ -125,6 +126,8 @@ describe('ObjectRegistry Component', () => { expect(screen.getByText('test reap queue')).toBeInTheDocument(); expect(screen.getByText('Terminated Vats')).toBeInTheDocument(); expect(screen.getByText('test terminated vats')).toBeInTheDocument(); + expect(screen.getByText('Compromised Vats')).toBeInTheDocument(); + expect(screen.getByText('test compromised vats')).toBeInTheDocument(); }); it('renders vat list with correct number of vats', () => { diff --git a/packages/extension/src/ui/components/ObjectRegistry.tsx b/packages/extension/src/ui/components/ObjectRegistry.tsx index d24aa4d2a..c95ec8f2d 100644 --- a/packages/extension/src/ui/components/ObjectRegistry.tsx +++ b/packages/extension/src/ui/components/ObjectRegistry.tsx @@ -57,15 +57,22 @@ export const ObjectRegistry: React.FC = () => { GC Actions - {objectRegistry.gcActions ?? 'None'} + {objectRegistry.gcActions ?? 'None'} Reap Queue - {objectRegistry.reapQueue ?? 'Empty'} + {objectRegistry.reapQueue ?? 'Empty'} Terminated Vats {objectRegistry.terminatedVats ?? 'None'} + + Compromised Vats + + {objectRegistry.compromisedVats ?? 'None'} diff --git a/packages/extension/src/ui/components/SendMessageForm.test.tsx b/packages/extension/src/ui/components/SendMessageForm.test.tsx index 40211948b..c410b6f41 100644 --- a/packages/extension/src/ui/components/SendMessageForm.test.tsx +++ b/packages/extension/src/ui/components/SendMessageForm.test.tsx @@ -40,6 +40,7 @@ describe('SendMessageForm Component', () => { gcActions: '', reapQueue: '', terminatedVats: '', + compromisedVats: '', vats: { vat1: { overview: { name: 'TestVat1', bundleSpec: '' }, diff --git a/packages/extension/src/ui/hooks/useDatabase.test.ts b/packages/extension/src/ui/hooks/useDatabase.test.ts index fd4a7b952..f6a168054 100644 --- a/packages/extension/src/ui/hooks/useDatabase.test.ts +++ b/packages/extension/src/ui/hooks/useDatabase.test.ts @@ -154,6 +154,7 @@ describe('useDatabase', () => { gcActions: '', reapQueue: '', terminatedVats: '', + compromisedVats: '', vats: {}, }; diff --git a/packages/extension/src/ui/services/db-parser.test.ts b/packages/extension/src/ui/services/db-parser.test.ts index 3bff11289..3b90e813c 100644 --- a/packages/extension/src/ui/services/db-parser.test.ts +++ b/packages/extension/src/ui/services/db-parser.test.ts @@ -11,6 +11,7 @@ describe('parseObjectRegistry', () => { { key: 'gcActions', value: '[]' }, { key: 'reapQueue', value: '[]' }, { key: 'vats.terminated', value: '[]' }, + { key: 'vats.compromised', value: '[]' }, { key: 'nextObjectId', value: '4' }, { key: 'nextPromiseId', value: '4' }, { key: 'nextVatId', value: '4' }, @@ -102,6 +103,7 @@ describe('parseObjectRegistry', () => { gcActions: '[]', reapQueue: '[]', terminatedVats: '[]', + compromisedVats: '[]', vats: { v1: { exportedPromises: [ diff --git a/packages/extension/src/ui/services/db-parser.ts b/packages/extension/src/ui/services/db-parser.ts index 147754f9e..00588188b 100644 --- a/packages/extension/src/ui/services/db-parser.ts +++ b/packages/extension/src/ui/services/db-parser.ts @@ -22,6 +22,7 @@ export function parseObjectRegistry( let gcActions = ''; let reapQueue = ''; let terminatedVats = ''; + let compromisedVats = ''; // 1) Collect for (const { key, value } of entries) { @@ -37,6 +38,10 @@ export function parseObjectRegistry( terminatedVats = value; continue; } + if (key === 'vats.compromised') { + compromisedVats = value; + continue; + } if (key.startsWith('vatConfig.')) { const vat = key.split('.')[1] as string; const config = JSON.parse(value); @@ -176,6 +181,7 @@ export function parseObjectRegistry( gcActions, reapQueue, terminatedVats, + compromisedVats, vats, }; } diff --git a/packages/extension/src/ui/types.ts b/packages/extension/src/ui/types.ts index a741f7296..d651a1f40 100644 --- a/packages/extension/src/ui/types.ts +++ b/packages/extension/src/ui/types.ts @@ -14,6 +14,7 @@ export type ObjectRegistry = { gcActions: string; reapQueue: string; terminatedVats: string; + compromisedVats: string; vats: Record; }; diff --git a/packages/extension/test/e2e/control-panel.test.ts b/packages/extension/test/e2e/control-panel.test.ts index 5614e9f70..ca47909b8 100644 --- a/packages/extension/test/e2e/control-panel.test.ts +++ b/packages/extension/test/e2e/control-panel.test.ts @@ -116,14 +116,13 @@ test.describe('Control Panel', () => { await expect(popupPage.locator('table tr')).toHaveCount(3); }); - // TODO: Fix this test once the ping method is implemented - test.skip('should ping a vat', async () => { + test('should ping a vat', async () => { await expect( popupPage.locator('td button:text("Ping")').first(), ).toBeVisible(); await popupPage.locator('td button:text("Ping")').first().click(); - await expect(messageOutput).toContainText('"method": "ping",'); - await expect(messageOutput).toContainText('{"result":"pong"}'); + await expect(messageOutput).toContainText('"method": "pingVat",'); + await expect(messageOutput).toContainText('pong'); }); test('should terminate all vats', async () => { @@ -147,6 +146,7 @@ test.describe('Control Panel', () => { { key: 'nextPromiseId', value: '4' }, { key: 'nextVatId', value: '4' }, { key: 'nextRemoteId', value: '1' }, + { key: 'vats.compromised', value: '[]' }, { key: 'initialized', value: 'true' }, ]); await expect(messageOutput).toContainText(expectedValues); @@ -170,6 +170,7 @@ test.describe('Control Panel', () => { { key: 'nextPromiseId', value: '1' }, { key: 'nextVatId', value: '1' }, { key: 'nextRemoteId', value: '1' }, + { key: 'vats.compromised', value: '[]' }, ]); await expect(messageOutput).toContainText(expectedValues); await expect(messageOutput).not.toContainText('"initialized":true'); diff --git a/packages/ocap-kernel/src/Kernel.ts b/packages/ocap-kernel/src/Kernel.ts index 2694987cb..665f87fae 100644 --- a/packages/ocap-kernel/src/Kernel.ts +++ b/packages/ocap-kernel/src/Kernel.ts @@ -325,10 +325,10 @@ export class Kernel { terminationError = new VatDeletedError(vatId); } - await vat.terminate(terminating, terminationError); await this.#vatWorkerService .terminate(vatId, terminationError) .catch(this.#logger.error); + await vat.terminate(terminating, terminationError); this.#vats.delete(vatId); } diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index d2f851180..29da0c6de 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -74,13 +74,17 @@ export class KernelQueue { // - consumeMessage=true (for hypothetical one-shot lifecycle ops): // rolls back to 'deliver', discarding the message. // - consumeMessage=false (default for sends/notifies): - // rolls back to 'start', re-queuing the message (e.g., to "splat" against a now-terminated vat, rejecting its result to the sender). - // Currently, consumeMessage defaults to false as such lifecycle ops are not distinct run-queue item types here. + // rolls back to 'start', re-queuing the message + // e.g., to "splat" against a now-terminated vat, rejecting + // its result to the sender. + // Currently, consumeMessage defaults to false as such lifecycle ops + // are not distinct run-queue item types here. this.#kernelStore.rollbackCrank( crankResults.consumeMessage ? 'deliver' : 'start', ); } - // Vat termination during delivery is triggered by an illegal syscall or by syscall.exit() + // Vat termination during delivery is triggered by an illegal syscall + // or by syscall.exit(). if (crankResults?.terminate) { const { vatId, info } = crankResults.terminate; await this.#terminateVat(vatId, info); diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index fa2b383ef..0cbf758e2 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -125,10 +125,10 @@ export function makeKernelStore(kdb: KernelDatabase) { // Garbage collection gcActions: provideCachedStoredValue('gcActions', '[]'), reapQueue: provideCachedStoredValue('reapQueue', '[]'), + compromisedVats: provideCachedStoredValue('vats.compromised', '[]'), terminatedVats: provideCachedStoredValue('vats.terminated', '[]'), inCrank: false, savepoints: [], - compromisedVats: provideCachedStoredValue('compromisedVats', '[]'), }; const id = getIdMethods(context); @@ -181,8 +181,10 @@ export function makeKernelStore(kdb: KernelDatabase) { context.nextPromiseId = provideCachedStoredValue('nextPromiseId', '1'); context.nextVatId = provideCachedStoredValue('nextVatId', '1'); context.nextRemoteId = provideCachedStoredValue('nextRemoteId', '1'); - context.compromisedVats = provideCachedStoredValue('compromisedVats', '[]'); - context.inCrank = false; + context.compromisedVats = provideCachedStoredValue( + 'vats.compromised', + '[]', + ); if (context.savepoints.length > 0) { kdb.releaseSavepoint('t0'); context.savepoints.length = 0; diff --git a/packages/ocap-kernel/src/store/methods/queue.test.ts b/packages/ocap-kernel/src/store/methods/queue.test.ts index d44a3dffb..d8e698249 100644 --- a/packages/ocap-kernel/src/store/methods/queue.test.ts +++ b/packages/ocap-kernel/src/store/methods/queue.test.ts @@ -1,9 +1,15 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; +import * as objectModule from './object.ts'; import { getQueueMethods } from './queue.ts'; import type { RunQueueItem } from '../../types.ts'; import type { StoreContext } from '../types.ts'; +// Mock dependencies +vi.mock('./object.ts', () => ({ + getObjectMethods: vi.fn(), +})); + describe('queue store methods', () => { let mockKV: Map; let mockRunQueue = { @@ -12,14 +18,35 @@ describe('queue store methods', () => { }; let context: StoreContext; let queueMethods: ReturnType; + const mockGetOwner = vi.fn(); beforeEach(() => { + vi.clearAllMocks(); + mockKV = new Map(); mockRunQueue = { enqueue: vi.fn(), dequeue: vi.fn(), }; + // Set up mock implementation for getObjectMethods + (objectModule.getObjectMethods as ReturnType).mockReturnValue( + { + getOwner: mockGetOwner, + }, + ); + + // Configure mock getOwner behavior + mockGetOwner.mockImplementation((target: string) => { + if (target === 'o+1') { + return 'v1'; + } + if (target === 'o+2') { + return 'v2'; + } + return undefined; + }); + context = { kv: { get: (key: string): string | undefined => mockKV.get(key), @@ -212,4 +239,89 @@ describe('queue store methods', () => { expect(() => queueMethods.runQueueLength()).toThrow('unknown queue run'); }); }); + + describe('getRunQueueItemTargetVatId', () => { + it('returns owner vat for send items', () => { + const sendItem: RunQueueItem = { + type: 'send', + target: 'o+1', + message: { methargs: { body: '', slots: [] }, result: null }, + }; + + const result = queueMethods.getRunQueueItemTargetVatId(sendItem); + + expect(mockGetOwner).toHaveBeenCalledWith('o+1', false); + expect(result).toBe('v1'); + }); + + it('returns vatId for notify items', () => { + const notifyItem: RunQueueItem = { + type: 'notify', + vatId: 'v2', + kpid: 'kp123', + }; + + const result = queueMethods.getRunQueueItemTargetVatId(notifyItem); + + expect(result).toBe('v2'); + }); + + it('returns vatId for dropExports items', () => { + const dropExportsItem: RunQueueItem = { + type: 'dropExports', + vatId: 'v3', + krefs: ['o+1', 'o+2'], + }; + + const result = queueMethods.getRunQueueItemTargetVatId(dropExportsItem); + + expect(result).toBe('v3'); + }); + + it('returns vatId for retireExports items', () => { + const retireExportsItem: RunQueueItem = { + type: 'retireExports', + vatId: 'v4', + krefs: ['o+1', 'o+2'], + }; + + const result = queueMethods.getRunQueueItemTargetVatId(retireExportsItem); + + expect(result).toBe('v4'); + }); + + it('returns vatId for retireImports items', () => { + const retireImportsItem: RunQueueItem = { + type: 'retireImports', + vatId: 'v5', + krefs: ['o-1', 'o-2'], + }; + + const result = queueMethods.getRunQueueItemTargetVatId(retireImportsItem); + + expect(result).toBe('v5'); + }); + + it('returns vatId for bringOutYourDead items', () => { + const bringOutYourDeadItem: RunQueueItem = { + type: 'bringOutYourDead', + vatId: 'v6', + }; + + const result = + queueMethods.getRunQueueItemTargetVatId(bringOutYourDeadItem); + + expect(result).toBe('v6'); + }); + + it('returns undefined for unknown item types', () => { + const unknownItem = { + type: 'unknown', + } as unknown as RunQueueItem; + + const result = queueMethods.getRunQueueItemTargetVatId(unknownItem); + + expect(result).toBeUndefined(); + }); + }); }); diff --git a/vitest.config.ts b/vitest.config.ts index 6baad30cb..549123c4c 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -80,10 +80,10 @@ export default defineConfig({ lines: 100, }, 'packages/extension/**': { - statements: 87.65, + statements: 87.75, functions: 87.79, - branches: 82.25, - lines: 87.67, + branches: 82.12, + lines: 87.78, }, 'packages/kernel-browser-runtime/**': { statements: 80.45, @@ -134,10 +134,10 @@ export default defineConfig({ lines: 73.58, }, 'packages/ocap-kernel/**': { - statements: 90.66, + statements: 90.72, functions: 95.16, - branches: 80.57, - lines: 90.63, + branches: 80.73, + lines: 90.69, }, 'packages/streams/**': { statements: 100, From b521b891c2133cbc8024b3d81a13e1f6374fda53 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 22 May 2025 10:56:24 +0200 Subject: [PATCH 11/25] Prevent updating vat kvstore on delivery errors --- packages/ocap-kernel/src/Kernel.ts | 1 + packages/ocap-kernel/src/KernelQueue.ts | 5 ++++ packages/ocap-kernel/src/VatHandle.ts | 5 +++- packages/ocap-kernel/src/store/index.test.ts | 1 + .../ocap-kernel/src/store/methods/vat.test.ts | 26 +++++++++++++++++++ packages/ocap-kernel/src/store/methods/vat.ts | 11 ++++++++ vitest.config.ts | 8 +++--- 7 files changed, 52 insertions(+), 5 deletions(-) diff --git a/packages/ocap-kernel/src/Kernel.ts b/packages/ocap-kernel/src/Kernel.ts index 665f87fae..9687b8aba 100644 --- a/packages/ocap-kernel/src/Kernel.ts +++ b/packages/ocap-kernel/src/Kernel.ts @@ -343,6 +343,7 @@ export class Kernel { this.#kernelStore.deleteVatConfig(vatId); // Mark for deletion (which will happen later, in vat-cleanup events) this.#kernelStore.markVatAsTerminated(vatId); + this.#kernelStore.clearVatCompromisedStatus(vatId); } /** diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index 29da0c6de..c53d25345 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -268,6 +268,11 @@ export class KernelQueue { item: RunQueueItem, vatId: VatId, ): Promise { + if (!this.#kernelStore.isVatActive(vatId)) { + this.#kernelStore.clearVatCompromisedStatus(vatId); + return; + } + // Ensure the vat is fully terminated if not already done await this.#terminateVat( vatId, diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index a15b1930a..75bc7d9ad 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -326,8 +326,11 @@ export class VatHandle { const result = await this.#rpcClient.call(method, params); if (method === 'initVat' || method === 'deliver') { const [[sets, deletes], deliveryError] = result as VatDeliveryResult; - this.#vatStore.updateKVData(sets, deletes); this.#vatSyscall.deliveryError = deliveryError ?? undefined; + const noErrors = !deliveryError && !this.#vatSyscall.illegalSyscall; + if (noErrors) { + this.#vatStore.updateKVData(sets, deletes); + } } return result; } diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 769daad3e..6ad4f7caa 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -100,6 +100,7 @@ describe('kernel store', () => { 'initKernelPromise', 'isObjectPinned', 'isRootObject', + 'isVatActive', 'isVatCompromised', 'isVatTerminated', 'kernelRefExists', diff --git a/packages/ocap-kernel/src/store/methods/vat.test.ts b/packages/ocap-kernel/src/store/methods/vat.test.ts index d86c0c35d..4a9ec6906 100644 --- a/packages/ocap-kernel/src/store/methods/vat.test.ts +++ b/packages/ocap-kernel/src/store/methods/vat.test.ts @@ -509,4 +509,30 @@ describe('vat store methods', () => { ); }); }); + + describe('isVatActive', () => { + it('returns true when vat configuration exists', () => { + mockKV.set(`vatConfig.${vatID1}`, JSON.stringify(vatConfig1)); + + const result = vatMethods.isVatActive(vatID1); + + expect(result).toBe(true); + }); + + it('returns false when vat configuration does not exist', () => { + const result = vatMethods.isVatActive(vatID1); + + expect(result).toBe(false); + }); + + it('returns false after vat configuration is deleted', () => { + mockKV.set(`vatConfig.${vatID1}`, JSON.stringify(vatConfig1)); + expect(vatMethods.isVatActive(vatID1)).toBe(true); + + mockKV.delete(`vatConfig.${vatID1}`); + + const result = vatMethods.isVatActive(vatID1); + expect(result).toBe(false); + }); + }); }); diff --git a/packages/ocap-kernel/src/store/methods/vat.ts b/packages/ocap-kernel/src/store/methods/vat.ts index 8850095eb..4d29d970a 100644 --- a/packages/ocap-kernel/src/store/methods/vat.ts +++ b/packages/ocap-kernel/src/store/methods/vat.ts @@ -91,6 +91,16 @@ export function getVatMethods(ctx: StoreContext) { ) as VatConfig; } + /** + * Check if a vat is active. + * + * @param vatID - The ID of the vat to check. + * @returns True if the vat is active, false otherwise. + */ + function isVatActive(vatID: VatId): boolean { + return kv.get(`${VAT_CONFIG_BASE}${vatID}`) !== undefined; + } + /** * Store the configuration for a vat. * @@ -349,5 +359,6 @@ export function getVatMethods(ctx: StoreContext) { cleanupTerminatedVat, nextTerminatedVatCleanup, exportFromVat, + isVatActive, }; } diff --git a/vitest.config.ts b/vitest.config.ts index 549123c4c..2d963564a 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -134,10 +134,10 @@ export default defineConfig({ lines: 73.58, }, 'packages/ocap-kernel/**': { - statements: 90.72, - functions: 95.16, - branches: 80.73, - lines: 90.69, + statements: 90.57, + functions: 95.18, + branches: 80.44, + lines: 90.54, }, 'packages/streams/**': { statements: 100, From 92aff19cbe9df87d1dd880e26154204be2dedb5f Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 22 May 2025 14:36:22 +0200 Subject: [PATCH 12/25] Remove compromised vat logic and wait for outstanding syscalls --- .../src/ui/components/ObjectRegistry.test.tsx | 3 - .../src/ui/components/ObjectRegistry.tsx | 11 +- .../ui/components/SendMessageForm.test.tsx | 1 - .../src/ui/hooks/useDatabase.test.ts | 1 - .../src/ui/services/db-parser.test.ts | 2 - .../extension/src/ui/services/db-parser.ts | 6 - packages/extension/src/ui/types.ts | 1 - .../extension/test/e2e/control-panel.test.ts | 2 - packages/ocap-kernel/src/Kernel.test.ts | 1 - packages/ocap-kernel/src/Kernel.ts | 1 - packages/ocap-kernel/src/KernelQueue.test.ts | 1 - packages/ocap-kernel/src/KernelQueue.ts | 44 +------ packages/ocap-kernel/src/VatHandle.ts | 10 +- packages/ocap-kernel/src/VatSupervisor.ts | 23 ++-- packages/ocap-kernel/src/VatSyscall.test.ts | 5 - packages/ocap-kernel/src/VatSyscall.ts | 27 ++++- packages/ocap-kernel/src/store/index.test.ts | 4 - packages/ocap-kernel/src/store/index.ts | 9 -- .../src/store/methods/compromised.test.ts | 107 ------------------ .../src/store/methods/compromised.ts | 61 ---------- packages/ocap-kernel/src/store/types.ts | 1 - vitest.config.ts | 14 +-- 22 files changed, 50 insertions(+), 285 deletions(-) delete mode 100644 packages/ocap-kernel/src/store/methods/compromised.test.ts delete mode 100644 packages/ocap-kernel/src/store/methods/compromised.ts diff --git a/packages/extension/src/ui/components/ObjectRegistry.test.tsx b/packages/extension/src/ui/components/ObjectRegistry.test.tsx index 89746ffa6..f4e5fa41b 100644 --- a/packages/extension/src/ui/components/ObjectRegistry.test.tsx +++ b/packages/extension/src/ui/components/ObjectRegistry.test.tsx @@ -34,7 +34,6 @@ describe('ObjectRegistry Component', () => { gcActions: 'test gc actions', reapQueue: 'test reap queue', terminatedVats: 'test terminated vats', - compromisedVats: 'test compromised vats', vats: { vat1: { overview: { name: 'TestVat1', bundleSpec: '' }, @@ -126,8 +125,6 @@ describe('ObjectRegistry Component', () => { expect(screen.getByText('test reap queue')).toBeInTheDocument(); expect(screen.getByText('Terminated Vats')).toBeInTheDocument(); expect(screen.getByText('test terminated vats')).toBeInTheDocument(); - expect(screen.getByText('Compromised Vats')).toBeInTheDocument(); - expect(screen.getByText('test compromised vats')).toBeInTheDocument(); }); it('renders vat list with correct number of vats', () => { diff --git a/packages/extension/src/ui/components/ObjectRegistry.tsx b/packages/extension/src/ui/components/ObjectRegistry.tsx index c95ec8f2d..d24aa4d2a 100644 --- a/packages/extension/src/ui/components/ObjectRegistry.tsx +++ b/packages/extension/src/ui/components/ObjectRegistry.tsx @@ -57,22 +57,15 @@ export const ObjectRegistry: React.FC = () => { GC Actions - {objectRegistry.gcActions ?? 'None'} + {objectRegistry.gcActions ?? 'None'} Reap Queue - {objectRegistry.reapQueue ?? 'Empty'} + {objectRegistry.reapQueue ?? 'Empty'} Terminated Vats {objectRegistry.terminatedVats ?? 'None'} - - Compromised Vats - - {objectRegistry.compromisedVats ?? 'None'} diff --git a/packages/extension/src/ui/components/SendMessageForm.test.tsx b/packages/extension/src/ui/components/SendMessageForm.test.tsx index c410b6f41..40211948b 100644 --- a/packages/extension/src/ui/components/SendMessageForm.test.tsx +++ b/packages/extension/src/ui/components/SendMessageForm.test.tsx @@ -40,7 +40,6 @@ describe('SendMessageForm Component', () => { gcActions: '', reapQueue: '', terminatedVats: '', - compromisedVats: '', vats: { vat1: { overview: { name: 'TestVat1', bundleSpec: '' }, diff --git a/packages/extension/src/ui/hooks/useDatabase.test.ts b/packages/extension/src/ui/hooks/useDatabase.test.ts index f6a168054..fd4a7b952 100644 --- a/packages/extension/src/ui/hooks/useDatabase.test.ts +++ b/packages/extension/src/ui/hooks/useDatabase.test.ts @@ -154,7 +154,6 @@ describe('useDatabase', () => { gcActions: '', reapQueue: '', terminatedVats: '', - compromisedVats: '', vats: {}, }; diff --git a/packages/extension/src/ui/services/db-parser.test.ts b/packages/extension/src/ui/services/db-parser.test.ts index 3b90e813c..3bff11289 100644 --- a/packages/extension/src/ui/services/db-parser.test.ts +++ b/packages/extension/src/ui/services/db-parser.test.ts @@ -11,7 +11,6 @@ describe('parseObjectRegistry', () => { { key: 'gcActions', value: '[]' }, { key: 'reapQueue', value: '[]' }, { key: 'vats.terminated', value: '[]' }, - { key: 'vats.compromised', value: '[]' }, { key: 'nextObjectId', value: '4' }, { key: 'nextPromiseId', value: '4' }, { key: 'nextVatId', value: '4' }, @@ -103,7 +102,6 @@ describe('parseObjectRegistry', () => { gcActions: '[]', reapQueue: '[]', terminatedVats: '[]', - compromisedVats: '[]', vats: { v1: { exportedPromises: [ diff --git a/packages/extension/src/ui/services/db-parser.ts b/packages/extension/src/ui/services/db-parser.ts index 00588188b..147754f9e 100644 --- a/packages/extension/src/ui/services/db-parser.ts +++ b/packages/extension/src/ui/services/db-parser.ts @@ -22,7 +22,6 @@ export function parseObjectRegistry( let gcActions = ''; let reapQueue = ''; let terminatedVats = ''; - let compromisedVats = ''; // 1) Collect for (const { key, value } of entries) { @@ -38,10 +37,6 @@ export function parseObjectRegistry( terminatedVats = value; continue; } - if (key === 'vats.compromised') { - compromisedVats = value; - continue; - } if (key.startsWith('vatConfig.')) { const vat = key.split('.')[1] as string; const config = JSON.parse(value); @@ -181,7 +176,6 @@ export function parseObjectRegistry( gcActions, reapQueue, terminatedVats, - compromisedVats, vats, }; } diff --git a/packages/extension/src/ui/types.ts b/packages/extension/src/ui/types.ts index d651a1f40..a741f7296 100644 --- a/packages/extension/src/ui/types.ts +++ b/packages/extension/src/ui/types.ts @@ -14,7 +14,6 @@ export type ObjectRegistry = { gcActions: string; reapQueue: string; terminatedVats: string; - compromisedVats: string; vats: Record; }; diff --git a/packages/extension/test/e2e/control-panel.test.ts b/packages/extension/test/e2e/control-panel.test.ts index ca47909b8..c3d1e22fb 100644 --- a/packages/extension/test/e2e/control-panel.test.ts +++ b/packages/extension/test/e2e/control-panel.test.ts @@ -146,7 +146,6 @@ test.describe('Control Panel', () => { { key: 'nextPromiseId', value: '4' }, { key: 'nextVatId', value: '4' }, { key: 'nextRemoteId', value: '1' }, - { key: 'vats.compromised', value: '[]' }, { key: 'initialized', value: 'true' }, ]); await expect(messageOutput).toContainText(expectedValues); @@ -170,7 +169,6 @@ test.describe('Control Panel', () => { { key: 'nextPromiseId', value: '1' }, { key: 'nextVatId', value: '1' }, { key: 'nextRemoteId', value: '1' }, - { key: 'vats.compromised', value: '[]' }, ]); await expect(messageOutput).toContainText(expectedValues); await expect(messageOutput).not.toContainText('"initialized":true'); diff --git a/packages/ocap-kernel/src/Kernel.test.ts b/packages/ocap-kernel/src/Kernel.test.ts index baf85a236..b30c6bd0d 100644 --- a/packages/ocap-kernel/src/Kernel.test.ts +++ b/packages/ocap-kernel/src/Kernel.test.ts @@ -335,7 +335,6 @@ describe('Kernel', () => { await kernel.launchVat(config); const vats = kernel.getVats(); expect(vats).toHaveLength(1); - console.log(vats); expect(vats).toStrictEqual([ { id: 'v1', diff --git a/packages/ocap-kernel/src/Kernel.ts b/packages/ocap-kernel/src/Kernel.ts index 9687b8aba..665f87fae 100644 --- a/packages/ocap-kernel/src/Kernel.ts +++ b/packages/ocap-kernel/src/Kernel.ts @@ -343,7 +343,6 @@ export class Kernel { this.#kernelStore.deleteVatConfig(vatId); // Mark for deletion (which will happen later, in vat-cleanup events) this.#kernelStore.markVatAsTerminated(vatId); - this.#kernelStore.clearVatCompromisedStatus(vatId); } /** diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index 13707ee6f..ef5b18039 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -55,7 +55,6 @@ describe('KernelQueue', () => { createCrankSavepoint: vi.fn(), rollbackCrank: vi.fn(), getRunQueueItemTargetVatId: vi.fn(), - isVatCompromised: vi.fn().mockReturnValue(false), } as unknown as KernelStore; kernelQueue = new KernelQueue(kernelStore, terminateVat); diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index c53d25345..740ee1ca7 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -3,7 +3,7 @@ import type { CapData } from '@endo/marshal'; import { makePromiseKit } from '@endo/promise-kit'; import { processGCActionSet } from './services/garbage-collection.ts'; -import { kser, makeError } from './services/kernel-marshal.ts'; +import { kser } from './services/kernel-marshal.ts'; import type { KernelStore } from './store/index.ts'; import { insistVatId } from './types.ts'; import type { @@ -59,15 +59,6 @@ export class KernelQueue { ): Promise { for await (const item of this.#runQueueItems()) { this.#kernelStore.nextTerminatedVatCleanup(); - - // Check if the target vat is compromised before attempting delivery - const targetVatId = this.#kernelStore.getRunQueueItemTargetVatId(item); - if (targetVatId && this.#kernelStore.isVatCompromised(targetVatId)) { - await this.#terminateCompromisedVat(item, targetVatId); - this.#kernelStore.collectGarbage(); - continue; - } - this.#kernelStore.createCrankSavepoint('deliver'); const crankResults = await deliver(item); if (crankResults?.abort) { @@ -257,37 +248,4 @@ export class KernelQueue { } } } - - /** - * Terminate a compromised vat and reject any pending promises. - * - * @param item - The run queue item that triggered the termination. - * @param vatId - The ID of the compromised vat. - */ - async #terminateCompromisedVat( - item: RunQueueItem, - vatId: VatId, - ): Promise { - if (!this.#kernelStore.isVatActive(vatId)) { - this.#kernelStore.clearVatCompromisedStatus(vatId); - return; - } - - // Ensure the vat is fully terminated if not already done - await this.#terminateVat( - vatId, - makeError(`Vat ${vatId} terminated due to prior syscall error.`), - ); - - // If the item was a 'send' with a result promise, reject that promise - if (item.type === 'send' && item.message.result) { - this.resolvePromises(undefined, [ - [ - item.message.result, - true, // rejected - makeError(`Vat ${vatId} terminated.`), - ], - ]); - } - } } diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index 75bc7d9ad..54c53b84a 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -211,6 +211,7 @@ export class VatHandle { * @returns The crank results. */ async deliverMessage(target: VRef, message: Message): Promise { + this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['message', target, message], @@ -225,6 +226,7 @@ export class VatHandle { * @returns The crank results. */ async deliverNotify(resolutions: VatOneResolution[]): Promise { + this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['notify', resolutions], @@ -239,6 +241,7 @@ export class VatHandle { * @returns The crank results. */ async deliverDropExports(vrefs: VRef[]): Promise { + this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['dropExports', vrefs], @@ -253,6 +256,7 @@ export class VatHandle { * @returns The crank results. */ async deliverRetireExports(vrefs: VRef[]): Promise { + this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['retireExports', vrefs], @@ -267,6 +271,7 @@ export class VatHandle { * @returns The crank results. */ async deliverRetireImports(vrefs: VRef[]): Promise { + this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['retireImports', vrefs], @@ -280,6 +285,7 @@ export class VatHandle { * @returns The crank results. */ async deliverBringOutYourDead(): Promise { + this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['bringOutYourDead'], @@ -340,7 +346,9 @@ export class VatHandle { * * @returns The crank outcome. */ - #deliveryCrankResults(): CrankResults { + async #deliveryCrankResults(): Promise { + await this.#vatSyscall.waitForSyscallsToComplete(); + const results: CrankResults = { didDelivery: this.vatId, }; diff --git a/packages/ocap-kernel/src/VatSupervisor.ts b/packages/ocap-kernel/src/VatSupervisor.ts index 8d4252fec..440890f6d 100644 --- a/packages/ocap-kernel/src/VatSupervisor.ts +++ b/packages/ocap-kernel/src/VatSupervisor.ts @@ -179,18 +179,13 @@ export class VatSupervisor { executeSyscall(vso: VatSyscallObject): VatSyscallResult { this.#syscallsInFlight.push( // IMPORTANT: Syscall architecture design explanation: - // 1. Vats operate on an "optimistic execution" model - they send syscalls and continue execution + // - Vats operate on an "optimistic execution" model - they send syscalls and continue execution // without waiting for responses, assuming success. - // 2. The Kernel processes syscalls asynchronously, and responses often arrive after the current crank is complete. - // 3. At the end of each crank (in #deliver), we explicitly reject all in-flight syscalls using - // this.#rpcClient.rejectAll() because: - // - The vat model requires synchronous syscall returns but asynchronous actual execution - // - Late-arriving responses would cause unexpected state changes if handled mid-crank - // - This gives us a clean slate for the next crank - // 4. We catch these rejections here to prevent unhandled promise rejections, as they're an - // expected part of the architecture, not errors + // - The Kernel processes syscalls asynchronously and failures are catched in VatHandle. this.#rpcClient .call('syscall', coerceVatSyscallObject(vso)) + // We catch these rejections here to prevent unhandled promise rejections, + // as they're an expected part of the architecture, not errors .catch(() => undefined), ); return ['ok', null]; @@ -210,13 +205,11 @@ export class VatSupervisor { deliveryError = error instanceof Error ? error.message : String(error); this.#logger.error(`Delivery error in vat ${this.id}:`, deliveryError); } finally { - // Clean up at the end of a crank: - // 1. Clear the syscallsInFlight array to prevent memory leaks + // Clean up at the end of a crank this.#syscallsInFlight.length = 0; - // 2. Reject all pending RPC requests to maintain the optimistic execution model - // This prevents late responses from affecting the vat in unexpected ways - // between cranks. Any actual responses that arrive later will be ignored - // since their message IDs will no longer be in the unresolvedMessages map. + // Reject all pending RPC requests to maintain the optimistic execution model + // This prevents late responses from affecting the vat in unexpected ways + // between cranks. this.#rpcClient.rejectAll(new Error('end of crank')); } diff --git a/packages/ocap-kernel/src/VatSyscall.test.ts b/packages/ocap-kernel/src/VatSyscall.test.ts index c2a6253c6..13d82235d 100644 --- a/packages/ocap-kernel/src/VatSyscall.test.ts +++ b/packages/ocap-kernel/src/VatSyscall.test.ts @@ -31,7 +31,6 @@ describe('VatSyscall', () => { getReachableFlag: vi.fn(), forgetKref: vi.fn(), getVatConfig: vi.fn(() => ({})), - markVatAsCompromised: vi.fn(), } as unknown as KernelStore; logger = { debug: vi.fn(), @@ -217,10 +216,6 @@ describe('VatSyscall', () => { expect(result).toStrictEqual(['error', 'vat not found']); expect(vatSys.illegalSyscall).toBeDefined(); - expect(kernelStore.markVatAsCompromised).toHaveBeenCalledWith('v1'); - expect(logger.error).toHaveBeenCalledWith( - 'Vat v1 marked as compromised due to fatal syscall error: vat not found', - ); }); it('handles general syscall errors', async () => { diff --git a/packages/ocap-kernel/src/VatSyscall.ts b/packages/ocap-kernel/src/VatSyscall.ts index 557145969..48da14a76 100644 --- a/packages/ocap-kernel/src/VatSyscall.ts +++ b/packages/ocap-kernel/src/VatSyscall.ts @@ -4,6 +4,7 @@ import type { VatSyscallObject, VatSyscallResult, } from '@agoric/swingset-liveslots'; +import { delay } from '@metamask/kernel-utils'; import { Logger } from '@metamask/logger'; import type { KernelQueue } from './KernelQueue.ts'; @@ -50,6 +51,9 @@ export class VatSyscall { | { reject: boolean; info: SwingSetCapData } | undefined; + /** The pending syscalls that were received from the vat */ + pendingSyscalls: number = 0; + /** * Construct a new VatSyscall instance. * @@ -179,6 +183,7 @@ export class VatSyscall { try { this.illegalSyscall = undefined; this.vatRequestedTermination = undefined; + this.pendingSyscalls += 1; // This is a safety check - this case should never happen if (!this.#kernelStore.getVatConfig(this.vatId)) { @@ -276,6 +281,8 @@ export class VatSyscall { 'error', error instanceof Error ? error.message : String(error), ]); + } finally { + this.pendingSyscalls -= 1; } } @@ -286,9 +293,21 @@ export class VatSyscall { */ #vatFatalSyscall(error: string): void { this.illegalSyscall = { vatId: this.vatId, info: makeError(error) }; - this.#kernelStore.markVatAsCompromised(this.vatId); - this.#logger.error( - `Vat ${this.vatId} marked as compromised due to fatal syscall error: ${error}`, - ); + } + + /** + * Reset the syscall counts. + */ + resetSyscallCounts(): void { + this.pendingSyscalls = 0; + } + + /** + * Wait for all syscalls to complete. + */ + async waitForSyscallsToComplete(): Promise { + while (this.pendingSyscalls > 0) { + await delay(10); + } } } diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 6ad4f7caa..56f4ca319 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -47,7 +47,6 @@ describe('kernel store', () => { 'cleanupTerminatedVat', 'clear', 'clearReachableFlag', - 'clearVatCompromisedStatus', 'collectGarbage', 'createCrankSavepoint', 'decRefCount', @@ -68,7 +67,6 @@ describe('kernel store', () => { 'forgetKref', 'forgetTerminatedVat', 'getAllVatRecords', - 'getCompromisedVats', 'getGCActions', 'getImporters', 'getKernelPromise', @@ -101,14 +99,12 @@ describe('kernel store', () => { 'isObjectPinned', 'isRootObject', 'isVatActive', - 'isVatCompromised', 'isVatTerminated', 'kernelRefExists', 'krefToEref', 'krefsToExistingErefs', 'kv', 'makeVatStore', - 'markVatAsCompromised', 'markVatAsTerminated', 'nextReapAction', 'nextTerminatedVatCleanup', diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 0cbf758e2..3a2bce01f 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -60,7 +60,6 @@ import type { KernelDatabase, KVStore, VatStore } from '@metamask/kernel-store'; import type { KRef, VatId } from '../types.ts'; import { getBaseMethods } from './methods/base.ts'; import { getCListMethods } from './methods/clist.ts'; -import { getCompromisedMethods } from './methods/compromised.ts'; import { getCrankMethods } from './methods/crank.ts'; import { getGCMethods } from './methods/gc.ts'; import { getIdMethods } from './methods/id.ts'; @@ -125,7 +124,6 @@ export function makeKernelStore(kdb: KernelDatabase) { // Garbage collection gcActions: provideCachedStoredValue('gcActions', '[]'), reapQueue: provideCachedStoredValue('reapQueue', '[]'), - compromisedVats: provideCachedStoredValue('vats.compromised', '[]'), terminatedVats: provideCachedStoredValue('vats.terminated', '[]'), inCrank: false, savepoints: [], @@ -143,7 +141,6 @@ export function makeKernelStore(kdb: KernelDatabase) { const translators = getTranslators(context); const pinned = getPinMethods(context); const crank = getCrankMethods(context, kdb); - const compromised = getCompromisedMethods(context); /** * Create a new VatStore for a vat. @@ -163,7 +160,6 @@ export function makeKernelStore(kdb: KernelDatabase) { */ function deleteVat(vatId: VatId): void { vat.deleteVatConfig(vatId); - compromised.clearVatCompromisedStatus(vatId); kdb.deleteVatStore(vatId); } @@ -181,10 +177,6 @@ export function makeKernelStore(kdb: KernelDatabase) { context.nextPromiseId = provideCachedStoredValue('nextPromiseId', '1'); context.nextVatId = provideCachedStoredValue('nextVatId', '1'); context.nextRemoteId = provideCachedStoredValue('nextRemoteId', '1'); - context.compromisedVats = provideCachedStoredValue( - 'vats.compromised', - '[]', - ); if (context.savepoints.length > 0) { kdb.releaseSavepoint('t0'); context.savepoints.length = 0; @@ -211,7 +203,6 @@ export function makeKernelStore(kdb: KernelDatabase) { ...translators, ...pinned, ...crank, - ...compromised, makeVatStore, deleteVat, clear, diff --git a/packages/ocap-kernel/src/store/methods/compromised.test.ts b/packages/ocap-kernel/src/store/methods/compromised.test.ts deleted file mode 100644 index b000ff9f3..000000000 --- a/packages/ocap-kernel/src/store/methods/compromised.test.ts +++ /dev/null @@ -1,107 +0,0 @@ -import { describe, it, expect, beforeEach, vi } from 'vitest'; -import type { MockInstance } from 'vitest'; - -import type { StoreContext } from '../types.ts'; -import { getCompromisedMethods } from './compromised.ts'; - -describe('compromised vat methods', () => { - let ctx: StoreContext; - let compromisedVatsMock: { - get: MockInstance; - set: MockInstance; - }; - let methods: ReturnType; - - beforeEach(() => { - // Create mock for the context's compromisedVats - compromisedVatsMock = { - get: vi.fn(), - set: vi.fn(), - }; - - // Initialize the mock context - ctx = { - compromisedVats: compromisedVatsMock, - } as unknown as StoreContext; - - // Initialize with empty compromised vats array - compromisedVatsMock.get.mockReturnValue('[]'); - - // Get the methods to test - methods = getCompromisedMethods(ctx); - }); - - describe('getCompromisedVats', () => { - it('returns an empty array when no vats are compromised', () => { - const result = methods.getCompromisedVats(); - expect(result).toStrictEqual([]); - expect(compromisedVatsMock.get).toHaveBeenCalled(); - }); - - it('returns array of compromised vat IDs', () => { - compromisedVatsMock.get.mockReturnValueOnce('["v1", "v3"]'); - const result = methods.getCompromisedVats(); - expect(result).toStrictEqual(['v1', 'v3']); - }); - - it('handles null return from store by returning empty array', () => { - compromisedVatsMock.get.mockReturnValueOnce(null); - const result = methods.getCompromisedVats(); - expect(result).toStrictEqual([]); - }); - }); - - describe('isVatCompromised', () => { - it('returns false for uncompromised vat', () => { - compromisedVatsMock.get.mockReturnValueOnce('["v1", "v3"]'); - const result = methods.isVatCompromised('v2'); - expect(result).toBe(false); - }); - - it('returns true for compromised vat', () => { - compromisedVatsMock.get.mockReturnValueOnce('["v1", "v3"]'); - const result = methods.isVatCompromised('v3'); - expect(result).toBe(true); - }); - }); - - describe('markVatAsCompromised', () => { - it('adds vat to compromised list when not already marked', () => { - compromisedVatsMock.get.mockReturnValueOnce('["v1"]'); - methods.markVatAsCompromised('v2'); - expect(compromisedVatsMock.set).toHaveBeenCalledWith('["v1","v2"]'); - }); - - it('does not modify list when vat is already marked as compromised', () => { - compromisedVatsMock.get.mockReturnValueOnce('["v1", "v2"]'); - methods.markVatAsCompromised('v2'); - expect(compromisedVatsMock.set).not.toHaveBeenCalled(); - }); - - it('initializes list when none exists', () => { - compromisedVatsMock.get.mockReturnValueOnce(null); - methods.markVatAsCompromised('v1'); - expect(compromisedVatsMock.set).toHaveBeenCalledWith('["v1"]'); - }); - }); - - describe('clearVatCompromisedStatus', () => { - it('removes vat from compromised list', () => { - compromisedVatsMock.get.mockReturnValueOnce('["v1","v2","v3"]'); - methods.clearVatCompromisedStatus('v2'); - expect(compromisedVatsMock.set).toHaveBeenCalledWith('["v1","v3"]'); - }); - - it('does nothing if vat is not in compromised list', () => { - compromisedVatsMock.get.mockReturnValueOnce('["v1","v3"]'); - methods.clearVatCompromisedStatus('v2'); - expect(compromisedVatsMock.set).toHaveBeenCalledWith('["v1","v3"]'); - }); - - it('clears empty list when last vat is removed', () => { - compromisedVatsMock.get.mockReturnValueOnce('["v1"]'); - methods.clearVatCompromisedStatus('v1'); - expect(compromisedVatsMock.set).toHaveBeenCalledWith('[]'); - }); - }); -}); diff --git a/packages/ocap-kernel/src/store/methods/compromised.ts b/packages/ocap-kernel/src/store/methods/compromised.ts deleted file mode 100644 index 95ee44522..000000000 --- a/packages/ocap-kernel/src/store/methods/compromised.ts +++ /dev/null @@ -1,61 +0,0 @@ -import type { VatId } from '../../types.ts'; -import type { StoreContext } from '../types.ts'; - -/** - * Get methods for tracking vats that have been compromised by syscall failures. - * - * @param ctx - The store context. - * @returns An object with methods for tracking compromised vats. - */ -// eslint-disable-next-line @typescript-eslint/explicit-function-return-type -export function getCompromisedMethods(ctx: StoreContext) { - /** - * Get the list of compromised vats. - * - * @returns An array of compromised vat IDs. - */ - function getCompromisedVats(): VatId[] { - return JSON.parse(ctx.compromisedVats.get() ?? '[]'); - } - - /** - * Check if a vat is compromised. - * - * @param vatId - The ID of the vat to check. - * @returns True if the vat is compromised, false otherwise. - */ - function isVatCompromised(vatId: VatId): boolean { - return getCompromisedVats().includes(vatId); - } - - /** - * Mark a vat as compromised. - * - * @param vatId - The ID of the vat to mark as compromised. - */ - function markVatAsCompromised(vatId: VatId): void { - const compromisedVats = getCompromisedVats(); - if (!compromisedVats.includes(vatId)) { - compromisedVats.push(vatId); - ctx.compromisedVats.set(JSON.stringify(compromisedVats)); - } - } - - /** - * Clear the compromised status for a vat. This is typically used - * when a vat is fully terminated and its state is cleaned up. - * - * @param vatId - The ID of the vat to clear. - */ - function clearVatCompromisedStatus(vatId: VatId): void { - const compromisedVats = getCompromisedVats().filter((id) => id !== vatId); - ctx.compromisedVats.set(JSON.stringify(compromisedVats)); - } - - return { - getCompromisedVats, - isVatCompromised, - markVatAsCompromised, - clearVatCompromisedStatus, - }; -} diff --git a/packages/ocap-kernel/src/store/types.ts b/packages/ocap-kernel/src/store/types.ts index 8f8c8f722..cf382bf89 100644 --- a/packages/ocap-kernel/src/store/types.ts +++ b/packages/ocap-kernel/src/store/types.ts @@ -16,7 +16,6 @@ export type StoreContext = { terminatedVats: StoredValue; inCrank: boolean; savepoints: string[]; - compromisedVats: StoredValue; }; export type StoredValue = { diff --git a/vitest.config.ts b/vitest.config.ts index 2d963564a..6cb6bff87 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -80,10 +80,10 @@ export default defineConfig({ lines: 100, }, 'packages/extension/**': { - statements: 87.75, + statements: 87.65, functions: 87.79, - branches: 82.12, - lines: 87.78, + branches: 82.25, + lines: 87.67, }, 'packages/kernel-browser-runtime/**': { statements: 80.45, @@ -134,10 +134,10 @@ export default defineConfig({ lines: 73.58, }, 'packages/ocap-kernel/**': { - statements: 90.57, - functions: 95.18, - branches: 80.44, - lines: 90.54, + statements: 91, + functions: 95.11, + branches: 81.12, + lines: 90.98, }, 'packages/streams/**': { statements: 100, From f76d6f09fe172be2719912f9398e5c46c61f6cf9 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 22 May 2025 14:43:37 +0200 Subject: [PATCH 13/25] Fix comment --- packages/ocap-kernel/src/VatHandle.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index 54c53b84a..a24d601f8 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -356,10 +356,11 @@ export class VatHandle { // These conditionals express a priority order: the consequences of an // illegal syscall take precedence over a vat requesting termination, etc. if (this.#vatSyscall.illegalSyscall) { - // For now, vat errors both rewind changes and terminate the vat. - // Some day, they might rewind changes but then suspend the vat. results.abort = true; const { info } = this.#vatSyscall.illegalSyscall; + // TODO: For now, vat errors both rewind changes and terminate the vat. + // Some day, they might rewind changes and retry the syscall. + // We should terminate the vat only after a certain # of failed retries. results.terminate = { vatId: this.vatId, reject: true, info }; } else if (this.#vatSyscall.deliveryError) { results.abort = true; From 01398f699196bd6e482c7017d4ec629cd03af1d6 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 22 May 2025 14:51:25 +0200 Subject: [PATCH 14/25] Prevent resetting pending count --- packages/ocap-kernel/src/VatHandle.ts | 6 ------ packages/ocap-kernel/src/VatSyscall.ts | 7 ------- vitest.config.ts | 6 +++--- 3 files changed, 3 insertions(+), 16 deletions(-) diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index a24d601f8..97c44c284 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -211,7 +211,6 @@ export class VatHandle { * @returns The crank results. */ async deliverMessage(target: VRef, message: Message): Promise { - this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['message', target, message], @@ -226,7 +225,6 @@ export class VatHandle { * @returns The crank results. */ async deliverNotify(resolutions: VatOneResolution[]): Promise { - this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['notify', resolutions], @@ -241,7 +239,6 @@ export class VatHandle { * @returns The crank results. */ async deliverDropExports(vrefs: VRef[]): Promise { - this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['dropExports', vrefs], @@ -256,7 +253,6 @@ export class VatHandle { * @returns The crank results. */ async deliverRetireExports(vrefs: VRef[]): Promise { - this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['retireExports', vrefs], @@ -271,7 +267,6 @@ export class VatHandle { * @returns The crank results. */ async deliverRetireImports(vrefs: VRef[]): Promise { - this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['retireImports', vrefs], @@ -285,7 +280,6 @@ export class VatHandle { * @returns The crank results. */ async deliverBringOutYourDead(): Promise { - this.#vatSyscall.resetSyscallCounts(); await this.sendVatCommand({ method: 'deliver', params: ['bringOutYourDead'], diff --git a/packages/ocap-kernel/src/VatSyscall.ts b/packages/ocap-kernel/src/VatSyscall.ts index 48da14a76..288362262 100644 --- a/packages/ocap-kernel/src/VatSyscall.ts +++ b/packages/ocap-kernel/src/VatSyscall.ts @@ -295,13 +295,6 @@ export class VatSyscall { this.illegalSyscall = { vatId: this.vatId, info: makeError(error) }; } - /** - * Reset the syscall counts. - */ - resetSyscallCounts(): void { - this.pendingSyscalls = 0; - } - /** * Wait for all syscalls to complete. */ diff --git a/vitest.config.ts b/vitest.config.ts index 6cb6bff87..f145b3139 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -134,10 +134,10 @@ export default defineConfig({ lines: 73.58, }, 'packages/ocap-kernel/**': { - statements: 91, - functions: 95.11, + statements: 90.96, + functions: 95.09, branches: 81.12, - lines: 90.98, + lines: 90.94, }, 'packages/streams/**': { statements: 100, From 74f8260c234fa0bbd1f7e7fc939e66b57cb3f365 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 22 May 2025 20:02:08 +0200 Subject: [PATCH 15/25] remove unused method --- packages/ocap-kernel/src/KernelQueue.test.ts | 1 - packages/ocap-kernel/src/store/index.test.ts | 1 - .../src/store/methods/queue.test.ts | 110 ------------------ .../ocap-kernel/src/store/methods/queue.ts | 29 +---- vitest.config.ts | 8 +- 5 files changed, 5 insertions(+), 144 deletions(-) diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index ef5b18039..4142d98eb 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -54,7 +54,6 @@ describe('KernelQueue', () => { endCrank: vi.fn(), createCrankSavepoint: vi.fn(), rollbackCrank: vi.fn(), - getRunQueueItemTargetVatId: vi.fn(), } as unknown as KernelStore; kernelQueue = new KernelQueue(kernelStore, terminateVat); diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 56f4ca319..7af16d168 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -85,7 +85,6 @@ describe('kernel store', () => { 'getReachableFlag', 'getRefCount', 'getRootObject', - 'getRunQueueItemTargetVatId', 'getTerminatedVats', 'getVatConfig', 'getVatIDs', diff --git a/packages/ocap-kernel/src/store/methods/queue.test.ts b/packages/ocap-kernel/src/store/methods/queue.test.ts index d8e698249..960c987c1 100644 --- a/packages/ocap-kernel/src/store/methods/queue.test.ts +++ b/packages/ocap-kernel/src/store/methods/queue.test.ts @@ -1,15 +1,9 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; -import * as objectModule from './object.ts'; import { getQueueMethods } from './queue.ts'; import type { RunQueueItem } from '../../types.ts'; import type { StoreContext } from '../types.ts'; -// Mock dependencies -vi.mock('./object.ts', () => ({ - getObjectMethods: vi.fn(), -})); - describe('queue store methods', () => { let mockKV: Map; let mockRunQueue = { @@ -18,7 +12,6 @@ describe('queue store methods', () => { }; let context: StoreContext; let queueMethods: ReturnType; - const mockGetOwner = vi.fn(); beforeEach(() => { vi.clearAllMocks(); @@ -29,24 +22,6 @@ describe('queue store methods', () => { dequeue: vi.fn(), }; - // Set up mock implementation for getObjectMethods - (objectModule.getObjectMethods as ReturnType).mockReturnValue( - { - getOwner: mockGetOwner, - }, - ); - - // Configure mock getOwner behavior - mockGetOwner.mockImplementation((target: string) => { - if (target === 'o+1') { - return 'v1'; - } - if (target === 'o+2') { - return 'v2'; - } - return undefined; - }); - context = { kv: { get: (key: string): string | undefined => mockKV.get(key), @@ -239,89 +214,4 @@ describe('queue store methods', () => { expect(() => queueMethods.runQueueLength()).toThrow('unknown queue run'); }); }); - - describe('getRunQueueItemTargetVatId', () => { - it('returns owner vat for send items', () => { - const sendItem: RunQueueItem = { - type: 'send', - target: 'o+1', - message: { methargs: { body: '', slots: [] }, result: null }, - }; - - const result = queueMethods.getRunQueueItemTargetVatId(sendItem); - - expect(mockGetOwner).toHaveBeenCalledWith('o+1', false); - expect(result).toBe('v1'); - }); - - it('returns vatId for notify items', () => { - const notifyItem: RunQueueItem = { - type: 'notify', - vatId: 'v2', - kpid: 'kp123', - }; - - const result = queueMethods.getRunQueueItemTargetVatId(notifyItem); - - expect(result).toBe('v2'); - }); - - it('returns vatId for dropExports items', () => { - const dropExportsItem: RunQueueItem = { - type: 'dropExports', - vatId: 'v3', - krefs: ['o+1', 'o+2'], - }; - - const result = queueMethods.getRunQueueItemTargetVatId(dropExportsItem); - - expect(result).toBe('v3'); - }); - - it('returns vatId for retireExports items', () => { - const retireExportsItem: RunQueueItem = { - type: 'retireExports', - vatId: 'v4', - krefs: ['o+1', 'o+2'], - }; - - const result = queueMethods.getRunQueueItemTargetVatId(retireExportsItem); - - expect(result).toBe('v4'); - }); - - it('returns vatId for retireImports items', () => { - const retireImportsItem: RunQueueItem = { - type: 'retireImports', - vatId: 'v5', - krefs: ['o-1', 'o-2'], - }; - - const result = queueMethods.getRunQueueItemTargetVatId(retireImportsItem); - - expect(result).toBe('v5'); - }); - - it('returns vatId for bringOutYourDead items', () => { - const bringOutYourDeadItem: RunQueueItem = { - type: 'bringOutYourDead', - vatId: 'v6', - }; - - const result = - queueMethods.getRunQueueItemTargetVatId(bringOutYourDeadItem); - - expect(result).toBe('v6'); - }); - - it('returns undefined for unknown item types', () => { - const unknownItem = { - type: 'unknown', - } as unknown as RunQueueItem; - - const result = queueMethods.getRunQueueItemTargetVatId(unknownItem); - - expect(result).toBeUndefined(); - }); - }); }); diff --git a/packages/ocap-kernel/src/store/methods/queue.ts b/packages/ocap-kernel/src/store/methods/queue.ts index cb0c7780f..1c92e8739 100644 --- a/packages/ocap-kernel/src/store/methods/queue.ts +++ b/packages/ocap-kernel/src/store/methods/queue.ts @@ -1,6 +1,5 @@ -import type { RunQueueItem, VatId } from '../../types.ts'; +import type { RunQueueItem } from '../../types.ts'; import type { StoreContext } from '../types.ts'; -import { getObjectMethods } from './object.ts'; /** * Get a queue store object that provides functionality for managing queues. * @@ -10,8 +9,6 @@ import { getObjectMethods } from './object.ts'; */ // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getQueueMethods(ctx: StoreContext) { - const { getOwner } = getObjectMethods(ctx); - /** * Find out how long some queue is. * @@ -62,34 +59,10 @@ export function getQueueMethods(ctx: StoreContext) { return ctx.runQueueLengthCache; } - /** - * Get the target VatId from a RunQueueItem. - * - * @param item - The RunQueueItem to get the target VatId from. - * @returns The target VatId, or undefined if the item is not a send. - */ - function getRunQueueItemTargetVatId(item: RunQueueItem): VatId | undefined { - switch (item.type) { - case 'send': { - return getOwner(item.target, false); - } - case 'notify': - case 'dropExports': - case 'retireExports': - case 'retireImports': - case 'bringOutYourDead': { - return item.vatId; - } - default: - return undefined; - } - } - return { getQueueLength, enqueueRun, dequeueRun, runQueueLength, - getRunQueueItemTargetVatId, }; } diff --git a/vitest.config.ts b/vitest.config.ts index f145b3139..cd37d3945 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -134,10 +134,10 @@ export default defineConfig({ lines: 73.58, }, 'packages/ocap-kernel/**': { - statements: 90.96, - functions: 95.09, - branches: 81.12, - lines: 90.94, + statements: 90.94, + functions: 95.07, + branches: 80.91, + lines: 90.92, }, 'packages/streams/**': { statements: 100, From 283fab68f41f957e02c2d1d6561d54ad9da84d7d Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 22 May 2025 20:03:20 +0200 Subject: [PATCH 16/25] cleanup --- packages/ocap-kernel/src/store/methods/queue.test.ts | 2 -- packages/ocap-kernel/src/store/methods/queue.ts | 1 + vitest.config.ts | 4 ++-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/ocap-kernel/src/store/methods/queue.test.ts b/packages/ocap-kernel/src/store/methods/queue.test.ts index 960c987c1..d44a3dffb 100644 --- a/packages/ocap-kernel/src/store/methods/queue.test.ts +++ b/packages/ocap-kernel/src/store/methods/queue.test.ts @@ -14,8 +14,6 @@ describe('queue store methods', () => { let queueMethods: ReturnType; beforeEach(() => { - vi.clearAllMocks(); - mockKV = new Map(); mockRunQueue = { enqueue: vi.fn(), diff --git a/packages/ocap-kernel/src/store/methods/queue.ts b/packages/ocap-kernel/src/store/methods/queue.ts index 1c92e8739..54693580e 100644 --- a/packages/ocap-kernel/src/store/methods/queue.ts +++ b/packages/ocap-kernel/src/store/methods/queue.ts @@ -1,5 +1,6 @@ import type { RunQueueItem } from '../../types.ts'; import type { StoreContext } from '../types.ts'; + /** * Get a queue store object that provides functionality for managing queues. * diff --git a/vitest.config.ts b/vitest.config.ts index cd37d3945..c38e635f1 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -134,10 +134,10 @@ export default defineConfig({ lines: 73.58, }, 'packages/ocap-kernel/**': { - statements: 90.94, + statements: 90.93, functions: 95.07, branches: 80.91, - lines: 90.92, + lines: 90.91, }, 'packages/streams/**': { statements: 100, From 2d804615cb9898961bf2351624d223be5531095e Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 22 May 2025 21:16:18 +0200 Subject: [PATCH 17/25] Mark handleSyscalls as sync --- packages/ocap-kernel/src/VatHandle.ts | 2 +- packages/ocap-kernel/src/VatSyscall.test.ts | 37 +++++++++++---------- packages/ocap-kernel/src/VatSyscall.ts | 4 +-- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index 97c44c284..99ab101d6 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -111,7 +111,7 @@ export class VatHandle { ); this.#rpcService = new RpcService(vatSyscallHandlers, { handleSyscall: async (params) => { - return await this.#vatSyscall.handleSyscall(params as VatSyscallObject); + return this.#vatSyscall.handleSyscall(params as VatSyscallObject); }, }); } diff --git a/packages/ocap-kernel/src/VatSyscall.test.ts b/packages/ocap-kernel/src/VatSyscall.test.ts index 13d82235d..57b9c450e 100644 --- a/packages/ocap-kernel/src/VatSyscall.test.ts +++ b/packages/ocap-kernel/src/VatSyscall.test.ts @@ -31,6 +31,7 @@ describe('VatSyscall', () => { getReachableFlag: vi.fn(), forgetKref: vi.fn(), getVatConfig: vi.fn(() => ({})), + isVatActive: vi.fn(() => true), } as unknown as KernelStore; logger = { debug: vi.fn(), @@ -43,14 +44,14 @@ describe('VatSyscall', () => { const target = 'o+1'; const message = {} as unknown as Message; const vso = ['send', target, message] as unknown as VatSyscallObject; - await vatSys.handleSyscall(vso); + vatSys.handleSyscall(vso); expect(kernelQueue.enqueueSend).toHaveBeenCalledWith(target, message); }); it('calls resolvePromises for resolve syscall', async () => { const resolution = ['kp1', false, {}] as unknown as VatOneResolution; const vso = ['resolve', [resolution]] as unknown as VatSyscallObject; - await vatSys.handleSyscall(vso); + vatSys.handleSyscall(vso); expect(kernelQueue.resolvePromises).toHaveBeenCalledWith('v1', [ resolution, ]); @@ -64,7 +65,7 @@ describe('VatSyscall', () => { state: 'unresolved', }); const vso = ['subscribe', 'kp1'] as unknown as VatSyscallObject; - await vatSys.handleSyscall(vso); + vatSys.handleSyscall(vso); expect(kernelStore.addPromiseSubscriber).toHaveBeenCalledWith( 'v1', 'kp1', @@ -78,7 +79,7 @@ describe('VatSyscall', () => { state: 'fulfilled', }); const vso = ['subscribe', 'kp1'] as unknown as VatSyscallObject; - await vatSys.handleSyscall(vso); + vatSys.handleSyscall(vso); expect(kernelQueue.enqueueNotify).toHaveBeenCalledWith('v1', 'kp1'); }); }); @@ -89,7 +90,7 @@ describe('VatSyscall', () => { 'dropImports', ['o-1', 'o-2'], ] as unknown as VatSyscallObject; - await vatSys.handleSyscall(vso); + vatSys.handleSyscall(vso); expect(kernelStore.clearReachableFlag).toHaveBeenCalledWith('v1', 'o-1'); expect(kernelStore.clearReachableFlag).toHaveBeenCalledWith('v1', 'o-2'); }); @@ -104,7 +105,7 @@ describe('VatSyscall', () => { throw new Error(errMsg); }); const vso = ['dropImports', [ref]] as unknown as VatSyscallObject; - const result = await vatSys.handleSyscall(vso); + const result = vatSys.handleSyscall(vso); expect(result).toStrictEqual(['error', errMsg]); }); }); @@ -115,7 +116,7 @@ describe('VatSyscall', () => { kernelStore.getReachableFlag as unknown as MockInstance ).mockReturnValueOnce(false); const vso = ['retireImports', ['o-1']] as unknown as VatSyscallObject; - await vatSys.handleSyscall(vso); + vatSys.handleSyscall(vso); expect(kernelStore.forgetKref).toHaveBeenCalledWith('v1', 'o-1'); }); @@ -129,7 +130,7 @@ describe('VatSyscall', () => { throw new Error('syscall.retireImports but o-1 is still reachable'); }); const vso = ['retireImports', ['o-1']] as unknown as VatSyscallObject; - const result = await vatSys.handleSyscall(vso); + const result = vatSys.handleSyscall(vso); expect(result).toStrictEqual([ 'error', 'syscall.retireImports but o-1 is still reachable', @@ -143,7 +144,7 @@ describe('VatSyscall', () => { kernelStore.getReachableFlag as unknown as MockInstance ).mockReturnValueOnce(false); const vso = ['retireExports', ['o+1']] as unknown as VatSyscallObject; - await vatSys.handleSyscall(vso); + vatSys.handleSyscall(vso); expect(kernelStore.forgetKref).toHaveBeenCalledWith('v1', 'o+1'); expect(logger.debug).toHaveBeenCalledWith( 'retireExports: deleted object o+1', @@ -160,7 +161,7 @@ describe('VatSyscall', () => { throw new Error('syscall.retireExports but o+1 is still reachable'); }); const vso = ['retireExports', ['o+1']] as unknown as VatSyscallObject; - const result = await vatSys.handleSyscall(vso); + const result = vatSys.handleSyscall(vso); expect(result).toStrictEqual([ 'error', 'syscall.retireExports but o+1 is still reachable', @@ -169,7 +170,7 @@ describe('VatSyscall', () => { it('abandons exports without reachability check', async () => { const vso = ['abandonExports', ['o+1']] as unknown as VatSyscallObject; - await vatSys.handleSyscall(vso); + vatSys.handleSyscall(vso); expect(kernelStore.forgetKref).toHaveBeenCalledWith('v1', 'o+1'); expect(logger.debug).toHaveBeenCalledWith( 'abandonExports: deleted object o+1', @@ -183,7 +184,7 @@ describe('VatSyscall', () => { throw new Error('vat v1 issued invalid syscall abandonExports for o-1'); }); const vso = ['abandonExports', ['o-1']] as unknown as VatSyscallObject; - const result = await vatSys.handleSyscall(vso); + const result = vatSys.handleSyscall(vso); expect(result).toStrictEqual([ 'error', 'vat v1 issued invalid syscall abandonExports for o-1', @@ -198,7 +199,7 @@ describe('VatSyscall', () => { true, { message: 'error' }, ] as unknown as VatSyscallObject; - await vatSys.handleSyscall(vso); + vatSys.handleSyscall(vso); expect(vatSys.vatRequestedTermination).toStrictEqual({ reject: true, info: { message: 'error' }, @@ -208,11 +209,11 @@ describe('VatSyscall', () => { describe('error handling', () => { it('handles vat not found error', async () => { - (kernelStore.getVatConfig as unknown as MockInstance).mockReturnValueOnce( - undefined, + (kernelStore.isVatActive as unknown as MockInstance).mockReturnValueOnce( + false, ); const vso = ['send', 'o+1', {}] as unknown as VatSyscallObject; - const result = await vatSys.handleSyscall(vso); + const result = vatSys.handleSyscall(vso); expect(result).toStrictEqual(['error', 'vat not found']); expect(vatSys.illegalSyscall).toBeDefined(); @@ -227,7 +228,7 @@ describe('VatSyscall', () => { }); const vso = ['send', 'o+1', {}] as unknown as VatSyscallObject; - const result = await vatSys.handleSyscall(vso); + const result = vatSys.handleSyscall(vso); expect(result).toStrictEqual(['error', 'test error']); expect(logger.error).toHaveBeenCalledWith( @@ -250,7 +251,7 @@ describe('VatSyscall', () => { // do nothing }); const vso = [op, []] as unknown as VatSyscallObject; - await vatSys.handleSyscall(vso); + vatSys.handleSyscall(vso); expect(spy).toHaveBeenCalledWith(expect.stringContaining(message), vso); spy.mockRestore(); }); diff --git a/packages/ocap-kernel/src/VatSyscall.ts b/packages/ocap-kernel/src/VatSyscall.ts index 288362262..411d2899d 100644 --- a/packages/ocap-kernel/src/VatSyscall.ts +++ b/packages/ocap-kernel/src/VatSyscall.ts @@ -179,14 +179,14 @@ export class VatSyscall { * @param vso - The syscall that was received. * @returns The result of the syscall. */ - async handleSyscall(vso: VatSyscallObject): Promise { + handleSyscall(vso: VatSyscallObject): VatSyscallResult { try { this.illegalSyscall = undefined; this.vatRequestedTermination = undefined; this.pendingSyscalls += 1; // This is a safety check - this case should never happen - if (!this.#kernelStore.getVatConfig(this.vatId)) { + if (!this.#kernelStore.isVatActive(this.vatId)) { this.#vatFatalSyscall('vat not found'); return harden(['error', 'vat not found']); } From 6e88625a7cf3eb055822d9c1fa8e1aa8d3fd962e Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 27 May 2025 20:15:30 +0200 Subject: [PATCH 18/25] docs: clarify VatHandle error handling comment --- packages/ocap-kernel/src/VatHandle.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index 99ab101d6..381d74a48 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -328,6 +328,8 @@ export class VatHandle { const [[sets, deletes], deliveryError] = result as VatDeliveryResult; this.#vatSyscall.deliveryError = deliveryError ?? undefined; const noErrors = !deliveryError && !this.#vatSyscall.illegalSyscall; + // On errors, we neither update KV data nor rollback previous changes. + // This is safe because vats are always terminated when errors occur. if (noErrors) { this.#vatStore.updateKVData(sets, deletes); } From 33960f3a4bb1f9fb31edafbe06ff949d882bbe71 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 27 May 2025 20:18:39 +0200 Subject: [PATCH 19/25] improve comment --- packages/ocap-kernel/src/VatHandle.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index 381d74a48..1ccb6367b 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -328,8 +328,9 @@ export class VatHandle { const [[sets, deletes], deliveryError] = result as VatDeliveryResult; this.#vatSyscall.deliveryError = deliveryError ?? undefined; const noErrors = !deliveryError && !this.#vatSyscall.illegalSyscall; - // On errors, we neither update KV data nor rollback previous changes. - // This is safe because vats are always terminated when errors occur. + // On errors, we neither update this vat's KV data nor rollback previous changes. + // This is safe because vats are always terminated when errors occur + // and they have their own databases. The main kernel database will be rolled back. if (noErrors) { this.#vatStore.updateKVData(sets, deletes); } From ef2bfdbeab061748704a97eb4b7a8f40fc16fdcb Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 27 May 2025 20:19:19 +0200 Subject: [PATCH 20/25] improve comment 2 :) --- packages/ocap-kernel/src/VatHandle.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index 1ccb6367b..86e24be6c 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -330,7 +330,8 @@ export class VatHandle { const noErrors = !deliveryError && !this.#vatSyscall.illegalSyscall; // On errors, we neither update this vat's KV data nor rollback previous changes. // This is safe because vats are always terminated when errors occur - // and they have their own databases. The main kernel database will be rolled back. + // and they have their own databases, which are deleted when the vat is terminated. + // The main kernel database will be rolled back. if (noErrors) { this.#vatStore.updateKVData(sets, deletes); } From 832b2b4b235ef82e3bfa3dc58a4f68e76c87c010 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 27 May 2025 20:29:32 +0200 Subject: [PATCH 21/25] add more comments --- packages/ocap-kernel/src/VatSyscall.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/ocap-kernel/src/VatSyscall.ts b/packages/ocap-kernel/src/VatSyscall.ts index 411d2899d..fcb5fdcd3 100644 --- a/packages/ocap-kernel/src/VatSyscall.ts +++ b/packages/ocap-kernel/src/VatSyscall.ts @@ -297,6 +297,11 @@ export class VatSyscall { /** * Wait for all syscalls to complete. + * + * This is useful because syscalls are asynchronous, + * and we need to wait for them to complete before returning the result. + * + * @returns A promise that resolves when all syscalls have completed. */ async waitForSyscallsToComplete(): Promise { while (this.pendingSyscalls > 0) { From 5dfd7736b639826dc4162b54058a61e00e778184 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 28 May 2025 14:48:24 +0200 Subject: [PATCH 22/25] rename assertion function --- .../kernel-store/src/sqlite/common.test.ts | 38 ++++++++++++------- packages/kernel-store/src/sqlite/common.ts | 6 +-- packages/kernel-store/src/sqlite/nodejs.ts | 12 ++++-- packages/kernel-store/src/sqlite/wasm.ts | 12 ++++-- 4 files changed, 44 insertions(+), 24 deletions(-) diff --git a/packages/kernel-store/src/sqlite/common.test.ts b/packages/kernel-store/src/sqlite/common.test.ts index c9bee1808..e64690ee0 100644 --- a/packages/kernel-store/src/sqlite/common.test.ts +++ b/packages/kernel-store/src/sqlite/common.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from 'vitest'; -import { SQL_QUERIES, safeIdentifier } from './common.ts'; +import { SQL_QUERIES, assertSafeIdentifier } from './common.ts'; describe('SQL_QUERIES', () => { // XXX Is this test actually useful? It's basically testing that the source code matches itself. @@ -64,27 +64,39 @@ describe('SQL_QUERIES', () => { }); }); -describe('safeIdentifier', () => { +describe('assertSafeIdentifier', () => { it('accepts valid SQL identifiers', () => { - expect(safeIdentifier('valid')).toBe('valid'); - expect(safeIdentifier('Valid')).toBe('Valid'); - expect(safeIdentifier('valid_name')).toBe('valid_name'); - expect(safeIdentifier('valid_name_123')).toBe('valid_name_123'); - expect(safeIdentifier('_leading_underscore')).toBe('_leading_underscore'); + expect(assertSafeIdentifier('valid')).toBe('valid'); + expect(assertSafeIdentifier('Valid')).toBe('Valid'); + expect(assertSafeIdentifier('valid_name')).toBe('valid_name'); + expect(assertSafeIdentifier('valid_name_123')).toBe('valid_name_123'); + expect(assertSafeIdentifier('_leading_underscore')).toBe( + '_leading_underscore', + ); }); it('rejects invalid SQL identifiers', () => { // Starting with a number - expect(() => safeIdentifier('123invalid')).toThrow('Invalid identifier'); + expect(() => assertSafeIdentifier('123invalid')).toThrow( + 'Invalid identifier', + ); // Containing invalid characters - expect(() => safeIdentifier('invalid-name')).toThrow('Invalid identifier'); - expect(() => safeIdentifier('invalid.name')).toThrow('Invalid identifier'); - expect(() => safeIdentifier('invalid;name')).toThrow('Invalid identifier'); - expect(() => safeIdentifier('invalid name')).toThrow('Invalid identifier'); + expect(() => assertSafeIdentifier('invalid-name')).toThrow( + 'Invalid identifier', + ); + expect(() => assertSafeIdentifier('invalid.name')).toThrow( + 'Invalid identifier', + ); + expect(() => assertSafeIdentifier('invalid;name')).toThrow( + 'Invalid identifier', + ); + expect(() => assertSafeIdentifier('invalid name')).toThrow( + 'Invalid identifier', + ); // Containing SQL injection attempts - expect(() => safeIdentifier("name'; DROP TABLE users--")).toThrow( + expect(() => assertSafeIdentifier("name'; DROP TABLE users--")).toThrow( 'Invalid identifier', ); }); diff --git a/packages/kernel-store/src/sqlite/common.ts b/packages/kernel-store/src/sqlite/common.ts index 45e45cd26..d8638eacd 100644 --- a/packages/kernel-store/src/sqlite/common.ts +++ b/packages/kernel-store/src/sqlite/common.ts @@ -60,8 +60,8 @@ export const SQL_QUERIES = { BEGIN_IMMEDIATE_TRANSACTION: `BEGIN IMMEDIATE TRANSACTION`, COMMIT_TRANSACTION: `COMMIT TRANSACTION`, ABORT_TRANSACTION: `ROLLBACK TRANSACTION`, - // SQLite’s parameter markers (?, ?NNN, :name, @name, $name) can only be used - // in places where a literal value is allowed. We can’t bind identifiers + // SQLite's parameter markers (?, ?NNN, :name, @name, $name) can only be used + // in places where a literal value is allowed. We can't bind identifiers // for table names, column names, or savepoint names. We use %NAME% as a // placeholder for the savepoint name. CREATE_SAVEPOINT: `SAVEPOINT %NAME%`, @@ -80,7 +80,7 @@ export const DEFAULT_DB_FILENAME = ':memory:'; * @param name - The string to check. * @returns The string if it is a valid identifier. */ -export function safeIdentifier(name: string): string { +export function assertSafeIdentifier(name: string): string { if (!/^[A-Za-z_]\w*$/u.test(name)) { throw new Error(`Invalid identifier: ${name}`); } diff --git a/packages/kernel-store/src/sqlite/nodejs.ts b/packages/kernel-store/src/sqlite/nodejs.ts index 40b24ee59..83ec5bce1 100644 --- a/packages/kernel-store/src/sqlite/nodejs.ts +++ b/packages/kernel-store/src/sqlite/nodejs.ts @@ -6,7 +6,11 @@ import { mkdir } from 'fs/promises'; import { tmpdir } from 'os'; import { join } from 'path'; -import { SQL_QUERIES, DEFAULT_DB_FILENAME, safeIdentifier } from './common.ts'; +import { + SQL_QUERIES, + DEFAULT_DB_FILENAME, + assertSafeIdentifier, +} from './common.ts'; import { getDBFolder } from './env.ts'; import type { KVStore, VatStore, KernelDatabase } from '../types.ts'; @@ -274,7 +278,7 @@ export async function makeSQLKernelDatabase({ // later will cause an autocommit. // See https://github.com/Agoric/agoric-sdk/issues/8423 beginIfNeeded(); - const point = safeIdentifier(name); + const point = assertSafeIdentifier(name); const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', point); db.exec(query); db._spStack.push(point); @@ -286,7 +290,7 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function rollbackSavepoint(name: string): void { - const point = safeIdentifier(name); + const point = assertSafeIdentifier(name); const idx = db._spStack.lastIndexOf(point); if (idx < 0) { throw new Error(`No such savepoint: ${point}`); @@ -305,7 +309,7 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function releaseSavepoint(name: string): void { - const point = safeIdentifier(name); + const point = assertSafeIdentifier(name); const idx = db._spStack.lastIndexOf(point); if (idx < 0) { throw new Error(`No such savepoint: ${point}`); diff --git a/packages/kernel-store/src/sqlite/wasm.ts b/packages/kernel-store/src/sqlite/wasm.ts index ca7cbe597..4063fc1f3 100644 --- a/packages/kernel-store/src/sqlite/wasm.ts +++ b/packages/kernel-store/src/sqlite/wasm.ts @@ -2,7 +2,11 @@ import { Logger } from '@metamask/logger'; import type { Database as SqliteDatabase } from '@sqlite.org/sqlite-wasm'; import sqlite3InitModule from '@sqlite.org/sqlite-wasm'; -import { DEFAULT_DB_FILENAME, safeIdentifier, SQL_QUERIES } from './common.ts'; +import { + DEFAULT_DB_FILENAME, + assertSafeIdentifier, + SQL_QUERIES, +} from './common.ts'; import { getDBFolder } from './env.ts'; import type { KVStore, VatStore, KernelDatabase } from '../types.ts'; @@ -353,7 +357,7 @@ export async function makeSQLKernelDatabase({ // later will cause an autocommit. // See https://github.com/Agoric/agoric-sdk/issues/8423 beginIfNeeded(); - const point = safeIdentifier(name); + const point = assertSafeIdentifier(name); const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', point); db.exec(query); db._spStack.push(point); @@ -365,7 +369,7 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function rollbackSavepoint(name: string): void { - const point = safeIdentifier(name); + const point = assertSafeIdentifier(name); const idx = db._spStack.lastIndexOf(point); if (idx < 0) { throw new Error(`No such savepoint: ${point}`); @@ -384,7 +388,7 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function releaseSavepoint(name: string): void { - const point = safeIdentifier(name); + const point = assertSafeIdentifier(name); const idx = db._spStack.lastIndexOf(point); if (idx < 0) { throw new Error(`No such savepoint: ${point}`); From 9b09d649459a82812a5abf34629d71881a9b2e05 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 28 May 2025 14:59:25 +0200 Subject: [PATCH 23/25] make it an assertion --- .../kernel-store/src/sqlite/common.test.ts | 12 +++++----- packages/kernel-store/src/sqlite/common.ts | 4 +--- packages/kernel-store/src/sqlite/nodejs.ts | 22 +++++++++---------- packages/kernel-store/src/sqlite/wasm.ts | 22 +++++++++---------- vitest.config.ts | 4 ++-- 5 files changed, 30 insertions(+), 34 deletions(-) diff --git a/packages/kernel-store/src/sqlite/common.test.ts b/packages/kernel-store/src/sqlite/common.test.ts index e64690ee0..852fdfbe9 100644 --- a/packages/kernel-store/src/sqlite/common.test.ts +++ b/packages/kernel-store/src/sqlite/common.test.ts @@ -66,13 +66,11 @@ describe('SQL_QUERIES', () => { describe('assertSafeIdentifier', () => { it('accepts valid SQL identifiers', () => { - expect(assertSafeIdentifier('valid')).toBe('valid'); - expect(assertSafeIdentifier('Valid')).toBe('Valid'); - expect(assertSafeIdentifier('valid_name')).toBe('valid_name'); - expect(assertSafeIdentifier('valid_name_123')).toBe('valid_name_123'); - expect(assertSafeIdentifier('_leading_underscore')).toBe( - '_leading_underscore', - ); + expect(() => assertSafeIdentifier('valid')).not.toThrow(); + expect(() => assertSafeIdentifier('Valid')).not.toThrow(); + expect(() => assertSafeIdentifier('valid_name')).not.toThrow(); + expect(() => assertSafeIdentifier('valid_name_123')).not.toThrow(); + expect(() => assertSafeIdentifier('_leading_underscore')).not.toThrow(); }); it('rejects invalid SQL identifiers', () => { diff --git a/packages/kernel-store/src/sqlite/common.ts b/packages/kernel-store/src/sqlite/common.ts index d8638eacd..672b07d9d 100644 --- a/packages/kernel-store/src/sqlite/common.ts +++ b/packages/kernel-store/src/sqlite/common.ts @@ -78,11 +78,9 @@ export const DEFAULT_DB_FILENAME = ':memory:'; * Check if a string is a valid SQLite identifier. * * @param name - The string to check. - * @returns The string if it is a valid identifier. */ -export function assertSafeIdentifier(name: string): string { +export function assertSafeIdentifier(name: string): void { if (!/^[A-Za-z_]\w*$/u.test(name)) { throw new Error(`Invalid identifier: ${name}`); } - return name; } diff --git a/packages/kernel-store/src/sqlite/nodejs.ts b/packages/kernel-store/src/sqlite/nodejs.ts index 83ec5bce1..3d9eb1051 100644 --- a/packages/kernel-store/src/sqlite/nodejs.ts +++ b/packages/kernel-store/src/sqlite/nodejs.ts @@ -278,10 +278,10 @@ export async function makeSQLKernelDatabase({ // later will cause an autocommit. // See https://github.com/Agoric/agoric-sdk/issues/8423 beginIfNeeded(); - const point = assertSafeIdentifier(name); - const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', point); + assertSafeIdentifier(name); + const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', name); db.exec(query); - db._spStack.push(point); + db._spStack.push(name); } /** @@ -290,12 +290,12 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function rollbackSavepoint(name: string): void { - const point = assertSafeIdentifier(name); - const idx = db._spStack.lastIndexOf(point); + assertSafeIdentifier(name); + const idx = db._spStack.lastIndexOf(name); if (idx < 0) { - throw new Error(`No such savepoint: ${point}`); + throw new Error(`No such savepoint: ${name}`); } - const query = SQL_QUERIES.ROLLBACK_SAVEPOINT.replace('%NAME%', point); + const query = SQL_QUERIES.ROLLBACK_SAVEPOINT.replace('%NAME%', name); db.exec(query); db._spStack.splice(idx); if (db._spStack.length === 0) { @@ -309,12 +309,12 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function releaseSavepoint(name: string): void { - const point = assertSafeIdentifier(name); - const idx = db._spStack.lastIndexOf(point); + assertSafeIdentifier(name); + const idx = db._spStack.lastIndexOf(name); if (idx < 0) { - throw new Error(`No such savepoint: ${point}`); + throw new Error(`No such savepoint: ${name}`); } - const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', point); + const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', name); db.exec(query); db._spStack.splice(idx); if (db._spStack.length === 0) { diff --git a/packages/kernel-store/src/sqlite/wasm.ts b/packages/kernel-store/src/sqlite/wasm.ts index 4063fc1f3..f52af874d 100644 --- a/packages/kernel-store/src/sqlite/wasm.ts +++ b/packages/kernel-store/src/sqlite/wasm.ts @@ -357,10 +357,10 @@ export async function makeSQLKernelDatabase({ // later will cause an autocommit. // See https://github.com/Agoric/agoric-sdk/issues/8423 beginIfNeeded(); - const point = assertSafeIdentifier(name); - const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', point); + assertSafeIdentifier(name); + const query = SQL_QUERIES.CREATE_SAVEPOINT.replace('%NAME%', name); db.exec(query); - db._spStack.push(point); + db._spStack.push(name); } /** @@ -369,12 +369,12 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function rollbackSavepoint(name: string): void { - const point = assertSafeIdentifier(name); - const idx = db._spStack.lastIndexOf(point); + assertSafeIdentifier(name); + const idx = db._spStack.lastIndexOf(name); if (idx < 0) { - throw new Error(`No such savepoint: ${point}`); + throw new Error(`No such savepoint: ${name}`); } - const query = SQL_QUERIES.ROLLBACK_SAVEPOINT.replace('%NAME%', point); + const query = SQL_QUERIES.ROLLBACK_SAVEPOINT.replace('%NAME%', name); db.exec(query); db._spStack.splice(idx); if (db._spStack.length === 0) { @@ -388,12 +388,12 @@ export async function makeSQLKernelDatabase({ * @param name - The name of the savepoint. */ function releaseSavepoint(name: string): void { - const point = assertSafeIdentifier(name); - const idx = db._spStack.lastIndexOf(point); + assertSafeIdentifier(name); + const idx = db._spStack.lastIndexOf(name); if (idx < 0) { - throw new Error(`No such savepoint: ${point}`); + throw new Error(`No such savepoint: ${name}`); } - const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', point); + const query = SQL_QUERIES.RELEASE_SAVEPOINT.replace('%NAME%', name); db.exec(query); db._spStack.splice(idx); if (db._spStack.length === 0) { diff --git a/vitest.config.ts b/vitest.config.ts index c38e635f1..0679fd664 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -110,10 +110,10 @@ export default defineConfig({ lines: 0, }, 'packages/kernel-store/**': { - statements: 98, + statements: 97.99, functions: 100, branches: 91.25, - lines: 97.99, + lines: 97.98, }, 'packages/kernel-utils/**': { statements: 100, From 8057d4acfe09f8487025e143e0b27b1bed8b5cd2 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 28 May 2025 21:08:20 +0200 Subject: [PATCH 24/25] rename methods --- packages/ocap-kernel/src/VatHandle.ts | 14 +++++++------- packages/ocap-kernel/src/VatSyscall.ts | 6 +++--- packages/ocap-kernel/src/store/index.test.ts | 1 + packages/ocap-kernel/src/store/index.ts | 5 +---- .../ocap-kernel/src/store/methods/crank.test.ts | 16 ++++++++++++++++ packages/ocap-kernel/src/store/methods/crank.ts | 11 +++++++++++ vitest.config.ts | 8 ++++---- 7 files changed, 43 insertions(+), 18 deletions(-) diff --git a/packages/ocap-kernel/src/VatHandle.ts b/packages/ocap-kernel/src/VatHandle.ts index 86e24be6c..b4bd89228 100644 --- a/packages/ocap-kernel/src/VatHandle.ts +++ b/packages/ocap-kernel/src/VatHandle.ts @@ -215,7 +215,7 @@ export class VatHandle { method: 'deliver', params: ['message', target, message], }); - return this.#deliveryCrankResults(); + return this.#getDeliveryCrankResults(); } /** @@ -229,7 +229,7 @@ export class VatHandle { method: 'deliver', params: ['notify', resolutions], }); - return this.#deliveryCrankResults(); + return this.#getDeliveryCrankResults(); } /** @@ -243,7 +243,7 @@ export class VatHandle { method: 'deliver', params: ['dropExports', vrefs], }); - return this.#deliveryCrankResults(); + return this.#getDeliveryCrankResults(); } /** @@ -257,7 +257,7 @@ export class VatHandle { method: 'deliver', params: ['retireExports', vrefs], }); - return this.#deliveryCrankResults(); + return this.#getDeliveryCrankResults(); } /** @@ -271,7 +271,7 @@ export class VatHandle { method: 'deliver', params: ['retireImports', vrefs], }); - return this.#deliveryCrankResults(); + return this.#getDeliveryCrankResults(); } /** @@ -284,7 +284,7 @@ export class VatHandle { method: 'deliver', params: ['bringOutYourDead'], }); - return this.#deliveryCrankResults(); + return this.#getDeliveryCrankResults(); } /** @@ -344,7 +344,7 @@ export class VatHandle { * * @returns The crank outcome. */ - async #deliveryCrankResults(): Promise { + async #getDeliveryCrankResults(): Promise { await this.#vatSyscall.waitForSyscallsToComplete(); const results: CrankResults = { diff --git a/packages/ocap-kernel/src/VatSyscall.ts b/packages/ocap-kernel/src/VatSyscall.ts index fcb5fdcd3..10954158a 100644 --- a/packages/ocap-kernel/src/VatSyscall.ts +++ b/packages/ocap-kernel/src/VatSyscall.ts @@ -187,7 +187,7 @@ export class VatSyscall { // This is a safety check - this case should never happen if (!this.#kernelStore.isVatActive(this.vatId)) { - this.#vatFatalSyscall('vat not found'); + this.#recordVatFatalSyscall('vat not found'); return harden(['error', 'vat not found']); } @@ -276,7 +276,7 @@ export class VatSyscall { return harden(['ok', null]); } catch (error) { this.#logger.error(`Fatal syscall error in vat ${this.vatId}`, error); - this.#vatFatalSyscall('syscall translation error: prepare to die'); + this.#recordVatFatalSyscall('syscall translation error: prepare to die'); return harden([ 'error', error instanceof Error ? error.message : String(error), @@ -291,7 +291,7 @@ export class VatSyscall { * * @param error - The error message to log. */ - #vatFatalSyscall(error: string): void { + #recordVatFatalSyscall(error: string): void { this.illegalSyscall = { vatId: this.vatId, info: makeError(error) }; } diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 7af16d168..2454f8846 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -108,6 +108,7 @@ describe('kernel store', () => { 'nextReapAction', 'nextTerminatedVatCleanup', 'pinObject', + 'releaseAllSavepoints', 'reset', 'resolveKernelPromise', 'retireKernelObjects', diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 3a2bce01f..cd83ae48b 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -177,10 +177,7 @@ export function makeKernelStore(kdb: KernelDatabase) { context.nextPromiseId = provideCachedStoredValue('nextPromiseId', '1'); context.nextVatId = provideCachedStoredValue('nextVatId', '1'); context.nextRemoteId = provideCachedStoredValue('nextRemoteId', '1'); - if (context.savepoints.length > 0) { - kdb.releaseSavepoint('t0'); - context.savepoints.length = 0; - } + crank.releaseAllSavepoints(); } /** diff --git a/packages/ocap-kernel/src/store/methods/crank.test.ts b/packages/ocap-kernel/src/store/methods/crank.test.ts index 334e1bff4..38bd62130 100644 --- a/packages/ocap-kernel/src/store/methods/crank.test.ts +++ b/packages/ocap-kernel/src/store/methods/crank.test.ts @@ -123,4 +123,20 @@ describe('crank methods', () => { ); }); }); + + describe('releaseAllSavepoints', () => { + it('should release all savepoints', () => { + context.inCrank = true; + context.savepoints = ['test']; + crankMethods.releaseAllSavepoints(); + expect(kdb.releaseSavepoint).toHaveBeenCalledWith('t0'); + expect(context.savepoints).toStrictEqual([]); + }); + + it('should not call releaseSavepoint if no savepoints exist', () => { + context.inCrank = true; + crankMethods.releaseAllSavepoints(); + expect(kdb.releaseSavepoint).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/ocap-kernel/src/store/methods/crank.ts b/packages/ocap-kernel/src/store/methods/crank.ts index be634b557..b5b8f900c 100644 --- a/packages/ocap-kernel/src/store/methods/crank.ts +++ b/packages/ocap-kernel/src/store/methods/crank.ts @@ -61,10 +61,21 @@ export function getCrankMethods(ctx: StoreContext, kdb: KernelDatabase) { ctx.inCrank = false; } + /** + * Release all savepoints. + */ + function releaseAllSavepoints(): void { + if (ctx.savepoints.length > 0) { + kdb.releaseSavepoint('t0'); + ctx.savepoints.length = 0; + } + } + return { startCrank, createCrankSavepoint, rollbackCrank, endCrank, + releaseAllSavepoints, }; } diff --git a/vitest.config.ts b/vitest.config.ts index 0679fd664..5c25a5aef 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -134,10 +134,10 @@ export default defineConfig({ lines: 73.58, }, 'packages/ocap-kernel/**': { - statements: 90.93, - functions: 95.07, - branches: 80.91, - lines: 90.91, + statements: 91.07, + functions: 95.09, + branches: 81.07, + lines: 91.05, }, 'packages/streams/**': { statements: 100, From 13be5f8bbdd495ca9aa1ff09b872f8b8ba478ac3 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 28 May 2025 21:47:56 +0200 Subject: [PATCH 25/25] Remove unused consumeMessage and add some tests --- packages/ocap-kernel/src/KernelQueue.test.ts | 137 ++++++++++++++++++- packages/ocap-kernel/src/KernelQueue.ts | 19 +-- packages/ocap-kernel/src/VatSyscall.test.ts | 25 ++++ packages/ocap-kernel/src/types.ts | 1 - vitest.config.ts | 6 +- 5 files changed, 171 insertions(+), 17 deletions(-) diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index 4142d98eb..75c54b59b 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -7,6 +7,7 @@ import type { MockInstance } from 'vitest'; import { KernelQueue } from './KernelQueue.ts'; import * as gc from './services/garbage-collection.ts'; import type { KernelStore } from './store/index.ts'; +import * as types from './types.ts'; import type { KRef, Message, @@ -81,10 +82,72 @@ describe('KernelQueue', () => { expect(processGCActionSetSpy).toHaveBeenCalled(); expect(kernelStore.nextReapAction).toHaveBeenCalled(); expect(kernelStore.nextTerminatedVatCleanup).toHaveBeenCalled(); - expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('deliver'); expect(deliver).toHaveBeenCalledWith(mockItem); expect(kernelStore.endCrank).toHaveBeenCalled(); }); + + it('rolls back crank when deliver returns abort', async () => { + const mockItem: RunQueueItem = { + type: 'send', + target: 'ko123', + message: {} as Message, + }; + (kernelStore.runQueueLength as unknown as MockInstance) + .mockReturnValueOnce(1) + .mockReturnValue(0); + (kernelStore.dequeueRun as unknown as MockInstance).mockReturnValueOnce( + mockItem, + ); + const deliver = vi.fn().mockResolvedValue({ abort: true }); + const collectGarbageError = new Error( + 'wakeUpTheRunQueue function already set', + ); + (kernelStore.collectGarbage as unknown as MockInstance).mockRejectedValue( + collectGarbageError, + ); + await expect(kernelQueue.run(deliver)).rejects.toThrow( + collectGarbageError.message, + ); + expect(kernelStore.startCrank).toHaveBeenCalled(); + expect(kernelStore.createCrankSavepoint).toHaveBeenCalledWith('start'); + expect(deliver).toHaveBeenCalledWith(mockItem); + expect(kernelStore.rollbackCrank).toHaveBeenCalledWith('start'); + expect(kernelStore.collectGarbage).toHaveBeenCalled(); + expect(kernelStore.endCrank).toHaveBeenCalled(); + }); + + it('terminates vat when deliver returns terminate', async () => { + const mockItem: RunQueueItem = { + type: 'send', + target: 'ko123', + message: {} as Message, + }; + const terminateInfo = { vatId: 'v1', info: { body: '"test"' } }; + (kernelStore.runQueueLength as unknown as MockInstance) + .mockReturnValueOnce(1) + .mockReturnValue(0); + (kernelStore.dequeueRun as unknown as MockInstance).mockReturnValueOnce( + mockItem, + ); + const deliver = vi.fn().mockResolvedValue({ terminate: terminateInfo }); + const collectGarbageError = new Error( + 'wakeUpTheRunQueue function already set', + ); + (kernelStore.collectGarbage as unknown as MockInstance).mockRejectedValue( + collectGarbageError, + ); + await expect(kernelQueue.run(deliver)).rejects.toThrow( + collectGarbageError.message, + ); + expect(kernelStore.startCrank).toHaveBeenCalled(); + expect(deliver).toHaveBeenCalledWith(mockItem); + expect(terminateVat).toHaveBeenCalledWith( + terminateInfo.vatId, + terminateInfo.info, + ); + expect(kernelStore.collectGarbage).toHaveBeenCalled(); + expect(kernelStore.endCrank).toHaveBeenCalled(); + }); }); describe('enqueueMessage', () => { @@ -251,6 +314,78 @@ describe('KernelQueue', () => { expect(kernelQueue.subscriptions.has(kpid)).toBe(false); }); + it('handles resolutions with undefined vatId (kernel decider)', () => { + const kpid = 'kp123'; + const resolution: VatOneResolution = [ + kpid, + false, + { body: 'resolved value', slots: ['slot1'] } as CapData, + ]; + (kernelStore.getKernelPromise as unknown as MockInstance).mockReturnValue( + { + state: 'unresolved', + decider: undefined, + subscribers: ['v2'], + }, + ); + const resolveHandler = vi.fn(); + kernelQueue.subscriptions.set(kpid, resolveHandler); + const insistVatIdSpy = vi.spyOn(types, 'insistVatId'); + kernelQueue.resolvePromises(undefined, [resolution]); + expect(insistVatIdSpy).not.toHaveBeenCalled(); + expect(kernelStore.incrementRefCount).toHaveBeenCalledWith( + kpid, + 'resolve|kpid', + ); + expect(kernelStore.incrementRefCount).toHaveBeenCalledWith( + 'slot1', + 'resolve|slot', + ); + expect(kernelStore.enqueueRun).toHaveBeenCalledWith({ + type: 'notify', + vatId: 'v2', + kpid, + }); + expect(kernelStore.resolveKernelPromise).toHaveBeenCalledWith( + kpid, + false, + resolution[2], + ); + expect(resolveHandler).toHaveBeenCalledWith(resolution[2]); + expect(kernelQueue.subscriptions.has(kpid)).toBe(false); + insistVatIdSpy.mockRestore(); + }); + + it('handles promises with no subscribers', () => { + const vatId = 'v1'; + const kpid = 'kpNoSubscribers'; + const resolution: VatOneResolution = [ + kpid, + false, + { body: 'resolved value', slots: [] } as CapData, + ]; + (kernelStore.getKernelPromise as unknown as MockInstance).mockReturnValue( + { + state: 'unresolved', + decider: vatId, + subscribers: [], + }, + ); + const resolveHandler = vi.fn(); + kernelQueue.subscriptions.set(kpid, resolveHandler); + kernelQueue.resolvePromises(vatId, [resolution]); + expect(kernelStore.enqueueRun).not.toHaveBeenCalledWith( + expect.objectContaining({ type: 'notify' }), + ); + expect(kernelStore.resolveKernelPromise).toHaveBeenCalledWith( + kpid, + false, + resolution[2], + ); + expect(resolveHandler).toHaveBeenCalledWith(resolution[2]); + expect(kernelQueue.subscriptions.has(kpid)).toBe(false); + }); + it('throws error if a promise is already resolved', () => { const vatId = 'v1'; const kpid = 'kp123'; diff --git a/packages/ocap-kernel/src/KernelQueue.ts b/packages/ocap-kernel/src/KernelQueue.ts index 740ee1ca7..280c2a4d6 100644 --- a/packages/ocap-kernel/src/KernelQueue.ts +++ b/packages/ocap-kernel/src/KernelQueue.ts @@ -59,20 +59,15 @@ export class KernelQueue { ): Promise { for await (const item of this.#runQueueItems()) { this.#kernelStore.nextTerminatedVatCleanup(); - this.#kernelStore.createCrankSavepoint('deliver'); const crankResults = await deliver(item); if (crankResults?.abort) { - // - consumeMessage=true (for hypothetical one-shot lifecycle ops): - // rolls back to 'deliver', discarding the message. - // - consumeMessage=false (default for sends/notifies): - // rolls back to 'start', re-queuing the message - // e.g., to "splat" against a now-terminated vat, rejecting - // its result to the sender. - // Currently, consumeMessage defaults to false as such lifecycle ops - // are not distinct run-queue item types here. - this.#kernelStore.rollbackCrank( - crankResults.consumeMessage ? 'deliver' : 'start', - ); + // Rollback the kernel state to before the failed delivery attempt. + // For active vats, this allows the message to be retried in a future crank. + // For terminated vats, the message will just go splat. + this.#kernelStore.rollbackCrank('start'); + // 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. } // Vat termination during delivery is triggered by an illegal syscall // or by syscall.exit(). diff --git a/packages/ocap-kernel/src/VatSyscall.test.ts b/packages/ocap-kernel/src/VatSyscall.test.ts index 57b9c450e..81e93a723 100644 --- a/packages/ocap-kernel/src/VatSyscall.test.ts +++ b/packages/ocap-kernel/src/VatSyscall.test.ts @@ -3,6 +3,7 @@ import type { VatOneResolution, VatSyscallObject, } from '@agoric/swingset-liveslots'; +import * as kernelUtils from '@metamask/kernel-utils'; import type { Logger } from '@metamask/logger'; import type { MockInstance } from 'vitest'; import { describe, it, expect, vi, beforeEach } from 'vitest'; @@ -256,4 +257,28 @@ describe('VatSyscall', () => { spy.mockRestore(); }); }); + + describe('waitForSyscallsToComplete', () => { + it('resolves immediately if pendingSyscalls is zero', async () => { + vatSys.pendingSyscalls = 0; + const delaySpy = vi.spyOn(kernelUtils, 'delay'); + await vatSys.waitForSyscallsToComplete(); + expect(delaySpy).not.toHaveBeenCalled(); + delaySpy.mockRestore(); + }); + + it('waits and resolves when pendingSyscalls becomes zero', async () => { + vatSys.pendingSyscalls = 2; + const delaySpy = vi + .spyOn(kernelUtils, 'delay') + .mockImplementation(async () => { + vatSys.pendingSyscalls -= 1; + return Promise.resolve(); + }); + await vatSys.waitForSyscallsToComplete(); + expect(delaySpy).toHaveBeenCalledTimes(2); + expect(vatSys.pendingSyscalls).toBe(0); + delaySpy.mockRestore(); + }); + }); }); diff --git a/packages/ocap-kernel/src/types.ts b/packages/ocap-kernel/src/types.ts index 3af503a64..58a69efa2 100644 --- a/packages/ocap-kernel/src/types.ts +++ b/packages/ocap-kernel/src/types.ts @@ -384,7 +384,6 @@ export const isGCAction = (value: unknown): value is GCAction => export type CrankResults = { didDelivery?: VatId; // the vat on which we made a delivery abort?: boolean; // changes should be discarded, not committed - consumeMessage?: boolean; // discard the aborted delivery terminate?: { vatId: VatId; reject: boolean; info: SwingSetCapData }; }; diff --git a/vitest.config.ts b/vitest.config.ts index 5c25a5aef..566c29108 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -134,10 +134,10 @@ export default defineConfig({ lines: 73.58, }, 'packages/ocap-kernel/**': { - statements: 91.07, + statements: 91.39, functions: 95.09, - branches: 81.07, - lines: 91.05, + branches: 81.99, + lines: 91.37, }, 'packages/streams/**': { statements: 100,