diff --git a/.changeset/autonumber-sequence-resync-on-collision.md b/.changeset/autonumber-sequence-resync-on-collision.md new file mode 100644 index 0000000000..b51a6437b0 --- /dev/null +++ b/.changeset/autonumber-sequence-resync-on-collision.md @@ -0,0 +1,73 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): re-seed a stale autonumber counter instead of burning a number per failed create (#5495) + +`getNextSequenceValue` bootstraps a counter from the data-table `MAX` exactly +once, in its `if (!existing)` branch; after that the data table is never +consulted again. Any row landing by a path that bypasses `fillAutoNumberFields` +— an `isSystem` seed replay, a `preserveAudit` historical import (both +strip-exempt under #5503 and keeping their explicit numbers), or direct SQL — +therefore never raises the sequence, and once the counter sits below `MAX` it is +permanently behind. Every subsequent create collided, burned a number and failed +the request, until the counter had ground past the seeded range one 409 at a +time. That is the "one-time storm per database" the filing reported from +HotCRM's 17.0 GA sweep: 25 consecutive `409 UNIQUE_VIOLATION`s with the +attempted number climbing by one per failure. + +Measured on `main` @ `86e6f6c`, counter seeded at 10 with rows 11–39 landed by a +bypass path: **29 caller-visible 409s before a create succeeded** at +`CASE-00040` on attempt 30. After this change the same fixture serves +`CASE-00040` on the caller's **first** attempt, and `last_value` reaches 40 by +one re-seed rather than 29 burns. + +`create()` now re-seeds the counter from the data-table `MAX` and retries +(bounded, 3 attempts) — but only when it can *prove* the collision was that +counter's. + +**Why the proof is not the conflicting column.** The obvious predicate ("retry +when the conflicting column is this autonumber field") needs +`uniqueViolationColumn()` (#6544) to name a column, and on a tenanted autonumber +it never does — for two independent reasons, both measured and both pinned by +tests. The filing's own message is a composite +(`UNIQUE constraint failed: crm_case.organization_id, crm_case.case_number`), +which that export refuses by contract; and what this repo builds today is +narrower still — ADR-0120 D3 makes the index +`(COALESCE(organization_id,'__global__'), field)`, an *expression* index, on +which SQLite reports `UNIQUE constraint failed: index 'uniq_…'` and names no +column at all. The "column not determinable" limb is not an edge case on this +path; it is the only limb that ever runs there. + +All three of `uniqueViolationColumn()`'s states are handled explicitly, because +collapsing any two of them silently is how a real 409 gets eaten: + +1. a column is named and it is one this driver generated → re-seed and retry; +2. a column is named and it is not → the duplicate is on a value the **caller** + supplied, so the original error is rethrown untouched; +3. no column is determinable → decided from the **data**, not the message: if + the value this driver just generated is already present in the same tenant + partition the counter covers, the collision was the counter's. If it is not, + the error is rethrown. One indexed lookup, on the failure path only — the + happy path is unchanged. + +No fifth dialect word-list: the judgement is `isUniqueViolationError` + +`uniqueViolationColumn` from `@objectstack/types`, per Prime Directive #12 and +the #5841 precedent. The re-seed's `MAX` scan is deliberately not wrapped in a +`catch`, so a read failure propagates instead of being folded into `0` or a +stale value (#6114's rule, #5979's family). + +Retrying is confined to the no-caller-transaction case. Inside a caller's +transaction the sequence `UPDATE` shares that transaction and rolls back with +the refused `INSERT`, so no number is burned (measured), and on Postgres a +constraint failure aborts the transaction outright — the caller owns that retry. + +The `getNextSequenceValue` docstring is reconciled rather than left to +contradict the code: a rolled-back insert burning a number is still by design, +and that sentence used to read as though it also covered a *persistently +failing* insert, which was the defect. + +Inherited by `TursoDriver` (local/replica) and `SqliteWasmDriver`, each pinned +by its own test rather than assumed from the base class (#6203). Turso's +**remote** transport is unaffected in both directions: it overrides `create` and +never enters `fillAutoNumberFields`, so it has neither the defect nor the fix. diff --git a/packages/drivers/driver-sql/src/sql-driver-autonumber-resync.test.ts b/packages/drivers/driver-sql/src/sql-driver-autonumber-resync.test.ts new file mode 100644 index 0000000000..e50b4533a9 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-autonumber-resync.test.ts @@ -0,0 +1,343 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5495] The autonumber counter re-seeds from the data table when it can prove + * a collision was its own — and refuses to when it cannot. + * + * # The defect + * + * `getNextSequenceValue` bootstraps from `MAX(existing)` exactly once, in its + * `if (!existing)` branch. After that the data table is never consulted again. + * Any row landing by a path that bypasses `fillAutoNumberFields` — an + * `isSystem` seed replay, a `preserveAudit` historical import (both + * strip-exempt under #5503, keeping their explicit numbers), or direct SQL — + * therefore never raises the sequence, and once the counter sits below `MAX` + * it is permanently behind. Every create then collides, burns a number and + * fails the request, until the counter has ground past the seeded range one + * 409 at a time. + * + * Measured on `main` @ `86e6f6c`, on the fixture below (counter seeded at 10, + * rows 11–39 landed by a bypass path): + * + * | | before | after | + * |:---|:---|:---| + * | caller-visible 409s | **29** | **0** | + * | value the caller finally got | `CASE-00040`, on attempt 30 | `CASE-00040`, on attempt 1 | + * | `last_value` afterwards | 40, reached by 29 burns | 40, reached by one re-seed | + * + * That is the filing's own shape: 25 consecutive 409s climbing one per failure, + * "a one-time storm per database" that stops once the counter grinds past the + * seeds. Which is also why the repro here starts from a **fresh database with + * seeded rows above the counter** — on a database already ground past them + * every create succeeds on attempt 1 and the defect is invisible. + * + * # Why the retry cannot be decided from the error text + * + * The obvious predicate — "retry when the conflicting column is this autonumber + * field" — needs `uniqueViolationColumn()` (#6544) to name a column. On the + * configuration this card was filed against it never does, for two independent + * reasons, both measured and both pinned below: + * + * - the filing's own message is a **composite** — + * `UNIQUE constraint failed: crm_case.organization_id, crm_case.case_number` + * — and `uniqueViolationColumn` refuses composites by contract; + * - what this repo builds **today** is narrower still: ADR-0120 D3 makes the + * tenanted unique index `(COALESCE(organization_id,'__global__'), field)`, + * an *expression* index, on which SQLite reports + * `UNIQUE constraint failed: index 'uniq_…'` and names no column at all. + * + * So the "column not determinable" limb is not an edge case on this path — it + * is the only limb that ever runs for a tenanted autonumber. The discriminator + * is therefore the DATA (`autoNumberValueExists`): if the value this driver + * just generated is already present, the collision was this counter's. If it is + * not, the duplicate is on a value the caller typed and the 409 is theirs. + * + * # On the assertion style + * + * ADR-0112's `code`/`status` envelope is a *wire* contract; what a refused + * INSERT throws here is the driver's raw error, which carries neither. These + * tests assert something stronger than a bare `toThrow()` in its place: the + * specific constraint text, `isUniqueViolationError` agreeing, and — for the + * case that must NOT be retried — that the error names the CALLER's field. + * That matches the spelling `sql-driver-unique-tenancy.test.ts` already uses, + * which #6543 deliberately left in place for exactly this reason. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { isUniqueViolationError, uniqueViolationColumn } from '@objectstack/types'; +import { SqlDriver } from './index.js'; + +const SEQUENCES_TABLE = '_objectstack_sequences'; + +const CRM_CASE = { + name: 'crm_case', + fields: { + organization_id: { type: 'string' }, + case_number: { type: 'autonumber', format: 'CASE-{00000}', unique: true }, + title: { type: 'string' }, + }, +} as any; + +describe('[#5495] autonumber sequence re-seed on a provable collision', () => { + let driver: SqlDriver; + + const knex = () => (driver as any).knex; + + /** `last_value` for one tenant's counter, or `undefined` when no row exists. */ + const lastValue = async (tenant: string): Promise => { + const rows = await knex()(SEQUENCES_TABLE).select('*'); + const row = rows.find((r: any) => r.object === 'crm_case' && r.field === 'case_number' && r.tenant_id === tenant); + return row ? Number(row.last_value) : undefined; + }; + + /** Land rows by a path that bypasses `fillAutoNumberFields`, as a seed replay does. */ + const bypassInsert = async (org: string, from: number, to: number) => { + const rows = []; + for (let n = from; n <= to; n++) { + rows.push({ id: `${org}-s${n}`, organization_id: org, case_number: `CASE-${String(n).padStart(5, '0')}`, title: `seed ${n}` }); + } + await knex()('crm_case').insert(rows); + }; + + beforeEach(async () => { + driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('serves the create that used to take 30 attempts on the first one', async () => { + await driver.initObjects([CRM_CASE]); + + // A fresh database holding seeded rows: the first create bootstraps from + // MAX and is correct. + await bypassInsert('orgA', 9, 9); + expect((await driver.create('crm_case', { organization_id: 'orgA', title: 'first' })).case_number).toBe('CASE-00010'); + expect(await lastValue('orgA')).toBe(10); + + // More seeded rows land ABOVE the counter. Nothing tells the sequence. + await bypassInsert('orgA', 11, 39); + + const created = await driver.create('crm_case', { organization_id: 'orgA', title: 'after the seeds' }); + expect(created.case_number).toBe('CASE-00040'); + expect(await lastValue('orgA')).toBe(40); + + // And the counter is simply +1 from here — the re-seed is not a mode the + // driver stays in. + expect((await driver.create('crm_case', { organization_id: 'orgA', title: 'next' })).case_number).toBe('CASE-00041'); + }); + + it('re-seeds only the tenant that collided', async () => { + await driver.initObjects([CRM_CASE]); + + await driver.create('crm_case', { organization_id: 'orgA', title: 'a1' }); + await driver.create('crm_case', { organization_id: 'orgB', title: 'b1' }); + await bypassInsert('orgA', 2, 30); + + expect((await driver.create('crm_case', { organization_id: 'orgA', title: 'a2' })).case_number).toBe('CASE-00031'); + // orgB never collided, so its counter is untouched by orgA's re-seed. + expect((await driver.create('crm_case', { organization_id: 'orgB', title: 'b2' })).case_number).toBe('CASE-00002'); + expect(await lastValue('orgB')).toBe(2); + }); + + it('does not swallow a duplicate on a field the CALLER supplied', async () => { + await driver.initObjects([ + { + name: 'crm_case', + fields: { + organization_id: { type: 'string' }, + email: { type: 'string', unique: 'global' }, + case_number: { type: 'autonumber', format: 'CASE-{00000}', unique: true }, + title: { type: 'string' }, + }, + } as any, + ]); + + await driver.create('crm_case', { organization_id: 'orgA', email: 'dup@example.com', title: 'one' }); + + const caught = await driver + .create('crm_case', { organization_id: 'orgA', email: 'dup@example.com', title: 'two' }) + .then( + () => undefined, + (e: unknown) => e, + ); + + // The 409 reaches the caller, and it is the CALLER's field that is named — + // not retried away because some unique constraint happened to fire. + expect(caught).toBeInstanceOf(Error); + expect((caught as Error).message).toMatch(/UNIQUE constraint failed|duplicate key value/); + expect(isUniqueViolationError(caught)).toBe(true); + expect(uniqueViolationColumn(caught)).toBe('email'); + }); + + it('re-seeds when the dialect DOES name the column (the determinable limb)', async () => { + // No tenant column, so the unique index is single-column and SQLite names + // it — the one shape where `uniqueViolationColumn` answers. + await driver.initObjects([ + { name: 'contract', fields: { contract_number: { type: 'autonumber', format: 'CTR-{0000}', unique: true }, name: { type: 'string' } } } as any, + ]); + + await driver.create('contract', { name: 'first' }); + await knex()('contract').insert({ id: 'bypass', contract_number: 'CTR-0002', name: 'seed replay' }); + + expect((await driver.create('contract', { name: 'after' })).contract_number).toBe('CTR-0003'); + }); + + it('leaves the caller transaction path exactly as it was', async () => { + await driver.initObjects([CRM_CASE]); + await driver.create('crm_case', { organization_id: 'orgA', title: 'warm' }); + await bypassInsert('orgA', 2, 2); + + const before = await lastValue('orgA'); + const caught = await knex() + .transaction(async (trx: any) => { + await driver.create('crm_case', { organization_id: 'orgA', title: 'in-trx' }, { transaction: trx } as any); + }) + .then( + () => undefined, + (e: unknown) => e, + ); + + // Still refused — the caller owns the retry inside their own transaction + // (on Postgres a constraint failure aborts it outright). And the sequence + // UPDATE rolled back with the INSERT, so nothing was burned. + expect(caught).toBeInstanceOf(Error); + expect((caught as Error).message).toMatch(/UNIQUE constraint failed|duplicate key value/); + expect(await lastValue('orgA')).toBe(before); + }); + + it('pins WHY the column-name predicate cannot carry this decision', async () => { + await driver.initObjects([CRM_CASE]); + + // 1. What this repo builds today: an expression composite, on which SQLite + // reports the INDEX and no column. + const indexes: any[] = await knex().raw( + `select name, sql from sqlite_master where type='index' and tbl_name='crm_case'`, + ); + const declared = indexes.find((r) => r.name === 'uniq_crm_case_organization_id_case_number'); + expect(declared?.sql).toContain(`COALESCE(\`organization_id\`, '__global__')`); + + await knex()('crm_case').insert({ id: 'd1', organization_id: 'orgA', case_number: 'CASE-00001', title: 'a' }); + const caught = await knex()('crm_case') + .insert({ id: 'd2', organization_id: 'orgA', case_number: 'CASE-00001', title: 'b' }) + .then( + () => undefined, + (e: unknown) => e, + ); + expect(isUniqueViolationError(caught)).toBe(true); + expect((caught as Error).message).toMatch(/UNIQUE constraint failed: index '/); + expect(uniqueViolationColumn(caught)).toBeUndefined(); + + // 2. And the message the filing itself reported — the plain composite that + // predates ADR-0120 D3 — is refused for the other reason. + expect( + uniqueViolationColumn('UNIQUE constraint failed: crm_case.organization_id, crm_case.case_number'), + ).toBeUndefined(); + }); + + /** + * The three-state routing, held against the shapes Postgres and MySQL + * actually hand knex. + * + * This package's unit suite boots SQLite only, so the other two dialects' + * errors are INJECTED rather than driven through a live server — the same + * method, and the same reason, as `sql-driver-unique-violation-predicate`. + * What it buys: the routing is what decides whether a caller's 409 survives, + * and on MySQL — where `uniqueViolationColumn` can never answer, by contract + * — state 3 is the only state that exists. Reasoning about that limb is not + * the same as pinning it. + */ + describe('three-state routing on the dialects this package ships', () => { + const reservation = { + field: 'case_number', + tableName: 'crm_case', + prefix: 'CASE-', + scope: '', + suffix: '', + tenantField: 'organization_id', + tenantId: 'orgA', + value: 'CASE-00001', + }; + + /** Route one injected error, against a table where `CASE-00001` DOES exist. */ + const route = async (error: unknown) => { + const colliding = await (driver as any).collidingAutoNumberReservations(error, [reservation]); + return colliding.map((r: any) => r.field); + }; + + beforeEach(async () => { + await driver.initObjects([CRM_CASE]); + await knex()('crm_case').insert({ id: 'taken', organization_id: 'orgA', case_number: 'CASE-00001', title: 'taken' }); + }); + + it('state 1 — Postgres names our column in its DETAIL line', async () => { + const err: any = new Error('insert into "crm_case" … - duplicate key value violates unique constraint "uniq_crm_case_case_number"'); + err.code = '23505'; + err.detail = 'Key (case_number)=(CASE-00001) already exists.'; + expect(uniqueViolationColumn(err)).toBe('case_number'); + expect(await route(err)).toEqual(['case_number']); + }); + + it('state 2 — Postgres names a column the CALLER supplied, so nothing is retried', async () => { + const err: any = new Error('insert into "crm_case" … - duplicate key value violates unique constraint "uniq_crm_case_email"'); + err.code = '23505'; + err.detail = 'Key (email)=(dup@example.com) already exists.'; + expect(uniqueViolationColumn(err)).toBe('email'); + // Decided WITHOUT probing the data — and the data would have said yes, + // since CASE-00001 is present. That is the whole point: a named column + // that is not ours ends the matter. + expect(await route(err)).toEqual([]); + }); + + it('state 3 — Postgres composite DETAIL names no single column, so the DATA decides', async () => { + const err: any = new Error('insert into "crm_case" … - duplicate key value violates unique constraint "uniq_crm_case_org_case"'); + err.code = '23505'; + err.detail = 'Key (organization_id, case_number)=(orgA, CASE-00001) already exists.'; + expect(uniqueViolationColumn(err)).toBeUndefined(); + expect(await route(err)).toEqual(['case_number']); + }); + + it('state 3 — MySQL can only ever name an index, so the DATA decides there always', async () => { + const err: any = new Error("insert into `crm_case` … - ER_DUP_ENTRY: Duplicate entry 'orgA-CASE-00001' for key 'uniq_crm_case_org_case'"); + err.errno = 1062; + err.code = 'ER_DUP_ENTRY'; + expect(isUniqueViolationError(err)).toBe(true); + expect(uniqueViolationColumn(err)).toBeUndefined(); + expect(await route(err)).toEqual(['case_number']); + }); + + it('state 3 — but an undeterminable violation on someone ELSE’s value is still not ours', async () => { + const err: any = new Error("insert into `crm_case` … - ER_DUP_ENTRY: Duplicate entry 'x' for key 'uniq_crm_case_email'"); + err.errno = 1062; + const other = { ...reservation, value: 'CASE-99999' }; // never written + const colliding = await (driver as any).collidingAutoNumberReservations(err, [other]); + expect(colliding).toEqual([]); + }); + + it('a failure that is not a unique violation at all is never routed here', async () => { + const err: any = new Error('insert into `crm_case` … - NOT NULL constraint failed: crm_case.title'); + expect(isUniqueViolationError(err)).toBe(false); + expect(await route(err)).toEqual([]); + }); + }); + + it('gives the caller the original error when the collision is not the counter’s', async () => { + // A value the caller supplies explicitly is not a reservation, so no probe + // and no retry can apply to it even though it IS the autonumber column. + await driver.initObjects([CRM_CASE]); + await driver.create('crm_case', { organization_id: 'orgA', title: 'first' }); + + const caught = await driver + .create('crm_case', { organization_id: 'orgA', case_number: 'CASE-00001', title: 'explicit duplicate' }) + .then( + () => undefined, + (e: unknown) => e, + ); + + expect(caught).toBeInstanceOf(Error); + expect(isUniqueViolationError(caught)).toBe(true); + // Unchanged: an explicit value is preserved, never regenerated. + expect(await lastValue('orgA')).toBe(1); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 4d5c6e7371..5613898b71 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -40,7 +40,7 @@ import type { DriverQuery, IDataDriver } from '@objectstack/spec/contracts'; import { StandardErrorCode } from '@objectstack/spec/api'; import { StorageNameMapping } from '@objectstack/spec/system'; import { ExternalSchemaModeViolationError } from '@objectstack/spec/shared'; -import { isUniqueViolationError, resolveTenancyPosture } from '@objectstack/types'; +import { isUniqueViolationError, uniqueViolationColumn, resolveTenancyPosture } from '@objectstack/types'; import { postureEnforcesWall } from '@objectstack/spec/security'; import { nextUtcCalendarDay } from '@objectstack/core'; import { @@ -123,6 +123,50 @@ function lastIdentifierSegment(raw: string): string { */ const SEQUENCES_TABLE = '_objectstack_sequences'; +/** + * How many times `create()` will re-seed a stale autonumber counter and retry + * a refused INSERT before giving the unique violation back to the caller + * (#5495). + * + * One is enough for the defect this bounds: a re-seed jumps the counter to the + * observed MAX in a single step, so the whole "one-time storm per database" + * collapses to one retry no matter how far behind the counter had fallen. The + * remaining budget is for concurrent writers taking the re-seeded number + * between our scan and our INSERT, which is a per-attempt race rather than a + * standing condition. The bound is what keeps a genuinely un-satisfiable insert + * from looping: at exhaustion the original error is rethrown unchanged. + */ +const AUTONUMBER_COLLISION_RETRIES = 3; + +/** + * One autonumber value this driver generated for a row it is about to insert + * (#5495) — the field, the value, and the coordinates under which that value's + * counter would be re-seeded. + * + * Its reason for existing is the distinction between "a number this driver + * handed out" and "a value the caller typed": only the former may be + * regenerated after a unique violation, and only the latter's 409 belongs to + * the client. + */ +export interface AutoNumberReservation { + /** The `auto_number` field on the object. */ + field: string; + /** Physical table the value lives in (external objects map away from the object name). */ + tableName: string; + /** Full rendered prefix — the LIKE anchor the re-seed scan uses. */ + prefix: string; + /** Rendered counter scope (date/`{field}` tokens); `''` for a fixed-prefix format. */ + scope: string; + /** Rendered text after the counter slot (#6468). */ + suffix: string; + /** Tenant column for this object, or `null` when it has none. */ + tenantField: string | null; + /** Tenant this row belongs to, or `null` for the global counter. */ + tenantId: string | null; + /** The rendered value written onto the row. */ + value: string; +} + // GLOBAL_TENANT ('__global__') — the sentinel for the NULL-organization // ("platform") bucket — is defined ONCE in schema-drift.ts and imported here: // since ADR-0120 D3 it names the same bucket in two subsystems (the autonumber @@ -3247,16 +3291,48 @@ export class SqlDriver implements IDataDriver { this.auditMissingTenant(object, 'create', options); this.injectTenantOnInsert(object, toInsert, options); - await this.fillAutoNumberFields(object, toInsert, options); - // Rotation (ADR-0057 P2): the base name is a read-only view — new rows - // land in the current shard. - const builder = this.getBuilder(this.rotationWriteTarget(object) ?? object, options); - const formatted = this.applyWriteColumnMap(object, this.formatInput(object, toInsert)); - this.stampInsertTimestamps(object, formatted); + // #5495. The autonumber counter is bootstrapped from the data table exactly + // once, so any row that lands by a path bypassing `fillAutoNumberFields` + // leaves it permanently behind `MAX` — and every create then collided, + // burned a number and failed the request until it had ground past the + // seeded range one 409 at a time. Re-seed and retry instead, but ONLY when + // the collision is provably this counter's; see + // {@link collidingAutoNumberReservations} for how that is decided and why + // it cannot be decided from the error text alone. + // + // Retrying is confined to the no-caller-transaction case on purpose. Inside + // a caller's transaction there is nothing to repair here — the sequence + // UPDATE shares that transaction and rolls back with the refused INSERT, so + // no number is burned (measured) — and there is something to break: on + // Postgres a constraint failure aborts the whole transaction, so a retry + // issued on it could not succeed anyway. The caller owns that retry. + const mayRetry = options?.transaction === undefined; + for (let attempt = 0; ; attempt++) { + const reservations = await this.fillAutoNumberFields(object, toInsert, options); + + // Rotation (ADR-0057 P2): the base name is a read-only view — new rows + // land in the current shard. + const builder = this.getBuilder(this.rotationWriteTarget(object) ?? object, options); + const formatted = this.applyWriteColumnMap(object, this.formatInput(object, toInsert)); + this.stampInsertTimestamps(object, formatted); - const result = await builder.insert(formatted).returning('*'); - return this.formatOutput(object, result[0]); + try { + const result = await builder.insert(formatted).returning('*'); + return this.formatOutput(object, result[0]); + } catch (error) { + if (!mayRetry || attempt >= AUTONUMBER_COLLISION_RETRIES) throw error; + const colliding = await this.collidingAutoNumberReservations(error, reservations, options); + if (colliding.length === 0) throw error; + for (const reservation of colliding) { + await this.resyncSequenceToDataMax(reservation); + // Clear only what collided, so the next pass regenerates exactly + // those fields. A reservation that did NOT collide keeps its value + // rather than burning a second number for nothing. + delete toInsert[reservation.field]; + } + } + } } /** @@ -3500,8 +3576,34 @@ export class SqlDriver implements IDataDriver { * transaction reads-modifies-writes at a time. A PK-violation race on * first insert is retried as an UPDATE. * - * Gaps are tolerated by design — a rolled-back insert "burns" a number, - * matching standard sequence semantics. + * ## Which gaps are by design, and which one was a defect (#5495) + * + * These are two different things, and the sentence that used to stand here + * ("gaps are tolerated by design — a rolled-back insert burns a number") + * covered only the first while reading as though it covered both: + * + * - **A rolled-back insert burns a number — still by design.** The caller + * asked for a value, got one, and abandoned it. That is what every + * standard sequence does, and nothing here tries to reclaim it. + * - **A *persistently failing* insert used to burn one number per attempt — + * that was the defect.** The bootstrap below runs exactly once, in the + * `if (!existing)` branch: after it, the data table is never consulted + * again. Any row landing by a path that bypasses `fillAutoNumberFields` + * (an `isSystem` seed replay or a `preserveAudit` import — both + * strip-exempt under #5503 and keeping their explicit numbers — or direct + * SQL) therefore never raises the sequence, and once the counter sits + * below `MAX` it is permanently behind. Every create then collided, + * burned a number, and failed the request, until the counter had ground + * past the seeded range one 409 at a time. That is the "one-time storm + * per database" #5495 reported from the field: 25 consecutive 409s. + * + * The second one is no longer reachable from `create()`, which re-seeds this + * counter from the data-table MAX and retries when it can prove the + * collision was this counter's — see {@link collidingAutoNumberReservations} + * and {@link resyncSequenceToDataMax}. The storm now costs one internal + * retry instead of one failed request per burned number. This function's own + * contract is unchanged: it still hands out one value per call and never + * takes one back. */ protected async getNextSequenceValue( object: string, @@ -3595,18 +3697,26 @@ export class SqlDriver implements IDataDriver { * plus `{field}` interpolation from the row), so it resets per period/group; * the full rendered prefix bootstraps the counter from existing data, and the * tenant scopes it for isolation. + * + * Returns one {@link AutoNumberReservation} per field it actually generated — + * the collision path in `create()` needs to know which values on the row are + * this driver's own and which the caller supplied, and under what scan + * coordinates a stale counter would be re-seeded (#5495). A field the caller + * filled is not reported, because a duplicate on it is the caller's 409 to + * receive, not a sequence to re-seed. */ protected async fillAutoNumberFields( object: string, row: Record, options?: DriverOptions, - ): Promise { + ): Promise { // Scan/seed the physical (remote) table for an external object; managed // objects fall through to the storage-mapped name. Config lookup stays // keyed by object name (matching initObjects/registerExternalObject). const tableName = this.physicalTableByObject[object] ?? StorageNameMapping.resolveTableName({ name: object } as any); const cfgs = this.autoNumberFields[object] || this.autoNumberFields[tableName]; - if (!cfgs || cfgs.length === 0) return; + if (!cfgs || cfgs.length === 0) return []; + const reservations: AutoNumberReservation[] = []; const parentTrx = options?.transaction as Knex.Transaction | undefined; const timezone = options?.timezone; const now = new Date(); @@ -3647,8 +3757,145 @@ export class SqlDriver implements IDataDriver { probe.scope, probe.suffix, ); - row[cfg.name] = renderAutonumber({ tokens: cfg.tokens, seq: next, record: row, now, timezone }).value; + const value = renderAutonumber({ tokens: cfg.tokens, seq: next, record: row, now, timezone }).value; + row[cfg.name] = value; + reservations.push({ + field: cfg.name, + tableName, + prefix: probe.prefix, + scope: probe.scope, + suffix: probe.suffix, + tenantField: cfg.tenantField ?? null, + tenantId, + value, + }); + } + return reservations; + } + + /** + * Does a row already hold `reservation.value` in the partition this + * counter covers? (#5495) + * + * This is the collision path's **dialect-free** discriminator, and it exists + * because the text-based one cannot answer for the configuration the field + * report came from. `uniqueViolationColumn()` (#6544) deliberately returns + * `undefined` for a composite key and for an index name — and the unique + * index this driver builds for a tenanted autonumber field is BOTH: ADR-0120 + * D3 makes it `(COALESCE(organization_id,'__global__'), field)`, an + * expression composite, on which SQLite reports + * `UNIQUE constraint failed: index 'uniq_…'` and names no column at all + * (measured). So on the exact shape #5495 was filed against, the conflicting + * column is *never* determinable, and a retry predicate resting on it would + * either never fire or fire blindly. + * + * Asking the data instead is decidable in every dialect: if the value this + * driver just generated is already present, the collision was this counter's + * and re-seeding is the right answer. If it is absent, the duplicate was on + * some OTHER unique field — a value the caller typed — and that 409 is theirs + * to receive, so the error is rethrown untouched. That is the distinction the + * card required and the reason a blanket "retry any unique violation" is + * wrong. + * + * Costs one indexed lookup, on the failure path only. The happy path is + * unchanged. + * + * The partition matches {@link scanMaxNumericTail} exactly — same tenant + * column, same tenant value — so "exists here" and "would be re-seeded from + * here" cannot disagree. Built directly on the query runner rather than via + * `getBuilder`, mirroring `scanMaxNumericTail`: this is the sequence's own + * bookkeeping read against an explicit tenant, not a caller-facing read. + */ + protected async autoNumberValueExists( + queryRunner: Knex | Knex.Transaction, + reservation: AutoNumberReservation, + ): Promise { + let builder = queryRunner(reservation.tableName).select(reservation.field).where(reservation.field, reservation.value); + if (reservation.tenantField && reservation.tenantId !== null) { + builder = builder.where(reservation.tenantField, reservation.tenantId); } + const hit = await builder.first(); + return hit !== undefined && hit !== null; + } + + /** + * Which of this row's generated autonumbers did the failed INSERT collide on? + * Empty means "none of them" — the caller must rethrow (#5495). + * + * Three states, all of them handled explicitly, because #6544's contract has + * three and collapsing any two of them silently is how a real 409 gets eaten: + * + * 1. **A column was named and it is one of ours** → that reservation + * collided. No probe needed; the dialect already answered. + * 2. **A column was named and it is not ours** → the duplicate is on a + * caller-supplied unique field. Return empty: the 409 is the client's and + * must reach them unchanged. Retrying here would silently swallow it. + * 3. **No column was determinable** — a composite key, an index name, or + * MySQL, which names only indexes. This is NOT treated as either of the + * first two. It is resolved by {@link autoNumberValueExists}, which reads + * the data instead of the message. On this repo's tenanted autonumber + * index, state 3 is the ONLY state that ever occurs. + */ + protected async collidingAutoNumberReservations( + error: unknown, + reservations: AutoNumberReservation[], + options?: DriverOptions, + ): Promise { + if (reservations.length === 0 || !isUniqueViolationError(error)) return []; + + const column = uniqueViolationColumn(error); + if (column !== undefined) return reservations.filter((r) => r.field === column); + + const runner: Knex | Knex.Transaction = (options?.transaction as Knex.Transaction | undefined) ?? this.knex; + const colliding: AutoNumberReservation[] = []; + for (const reservation of reservations) { + if (await this.autoNumberValueExists(runner, reservation)) colliding.push(reservation); + } + return colliding; + } + + /** + * Re-seed one counter from the data-table MAX, the operation the original + * bootstrap performs exactly once and never again (#5495). + * + * Only ever moves a counter FORWARD. A counter already at or ahead of the + * observed MAX is left alone: the collision that brought us here was then a + * concurrent writer taking the number rather than a stale counter, and + * rewinding would hand out numbers that are already in use. + * + * ## Read failures are not swallowed + * + * The MAX scan is deliberately NOT wrapped in a catch. #6114's rule for the + * engine's seeding path is that a read failure must be type-discriminated + * (`isMissingTableError`) and never folded into `0` or a stale value — the + * #5979 family. Here the stronger form is available: there is nothing to + * discriminate, because a scan failure simply propagates. Note also that the + * benign case that rule exists for cannot arise on this path — we are here + * *because* an INSERT into this very table was refused by a constraint, so + * the table demonstrably exists. + */ + protected async resyncSequenceToDataMax(reservation: AutoNumberReservation): Promise { + await this.ensureSequencesTable(); + const resolvedTenantId = + reservation.tenantField && reservation.tenantId ? String(reservation.tenantId) : GLOBAL_TENANT; + const key = this.sequencesHasKeyHash + ? { key_hash: this.sequenceKeyHash(reservation.tableName, resolvedTenantId, reservation.field, reservation.scope) } + : { object: reservation.tableName, tenant_id: resolvedTenantId, field: reservation.field }; + + await this.knex.transaction(async (trx) => { + const observedMax = await this.scanMaxNumericTail( + trx, + reservation.tableName, + reservation.field, + reservation.prefix, + reservation.tenantField, + resolvedTenantId === GLOBAL_TENANT ? null : resolvedTenantId, + reservation.suffix, + ); + const existing = await trx(SEQUENCES_TABLE).where(key).first(); + if (!existing || Number(existing.last_value) >= observedMax) return; + await trx(SEQUENCES_TABLE).where(key).update({ last_value: observedMax, updated_at: this.knex.fn.now() }); + }); } /** diff --git a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-resync.test.ts b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-resync.test.ts new file mode 100644 index 0000000000..1a5e9af904 --- /dev/null +++ b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-resync.test.ts @@ -0,0 +1,58 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5495] The wasm face re-seeds a stale autonumber counter too. + * + * `SqliteWasmDriver extends SqlDriver` and overrides neither `create` nor the + * autonumber helpers, so the re-seed is inherited. What this pins is that the + * inheritance actually DELIVERS it: this driver swaps knex's transport for a + * custom sql.js dialect, and the collision path depends on two things + * surviving that swap — the refused INSERT still throwing an error + * `isUniqueViolationError` recognises, and the probe read + * (`autoNumberValueExists`) seeing the row that caused the refusal. + * + * "It inherits the base class, therefore it is fine" is the assumption #4405 + * and #5240 exist to disprove on this driver, and #6203 is the shape where one + * fix landed on one face and left the other answering differently. So the + * fourth backend is verified rather than assumed. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqliteWasmDriver } from './index.js'; + +describe('[#5495] driver-sqlite-wasm inherits the autonumber re-seed', () => { + let driver: SqliteWasmDriver; + + beforeEach(async () => { + driver = new SqliteWasmDriver({ filename: ':memory:' }); + await driver.initObjects([ + { + name: 'crm_case', + fields: { + organization_id: { type: 'string' }, + case_number: { type: 'autonumber', format: 'CASE-{00000}', unique: true }, + title: { type: 'string' }, + }, + } as any, + ]); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('serves the create on the first attempt after a seed replay lands above the counter', async () => { + const knex = (driver as any).knex; + + await driver.create('crm_case', { organization_id: 'orgA', title: 'first' }); + + const rows = []; + for (let n = 2; n <= 30; n++) { + rows.push({ id: `s${n}`, organization_id: 'orgA', case_number: `CASE-${String(n).padStart(5, '0')}`, title: `seed ${n}` }); + } + await knex('crm_case').insert(rows); + + const created = await driver.create('crm_case', { organization_id: 'orgA', title: 'after the seeds' }); + expect(created.case_number).toBe('CASE-00031'); + }); +}); diff --git a/packages/drivers/driver-turso/src/turso-autonumber-resync.test.ts b/packages/drivers/driver-turso/src/turso-autonumber-resync.test.ts new file mode 100644 index 0000000000..7190ba40d9 --- /dev/null +++ b/packages/drivers/driver-turso/src/turso-autonumber-resync.test.ts @@ -0,0 +1,80 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5495] The Turso faces, held against each other on the autonumber re-seed. + * + * `TursoDriver extends SqlDriver` but picks its engine from the `url` it was + * constructed with, and #6203 is the shape where that costs a fix half its + * reach: one driver, two answers. So both faces are stated here rather than + * one being assumed from the other. + * + * - **LOCAL (and replica, same local engine)** inherits `SqlDriver.create` and + * with it the re-seed. Asserted below on rows. + * - **REMOTE** overrides `create` to `RemoteTransport.create`, which builds + * its own `INSERT` and never enters `fillAutoNumberFields` at all — so it + * neither has this defect nor receives this fix. That is not a gap this card + * closes: on that face `auto_number` is only a column-type mapping + * (`remote-transport.ts` maps it to `TEXT`) and no sequence machinery exists + * to be stale. It is stated here so the boundary is on the record, and the + * absence is pinned as an ASSERTION rather than a comment, so that wiring + * autonumber into the remote transport later cannot silently inherit this + * file's green. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { TursoDriver } from './index.js'; + +describe('[#5495] TursoDriver autonumber re-seed', () => { + let driver: TursoDriver; + + beforeEach(async () => { + driver = new TursoDriver({ url: ':memory:' }); + expect(driver.transportMode).toBe('local'); + await driver.initObjects([ + { + name: 'crm_case', + fields: { + organization_id: { type: 'string' }, + case_number: { type: 'autonumber', format: 'CASE-{00000}', unique: true }, + title: { type: 'string' }, + }, + } as any, + ]); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('LOCAL: serves the create on the first attempt after a seed replay lands above the counter', async () => { + const knex = (driver as any).knex; + + await driver.create('crm_case', { organization_id: 'orgA', title: 'first' }, { bypassTenantAudit: true } as any); + + const rows = []; + for (let n = 2; n <= 30; n++) { + rows.push({ id: `s${n}`, organization_id: 'orgA', case_number: `CASE-${String(n).padStart(5, '0')}`, title: `seed ${n}` }); + } + await knex('crm_case').insert(rows); + + const created = await driver.create( + 'crm_case', + { organization_id: 'orgA', title: 'after the seeds' }, + { bypassTenantAudit: true } as any, + ); + expect(created.case_number).toBe('CASE-00031'); + }); + + it('REMOTE: the transport that bypasses this path has no autonumber machinery to re-seed', async () => { + const remote = new TursoDriver({ url: 'libsql://example.turso.io', authToken: 'placeholder' }); + expect(remote.transportMode).toBe('remote'); + + // The boundary, stated as a fact about the code rather than about a live + // connection: `RemoteTransport` has no autonumber surface at all. When one + // is added, this assertion is the thing that has to be revisited. + const transportSurface = Object.getOwnPropertyNames( + Object.getPrototypeOf((remote as any).remoteTransport), + ); + expect(transportSurface.some((m) => /autonumber|sequence/i.test(m))).toBe(false); + }); +});