feat(kernel): Handle syscall failures

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 () => {

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, assertSafeIdentifier } from './common.ts';
 
 describe('SQL_QUERIES', () => {
   // XXX Is this test actually useful? It's basically testing that the source code matches itself.
@@ -40,10 +40,12 @@ 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',
       'COMMIT_TRANSACTION',
+      'CREATE_SAVEPOINT',
       'CREATE_TABLE',
       'CREATE_TABLE_VS',
       'DELETE',
@@ -54,8 +56,46 @@ describe('SQL_QUERIES', () => {
       'GET',
       'GET_ALL_VS',
       'GET_NEXT',
+      'RELEASE_SAVEPOINT',
+      'ROLLBACK_SAVEPOINT',
       'SET',
       'SET_VS',
     ]);
   });
 });
+
+describe('assertSafeIdentifier', () => {
+  it('accepts valid SQL identifiers', () => {
+    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', () => {
+    // Starting with a number
+    expect(() => assertSafeIdentifier('123invalid')).toThrow(
+      'Invalid identifier',
+    );
+
+    // Containing invalid characters
+    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(() => assertSafeIdentifier("name'; DROP TABLE users--")).toThrow(
+      'Invalid identifier',
+    );
+  });
+});

packages/kernel-store/src/sqlite/common.ts

@@ -52,30 +52,35 @@ 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`,
+  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
+  // 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.
+ */
+export function assertSafeIdentifier(name: string): void {
+  if (!/^[A-Za-z_]\w*$/u.test(name)) {
+    throw new Error(`Invalid identifier: ${name}`);
+  }
+}

packages/kernel-store/src/sqlite/nodejs.test.ts

@@ -25,6 +25,10 @@ const mockStatement = {
 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', () => ({
@@ -137,6 +141,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 +198,124 @@ describe('makeSQLKernelDatabase', () => {
       });
     });
   });
+
+  describe('savepoint functionality', () => {
+    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(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.createSavepoint('test_point');
+      db.rollbackSavepoint('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(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([]);
+    });
+  });
 });