From 1a5bc63fb530c6f3307571b4150d74d8dba2559a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Fri, 3 Jul 2026 15:45:29 -0700 Subject: [PATCH 1/3] fix(driver-sql): pre-create autonumber sequences table to avoid batch/tx deadlock Lazy creation of _objectstack_sequences on the first autonumber INSERT asked the pool for a second connection. Inside a /api/v1/batch transaction on SQLite (pool max=1) the only connection is already held, so the acquire blocked until 'Knex: Timeout acquiring a connection'. Postgres/MySQL hit the same pool exhaustion under concurrent cold first-writes. - initObjects now pre-creates the counter table outside any data transaction. - The lazy fallback runs its DDL on the caller's transaction on SQLite only (never on MySQL, where DDL would implicitly commit the caller's transaction). - Regression test reproduces the deadlock (fails on old code, passes now). --- .changeset/fix-autonumber-batch-deadlock.md | 22 ++++ .../src/sql-driver-autonumber-tx.test.ts | 119 ++++++++++++++++++ packages/plugins/driver-sql/src/sql-driver.ts | 81 +++++++++--- 3 files changed, 202 insertions(+), 20 deletions(-) create mode 100644 .changeset/fix-autonumber-batch-deadlock.md create mode 100644 packages/plugins/driver-sql/src/sql-driver-autonumber-tx.test.ts diff --git a/.changeset/fix-autonumber-batch-deadlock.md b/.changeset/fix-autonumber-batch-deadlock.md new file mode 100644 index 0000000000..e595c7f019 --- /dev/null +++ b/.changeset/fix-autonumber-batch-deadlock.md @@ -0,0 +1,22 @@ +--- +'@objectstack/driver-sql': patch +--- + +Fix a connection-pool deadlock when the first `auto_number` write after process +start goes through a transaction (e.g. `POST /api/v1/batch`, which wraps every +operation in one `ql.transaction(...)`). + +The sequence-counter table (`_objectstack_sequences`) was created lazily on the +first autonumber INSERT via a bare `this.knex.schema.*` call that asks the pool +for a second connection. On SQLite (better-sqlite3, pool max=1) the open batch +transaction already holds the only connection, so the acquire blocked until +`Knex: Timeout acquiring a connection`. Postgres/MySQL are exposed to the same +pool-exhaustion deadlock under concurrent cold first-writes. + +Fixes: +- `initObjects` now pre-creates the counter table up front, outside any data + transaction, so the first write never runs DDL (primary fix). +- The lazy fallback (`ensureSequencesTable`) now runs its DDL on the caller's own + transaction on SQLite instead of grabbing a second connection. It deliberately + does not route DDL through the caller's transaction on MySQL, where DDL would + implicitly commit the caller's in-flight transaction. diff --git a/packages/plugins/driver-sql/src/sql-driver-autonumber-tx.test.ts b/packages/plugins/driver-sql/src/sql-driver-autonumber-tx.test.ts new file mode 100644 index 0000000000..0864f92113 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-autonumber-tx.test.ts @@ -0,0 +1,119 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +// +// Regression guard for the auto_number-in-transaction deadlock. +// +// Reported: on SQLite (better-sqlite3, pool max=1), the first autonumber write +// after process start that goes through a transaction — e.g. POST /api/v1/batch, +// which wraps every operation in one `ql.transaction(...)` — dead-locked with +// "Knex: Timeout acquiring a connection. The pool is probably full." +// +// Root cause: the sequence-counter table (`_objectstack_sequences`) was created +// lazily on the first autonumber INSERT, via a bare `this.knex.schema.*` call +// that asks the pool for a SECOND connection. Inside a batch transaction the +// only pooled connection is already held, so the acquire blocked until timeout. +// +// Fixes under test: +// 1. `initObjects` pre-creates the table outside any data transaction, so the +// first write never runs DDL (primary fix — covers the real batch path). +// 2. The lazy fallback (`ensureSequencesTable`) runs its DDL on the caller's +// own transaction on SQLite instead of grabbing a second connection, so +// even a cold-cache in-transaction first write cannot deadlock. + +import { describe, it, expect, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +const SEQUENCES_TABLE = '_objectstack_sequences'; + +function withTimeout(p: Promise, ms: number, label: string): Promise { + return Promise.race([ + p, + new Promise((_, rej) => setTimeout(() => rej(new Error(`TIMEOUT: ${label} (deadlock)`)), ms)), + ]); +} + +describe('SqlDriver auto_number in-transaction (deadlock regression)', () => { + let driver: SqlDriver | undefined; + + afterEach(async () => { + if (driver) await driver.disconnect(); + driver = undefined; + }); + + async function setup() { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([ + { + name: 'contract', + fields: { + organization_id: { type: 'string' }, + contract_number: { type: 'autonumber', format: 'CTR-{0000}' }, + name: { type: 'string' }, + }, + }, + ]); + return (driver as any).knex; + } + + it('pre-creates the sequences table during initObjects (before any write)', async () => { + const k = await setup(); + // The counter table must exist immediately after initObjects, without a + // single create() — this is what keeps the first write off the DDL path. + expect(await k.schema.hasTable(SEQUENCES_TABLE)).toBe(true); + expect((driver as any).sequencesTableReady).toBe(true); + }); + + it('commits a batch-style transaction whose FIRST write fills an autonumber', async () => { + const k = await setup(); + + // Two autonumber creates inside a single transaction, mirroring how the REST + // /batch endpoint wraps operations. Must commit, not hang. + const trx = await driver!.beginTransaction(); + const r1 = await withTimeout( + driver!.create('contract', { organization_id: 'org_x', name: 'A' }, { transaction: trx } as any), + 6000, + 'batch create #1 (autonumber)', + ); + const r2 = await withTimeout( + driver!.create('contract', { organization_id: 'org_x', name: 'B' }, { transaction: trx } as any), + 6000, + 'batch create #2 (autonumber)', + ); + await withTimeout(driver!.commit(trx), 6000, 'commit'); + + expect(r1.contract_number).toBe('CTR-0001'); + expect(r2.contract_number).toBe('CTR-0002'); + + const rows = await k('contract').select('contract_number').orderBy('contract_number'); + expect(rows.map((r: any) => r.contract_number)).toEqual(['CTR-0001', 'CTR-0002']); + }); + + it('does not deadlock even with a COLD cache inside the transaction (lazy fallback)', async () => { + const k = await setup(); + + // Simulate the path where the table was NOT pre-created (an external object, + // or a consumer that writes without initObjects): drop it and clear the + // process cache, then take the single connection with a transaction and let + // the first autonumber write hit the lazy ensure. On the old code this asked + // the pool for a second connection and blocked until acquire-timeout. + await k.schema.dropTableIfExists(SEQUENCES_TABLE); + (driver as any).sequencesTableReady = false; + (driver as any).sequencesHasKeyHash = false; + (driver as any).sequencesTableEnsurePromise = null; + + const trx = await driver!.beginTransaction(); + const r1 = await withTimeout( + driver!.create('contract', { organization_id: 'org_cold', name: 'C' }, { transaction: trx } as any), + 6000, + 'cold-cache create inside tx', + ); + await withTimeout(driver!.commit(trx), 6000, 'commit'); + + expect(r1.contract_number).toBe('CTR-0001'); + // The table the fallback created on the transaction survives the commit. + expect(await k.schema.hasTable(SEQUENCES_TABLE)).toBe(true); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 98552dc9ad..c8d2962f49 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -739,29 +739,42 @@ export class SqlDriver implements IDataDriver { * generous non-indexed column. Fixed-prefix formats use the empty scope and * keep their single global counter (backward compatible). */ - protected async ensureSequencesTable(): Promise { + protected async ensureSequencesTable(parentTrx?: Knex.Transaction): Promise { if (this.sequencesTableReady) return; if (this.sequencesTableEnsurePromise) { await this.sequencesTableEnsurePromise; return; } + // Which connection runs the DDL below. Normally a fresh pooled connection + // (`this.knex`), because `initObjects` pre-creates the table outside any data + // transaction. This lazy path is the fallback (e.g. an external object, or a + // consumer that writes without `initObjects`). If we are already inside the + // caller's transaction AND the pool can only ever hand out one connection + // (SQLite, pool max=1), that connection is busy with the open transaction — + // a bare `this.knex` here would block forever acquiring a second one and then + // fail with a Knex acquire-timeout (the reported batch/autonumber deadlock). + // Run the DDL on the caller's own transaction instead; SQLite permits DDL + // inside a transaction. We deliberately do NOT route DDL through `parentTrx` + // on MySQL, where DDL implicitly commits the caller's transaction; there the + // roomy pool (max=10) lets a fresh connection create the table safely. + const runner: Knex | Knex.Transaction = parentTrx && this.isSqlite ? parentTrx : this.knex; this.sequencesTableEnsurePromise = (async () => { - const exists = await this.knex.schema.hasTable(SEQUENCES_TABLE); + const exists = await runner.schema.hasTable(SEQUENCES_TABLE); if (!exists) { try { - await this.createSequencesTable(SEQUENCES_TABLE); + await this.createSequencesTable(SEQUENCES_TABLE, runner); this.sequencesHasKeyHash = true; } catch (err: any) { // Race or cross-process create — re-check existence; ignore // "already exists" errors from any dialect. - const stillMissing = !(await this.knex.schema.hasTable(SEQUENCES_TABLE)); + const stillMissing = !(await runner.schema.hasTable(SEQUENCES_TABLE)); if (stillMissing) throw err; // A racing creator may have used an older schema. Migrate in place. - await this.ensureSequencesKeyHashShape(); + await this.ensureSequencesKeyHashShape(runner); } } else { // Pre-existing table may predate the `key_hash`/`scope` shape. Migrate. - await this.ensureSequencesKeyHashShape(); + await this.ensureSequencesKeyHashShape(runner); } this.sequencesTableReady = true; })(); @@ -779,9 +792,16 @@ export class SqlDriver implements IDataDriver { .digest('hex'); } - /** Create the current `key_hash`-keyed sequences table shape. */ - protected async createSequencesTable(table: string): Promise { - await this.knex.schema.createTable(table, (t) => { + /** + * Create the current `key_hash`-keyed sequences table shape. `runner` is the + * connection the DDL runs on (a fresh pooled connection by default, or the + * caller's transaction on SQLite — see {@link ensureSequencesTable}). + */ + protected async createSequencesTable( + table: string, + runner: Knex | Knex.Transaction = this.knex, + ): Promise { + await runner.schema.createTable(table, (t) => { t.string('key_hash', 64).notNullable().primary(); t.string('object').notNullable(); t.string('tenant_id').notNullable(); @@ -806,17 +826,19 @@ export class SqlDriver implements IDataDriver { * sequences keep working via the legacy key and per-scope writes error * actionably (see getNextSequenceValue), rather than corrupting data. */ - protected async ensureSequencesKeyHashShape(): Promise { - if (await this.knex.schema.hasColumn(SEQUENCES_TABLE, 'key_hash')) { + protected async ensureSequencesKeyHashShape( + runner: Knex | Knex.Transaction = this.knex, + ): Promise { + if (await runner.schema.hasColumn(SEQUENCES_TABLE, 'key_hash')) { this.sequencesHasKeyHash = true; return; } - const hasScope = await this.knex.schema.hasColumn(SEQUENCES_TABLE, 'scope'); + const hasScope = await runner.schema.hasColumn(SEQUENCES_TABLE, 'scope'); const TMP = `${SEQUENCES_TABLE}__rebuild`; try { - const rows: any[] = await this.knex(SEQUENCES_TABLE).select('*'); - await this.knex.schema.dropTableIfExists(TMP); - await this.createSequencesTable(TMP); + const rows: any[] = await runner(SEQUENCES_TABLE).select('*'); + await runner.schema.dropTableIfExists(TMP); + await this.createSequencesTable(TMP, runner); const migrated = rows.map((r) => { const scope = hasScope && r.scope != null ? String(r.scope) : ''; return { @@ -829,15 +851,15 @@ export class SqlDriver implements IDataDriver { updated_at: r.updated_at ?? this.knex.fn.now(), }; }); - if (migrated.length > 0) await this.knex(TMP).insert(migrated); - await this.knex.schema.dropTable(SEQUENCES_TABLE); - await this.knex.schema.renameTable(TMP, SEQUENCES_TABLE); + if (migrated.length > 0) await runner(TMP).insert(migrated); + await runner.schema.dropTable(SEQUENCES_TABLE); + await runner.schema.renameTable(TMP, SEQUENCES_TABLE); this.sequencesHasKeyHash = true; } catch (err) { // Leave the original table intact; fall back to legacy keying for // fixed-prefix sequences and refuse per-scope writes until migrated. this.sequencesHasKeyHash = false; - await this.knex.schema.dropTableIfExists(TMP).catch(() => {}); + await runner.schema.dropTableIfExists(TMP).catch(() => {}); this.logger.warn( `[autonumber] Failed to migrate ${SEQUENCES_TABLE} to the key_hash shape. ` + `Fixed-prefix autonumbers keep working; date/{field}/per-parent formats will ` + @@ -902,7 +924,11 @@ export class SqlDriver implements IDataDriver { parentTrx?: Knex.Transaction, scope = '', ): Promise { - await this.ensureSequencesTable(); + // Pass the caller's transaction so a cold-cache first write inside a batch + // transaction ensures the table on the right connection instead of dead- + // locking on a second one (SQLite pool max=1). `initObjects` normally warms + // this up front, making the call a no-op — this only bites the lazy path. + await this.ensureSequencesTable(parentTrx); const resolvedTenantId = tenantField && tenantId ? String(tenantId) : GLOBAL_TENANT; if (scope !== '' && !this.sequencesHasKeyHash) { // The legacy sequences table could not be migrated to the key_hash shape, @@ -1720,6 +1746,21 @@ export class SqlDriver implements IDataDriver { await this.reconcileAndWarnDrift(tableName, obj.fields ?? {}); } } + + // Pre-create the auto_number counter table now, while we hold a fresh pooled + // connection and are NOT inside any data transaction. Creating it lazily on + // the first autonumber INSERT dead-locks a `/api/v1/batch` write on SQLite + // (pool max=1: the open batch transaction owns the only connection, so the + // lazy `ensureSequencesTable` blocks forever acquiring a second one) and + // risks the same pool exhaustion under concurrent first-writes on + // Postgres/MySQL. Idempotent and skipped entirely when nothing uses + // auto_number, so it costs one `hasTable` at boot in the common case. + const usesAutoNumber = Object.values(this.autoNumberFields).some( + (cols) => Array.isArray(cols) && cols.length > 0, + ); + if (usesAutoNumber) { + await this.ensureSequencesTable(); + } } // ── Managed-schema drift & reconcile (#2186) ─────────────────────────────── From ed4221820ae34545cf06c2918dd369f733b1e0e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Fri, 3 Jul 2026 15:59:27 -0700 Subject: [PATCH 2/3] fix(driver-sql): only cache sequences-ready flag when DDL ran durably MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Harden the lazy ensureSequencesTable fallback: when the table is created on the caller's transaction (SQLite in-tx path), the create is commit-conditional — a rollback would drop it. Cache sequencesTableReady only when the DDL ran on a durable pooled connection (this.knex); on the in-tx path leave the flag unset so the next write cheaply re-verifies via hasTable instead of trusting a stale process-level flag. initObjects still sets it durably up front, so the hot path is unchanged. --- .../driver-sql/src/sql-driver-autonumber-tokens.test.ts | 3 ++- packages/plugins/driver-sql/src/sql-driver.ts | 8 +++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/plugins/driver-sql/src/sql-driver-autonumber-tokens.test.ts b/packages/plugins/driver-sql/src/sql-driver-autonumber-tokens.test.ts index 817f6da3be..6cc3106487 100644 --- a/packages/plugins/driver-sql/src/sql-driver-autonumber-tokens.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-autonumber-tokens.test.ts @@ -209,7 +209,8 @@ describe('SqlDriver auto_number format tokens', () => { await driver.initObjects([ { name: 'invoice', fields: { invoice_number: { type: 'autonumber', format: 'INV-{0000}' } } }, ]); - // First create triggers the lazy table-ensure → in-place migration. + // initObjects pre-ensures the table → the in-place migration runs at init, + // before any write; the first create then reads the already-migrated counter. const r = await driver.create('invoice', {}); const cols = await k('_objectstack_sequences').columnInfo(); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index c8d2962f49..a95a6aa9d3 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -776,7 +776,13 @@ export class SqlDriver implements IDataDriver { // Pre-existing table may predate the `key_hash`/`scope` shape. Migrate. await this.ensureSequencesKeyHashShape(runner); } - this.sequencesTableReady = true; + // Cache "ready" only when the DDL ran on a durable connection. If it rode + // the caller's transaction (the SQLite in-tx fallback above), the table is + // commit-conditional — a rollback would drop it — so leave the flag unset + // and re-verify (a cheap `hasTable`) on the next write rather than trusting + // a stale process-level flag. `initObjects` sets it durably up front, so + // the hot path is unaffected. + if (runner === this.knex) this.sequencesTableReady = true; })(); try { await this.sequencesTableEnsurePromise; From 15bcf2c62e6df4ca87ee4739808f8985a46b5ece Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Fri, 3 Jul 2026 16:28:29 -0700 Subject: [PATCH 3/3] feat(driver-sql): dev guard that fails fast on the SQLite single-connection deadlock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns the latent 'bare this.knex query while a transaction holds SQLite's only pooled connection' hang (surfacing as 'Knex: Timeout acquiring a connection') into an immediate, actionable error at the call site. - Track open transactions (beginTransaction++/commit+rollback--) with a WeakSet so the decrement is idempotent on double close. - assertBareKnexSafe(op): throws only in the danger combination (non-production + SQLite + an open transaction). No-op in production and on non-SQLite dialects — zero hot-path cost. - Wire it into ensureSequencesTable's bare-knex branch, catching the 'opened a tx but forgot to thread it through as parentTrx' regression. Adds sql-driver-sqlite-tx-guard.test.ts (11 cases: tx bookkeeping, guard fires on the deadlock shape, guard stays quiet on legitimate paths, and the production/non-sqlite/no-tx branch matrix). --- .changeset/fix-autonumber-batch-deadlock.md | 7 + .../src/sql-driver-sqlite-tx-guard.test.ts | 196 ++++++++++++++++++ packages/plugins/driver-sql/src/sql-driver.ts | 70 ++++++- 3 files changed, 270 insertions(+), 3 deletions(-) create mode 100644 packages/plugins/driver-sql/src/sql-driver-sqlite-tx-guard.test.ts diff --git a/.changeset/fix-autonumber-batch-deadlock.md b/.changeset/fix-autonumber-batch-deadlock.md index e595c7f019..c399dbf469 100644 --- a/.changeset/fix-autonumber-batch-deadlock.md +++ b/.changeset/fix-autonumber-batch-deadlock.md @@ -20,3 +20,10 @@ Fixes: transaction on SQLite instead of grabbing a second connection. It deliberately does not route DDL through the caller's transaction on MySQL, where DDL would implicitly commit the caller's in-flight transaction. +- Added a dev/test guard (`assertBareKnexSafe`): on SQLite, issuing a bare + `this.knex` query while a transaction holds the single pooled connection now + fails fast with an actionable error instead of hanging until the opaque + `Knex: Timeout acquiring a connection`. No-op in production and on non-SQLite + dialects, so it adds no runtime cost on the hot path — it just turns this whole + class of "forgot to thread the transaction through" bug into an immediate, + self-explaining failure at the call site. diff --git a/packages/plugins/driver-sql/src/sql-driver-sqlite-tx-guard.test.ts b/packages/plugins/driver-sql/src/sql-driver-sqlite-tx-guard.test.ts new file mode 100644 index 0000000000..ccdba00dd0 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-sqlite-tx-guard.test.ts @@ -0,0 +1,196 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +// +// Dev/test guard against the SQLite single-connection dead-lock. +// +// SQLite's connection pool hands out exactly one connection. Issuing a bare +// `this.knex` query while a transaction holds that connection blocks forever +// acquiring a second one and finally fails with the opaque "Knex: Timeout +// acquiring a connection" — the reported /api/v1/batch autonumber dead-lock. +// +// `assertBareKnexSafe` turns that latent, timeout-delayed hang into an +// immediate, actionable error at the call site. This is the regression tripwire +// for "a caller opened a transaction but forgot to thread it through": the +// sequence-table ensure would otherwise silently fall back to `this.knex` and +// dead-lock. The guard is a no-op in production and on non-SQLite dialects. + +import { describe, it, expect, afterEach, beforeEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +const SEQUENCES_TABLE = '_objectstack_sequences'; + +function withTimeout(p: Promise, ms: number, label: string): Promise { + return Promise.race([ + p, + new Promise((_, rej) => setTimeout(() => rej(new Error(`TIMEOUT: ${label}`)), ms)), + ]); +} + +describe('SqlDriver SQLite single-connection tx guard', () => { + let driver: SqlDriver | undefined; + const savedEnv = process.env.NODE_ENV; + + afterEach(async () => { + if (savedEnv === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = savedEnv; + if (driver) await driver.disconnect(); + driver = undefined; + }); + + async function setup() { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([ + { + name: 'contract', + fields: { + contract_number: { type: 'autonumber', format: 'CTR-{0000}' }, + name: { type: 'string' }, + }, + }, + ]); + return (driver as any).knex; + } + + /** Force the lazy fallback: drop the pre-created table and clear the cache. */ + function coldStart() { + (driver as any).sequencesTableReady = false; + (driver as any).sequencesHasKeyHash = false; + (driver as any).sequencesTableEnsurePromise = null; + } + + describe('beginTransaction bookkeeping', () => { + it('counts an open transaction and releases it on commit', async () => { + await setup(); + expect((driver as any).activeTransactions).toBe(0); + const trx = await driver!.beginTransaction(); + expect((driver as any).activeTransactions).toBe(1); + await driver!.commit(trx); + expect((driver as any).activeTransactions).toBe(0); + }); + + it('releases the count on rollback too', async () => { + await setup(); + const trx = await driver!.beginTransaction(); + expect((driver as any).activeTransactions).toBe(1); + await driver!.rollback(trx); + expect((driver as any).activeTransactions).toBe(0); + }); + + it('releases via the deprecated commitTransaction/rollbackTransaction aliases', async () => { + await setup(); + const t1 = await driver!.beginTransaction(); + await driver!.commitTransaction(t1); + expect((driver as any).activeTransactions).toBe(0); + const t2 = await driver!.beginTransaction(); + await driver!.rollbackTransaction(t2); + expect((driver as any).activeTransactions).toBe(0); + }); + + it('does not double-decrement when a transaction is closed twice', async () => { + await setup(); + const trx = await driver!.beginTransaction(); + expect((driver as any).activeTransactions).toBe(1); + await driver!.commit(trx); + expect((driver as any).activeTransactions).toBe(0); + // A redundant second close must not drive the count negative. + await driver!.commit(trx).catch(() => {}); + expect((driver as any).activeTransactions).toBe(0); + }); + }); + + describe('guard fires on the dead-lock shape', () => { + it('fails fast (not a 6s timeout) when a cold ensure runs bare-knex mid-transaction', async () => { + const k = await setup(); + await k.schema.dropTableIfExists(SEQUENCES_TABLE); + coldStart(); + + // A transaction now owns the single connection. A create WITHOUT threading + // that transaction through hits the lazy ensure on `this.knex` — the guard + // must reject immediately instead of blocking on connection acquisition. + const trx = await driver!.beginTransaction(); + try { + await withTimeout( + (driver as any).ensureSequencesTable(undefined), + 2000, + 'ensureSequencesTable should have thrown, not hung', + ); + throw new Error('expected the guard to throw'); + } catch (err: any) { + expect(err.message).toMatch(/transaction is open|dead-lock|Timeout acquiring/i); + // Specifically the guard's message, not a real acquire-timeout. + expect(err.message).not.toMatch(/^TIMEOUT:/); + } finally { + await driver!.rollback(trx); + } + }); + + it('surfaces the guard through a real create() that forgot to pass the tx', async () => { + const k = await setup(); + await k.schema.dropTableIfExists(SEQUENCES_TABLE); + coldStart(); + + const trx = await driver!.beginTransaction(); + try { + await expect( + withTimeout( + driver!.create('contract', { name: 'A' }), // no { transaction: trx } — the bug shape + 2000, + 'create should have failed fast', + ), + ).rejects.toThrow(/transaction is open|dead-lock/i); + } finally { + await driver!.rollback(trx); + } + }); + }); + + describe('guard does NOT fire on legitimate paths', () => { + it('allows a cold ensure that correctly rides the caller transaction', async () => { + const k = await setup(); + await k.schema.dropTableIfExists(SEQUENCES_TABLE); + coldStart(); + + const trx = await driver!.beginTransaction(); + // parentTrx passed → runs on the transaction, no bare-knex, no throw. + await expect((driver as any).ensureSequencesTable(trx)).resolves.toBeUndefined(); + await driver!.commit(trx); + expect(await k.schema.hasTable(SEQUENCES_TABLE)).toBe(true); + }); + + it('allows bare-knex ensure when NO transaction is open (the initObjects path)', async () => { + const k = await setup(); + await k.schema.dropTableIfExists(SEQUENCES_TABLE); + coldStart(); + // activeTransactions === 0 → bare knex is safe. + await expect((driver as any).ensureSequencesTable(undefined)).resolves.toBeUndefined(); + expect(await k.schema.hasTable(SEQUENCES_TABLE)).toBe(true); + }); + }); + + describe('assertBareKnexSafe branch matrix', () => { + beforeEach(async () => { + await setup(); + }); + + it('throws only in the danger combination: non-prod + sqlite + open tx', () => { + (driver as any).activeTransactions = 1; + delete process.env.NODE_ENV; + expect(() => (driver as any).assertBareKnexSafe('x')).toThrow(/transaction is open/i); + }); + + it('is a no-op in production (guard must never break real deployments)', () => { + (driver as any).activeTransactions = 1; + process.env.NODE_ENV = 'production'; + expect(() => (driver as any).assertBareKnexSafe('x')).not.toThrow(); + }); + + it('is a no-op when no transaction is open', () => { + (driver as any).activeTransactions = 0; + delete process.env.NODE_ENV; + expect(() => (driver as any).assertBareKnexSafe('x')).not.toThrow(); + }); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index a95a6aa9d3..6eea543e9d 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -370,6 +370,19 @@ export class SqlDriver implements IDataDriver { /** In-flight ensure promise; deduplicates concurrent first calls. */ protected sequencesTableEnsurePromise: Promise | null = null; + /** + * Count of transactions currently open through `beginTransaction`. On SQLite + * the pool holds a single connection, so while this is > 0 that connection is + * busy and any bare `this.knex` query would dead-lock acquiring a second one. + * Used only by the dev/test guard `assertBareKnexSafe`. Incremented in + * `beginTransaction`, decremented in `commit`/`rollback`; the `openTransactions` + * set makes the decrement idempotent so a double commit/rollback can't drive + * the count negative or double-count. + */ + protected activeTransactions = 0; + /** Transactions counted in `activeTransactions` and not yet released. */ + protected openTransactions = new WeakSet(); + /** * Per-table tenant-isolation column. Populated during `initObjects` by * detecting an `organization_id` field. When set and the caller passes @@ -758,6 +771,11 @@ export class SqlDriver implements IDataDriver { // on MySQL, where DDL implicitly commits the caller's transaction; there the // roomy pool (max=10) lets a fresh connection create the table safely. const runner: Knex | Knex.Transaction = parentTrx && this.isSqlite ? parentTrx : this.knex; + // If we are about to run DDL on a fresh pooled connection while a SQLite + // transaction holds the only one, fail fast with a clear message instead of + // dead-locking. This catches the "caller opened a transaction but did not + // thread it through as parentTrx" regression at the call site (dev/test only). + if (runner === this.knex) this.assertBareKnexSafe('ensureSequencesTable'); this.sequencesTableEnsurePromise = (async () => { const exists = await runner.schema.hasTable(SEQUENCES_TABLE); if (!exists) { @@ -1270,17 +1288,63 @@ export class SqlDriver implements IDataDriver { // =================================== async beginTransaction(): Promise { - return await this.knex.transaction(); + const trx = await this.knex.transaction(); + this.openTransactions.add(trx as unknown as object); + this.activeTransactions++; + return trx; + } + + /** Idempotently drop a transaction from the open-count (safe on double close). */ + protected releaseTransaction(transaction: unknown): void { + const key = transaction as object; + if (key && this.openTransactions.has(key)) { + this.openTransactions.delete(key); + this.activeTransactions = Math.max(0, this.activeTransactions - 1); + } + } + + /** + * Dev/test guard against the SQLite single-connection dead-lock. SQLite's pool + * hands out exactly one connection, so issuing a bare `this.knex` query while a + * transaction holds that connection blocks forever acquiring a second one and + * finally fails with an opaque `Knex: Timeout acquiring a connection`. This + * turns that into an immediate, actionable error at the call site. + * + * No-op in production (zero overhead on the hot path) and on every non-SQLite + * dialect, whose roomy pools (max ≥ 10) cannot exhibit the single-connection + * dead-lock. Callers that legitimately need the connection during a + * transaction must bind the operation to that transaction instead of + * `this.knex`. + */ + protected assertBareKnexSafe(op: string): void { + if (this.isProductionEnv()) return; + if (!this.isSqlite) return; + if (this.activeTransactions === 0) return; + throw new Error( + `[driver-sql] refusing to run '${op}' on a fresh pooled connection while a ` + + `transaction is open: SQLite's pool has a single connection, so acquiring a ` + + `second one would dead-lock (surfacing later as "Knex: Timeout acquiring a ` + + `connection"). Bind this operation to the active transaction instead of using ` + + `this.knex.`, + ); } /** IDataDriver standard */ async commit(transaction: unknown): Promise { - await (transaction as Knex.Transaction).commit(); + try { + await (transaction as Knex.Transaction).commit(); + } finally { + this.releaseTransaction(transaction); + } } /** IDataDriver standard */ async rollback(transaction: unknown): Promise { - await (transaction as Knex.Transaction).rollback(); + try { + await (transaction as Knex.Transaction).rollback(); + } finally { + this.releaseTransaction(transaction); + } } /** @deprecated Use commit() instead */