diff --git a/packages/kernel-store/CHANGELOG.md b/packages/kernel-store/CHANGELOG.md index 789a9e3d42..88c40465be 100644 --- a/packages/kernel-store/CHANGELOG.md +++ b/packages/kernel-store/CHANGELOG.md @@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- After SQLite rolls a transaction back on its own, as it can on a full disk or an I/O error, the wasm driver no longer throws `cannot commit - no transaction is active` from later savepoint releases ([#1089](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1089)) +- A read or write that fails in the wasm driver no longer makes every later call to the same operation throw `SQLITE_MISUSE` ([#1089](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1089)) +- After SQLite rolls a transaction back on its own, the Node driver commits the writes under a later savepoint instead of leaving them in a transaction that never commits ([#1089](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1089)) - Neither SQLite driver logs the contents of the store any more: the Node driver opens the database without `verbose`, and the wasm driver no longer logs kv keys and values. Both wrote vat state in plain text to whatever logger the embedder supplied ([#1086](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1086)) - `rollbackSavepoint` discards the enclosing transaction when `ROLLBACK TO` itself fails, instead of leaving the savepoint on its stack and the transaction open ([#1005](https://github.com/MetaMask/ocap-kernel/pull/1005)) - Nothing would ever commit or abort that transaction, so every later write on the connection silently joined it, reported success, and vanished on close. Discarding it is no wider than the caller asked for: the transaction begins with the outermost savepoint, so it holds only the work the rollback was abandoning diff --git a/packages/kernel-store/src/sqlite/nodejs.test.ts b/packages/kernel-store/src/sqlite/nodejs.test.ts index 28c15f4f8a..c4773a0d65 100644 --- a/packages/kernel-store/src/sqlite/nodejs.test.ts +++ b/packages/kernel-store/src/sqlite/nodejs.test.ts @@ -17,24 +17,59 @@ const mockKVDataForMap: [string, string][] = [ ['key2', 'value2'], ]; -const mockStatement = { +const makeMockStatement = () => ({ run: vi.fn(), get: vi.fn(), all: vi.fn(), pluck: vi.fn(), iterate: vi.fn(() => mockKVData), -}; +}); + +const mockStatement = makeMockStatement(); +const mockBegin = makeMockStatement(); +const mockCommit = makeMockStatement(); +const mockAbort = makeMockStatement(); const mockDb = { - prepare: vi.fn(() => mockStatement), + prepare: vi.fn((sql: string) => { + switch (sql) { + case SQL_QUERIES.BEGIN_TRANSACTION: + return mockBegin; + case SQL_QUERIES.COMMIT_TRANSACTION: + return mockCommit; + case SQL_QUERIES.ABORT_TRANSACTION: + return mockAbort; + default: + return mockStatement; + } + }), transaction: vi.fn((fn) => fn), exec: vi.fn(), + // better-sqlite3 reports this live, so the mock has to move it the way SQLite + // would: running BEGIN, COMMIT or ABORT is what changes it. inTransaction: false, _spStack: [] as string[], close: vi.fn(), }; +const resetStatements = (): void => { + [mockStatement, mockBegin, mockCommit, mockAbort].forEach((statement) => + Object.values(statement).forEach((mock) => mock.mockReset()), + ); + mockStatement.iterate.mockReturnValue(mockKVData); + mockBegin.run.mockImplementation(() => { + mockDb.inTransaction = true; + }); + mockCommit.run.mockImplementation(() => { + mockDb.inTransaction = false; + }); + mockAbort.run.mockImplementation(() => { + mockDb.inTransaction = false; + }); +}; +resetStatements(); + vi.mock('better-sqlite3', () => ({ default: vi.fn(function () { return mockDb; @@ -53,7 +88,9 @@ describe('makeSQLKernelDatabase', () => { const mockMkdir = vi.mocked(mkdir).mockResolvedValue(''); beforeEach(() => { - Object.values(mockStatement).forEach((mock) => mock.mockReset()); + resetStatements(); + mockDb.inTransaction = false; + mockDb._spStack = []; }); it('creates kv table', async () => { @@ -253,8 +290,6 @@ describe('makeSQLKernelDatabase', () => { describe('savepoint functionality', () => { beforeEach(() => { mockDb.exec.mockClear(); - mockDb.inTransaction = false; - mockDb._spStack = []; }); it('creates a savepoint using sanitized name', async () => { @@ -302,6 +337,7 @@ describe('makeSQLKernelDatabase', () => { it('createSavepoint begins transaction if needed', async () => { const db = await makeSQLKernelDatabase({}); db.createSavepoint('test_point'); + expect(mockBegin.run).toHaveBeenCalled(); expect(mockDb._spStack).toContain('test_point'); expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT test_point'); }); @@ -330,6 +366,8 @@ describe('makeSQLKernelDatabase', () => { mockDb._spStack = ['point1']; db.rollbackSavepoint('point1'); expect(mockDb._spStack).toStrictEqual([]); + expect(mockAbort.run).toHaveBeenCalled(); + expect(mockDb.inTransaction).toBe(false); }); // Otherwise every later write on this connection joins a transaction nothing @@ -348,9 +386,8 @@ describe('makeSQLKernelDatabase', () => { ); expect(mockDb._spStack).toStrictEqual([]); - // The abort is the only prepared statement this path runs. - expect(mockStatement.run).toHaveBeenCalledOnce(); - mockDb.inTransaction = false; + expect(mockAbort.run).toHaveBeenCalledOnce(); + expect(mockDb.inTransaction).toBe(false); }); // The rollback failure is the diagnosis; a failed abort on top of it only @@ -362,7 +399,7 @@ describe('makeSQLKernelDatabase', () => { mockDb.exec.mockImplementationOnce(() => { throw new Error('disk I/O error'); }); - mockStatement.run.mockImplementationOnce(() => { + mockAbort.run.mockImplementationOnce(() => { throw new Error('cannot rollback'); }); @@ -371,7 +408,7 @@ describe('makeSQLKernelDatabase', () => { ); expect(mockDb._spStack).toStrictEqual([]); - mockDb.inTransaction = false; + expect(mockAbort.run).toHaveBeenCalledOnce(); }); it('releaseSavepoint validates savepoint exists', async () => { @@ -398,17 +435,33 @@ describe('makeSQLKernelDatabase', () => { mockDb._spStack = ['point1']; db.releaseSavepoint('point1'); expect(mockDb._spStack).toStrictEqual([]); + expect(mockCommit.run).toHaveBeenCalled(); + expect(mockDb.inTransaction).toBe(false); }); it('supports nested savepoints', async () => { const db = await makeSQLKernelDatabase({}); db.createSavepoint('outer'); db.createSavepoint('inner'); + expect(mockBegin.run).toHaveBeenCalledOnce(); expect(mockDb._spStack).toStrictEqual(['outer', 'inner']); db.rollbackSavepoint('inner'); expect(mockDb._spStack).toStrictEqual(['outer']); db.releaseSavepoint('outer'); expect(mockDb._spStack).toStrictEqual([]); + expect(mockCommit.run).toHaveBeenCalledOnce(); + }); + + it('drops savepoints SQLite discarded with the transaction', async () => { + const db = await makeSQLKernelDatabase({}); + db.createSavepoint('t0'); + mockDb.inTransaction = false; + + db.createSavepoint('t1'); + db.releaseSavepoint('t1'); + + expect(mockDb._spStack).toStrictEqual([]); + expect(mockCommit.run).toHaveBeenCalledOnce(); }); }); diff --git a/packages/kernel-store/src/sqlite/nodejs.transactions.test.ts b/packages/kernel-store/src/sqlite/nodejs.transactions.test.ts new file mode 100644 index 0000000000..964d34eeeb --- /dev/null +++ b/packages/kernel-store/src/sqlite/nodejs.transactions.test.ts @@ -0,0 +1,30 @@ +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { describe, it, expect, onTestFinished } from 'vitest'; + +import { makeSQLKernelDatabase } from './nodejs.ts'; + +describe('the Node driver on real SQLite', () => { + it('commits a savepoint opened after SQLite ends the transaction itself', async () => { + const dir = await mkdtemp(join(tmpdir(), 'kernel-store-')); + onTestFinished(async () => rm(dir, { recursive: true, force: true })); + const dbFilename = join(dir, 'store.db'); + const kdb = await makeSQLKernelDatabase({ dbFilename }); + kdb.createSavepoint('t0'); + kdb.executeQuery('PRAGMA max_page_count = 1'); + expect(() => kdb.kernelKVStore.set('key', 'x'.repeat(100_000))).toThrow( + 'database or disk is full', + ); + kdb.executeQuery('PRAGMA max_page_count = 1073741823'); + + kdb.createSavepoint('t1'); + kdb.kernelKVStore.set('key', 'value'); + kdb.releaseSavepoint('t1'); + kdb.close(); + + const reopened = await makeSQLKernelDatabase({ dbFilename }); + expect(reopened.kernelKVStore.get('key')).toBe('value'); + reopened.close(); + }); +}); diff --git a/packages/kernel-store/src/sqlite/nodejs.ts b/packages/kernel-store/src/sqlite/nodejs.ts index 6d43debb4d..7ade49ce0f 100644 --- a/packages/kernel-store/src/sqlite/nodejs.ts +++ b/packages/kernel-store/src/sqlite/nodejs.ts @@ -157,6 +157,7 @@ export async function makeSQLKernelDatabase({ return false; } sqlBeginTransaction.run(); + db._spStack.length = 0; return true; } diff --git a/packages/kernel-store/src/sqlite/wasm.test.ts b/packages/kernel-store/src/sqlite/wasm.test.ts index d208247571..c0bb92e0d3 100644 --- a/packages/kernel-store/src/sqlite/wasm.test.ts +++ b/packages/kernel-store/src/sqlite/wasm.test.ts @@ -6,6 +6,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { SQL_QUERIES } from './common.ts'; import { getDBFolder } from './env.ts'; import { makeSQLKernelDatabase } from './wasm.ts'; +import type { KernelDatabase } from '../types.ts'; const mockKVData = [ { key: 'key1', value: 'value1' }, @@ -17,7 +18,7 @@ const mockKVDataForMap: [string, string][] = [ ['key2', 'value2'], ]; -const mockStatement = { +const makeMockStatement = () => ({ bind: vi.fn(), step: vi.fn(), getString: vi.fn(), @@ -25,16 +26,70 @@ const mockStatement = { get: vi.fn(), getColumnName: vi.fn(), columnCount: 2, +}); + +const mockStatement = makeMockStatement(); +const mockBegin = makeMockStatement(); +const mockCommit = makeMockStatement(); +const mockAbort = makeMockStatement(); + +// The driver asks SQLite whether a transaction is open, so the mock answers as +// SQLite would: stepping BEGIN, COMMIT or ABORT moves `txOpen`, which +// `sqlite3_get_autocommit` reports. +let txOpen = false; + +const resetStatements = (): void => { + [mockStatement, mockBegin, mockCommit, mockAbort].forEach((statement) => + Object.values(statement).forEach((value) => { + if (typeof value === 'function') { + value.mockReset(); + } + }), + ); + mockBegin.step.mockImplementation(() => { + txOpen = true; + return false; + }); + mockCommit.step.mockImplementation(() => { + txOpen = false; + return false; + }); + mockAbort.step.mockImplementation(() => { + txOpen = false; + return false; + }); }; +resetStatements(); -const mockDb = { +// `initDB` defines `inTransaction` non-configurably, so each call needs a fresh +// mock database. +const makeMockDb = () => ({ exec: vi.fn(), - prepare: vi.fn(() => mockStatement), - - _inTx: false, + prepare: vi.fn((sql: string) => { + switch (sql) { + case SQL_QUERIES.BEGIN_TRANSACTION: + return mockBegin; + case SQL_QUERIES.COMMIT_TRANSACTION: + return mockCommit; + case SQL_QUERIES.ABORT_TRANSACTION: + return mockAbort; + default: + return mockStatement; + } + }), + + pointer: 1, _spStack: [] as string[], close: vi.fn(), +}); + +let mockDb = makeMockDb(); + +const resetMocks = (): void => { + mockDb = makeMockDb(); + txOpen = false; + resetStatements(); }; const OpfsDbMock = vi.fn(function () { return mockDb; @@ -42,8 +97,10 @@ const OpfsDbMock = vi.fn(function () { const DBMock = vi.fn(function () { return mockDb; }); +const mockCapi = { sqlite3_get_autocommit: () => (txOpen ? 0 : 1) }; vi.mock('@sqlite.org/sqlite-wasm', () => ({ default: vi.fn(async () => ({ + capi: mockCapi, oo1: { OpfsDb: OpfsDbMock, DB: DBMock, @@ -56,14 +113,7 @@ vi.mock('./env.ts', () => ({ })); describe('makeSQLKernelDatabase', () => { - beforeEach(() => { - Object.values(mockStatement) - .filter( - (value): value is ReturnType => - typeof value === 'function', - ) - .forEach((mockFn) => mockFn.mockReset()); - }); + beforeEach(resetMocks); it('initializes with OPFS when available', async () => { await makeSQLKernelDatabase({}); @@ -76,6 +126,7 @@ describe('makeSQLKernelDatabase', () => { ).default.mockImplementationOnce( async () => ({ + capi: mockCapi, oo1: { OpfsDb: undefined, DB: vi.fn(function () { @@ -313,6 +364,22 @@ describe('makeSQLKernelDatabase', () => { expect(mockDb.exec).toHaveBeenCalledWith(SQL_QUERIES.CREATE_TABLE); }); + it('refuses a sqlite3 build without sqlite3_get_autocommit', async () => { + vi.mocked( + await import('@sqlite.org/sqlite-wasm'), + ).default.mockImplementationOnce( + async () => + ({ + capi: {}, + oo1: { OpfsDb: OpfsDbMock, DB: DBMock }, + }) as unknown as Sqlite3Static, + ); + + await expect(makeSQLKernelDatabase({})).rejects.toThrow( + 'sqlite3 capi lacks sqlite3_get_autocommit', + ); + }); + it('should log if logger is provided', async () => { const logger = { debug: vi.fn(), @@ -390,15 +457,37 @@ describe('makeSQLKernelDatabase', () => { ); expect(mockStatement.reset).toHaveBeenCalled(); }); - }); - describe('savepoint functionality', () => { - beforeEach(() => { - mockDb.exec.mockClear(); - mockDb._inTx = false; - mockDb._spStack = []; + it.each([ + ['get', (db: KernelDatabase) => db.kernelKVStore.get('key')], + [ + 'getNextKey', + (db: KernelDatabase) => db.kernelKVStore.getNextKey('key'), + ], + ['set', (db: KernelDatabase) => db.kernelKVStore.set('key', 'value')], + ['delete', (db: KernelDatabase) => db.kernelKVStore.delete('key')], + [ + 'a vatstore set', + (db: KernelDatabase) => + db.makeVatStore('v1').updateKVData([['key', 'value']], []), + ], + [ + 'a vatstore delete', + (db: KernelDatabase) => db.makeVatStore('v1').updateKVData([], ['key']), + ], + ['deleteVatStore', (db: KernelDatabase) => db.deleteVatStore('v1')], + ])('resets the statement when %s fails', async (_name, operation) => { + const db = await makeSQLKernelDatabase({}); + mockStatement.step.mockImplementationOnce(() => { + throw new Error('Database error'); + }); + + expect(() => operation(db)).toThrowError('Database error'); + expect(mockStatement.reset).toHaveBeenCalledOnce(); }); + }); + describe('savepoint functionality', () => { it('creates a savepoint using sanitized name', async () => { const db = await makeSQLKernelDatabase({}); db.createSavepoint('valid_name'); @@ -444,14 +533,14 @@ describe('makeSQLKernelDatabase', () => { it('createSavepoint begins transaction if needed', async () => { const db = await makeSQLKernelDatabase({}); db.createSavepoint('test_point'); - expect(mockDb._inTx).toBe(true); + expect(mockBegin.step).toHaveBeenCalled(); 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; + txOpen = true; mockDb._spStack = ['existing_point']; expect(() => db.rollbackSavepoint('nonexistent_point')).toThrowError( 'No such savepoint: nonexistent_point', @@ -460,7 +549,7 @@ describe('makeSQLKernelDatabase', () => { it('rollbackSavepoint removes all points after target', async () => { const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; + txOpen = true; mockDb._spStack = ['point1', 'point2', 'point3']; db.rollbackSavepoint('point2'); expect(mockDb._spStack).toStrictEqual(['point1']); @@ -469,18 +558,19 @@ describe('makeSQLKernelDatabase', () => { it('rollbackSavepoint closes transaction if no savepoints remain', async () => { const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; + txOpen = true; mockDb._spStack = ['point1']; db.rollbackSavepoint('point1'); expect(mockDb._spStack).toStrictEqual([]); - expect(mockDb._inTx).toBe(false); + expect(mockAbort.step).toHaveBeenCalled(); + expect(txOpen).toBe(false); }); // Otherwise every later write on this connection joins a transaction nothing // will ever commit, reports success, and vanishes on close. it('rollbackSavepoint discards the transaction when the rollback fails', async () => { const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; + txOpen = true; mockDb._spStack = ['point1']; mockDb.exec.mockImplementationOnce(() => { throw new Error('disk I/O error'); @@ -491,19 +581,20 @@ describe('makeSQLKernelDatabase', () => { ); expect(mockDb._spStack).toStrictEqual([]); - expect(mockDb._inTx).toBe(false); + expect(mockAbort.step).toHaveBeenCalled(); + expect(txOpen).toBe(false); }); // The rollback failure is the diagnosis; a failed abort on top of it only // repeats that the same connection is broken. it('rollbackSavepoint reports the rollback failure even if the abort fails too', async () => { const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; + txOpen = true; mockDb._spStack = ['point1']; mockDb.exec.mockImplementationOnce(() => { throw new Error('disk I/O error'); }); - mockStatement.step.mockImplementationOnce(() => { + mockAbort.step.mockImplementationOnce(() => { throw new Error('cannot rollback'); }); @@ -512,12 +603,12 @@ describe('makeSQLKernelDatabase', () => { ); expect(mockDb._spStack).toStrictEqual([]); - mockDb._inTx = false; + expect(mockAbort.step).toHaveBeenCalled(); }); it('releaseSavepoint validates savepoint exists', async () => { const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; + txOpen = true; mockDb._spStack = ['existing_point']; expect(() => db.releaseSavepoint('nonexistent_point')).toThrowError( 'No such savepoint: nonexistent_point', @@ -526,7 +617,7 @@ describe('makeSQLKernelDatabase', () => { it('releaseSavepoint removes all points after target', async () => { const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; + txOpen = true; mockDb._spStack = ['point1', 'point2', 'point3']; db.releaseSavepoint('point2'); expect(mockDb._spStack).toStrictEqual(['point1']); @@ -535,11 +626,12 @@ describe('makeSQLKernelDatabase', () => { it('releaseSavepoint commits transaction if no savepoints remain', async () => { const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; + txOpen = true; mockDb._spStack = ['point1']; db.releaseSavepoint('point1'); expect(mockDb._spStack).toStrictEqual([]); - expect(mockDb._inTx).toBe(false); + expect(mockCommit.step).toHaveBeenCalled(); + expect(txOpen).toBe(false); }); it('supports nested savepoints', async () => { @@ -549,10 +641,12 @@ describe('makeSQLKernelDatabase', () => { expect(mockDb._spStack).toStrictEqual(['outer', 'inner']); db.rollbackSavepoint('inner'); expect(mockDb._spStack).toStrictEqual(['outer']); - expect(mockDb._inTx).toBe(true); + expect(mockBegin.step).toHaveBeenCalledOnce(); + expect(txOpen).toBe(true); db.releaseSavepoint('outer'); expect(mockDb._spStack).toStrictEqual([]); - expect(mockDb._inTx).toBe(false); + expect(mockCommit.step).toHaveBeenCalledOnce(); + expect(txOpen).toBe(false); }); }); @@ -571,21 +665,6 @@ describe('makeSQLKernelDatabase', () => { 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')).toThrowError('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) { @@ -623,20 +702,11 @@ describe('makeSQLKernelDatabase', () => { }); 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 = []; - }); + beforeEach(resetMocks); it('safeMutate rollbacks transaction on error', async () => { const db = await makeSQLKernelDatabase({}); - mockDb._inTx = false; + txOpen = false; mockDb._spStack = []; mockStatement.step.mockImplementationOnce(() => { throw new Error('Database error'); @@ -645,21 +715,34 @@ describe('transaction management', () => { expect(() => vatStore.updateKVData([['key', 'value']], [])).toThrowError( 'Database error', ); - expect(mockStatement.step).toHaveBeenCalled(); + expect(mockAbort.step).toHaveBeenCalled(); + expect(txOpen).toBe(false); + }); + + it('reports the write failure, not a rollback with nothing to undo', async () => { + const db = await makeSQLKernelDatabase({}); + mockStatement.step.mockImplementationOnce(() => { + // SQLite ends the transaction as it fails the write. + txOpen = false; + throw new Error('database or disk is full'); + }); + mockAbort.step.mockImplementation(() => { + throw new Error('cannot rollback - no transaction is active'); + }); + + expect(() => + db.makeVatStore('test-vat').updateKVData([['key', 'value']], []), + ).toThrowError('database or disk is full'); }); it('safeMutate does not commit if already in transaction', async () => { const db = await makeSQLKernelDatabase({}); - mockDb._inTx = true; + txOpen = 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' }), - ); + expect(mockBegin.step).not.toHaveBeenCalled(); + expect(mockCommit.step).not.toHaveBeenCalled(); }); describe('close functionality', () => { diff --git a/packages/kernel-store/src/sqlite/wasm.transactions.test.ts b/packages/kernel-store/src/sqlite/wasm.transactions.test.ts new file mode 100644 index 0000000000..bd8d885154 --- /dev/null +++ b/packages/kernel-store/src/sqlite/wasm.transactions.test.ts @@ -0,0 +1,129 @@ +import { describe, it, expect } from 'vitest'; + +import { initDB, makeSQLKernelDatabase } from './wasm.ts'; +import type { KernelDatabase } from '../types.ts'; + +// Kept out of `wasm.test.ts`, whose `vi.mock` replaces sqlite-wasm for the +// whole file. + +const makeDb = async (): Promise => + makeSQLKernelDatabase({ dbFilename: ':memory:' }); + +// The SQLITE_FULL also rolls back any open transaction. +const failWithFullDisk = (kdb: KernelDatabase, write: () => void): void => { + // SQLite raises a limit below the database's current size to that size. + kdb.executeQuery('PRAGMA max_page_count = 1'); + expect(write).toThrow('database or disk is full'); + kdb.executeQuery('PRAGMA max_page_count = 1073741823'); +}; + +const tooBig = 'x'.repeat(100_000); + +describe('the wasm driver on real SQLite', () => { + it('undoes the writes a rolled-back savepoint covers', async () => { + const kdb = await makeDb(); + kdb.kernelKVStore.set('kept', 'before'); + kdb.createSavepoint('t0'); + kdb.kernelKVStore.set('undone', 'during'); + kdb.kernelKVStore.set('kept', 'during'); + kdb.rollbackSavepoint('t0'); + + expect(kdb.kernelKVStore.get('undone')).toBeUndefined(); + expect(kdb.kernelKVStore.get('kept')).toBe('before'); + }); + + it('keeps the writes a released savepoint covers', async () => { + const kdb = await makeDb(); + kdb.createSavepoint('t0'); + kdb.kernelKVStore.set('key', 'value'); + kdb.releaseSavepoint('t0'); + + expect(kdb.kernelKVStore.get('key')).toBe('value'); + }); + + it('rolls the inner savepoint back without disturbing the outer one', async () => { + const kdb = await makeDb(); + kdb.createSavepoint('t0'); + kdb.kernelKVStore.set('outer', 'yes'); + kdb.createSavepoint('t1'); + kdb.kernelKVStore.set('inner', 'yes'); + kdb.rollbackSavepoint('t1'); + kdb.releaseSavepoint('t0'); + + expect(kdb.kernelKVStore.get('outer')).toBe('yes'); + expect(kdb.kernelKVStore.get('inner')).toBeUndefined(); + }); + + it('opens a transaction for the outermost savepoint', async () => { + const kdb = await makeDb(); + kdb.createSavepoint('t0'); + kdb.kernelKVStore.set('key', 'value'); + // A transaction the `SAVEPOINT` opened ends here; one `BEGIN` opened does not. + kdb.executeQuery('RELEASE SAVEPOINT t0'); + + // SQLite refuses this unless a transaction is open. + expect(() => kdb.executeQuery('ROLLBACK TRANSACTION')).not.toThrow(); + expect(kdb.kernelKVStore.get('key')).toBeUndefined(); + }); + + it.each([ + [ + 'kv', + (kdb: KernelDatabase, value: string) => + kdb.kernelKVStore.set('key', value), + (kdb: KernelDatabase) => kdb.kernelKVStore.get('key'), + ], + [ + 'vatstore', + (kdb: KernelDatabase, value: string) => + kdb.makeVatStore('v1').updateKVData([['key', value]], []), + (kdb: KernelDatabase) => + new Map(kdb.makeVatStore('v1').getKVData()).get('key'), + ], + ])('keeps taking %s writes after one fails', async (_name, write, read) => { + const kdb = await makeDb(); + failWithFullDisk(kdb, () => write(kdb, tooBig)); + + write(kdb, 'value'); + + expect(read(kdb)).toBe('value'); + }); + + it('reports no transaction once the database is closed', async () => { + const db = await initDB(':memory:'); + db.exec('BEGIN TRANSACTION'); + expect(db.inTransaction).toBe(true); + + db.close(); + + expect(db.inTransaction).toBe(false); + }); + + it('commits a write made after SQLite ends the transaction itself', async () => { + const kdb = await makeDb(); + kdb.createSavepoint('t0'); + failWithFullDisk(kdb, () => kdb.kernelKVStore.set('key', tooBig)); + + kdb.makeVatStore('v1').updateKVData([['key', 'value']], []); + + expect(() => kdb.executeQuery('COMMIT TRANSACTION')).toThrow( + 'cannot commit - no transaction is active', + ); + expect(kdb.makeVatStore('v1').getKVData()).toStrictEqual([ + ['key', 'value'], + ]); + }); + + it('takes the next crank in a transaction of its own', async () => { + const kdb = await makeDb(); + kdb.createSavepoint('t0'); + failWithFullDisk(kdb, () => kdb.kernelKVStore.set('key', tooBig)); + expect(() => kdb.rollbackSavepoint('t0')).toThrow('no such savepoint: t0'); + + kdb.createSavepoint('t0'); + kdb.kernelKVStore.set('key', 'value'); + kdb.releaseSavepoint('t0'); + + expect(kdb.kernelKVStore.get('key')).toBe('value'); + }); +}); diff --git a/packages/kernel-store/src/sqlite/wasm.ts b/packages/kernel-store/src/sqlite/wasm.ts index 5c5db25b0e..c35add6514 100644 --- a/packages/kernel-store/src/sqlite/wasm.ts +++ b/packages/kernel-store/src/sqlite/wasm.ts @@ -1,5 +1,8 @@ import { Logger } from '@metamask/logger'; -import type { Database as SqliteDatabase } from '@sqlite.org/sqlite-wasm'; +import type { + Database as SqliteDatabase, + Sqlite3Static, +} from '@sqlite.org/sqlite-wasm'; import sqlite3InitModule from '@sqlite.org/sqlite-wasm'; import { @@ -11,11 +14,18 @@ import { getDBFolder } from './env.ts'; import type { KVStore, VatStore, KernelDatabase } from '../types.ts'; export type Database = SqliteDatabase & { - _inTx: boolean; + /** + * Asked of SQLite on every read, never cached: SQLite can roll a transaction + * back on its own after an error such as SQLITE_FULL or SQLITE_IOERR. + */ + readonly inTransaction: boolean; // stack of active savepoint names _spStack: string[]; }; +/** `sqlite3_get_autocommit` is bound by the wasm build but absent from its types. */ +type AutocommitCapi = { sqlite3_get_autocommit: (pDb: number) => number }; + /** * Ensure that SQLite is initialized. * @@ -28,6 +38,12 @@ export async function initDB( logger?: Logger, ): Promise { const sqlite3 = await sqlite3InitModule(); + const { sqlite3_get_autocommit: getAutocommit } = + sqlite3.capi as Sqlite3Static['capi'] & AutocommitCapi; + if (typeof getAutocommit !== 'function') { + throw Error('sqlite3 capi lacks sqlite3_get_autocommit'); + } + let db: SqliteDatabase; if (sqlite3.oo1.OpfsDb) { @@ -41,7 +57,10 @@ export async function initDB( } const dbWithTx = db as Database; - dbWithTx._inTx = false; + Object.defineProperty(dbWithTx, 'inTransaction', { + get: () => + dbWithTx.pointer !== undefined && getAutocommit(dbWithTx.pointer) === 0, + }); dbWithTx._spStack = []; return dbWithTx; @@ -66,15 +85,20 @@ function makeKVStore(db: Database): KVStore { * @returns The value at that key. */ function kvGet(key: string, required: boolean): string | undefined { - sqlKVGet.bind([key]); - if (sqlKVGet.step()) { - const result = sqlKVGet.getString(0); - if (result) { - sqlKVGet.reset(); - return result; + let result: string | null = null; + // Reset even when the step throws: SQLite refuses to bind a statement left + // unreset, so one failure would fail every later call. + try { + sqlKVGet.bind([key]); + if (sqlKVGet.step()) { + result = sqlKVGet.getString(0); } + } finally { + sqlKVGet.reset(); + } + if (result) { + return result; } - sqlKVGet.reset(); if (required) { throw Error(`no record matching key '${key}'`); } @@ -92,15 +116,18 @@ function makeKVStore(db: Database): KVStore { * last key in the store. */ function kvGetNextKey(previousKey: string): string | undefined { - sqlKVGetNextKey.bind([previousKey]); - if (sqlKVGetNextKey.step()) { - const result = sqlKVGetNextKey.getString(0); - if (result) { - sqlKVGetNextKey.reset(); - return result; + let result: string | null = null; + try { + sqlKVGetNextKey.bind([previousKey]); + if (sqlKVGetNextKey.step()) { + result = sqlKVGetNextKey.getString(0); } + } finally { + sqlKVGetNextKey.reset(); + } + if (result) { + return result; } - sqlKVGetNextKey.reset(); return undefined; } @@ -113,9 +140,12 @@ function makeKVStore(db: Database): KVStore { * @param value - The value to assign to it. */ function kvSet(key: string, value: string): void { - sqlKVSet.bind([key, value]); - sqlKVSet.step(); - sqlKVSet.reset(); + try { + sqlKVSet.bind([key, value]); + sqlKVSet.step(); + } finally { + sqlKVSet.reset(); + } } const sqlKVDelete = db.prepare(SQL_QUERIES.DELETE); @@ -126,9 +156,12 @@ function makeKVStore(db: Database): KVStore { * @param key - The key to remove. */ function kvDelete(key: string): void { - sqlKVDelete.bind([key]); - sqlKVDelete.step(); - sqlKVDelete.reset(); + try { + sqlKVDelete.bind([key]); + sqlKVDelete.step(); + } finally { + sqlKVDelete.reset(); + } } return { @@ -180,12 +213,14 @@ export async function makeSQLKernelDatabase({ * @returns True if a new transaction was started, false if already in one */ function beginIfNeeded(): boolean { - if (db._inTx) { + if (db.inTransaction) { return false; } sqlBeginTransaction.step(); sqlBeginTransaction.reset(); - db._inTx = true; + // Any name still here died with a transaction SQLite ended, and would keep + // `commitIfNeeded` from ever committing this one. + db._spStack.length = 0; return true; } @@ -193,10 +228,9 @@ export async function makeSQLKernelDatabase({ * Commit a transaction if one is active and no savepoints remain */ function commitIfNeeded(): void { - if (db._inTx && db._spStack.length === 0) { + if (db.inTransaction && db._spStack.length === 0) { sqlCommitTransaction.step(); sqlCommitTransaction.reset(); - db._inTx = false; } } @@ -204,10 +238,9 @@ export async function makeSQLKernelDatabase({ * Rollback a transaction */ function rollbackIfNeeded(): void { - if (db._inTx) { + if (db.inTransaction) { sqlAbortTransaction.step(); sqlAbortTransaction.reset(); - db._inTx = false; db._spStack.length = 0; } } @@ -307,14 +340,20 @@ export async function makeSQLKernelDatabase({ function updateKVData(sets: [string, string][], deletes: string[]): void { safeMutate(() => { for (const [key, value] of sets) { - sqlVatstoreSet.bind([vatID, key, value]); - sqlVatstoreSet.step(); - sqlVatstoreSet.reset(); + try { + sqlVatstoreSet.bind([vatID, key, value]); + sqlVatstoreSet.step(); + } finally { + sqlVatstoreSet.reset(); + } } for (const value of deletes) { - sqlVatstoreDelete.bind([vatID, value]); - sqlVatstoreDelete.step(); - sqlVatstoreDelete.reset(); + try { + sqlVatstoreDelete.bind([vatID, value]); + sqlVatstoreDelete.step(); + } finally { + sqlVatstoreDelete.reset(); + } } }); } @@ -331,9 +370,12 @@ export async function makeSQLKernelDatabase({ * @param vatId - The vat whose store is to be deleted. */ function deleteVatStore(vatId: string): void { - sqlVatstoreDeleteAll.bind([vatId]); - sqlVatstoreDeleteAll.step(); - sqlVatstoreDeleteAll.reset(); + try { + sqlVatstoreDeleteAll.bind([vatId]); + sqlVatstoreDeleteAll.step(); + } finally { + sqlVatstoreDeleteAll.reset(); + } } /**