From 41a5fd3ee93a14688f108dae4855dde5998eea31 Mon Sep 17 00:00:00 2001 From: os-dev Date: Sun, 9 Aug 2026 04:29:12 +0000 Subject: [PATCH 1/2] fix(driver-sql): re-seed a stale autonumber counter instead of burning a number per failed create (#5495) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_015XLbsWE5G58ybd1Leq6bNg --- ...autonumber-sequence-resync-on-collision.md | 73 +++++ .../src/sql-driver-autonumber-resync.test.ts | 257 ++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 275 +++++++++++++++++- .../src/sqlite-wasm-autonumber-resync.test.ts | 58 ++++ .../src/turso-autonumber-resync.test.ts | 80 +++++ 5 files changed, 729 insertions(+), 14 deletions(-) create mode 100644 .changeset/autonumber-sequence-resync-on-collision.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-autonumber-resync.test.ts create mode 100644 packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-resync.test.ts create mode 100644 packages/drivers/driver-turso/src/turso-autonumber-resync.test.ts 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..a4f8426131 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-autonumber-resync.test.ts @@ -0,0 +1,257 @@ +// 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(); + }); + + 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); + }); +}); From c79eb9c9524d6ffb6e22062d166ab54f8247cbcb Mon Sep 17 00:00:00 2001 From: os-dev Date: Sun, 9 Aug 2026 04:53:39 +0000 Subject: [PATCH 2/2] test(driver-sql): pin the three-state collision routing on Postgres and MySQL shapes (#5495) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SQLite tests cover the path end to end, but SQLite is the one dialect where the decision is easy. Postgres names a column in its DETAIL line (state 1/2) or a composite (state 3); MySQL names only an index and so can only ever reach state 3 — which means on MySQL the data probe is not a fallback, it is the whole mechanism. This package's unit suite boots SQLite only, so the shapes are injected rather than driven through live servers — same method and same reason as sql-driver-unique-violation-predicate.test.ts. The state-2 case asserts the part that matters most: a named column that is not ours ends the matter WITHOUT probing, even when the probe would have said yes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015XLbsWE5G58ybd1Leq6bNg --- .../src/sql-driver-autonumber-resync.test.ts | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) 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 index a4f8426131..e50b4533a9 100644 --- a/packages/drivers/driver-sql/src/sql-driver-autonumber-resync.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-autonumber-resync.test.ts @@ -236,6 +236,92 @@ describe('[#5495] autonumber sequence re-seed on a provable collision', () => { ).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.