Skip to content

Commit 15bcf2c

Browse files
committed
feat(driver-sql): dev guard that fails fast on the SQLite single-connection deadlock
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).
1 parent ed42218 commit 15bcf2c

3 files changed

Lines changed: 270 additions & 3 deletions

File tree

.changeset/fix-autonumber-batch-deadlock.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,3 +20,10 @@ Fixes:
2020
transaction on SQLite instead of grabbing a second connection. It deliberately
2121
does not route DDL through the caller's transaction on MySQL, where DDL would
2222
implicitly commit the caller's in-flight transaction.
23+
- Added a dev/test guard (`assertBareKnexSafe`): on SQLite, issuing a bare
24+
`this.knex` query while a transaction holds the single pooled connection now
25+
fails fast with an actionable error instead of hanging until the opaque
26+
`Knex: Timeout acquiring a connection`. No-op in production and on non-SQLite
27+
dialects, so it adds no runtime cost on the hot path — it just turns this whole
28+
class of "forgot to thread the transaction through" bug into an immediate,
29+
self-explaining failure at the call site.
Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,196 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// Dev/test guard against the SQLite single-connection dead-lock.
4+
//
5+
// SQLite's connection pool hands out exactly one connection. Issuing a bare
6+
// `this.knex` query while a transaction holds that connection blocks forever
7+
// acquiring a second one and finally fails with the opaque "Knex: Timeout
8+
// acquiring a connection" — the reported /api/v1/batch autonumber dead-lock.
9+
//
10+
// `assertBareKnexSafe` turns that latent, timeout-delayed hang into an
11+
// immediate, actionable error at the call site. This is the regression tripwire
12+
// for "a caller opened a transaction but forgot to thread it through": the
13+
// sequence-table ensure would otherwise silently fall back to `this.knex` and
14+
// dead-lock. The guard is a no-op in production and on non-SQLite dialects.
15+
16+
import { describe, it, expect, afterEach, beforeEach } from 'vitest';
17+
import { SqlDriver } from '../src/index.js';
18+
19+
const SEQUENCES_TABLE = '_objectstack_sequences';
20+
21+
function withTimeout<T>(p: Promise<T>, ms: number, label: string): Promise<T> {
22+
return Promise.race([
23+
p,
24+
new Promise<T>((_, rej) => setTimeout(() => rej(new Error(`TIMEOUT: ${label}`)), ms)),
25+
]);
26+
}
27+
28+
describe('SqlDriver SQLite single-connection tx guard', () => {
29+
let driver: SqlDriver | undefined;
30+
const savedEnv = process.env.NODE_ENV;
31+
32+
afterEach(async () => {
33+
if (savedEnv === undefined) delete process.env.NODE_ENV;
34+
else process.env.NODE_ENV = savedEnv;
35+
if (driver) await driver.disconnect();
36+
driver = undefined;
37+
});
38+
39+
async function setup() {
40+
driver = new SqlDriver({
41+
client: 'better-sqlite3',
42+
connection: { filename: ':memory:' },
43+
useNullAsDefault: true,
44+
});
45+
await driver.initObjects([
46+
{
47+
name: 'contract',
48+
fields: {
49+
contract_number: { type: 'autonumber', format: 'CTR-{0000}' },
50+
name: { type: 'string' },
51+
},
52+
},
53+
]);
54+
return (driver as any).knex;
55+
}
56+
57+
/** Force the lazy fallback: drop the pre-created table and clear the cache. */
58+
function coldStart() {
59+
(driver as any).sequencesTableReady = false;
60+
(driver as any).sequencesHasKeyHash = false;
61+
(driver as any).sequencesTableEnsurePromise = null;
62+
}
63+
64+
describe('beginTransaction bookkeeping', () => {
65+
it('counts an open transaction and releases it on commit', async () => {
66+
await setup();
67+
expect((driver as any).activeTransactions).toBe(0);
68+
const trx = await driver!.beginTransaction();
69+
expect((driver as any).activeTransactions).toBe(1);
70+
await driver!.commit(trx);
71+
expect((driver as any).activeTransactions).toBe(0);
72+
});
73+
74+
it('releases the count on rollback too', async () => {
75+
await setup();
76+
const trx = await driver!.beginTransaction();
77+
expect((driver as any).activeTransactions).toBe(1);
78+
await driver!.rollback(trx);
79+
expect((driver as any).activeTransactions).toBe(0);
80+
});
81+
82+
it('releases via the deprecated commitTransaction/rollbackTransaction aliases', async () => {
83+
await setup();
84+
const t1 = await driver!.beginTransaction();
85+
await driver!.commitTransaction(t1);
86+
expect((driver as any).activeTransactions).toBe(0);
87+
const t2 = await driver!.beginTransaction();
88+
await driver!.rollbackTransaction(t2);
89+
expect((driver as any).activeTransactions).toBe(0);
90+
});
91+
92+
it('does not double-decrement when a transaction is closed twice', async () => {
93+
await setup();
94+
const trx = await driver!.beginTransaction();
95+
expect((driver as any).activeTransactions).toBe(1);
96+
await driver!.commit(trx);
97+
expect((driver as any).activeTransactions).toBe(0);
98+
// A redundant second close must not drive the count negative.
99+
await driver!.commit(trx).catch(() => {});
100+
expect((driver as any).activeTransactions).toBe(0);
101+
});
102+
});
103+
104+
describe('guard fires on the dead-lock shape', () => {
105+
it('fails fast (not a 6s timeout) when a cold ensure runs bare-knex mid-transaction', async () => {
106+
const k = await setup();
107+
await k.schema.dropTableIfExists(SEQUENCES_TABLE);
108+
coldStart();
109+
110+
// A transaction now owns the single connection. A create WITHOUT threading
111+
// that transaction through hits the lazy ensure on `this.knex` — the guard
112+
// must reject immediately instead of blocking on connection acquisition.
113+
const trx = await driver!.beginTransaction();
114+
try {
115+
await withTimeout(
116+
(driver as any).ensureSequencesTable(undefined),
117+
2000,
118+
'ensureSequencesTable should have thrown, not hung',
119+
);
120+
throw new Error('expected the guard to throw');
121+
} catch (err: any) {
122+
expect(err.message).toMatch(/transaction is open|dead-lock|Timeout acquiring/i);
123+
// Specifically the guard's message, not a real acquire-timeout.
124+
expect(err.message).not.toMatch(/^TIMEOUT:/);
125+
} finally {
126+
await driver!.rollback(trx);
127+
}
128+
});
129+
130+
it('surfaces the guard through a real create() that forgot to pass the tx', async () => {
131+
const k = await setup();
132+
await k.schema.dropTableIfExists(SEQUENCES_TABLE);
133+
coldStart();
134+
135+
const trx = await driver!.beginTransaction();
136+
try {
137+
await expect(
138+
withTimeout(
139+
driver!.create('contract', { name: 'A' }), // no { transaction: trx } — the bug shape
140+
2000,
141+
'create should have failed fast',
142+
),
143+
).rejects.toThrow(/transaction is open|dead-lock/i);
144+
} finally {
145+
await driver!.rollback(trx);
146+
}
147+
});
148+
});
149+
150+
describe('guard does NOT fire on legitimate paths', () => {
151+
it('allows a cold ensure that correctly rides the caller transaction', async () => {
152+
const k = await setup();
153+
await k.schema.dropTableIfExists(SEQUENCES_TABLE);
154+
coldStart();
155+
156+
const trx = await driver!.beginTransaction();
157+
// parentTrx passed → runs on the transaction, no bare-knex, no throw.
158+
await expect((driver as any).ensureSequencesTable(trx)).resolves.toBeUndefined();
159+
await driver!.commit(trx);
160+
expect(await k.schema.hasTable(SEQUENCES_TABLE)).toBe(true);
161+
});
162+
163+
it('allows bare-knex ensure when NO transaction is open (the initObjects path)', async () => {
164+
const k = await setup();
165+
await k.schema.dropTableIfExists(SEQUENCES_TABLE);
166+
coldStart();
167+
// activeTransactions === 0 → bare knex is safe.
168+
await expect((driver as any).ensureSequencesTable(undefined)).resolves.toBeUndefined();
169+
expect(await k.schema.hasTable(SEQUENCES_TABLE)).toBe(true);
170+
});
171+
});
172+
173+
describe('assertBareKnexSafe branch matrix', () => {
174+
beforeEach(async () => {
175+
await setup();
176+
});
177+
178+
it('throws only in the danger combination: non-prod + sqlite + open tx', () => {
179+
(driver as any).activeTransactions = 1;
180+
delete process.env.NODE_ENV;
181+
expect(() => (driver as any).assertBareKnexSafe('x')).toThrow(/transaction is open/i);
182+
});
183+
184+
it('is a no-op in production (guard must never break real deployments)', () => {
185+
(driver as any).activeTransactions = 1;
186+
process.env.NODE_ENV = 'production';
187+
expect(() => (driver as any).assertBareKnexSafe('x')).not.toThrow();
188+
});
189+
190+
it('is a no-op when no transaction is open', () => {
191+
(driver as any).activeTransactions = 0;
192+
delete process.env.NODE_ENV;
193+
expect(() => (driver as any).assertBareKnexSafe('x')).not.toThrow();
194+
});
195+
});
196+
});

