Skip to content

Commit 41a5fd3

Browse files
os-steveclaude
andcommitted
fix(driver-sql): re-seed a stale autonumber counter instead of burning a number per failed create (#5495)
The counter bootstraps from the data-table MAX exactly once, in `getNextSequenceValue`'s `if (!existing)` branch; afterwards the data table is never consulted again. Any row landing by a path that bypasses `fillAutoNumberFields` — an `isSystem` seed replay, a `preserveAudit` import, or direct SQL — never raises the sequence, so once it sits below MAX it is permanently behind and every create collides, burns a number and fails the request until it has ground past the seeded range one 409 at a time. Measured on main @ 86e6f6c with the counter seeded at 10 and rows 11-39 landed by a bypass path: 29 caller-visible 409s before a create succeeded at CASE-00040 on attempt 30. Now: CASE-00040 on the caller's first attempt. `create()` re-seeds from MAX and retries (bounded) only when it can prove the collision was that counter's. The proof cannot be the conflicting column: `uniqueViolationColumn()` refuses composites, and ADR-0120 D3 makes the tenanted index an expression composite on which SQLite names only the index — so on this path the column is never determinable. All three of that export's states are handled explicitly; the indeterminate one is decided from the data (does the generated value already exist in this tenant partition?), never by a hand-written dialect word-list. Retry is confined to the no-caller-transaction case: inside a caller's transaction the sequence UPDATE rolls back with the INSERT, so nothing is burned, and on Postgres the transaction is aborted anyway. The "gaps are tolerated by design" docstring is reconciled with the change rather than left to contradict it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015XLbsWE5G58ybd1Leq6bNg
1 parent 86e6f6c commit 41a5fd3

5 files changed

Lines changed: 729 additions & 14 deletions

File tree

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
fix(driver-sql): re-seed a stale autonumber counter instead of burning a number per failed create (#5495)
6+
7+
`getNextSequenceValue` bootstraps a counter from the data-table `MAX` exactly
8+
once, in its `if (!existing)` branch; after that the data table is never
9+
consulted again. Any row landing by a path that bypasses `fillAutoNumberFields`
10+
— an `isSystem` seed replay, a `preserveAudit` historical import (both
11+
strip-exempt under #5503 and keeping their explicit numbers), or direct SQL —
12+
therefore never raises the sequence, and once the counter sits below `MAX` it is
13+
permanently behind. Every subsequent create collided, burned a number and failed
14+
the request, until the counter had ground past the seeded range one 409 at a
15+
time. That is the "one-time storm per database" the filing reported from
16+
HotCRM's 17.0 GA sweep: 25 consecutive `409 UNIQUE_VIOLATION`s with the
17+
attempted number climbing by one per failure.
18+
19+
Measured on `main` @ `86e6f6c`, counter seeded at 10 with rows 11–39 landed by a
20+
bypass path: **29 caller-visible 409s before a create succeeded** at
21+
`CASE-00040` on attempt 30. After this change the same fixture serves
22+
`CASE-00040` on the caller's **first** attempt, and `last_value` reaches 40 by
23+
one re-seed rather than 29 burns.
24+
25+
`create()` now re-seeds the counter from the data-table `MAX` and retries
26+
(bounded, 3 attempts) — but only when it can *prove* the collision was that
27+
counter's.
28+
29+
**Why the proof is not the conflicting column.** The obvious predicate ("retry
30+
when the conflicting column is this autonumber field") needs
31+
`uniqueViolationColumn()` (#6544) to name a column, and on a tenanted autonumber
32+
it never does — for two independent reasons, both measured and both pinned by
33+
tests. The filing's own message is a composite
34+
(`UNIQUE constraint failed: crm_case.organization_id, crm_case.case_number`),
35+
which that export refuses by contract; and what this repo builds today is
36+
narrower still — ADR-0120 D3 makes the index
37+
`(COALESCE(organization_id,'__global__'), field)`, an *expression* index, on
38+
which SQLite reports `UNIQUE constraint failed: index 'uniq_…'` and names no
39+
column at all. The "column not determinable" limb is not an edge case on this
40+
path; it is the only limb that ever runs there.
41+
42+
All three of `uniqueViolationColumn()`'s states are handled explicitly, because
43+
collapsing any two of them silently is how a real 409 gets eaten:
44+
45+
1. a column is named and it is one this driver generated → re-seed and retry;
46+
2. a column is named and it is not → the duplicate is on a value the **caller**
47+
supplied, so the original error is rethrown untouched;
48+
3. no column is determinable → decided from the **data**, not the message: if
49+
the value this driver just generated is already present in the same tenant
50+
partition the counter covers, the collision was the counter's. If it is not,
51+
the error is rethrown. One indexed lookup, on the failure path only — the
52+
happy path is unchanged.
53+
54+
No fifth dialect word-list: the judgement is `isUniqueViolationError` +
55+
`uniqueViolationColumn` from `@objectstack/types`, per Prime Directive #12 and
56+
the #5841 precedent. The re-seed's `MAX` scan is deliberately not wrapped in a
57+
`catch`, so a read failure propagates instead of being folded into `0` or a
58+
stale value (#6114's rule, #5979's family).
59+
60+
Retrying is confined to the no-caller-transaction case. Inside a caller's
61+
transaction the sequence `UPDATE` shares that transaction and rolls back with
62+
the refused `INSERT`, so no number is burned (measured), and on Postgres a
63+
constraint failure aborts the transaction outright — the caller owns that retry.
64+
65+
The `getNextSequenceValue` docstring is reconciled rather than left to
66+
contradict the code: a rolled-back insert burning a number is still by design,
67+
and that sentence used to read as though it also covered a *persistently
68+
failing* insert, which was the defect.
69+
70+
Inherited by `TursoDriver` (local/replica) and `SqliteWasmDriver`, each pinned
71+
by its own test rather than assumed from the base class (#6203). Turso's
72+
**remote** transport is unaffected in both directions: it overrides `create` and
73+
never enters `fillAutoNumberFields`, so it has neither the defect nor the fix.
Lines changed: 257 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,257 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#5495] The autonumber counter re-seeds from the data table when it can prove
5+
* a collision was its own — and refuses to when it cannot.
6+
*
7+
* # The defect
8+
*
9+
* `getNextSequenceValue` bootstraps from `MAX(existing)` exactly once, in its
10+
* `if (!existing)` branch. After that the data table is never consulted again.
11+
* Any row landing by a path that bypasses `fillAutoNumberFields` — an
12+
* `isSystem` seed replay, a `preserveAudit` historical import (both
13+
* strip-exempt under #5503, keeping their explicit numbers), or direct SQL —
14+
* therefore never raises the sequence, and once the counter sits below `MAX`
15+
* it is permanently behind. Every create then collides, burns a number and
16+
* fails the request, until the counter has ground past the seeded range one
17+
* 409 at a time.
18+
*
19+
* Measured on `main` @ `86e6f6c`, on the fixture below (counter seeded at 10,
20+
* rows 11–39 landed by a bypass path):
21+
*
22+
* | | before | after |
23+
* |:---|:---|:---|
24+
* | caller-visible 409s | **29** | **0** |
25+
* | value the caller finally got | `CASE-00040`, on attempt 30 | `CASE-00040`, on attempt 1 |
26+
* | `last_value` afterwards | 40, reached by 29 burns | 40, reached by one re-seed |
27+
*
28+
* That is the filing's own shape: 25 consecutive 409s climbing one per failure,
29+
* "a one-time storm per database" that stops once the counter grinds past the
30+
* seeds. Which is also why the repro here starts from a **fresh database with
31+
* seeded rows above the counter** — on a database already ground past them
32+
* every create succeeds on attempt 1 and the defect is invisible.
33+
*
34+
* # Why the retry cannot be decided from the error text
35+
*
36+
* The obvious predicate — "retry when the conflicting column is this autonumber
37+
* field" — needs `uniqueViolationColumn()` (#6544) to name a column. On the
38+
* configuration this card was filed against it never does, for two independent
39+
* reasons, both measured and both pinned below:
40+
*
41+
* - the filing's own message is a **composite** —
42+
* `UNIQUE constraint failed: crm_case.organization_id, crm_case.case_number`
43+
* — and `uniqueViolationColumn` refuses composites by contract;
44+
* - what this repo builds **today** is narrower still: ADR-0120 D3 makes the
45+
* tenanted unique index `(COALESCE(organization_id,'__global__'), field)`,
46+
* an *expression* index, on which SQLite reports
47+
* `UNIQUE constraint failed: index 'uniq_…'` and names no column at all.
48+
*
49+
* So the "column not determinable" limb is not an edge case on this path — it
50+
* is the only limb that ever runs for a tenanted autonumber. The discriminator
51+
* is therefore the DATA (`autoNumberValueExists`): if the value this driver
52+
* just generated is already present, the collision was this counter's. If it is
53+
* not, the duplicate is on a value the caller typed and the 409 is theirs.
54+
*
55+
* # On the assertion style
56+
*
57+
* ADR-0112's `code`/`status` envelope is a *wire* contract; what a refused
58+
* INSERT throws here is the driver's raw error, which carries neither. These
59+
* tests assert something stronger than a bare `toThrow()` in its place: the
60+
* specific constraint text, `isUniqueViolationError` agreeing, and — for the
61+
* case that must NOT be retried — that the error names the CALLER's field.
62+
* That matches the spelling `sql-driver-unique-tenancy.test.ts` already uses,
63+
* which #6543 deliberately left in place for exactly this reason.
64+
*/
65+
66+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
67+
import { isUniqueViolationError, uniqueViolationColumn } from '@objectstack/types';
68+
import { SqlDriver } from './index.js';
69+
70+
const SEQUENCES_TABLE = '_objectstack_sequences';
71+
72+
const CRM_CASE = {
73+
name: 'crm_case',
74+
fields: {
75+
organization_id: { type: 'string' },
76+
case_number: { type: 'autonumber', format: 'CASE-{00000}', unique: true },
77+
title: { type: 'string' },
78+
},
79+
} as any;
80+
81+
describe('[#5495] autonumber sequence re-seed on a provable collision', () => {
82+
let driver: SqlDriver;
83+
84+
const knex = () => (driver as any).knex;
85+
86+
/** `last_value` for one tenant's counter, or `undefined` when no row exists. */
87+
const lastValue = async (tenant: string): Promise<number | undefined> => {
88+
const rows = await knex()(SEQUENCES_TABLE).select('*');
89+
const row = rows.find((r: any) => r.object === 'crm_case' && r.field === 'case_number' && r.tenant_id === tenant);
90+
return row ? Number(row.last_value) : undefined;
91+
};
92+
93+
/** Land rows by a path that bypasses `fillAutoNumberFields`, as a seed replay does. */
94+
const bypassInsert = async (org: string, from: number, to: number) => {
95+
const rows = [];
96+
for (let n = from; n <= to; n++) {
97+
rows.push({ id: `${org}-s${n}`, organization_id: org, case_number: `CASE-${String(n).padStart(5, '0')}`, title: `seed ${n}` });
98+
}
99+
await knex()('crm_case').insert(rows);
100+
};
101+
102+
beforeEach(async () => {
103+
driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true });
104+
});
105+
106+
afterEach(async () => {
107+
await driver.disconnect();
108+
});
109+
110+
it('serves the create that used to take 30 attempts on the first one', async () => {
111+
await driver.initObjects([CRM_CASE]);
112+
113+
// A fresh database holding seeded rows: the first create bootstraps from
114+
// MAX and is correct.
115+
await bypassInsert('orgA', 9, 9);
116+
expect((await driver.create('crm_case', { organization_id: 'orgA', title: 'first' })).case_number).toBe('CASE-00010');
117+
expect(await lastValue('orgA')).toBe(10);
118+
119+
// More seeded rows land ABOVE the counter. Nothing tells the sequence.
120+
await bypassInsert('orgA', 11, 39);
121+
122+
const created = await driver.create('crm_case', { organization_id: 'orgA', title: 'after the seeds' });
123+
expect(created.case_number).toBe('CASE-00040');
124+
expect(await lastValue('orgA')).toBe(40);
125+
126+
// And the counter is simply +1 from here — the re-seed is not a mode the
127+
// driver stays in.
128+
expect((await driver.create('crm_case', { organization_id: 'orgA', title: 'next' })).case_number).toBe('CASE-00041');
129+
});
130+
131+
it('re-seeds only the tenant that collided', async () => {
132+
await driver.initObjects([CRM_CASE]);
133+
134+
await driver.create('crm_case', { organization_id: 'orgA', title: 'a1' });
135+
await driver.create('crm_case', { organization_id: 'orgB', title: 'b1' });
136+
await bypassInsert('orgA', 2, 30);
137+
138+
expect((await driver.create('crm_case', { organization_id: 'orgA', title: 'a2' })).case_number).toBe('CASE-00031');
139+
// orgB never collided, so its counter is untouched by orgA's re-seed.
140+
expect((await driver.create('crm_case', { organization_id: 'orgB', title: 'b2' })).case_number).toBe('CASE-00002');
141+
expect(await lastValue('orgB')).toBe(2);
142+
});
143+
144+
it('does not swallow a duplicate on a field the CALLER supplied', async () => {
145+
await driver.initObjects([
146+
{
147+
name: 'crm_case',
148+
fields: {
149+
organization_id: { type: 'string' },
150+
email: { type: 'string', unique: 'global' },
151+
case_number: { type: 'autonumber', format: 'CASE-{00000}', unique: true },
152+
title: { type: 'string' },
153+
},
154+
} as any,
155+
]);
156+
157+
await driver.create('crm_case', { organization_id: 'orgA', email: 'dup@example.com', title: 'one' });
158+
159+
const caught = await driver
160+
.create('crm_case', { organization_id: 'orgA', email: 'dup@example.com', title: 'two' })
161+
.then(
162+
() => undefined,
163+
(e: unknown) => e,
164+
);
165+
166+
// The 409 reaches the caller, and it is the CALLER's field that is named —
167+
// not retried away because some unique constraint happened to fire.
168+
expect(caught).toBeInstanceOf(Error);
169+
expect((caught as Error).message).toMatch(/UNIQUE constraint failed|duplicate key value/);
170+
expect(isUniqueViolationError(caught)).toBe(true);
171+
expect(uniqueViolationColumn(caught)).toBe('email');
172+
});
173+
174+
it('re-seeds when the dialect DOES name the column (the determinable limb)', async () => {
175+
// No tenant column, so the unique index is single-column and SQLite names
176+
// it — the one shape where `uniqueViolationColumn` answers.
177+
await driver.initObjects([
178+
{ name: 'contract', fields: { contract_number: { type: 'autonumber', format: 'CTR-{0000}', unique: true }, name: { type: 'string' } } } as any,
179+
]);
180+
181+
await driver.create('contract', { name: 'first' });
182+
await knex()('contract').insert({ id: 'bypass', contract_number: 'CTR-0002', name: 'seed replay' });
183+
184+
expect((await driver.create('contract', { name: 'after' })).contract_number).toBe('CTR-0003');
185+
});
186+
187+
it('leaves the caller transaction path exactly as it was', async () => {
188+
await driver.initObjects([CRM_CASE]);
189+
await driver.create('crm_case', { organization_id: 'orgA', title: 'warm' });
190+
await bypassInsert('orgA', 2, 2);
191+
192+
const before = await lastValue('orgA');
193+
const caught = await knex()
194+
.transaction(async (trx: any) => {
195+
await driver.create('crm_case', { organization_id: 'orgA', title: 'in-trx' }, { transaction: trx } as any);
196+
})
197+
.then(
198+
() => undefined,
199+
(e: unknown) => e,
200+
);
201+
202+
// Still refused — the caller owns the retry inside their own transaction
203+
// (on Postgres a constraint failure aborts it outright). And the sequence
204+
// UPDATE rolled back with the INSERT, so nothing was burned.
205+
expect(caught).toBeInstanceOf(Error);
206+
expect((caught as Error).message).toMatch(/UNIQUE constraint failed|duplicate key value/);
207+
expect(await lastValue('orgA')).toBe(before);
208+
});
209+
210+
it('pins WHY the column-name predicate cannot carry this decision', async () => {
211+
await driver.initObjects([CRM_CASE]);
212+
213+
// 1. What this repo builds today: an expression composite, on which SQLite
214+
// reports the INDEX and no column.
215+
const indexes: any[] = await knex().raw(
216+
`select name, sql from sqlite_master where type='index' and tbl_name='crm_case'`,
217+
);
218+
const declared = indexes.find((r) => r.name === 'uniq_crm_case_organization_id_case_number');
219+
expect(declared?.sql).toContain(`COALESCE(\`organization_id\`, '__global__')`);
220+
221+
await knex()('crm_case').insert({ id: 'd1', organization_id: 'orgA', case_number: 'CASE-00001', title: 'a' });
222+
const caught = await knex()('crm_case')
223+
.insert({ id: 'd2', organization_id: 'orgA', case_number: 'CASE-00001', title: 'b' })
224+
.then(
225+
() => undefined,
226+
(e: unknown) => e,
227+
);
228+
expect(isUniqueViolationError(caught)).toBe(true);
229+
expect((caught as Error).message).toMatch(/UNIQUE constraint failed: index '/);
230+
expect(uniqueViolationColumn(caught)).toBeUndefined();
231+
232+
// 2. And the message the filing itself reported — the plain composite that
233+
// predates ADR-0120 D3 — is refused for the other reason.
234+
expect(
235+
uniqueViolationColumn('UNIQUE constraint failed: crm_case.organization_id, crm_case.case_number'),
236+
).toBeUndefined();
237+
});
238+
239+
it('gives the caller the original error when the collision is not the counter’s', async () => {
240+
// A value the caller supplies explicitly is not a reservation, so no probe
241+
// and no retry can apply to it even though it IS the autonumber column.
242+
await driver.initObjects([CRM_CASE]);
243+
await driver.create('crm_case', { organization_id: 'orgA', title: 'first' });
244+
245+
const caught = await driver
246+
.create('crm_case', { organization_id: 'orgA', case_number: 'CASE-00001', title: 'explicit duplicate' })
247+
.then(
248+
() => undefined,
249+
(e: unknown) => e,
250+
);
251+
252+
expect(caught).toBeInstanceOf(Error);
253+
expect(isUniqueViolationError(caught)).toBe(true);
254+
// Unchanged: an explicit value is preserved, never regenerated.
255+
expect(await lastValue('orgA')).toBe(1);
256+
});
257+
});

0 commit comments

Comments
 (0)