Skip to content

Commit 8d87930

Browse files
authored
fix(driver-sql): pre-create autonumber sequences table to avoid batch/tx deadlock (#2537)
* 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). * fix(driver-sql): only cache sequences-ready flag when DDL ran durably 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. * 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 852bc8e commit 8d87930

5 files changed

Lines changed: 482 additions & 26 deletions

File tree

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
---
2+
'@objectstack/driver-sql': patch
3+
---
4+
5+
Fix a connection-pool deadlock when the first `auto_number` write after process
6+
start goes through a transaction (e.g. `POST /api/v1/batch`, which wraps every
7+
operation in one `ql.transaction(...)`).
8+
9+
The sequence-counter table (`_objectstack_sequences`) was created lazily on the
10+
first autonumber INSERT via a bare `this.knex.schema.*` call that asks the pool
11+
for a second connection. On SQLite (better-sqlite3, pool max=1) the open batch
12+
transaction already holds the only connection, so the acquire blocked until
13+
`Knex: Timeout acquiring a connection`. Postgres/MySQL are exposed to the same
14+
pool-exhaustion deadlock under concurrent cold first-writes.
15+
16+
Fixes:
17+
- `initObjects` now pre-creates the counter table up front, outside any data
18+
transaction, so the first write never runs DDL (primary fix).
19+
- The lazy fallback (`ensureSequencesTable`) now runs its DDL on the caller's own
20+
transaction on SQLite instead of grabbing a second connection. It deliberately
21+
does not route DDL through the caller's transaction on MySQL, where DDL would
22+
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.

packages/plugins/driver-sql/src/sql-driver-autonumber-tokens.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,8 @@ describe('SqlDriver auto_number format tokens', () => {
209209
await driver.initObjects([
210210
{ name: 'invoice', fields: { invoice_number: { type: 'autonumber', format: 'INV-{0000}' } } },
211211
]);
212-
// First create triggers the lazy table-ensure → in-place migration.
212+
// initObjects pre-ensures the table → the in-place migration runs at init,
213+
// before any write; the first create then reads the already-migrated counter.
213214
const r = await driver.create('invoice', {});
214215

215216
const cols = await k('_objectstack_sequences').columnInfo();
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// Regression guard for the auto_number-in-transaction deadlock.
4+
//
5+
// Reported: on SQLite (better-sqlite3, pool max=1), the first autonumber write
6+
// after process start that goes through a transaction — e.g. POST /api/v1/batch,
7+
// which wraps every operation in one `ql.transaction(...)` — dead-locked with
8+
// "Knex: Timeout acquiring a connection. The pool is probably full."
9+
//
10+
// Root cause: the sequence-counter table (`_objectstack_sequences`) was created
11+
// lazily on the first autonumber INSERT, via a bare `this.knex.schema.*` call
12+
// that asks the pool for a SECOND connection. Inside a batch transaction the
13+
// only pooled connection is already held, so the acquire blocked until timeout.
14+
//
15+
// Fixes under test:
16+
// 1. `initObjects` pre-creates the table outside any data transaction, so the
17+
// first write never runs DDL (primary fix — covers the real batch path).
18+
// 2. The lazy fallback (`ensureSequencesTable`) runs its DDL on the caller's
19+
// own transaction on SQLite instead of grabbing a second connection, so
20+
// even a cold-cache in-transaction first write cannot deadlock.
21+
22+
import { describe, it, expect, afterEach } from 'vitest';
23+
import { SqlDriver } from '../src/index.js';
24+
25+
const SEQUENCES_TABLE = '_objectstack_sequences';
26+
27+
function withTimeout<T>(p: Promise<T>, ms: number, label: string): Promise<T> {
28+
return Promise.race([
29+
p,
30+
new Promise<T>((_, rej) => setTimeout(() => rej(new Error(`TIMEOUT: ${label} (deadlock)`)), ms)),
31+
]);
32+
}
33+
34+
describe('SqlDriver auto_number in-transaction (deadlock regression)', () => {
35+
let driver: SqlDriver | undefined;
36+
37+
afterEach(async () => {
38+
if (driver) await driver.disconnect();
39+
driver = undefined;
40+
});
41+
42+
async function setup() {
43+
driver = new SqlDriver({
44+
client: 'better-sqlite3',
45+
connection: { filename: ':memory:' },
46+
useNullAsDefault: true,
47+
});
48+
await driver.initObjects([
49+
{
50+
name: 'contract',
51+
fields: {
52+
organization_id: { type: 'string' },
53+
contract_number: { type: 'autonumber', format: 'CTR-{0000}' },
54+
name: { type: 'string' },
55+
},
56+
},
57+
]);
58+
return (driver as any).knex;
59+
}
60+
61+
it('pre-creates the sequences table during initObjects (before any write)', async () => {
62+
const k = await setup();
63+
// The counter table must exist immediately after initObjects, without a
64+
// single create() — this is what keeps the first write off the DDL path.
65+
expect(await k.schema.hasTable(SEQUENCES_TABLE)).toBe(true);
66+
expect((driver as any).sequencesTableReady).toBe(true);
67+
});
68+
69+
it('commits a batch-style transaction whose FIRST write fills an autonumber', async () => {
70+
const k = await setup();
71+
72+
// Two autonumber creates inside a single transaction, mirroring how the REST
73+
// /batch endpoint wraps operations. Must commit, not hang.
74+
const trx = await driver!.beginTransaction();
75+
const r1 = await withTimeout(
76+
driver!.create('contract', { organization_id: 'org_x', name: 'A' }, { transaction: trx } as any),
77+
6000,
78+
'batch create #1 (autonumber)',
79+
);
80+
const r2 = await withTimeout(
81+
driver!.create('contract', { organization_id: 'org_x', name: 'B' }, { transaction: trx } as any),
82+
6000,
83+
'batch create #2 (autonumber)',
84+
);
85+
await withTimeout(driver!.commit(trx), 6000, 'commit');
86+
87+
expect(r1.contract_number).toBe('CTR-0001');
88+
expect(r2.contract_number).toBe('CTR-0002');
89+
90+
const rows = await k('contract').select('contract_number').orderBy('contract_number');
91+
expect(rows.map((r: any) => r.contract_number)).toEqual(['CTR-0001', 'CTR-0002']);
92+
});
93+
94+
it('does not deadlock even with a COLD cache inside the transaction (lazy fallback)', async () => {
95+
const k = await setup();
96+
97+
// Simulate the path where the table was NOT pre-created (an external object,
98+
// or a consumer that writes without initObjects): drop it and clear the
99+
// process cache, then take the single connection with a transaction and let
100+
// the first autonumber write hit the lazy ensure. On the old code this asked
101+
// the pool for a second connection and blocked until acquire-timeout.
102+
await k.schema.dropTableIfExists(SEQUENCES_TABLE);
103+
(driver as any).sequencesTableReady = false;
104+
(driver as any).sequencesHasKeyHash = false;
105+
(driver as any).sequencesTableEnsurePromise = null;
106+
107+
const trx = await driver!.beginTransaction();
108+
const r1 = await withTimeout(
109+
driver!.create('contract', { organization_id: 'org_cold', name: 'C' }, { transaction: trx } as any),
110+
6000,
111+
'cold-cache create inside tx',
112+
);
113+
await withTimeout(driver!.commit(trx), 6000, 'commit');
114+
115+
expect(r1.contract_number).toBe('CTR-0001');
116+
// The table the fallback created on the transaction survives the commit.
117+
expect(await k.schema.hasTable(SEQUENCES_TABLE)).toBe(true);
118+
});
119+
});
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+
});

0 commit comments

Comments
 (0)