packages/plugins/driver-sql/src/sql-driver.ts

Lines changed: 67 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,19 @@ export class SqlDriver implements IDataDriver {
370370
/** In-flight ensure promise; deduplicates concurrent first calls. */
371371
protected sequencesTableEnsurePromise: Promise<void> | null = null;
372372

373+
/**
374+
* Count of transactions currently open through `beginTransaction`. On SQLite
375+
* the pool holds a single connection, so while this is > 0 that connection is
376+
* busy and any bare `this.knex` query would dead-lock acquiring a second one.
377+
* Used only by the dev/test guard `assertBareKnexSafe`. Incremented in
378+
* `beginTransaction`, decremented in `commit`/`rollback`; the `openTransactions`
379+
* set makes the decrement idempotent so a double commit/rollback can't drive
380+
* the count negative or double-count.
381+
*/
382+
protected activeTransactions = 0;
383+
/** Transactions counted in `activeTransactions` and not yet released. */
384+
protected openTransactions = new WeakSet<object>();
385+
373386
/**
374387
* Per-table tenant-isolation column. Populated during `initObjects` by
375388
* detecting an `organization_id` field. When set and the caller passes
@@ -758,6 +771,11 @@ export class SqlDriver implements IDataDriver {
758771
// on MySQL, where DDL implicitly commits the caller's transaction; there the
759772
// roomy pool (max=10) lets a fresh connection create the table safely.
760773
const runner: Knex | Knex.Transaction = parentTrx && this.isSqlite ? parentTrx : this.knex;
774+
// If we are about to run DDL on a fresh pooled connection while a SQLite
775+
// transaction holds the only one, fail fast with a clear message instead of
776+
// dead-locking. This catches the "caller opened a transaction but did not
777+
// thread it through as parentTrx" regression at the call site (dev/test only).
778+
if (runner === this.knex) this.assertBareKnexSafe('ensureSequencesTable');
761779
this.sequencesTableEnsurePromise = (async () => {
762780
const exists = await runner.schema.hasTable(SEQUENCES_TABLE);
763781
if (!exists) {
@@ -1270,17 +1288,63 @@ export class SqlDriver implements IDataDriver {
12701288
// ===================================
12711289

12721290
async beginTransaction(): Promise<Knex.Transaction> {
1273-
return await this.knex.transaction();
1291+
const trx = await this.knex.transaction();
1292+
this.openTransactions.add(trx as unknown as object);
1293+
this.activeTransactions++;
1294+
return trx;
1295+
}
1296+
1297+
/** Idempotently drop a transaction from the open-count (safe on double close). */
1298+
protected releaseTransaction(transaction: unknown): void {
1299+
const key = transaction as object;
1300+
if (key && this.openTransactions.has(key)) {
1301+
this.openTransactions.delete(key);
1302+
this.activeTransactions = Math.max(0, this.activeTransactions - 1);
1303+
}
1304+
}
1305+
1306+
/**
1307+
* Dev/test guard against the SQLite single-connection dead-lock. SQLite's pool
1308+
* hands out exactly one connection, so issuing a bare `this.knex` query while a
1309+
* transaction holds that connection blocks forever acquiring a second one and
1310+
* finally fails with an opaque `Knex: Timeout acquiring a connection`. This
1311+
* turns that into an immediate, actionable error at the call site.
1312+
*
1313+
* No-op in production (zero overhead on the hot path) and on every non-SQLite
1314+
* dialect, whose roomy pools (max ≥ 10) cannot exhibit the single-connection
1315+
* dead-lock. Callers that legitimately need the connection during a
1316+
* transaction must bind the operation to that transaction instead of
1317+
* `this.knex`.
1318+
*/
1319+
protected assertBareKnexSafe(op: string): void {
1320+
if (this.isProductionEnv()) return;
1321+
if (!this.isSqlite) return;
1322+
if (this.activeTransactions === 0) return;
1323+
throw new Error(
1324+
`[driver-sql] refusing to run '${op}' on a fresh pooled connection while a ` +
1325+
`transaction is open: SQLite's pool has a single connection, so acquiring a ` +
1326+
`second one would dead-lock (surfacing later as "Knex: Timeout acquiring a ` +
1327+
`connection"). Bind this operation to the active transaction instead of using ` +
1328+
`this.knex.`,
1329+
);
12741330
}
12751331

12761332
/** IDataDriver standard */
12771333
async commit(transaction: unknown): Promise<void> {
1278-
await (transaction as Knex.Transaction).commit();
1334+
try {
1335+
await (transaction as Knex.Transaction).commit();
1336+
} finally {
1337+
this.releaseTransaction(transaction);
1338+
}
12791339
}
12801340

12811341
/** IDataDriver standard */
12821342
async rollback(transaction: unknown): Promise<void> {
1283-
await (transaction as Knex.Transaction).rollback();
1343+
try {
1344+
await (transaction as Knex.Transaction).rollback();
1345+
} finally {
1346+
this.releaseTransaction(transaction);
1347+
}
12841348
}
12851349

12861350
/** @deprecated Use commit() instead */

0 commit comments

Comments
 (0)