From 3c202c653fb11d3e1580a07b3354140e7c79e217 Mon Sep 17 00:00:00 2001 From: Imran Munir Date: Sat, 8 Aug 2026 07:33:32 +0100 Subject: [PATCH] fix(wallet-toolbox): pass trx to readSettings from verifyReadyForDatabaseAccess StorageKnex.readSettings took no trx parameter and called this.toDb() with no argument, so it always ran on the pool. verifyReadyForDatabaseAccess(trx) then dropped its own trx when lazily populating the settings cache: this._settings ??= await this.readSettings() knex forces {min:1,max:1} on the sqlite dialect, so when that cache is cold and the first access happens inside a caller's transaction, the settings read asks for a second connection that can never be granted: the transaction will not release until the query returns, and the query cannot run until the transaction releases. It fails with "KnexTimeoutError: Timeout acquiring a connection. The pool is probably full. Are you missing a .transacting(trx) call?". verifyReadyForDatabaseAccess is on every write path, so any StorageKnex whose first database access is transactional self-deadlocks. It was already using trx correctly for the PRAGMA two lines below. This is the same defect as ef710c3 (#426) fixed in getProvenOrRawTx, and the last remaining toDb() call in the file that cannot receive a caller's transaction. The abstract already declared the parameter -- StorageReader.readSettings(trx?: sdk.TrxToken) -- and StorageIdb, StorageMySQLDojoReader and getBeefForTxid all accept it. Only StorageKnex dropped it, so this restores the existing contract rather than changing an API. Adds a regression test with acquireConnectionTimeout lowered to 5s so a regression fails in seconds rather than the 60s default. Verified it fails without the fix (both cases, with the KnexTimeoutError above) and passes with it. #426 corrected getProvenOrRawTx without locking the behaviour down; this covers both call sites. --- .../wallet-toolbox/src/storage/StorageKnex.ts | 6 +- .../storage/__test/readSettingsTrx.test.ts | 88 +++++++++++++++++++ 2 files changed, 91 insertions(+), 3 deletions(-) create mode 100644 packages/wallet/wallet-toolbox/src/storage/__test/readSettingsTrx.test.ts diff --git a/packages/wallet/wallet-toolbox/src/storage/StorageKnex.ts b/packages/wallet/wallet-toolbox/src/storage/StorageKnex.ts index 01f7f67cc..0715a817f 100644 --- a/packages/wallet/wallet-toolbox/src/storage/StorageKnex.ts +++ b/packages/wallet/wallet-toolbox/src/storage/StorageKnex.ts @@ -134,8 +134,8 @@ export class StorageKnex extends StorageProvider implements WalletStorageProvide protected override supportsActionBatchPersistence (): boolean { return true } protected override requiresActionBatchCleanupBeforeCreateAction (): boolean { return false } - async readSettings (): Promise { - return this.validateEntity(verifyOne(await this.toDb()('settings'))) + async readSettings (trx?: TrxToken): Promise { + return this.validateEntity(verifyOne(await this.toDb(trx)('settings'))) } override async getProvenOrRawTx (txid: string, trx?: TrxToken): Promise { @@ -1317,7 +1317,7 @@ export class StorageKnex extends StorageProvider implements WalletStorageProvide * @param trx */ async verifyReadyForDatabaseAccess (trx?: TrxToken): Promise { - this._settings ??= await this.readSettings() + this._settings ??= await this.readSettings(trx) // Always run the PRAGMA for SQLite to ensure foreign key constraints are enabled. // This is necessary because PRAGMA foreign_keys is a per-connection setting, diff --git a/packages/wallet/wallet-toolbox/src/storage/__test/readSettingsTrx.test.ts b/packages/wallet/wallet-toolbox/src/storage/__test/readSettingsTrx.test.ts new file mode 100644 index 000000000..27ed6499f --- /dev/null +++ b/packages/wallet/wallet-toolbox/src/storage/__test/readSettingsTrx.test.ts @@ -0,0 +1,88 @@ +import { knex as makeKnex } from 'knex' +import { _tu } from '../../../test/utils/TestUtilsWalletStorage' +import { sdk } from '../../index.client' +import { StorageKnex } from '../StorageKnex' + +/** + * Regression test for a self-deadlock on single-connection pools. + * + * knex forces `{ min: 1, max: 1 }` on the sqlite dialect, so there is exactly one connection. + * Any query issued against `this.knex` while a caller's transaction holds that connection can + * never be granted: the transaction will not release until the query returns, and the query + * cannot run until the transaction releases. It fails with + * `KnexTimeoutError: Timeout acquiring a connection... Are you missing a .transacting(trx) call?` + * + * `verifyReadyForDatabaseAccess(trx)` is on every write path and lazily populates `_settings`. + * When that cache is cold and the first access happens inside a transaction, `readSettings` + * must run on the caller's transaction rather than the pool, or the whole write deadlocks. + * + * `acquireConnectionTimeout` is lowered so a regression fails in seconds rather than the + * 60s default. + */ +describe('readSettings honours a caller-supplied transaction', () => { + jest.setTimeout(60000) + + const chain: sdk.Chain = 'test' + let dbFile: string + + beforeAll(async () => { + // Migrate once so a settings row exists, then drop this storage entirely: the test needs a + // StorageKnex whose `_settings` cache is COLD, which `makeAvailable()` would defeat. + dbFile = await _tu.newTmpFile('readsettingstrx.sqlite', false, false, false) + const seed = new StorageKnex({ + ...StorageKnex.defaultOptions(), + chain, + knex: _tu.createLocalSQLite(dbFile) + }) + await seed.dropAllData() + await seed.migrate('readSettings trx test', '1'.repeat(64)) + await seed.destroy() + }) + + test('verifyReadyForDatabaseAccess does not deadlock when settings are read inside a transaction', async () => { + const knex = makeKnex({ + client: 'better-sqlite3', + connection: { filename: dbFile }, + useNullAsDefault: true, + pool: { min: 1, max: 1 }, + acquireConnectionTimeout: 5000 + }) + const storage = new StorageKnex({ ...StorageKnex.defaultOptions(), chain, knex }) + + try { + // Cold cache: the settings read below is this instance's first database access. + expect(storage['_settings']).toBeUndefined() + + const dbtype = await knex.transaction(async trx => { + return await storage.verifyReadyForDatabaseAccess(trx as unknown as sdk.TrxToken) + }) + + expect(dbtype).toBe('SQLite') + expect(storage['_settings']).toBeDefined() + } finally { + await storage.destroy() + } + }) + + test('readSettings accepts a transaction directly', async () => { + const knex = makeKnex({ + client: 'better-sqlite3', + connection: { filename: dbFile }, + useNullAsDefault: true, + pool: { min: 1, max: 1 }, + acquireConnectionTimeout: 5000 + }) + const storage = new StorageKnex({ ...StorageKnex.defaultOptions(), chain, knex }) + + try { + const settings = await knex.transaction(async trx => { + return await storage.readSettings(trx as unknown as sdk.TrxToken) + }) + + expect(settings.chain).toBe(chain) + expect(settings.dbtype).toBe('SQLite') + } finally { + await storage.destroy() + } + }) +})