From 72b5ba54faba8ea14d2f65d5d38129b8a9a6fb9e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 08:18:48 +0000 Subject: [PATCH 1/2] fix(driver-sql): bulkCreate and upsert re-seed a stale autonumber counter (#6943) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `create()` learned this at #5495; `bulkCreate()` and `upsert()` call the same `fillAutoNumberFields` and did not. Measured, they are not the same defect: - `upsert` is `create()`'s old shape exactly — single row, one burned number per refused call. Its `ON CONFLICT DO UPDATE` absorbs a merge-key conflict only; the tenanted autonumber sits under a different unique index. - `bulkCreate` is worse. Each row reserves in its own committed transaction and the batch goes in as ONE insert, so one colliding row burns every number the batch reserved and fails the whole request — on the path framework#2678 made the common case for seed/import, which is what creates the staleness. Both now reuse #5495's machinery unchanged: `collidingAutoNumberReservations` for the three-state routing, `autoNumberValueExists` as the data-based discriminator, forward-only `resyncSequenceToDataMax`. Batch semantics are unchanged by measurement, not by choice: `insert(rows[])` is a single statement, so the batch was already all-or-nothing and re-issuing it whole preserves the contract exactly. Per-row retry was rejected — it would have to split the statement and invent partial success. Re-issue is per counter, not per row: a batch straddling the seeded range would otherwise regenerate only its low rows and hand them numbers above the kept ones, an intra-batch duplicate. Retry stays confined to the no-caller-transaction case, as at #5495. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DZrSGPUVrFqYCKc3ELYRqR --- .../driver-sql-batch-autonumber-resync.md | 72 ++++ ...sql-driver-autonumber-batch-resync.test.ts | 328 ++++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 214 +++++++++--- ...qlite-wasm-autonumber-batch-resync.test.ts | 79 +++++ .../src/turso-autonumber-batch-resync.test.ts | 106 ++++++ 5 files changed, 760 insertions(+), 39 deletions(-) create mode 100644 .changeset/driver-sql-batch-autonumber-resync.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-autonumber-batch-resync.test.ts create mode 100644 packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-batch-resync.test.ts create mode 100644 packages/drivers/driver-turso/src/turso-autonumber-batch-resync.test.ts diff --git a/.changeset/driver-sql-batch-autonumber-resync.md b/.changeset/driver-sql-batch-autonumber-resync.md new file mode 100644 index 0000000000..cbff357087 --- /dev/null +++ b/.changeset/driver-sql-batch-autonumber-resync.md @@ -0,0 +1,72 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): `bulkCreate` and `upsert` re-seed a stale autonumber counter instead of burning the whole batch (#6943) + +#5495 taught `create()` to re-seed a stale autonumber counter and retry instead +of burning one number per failed insert. `bulkCreate()` and `upsert()` call the +same `fillAutoNumberFields` and did not get that fix. They are not, however, the +same defect as each other — measured on `main` @ `c8ff269`, on a fresh database +with seeded rows above the counter (the one-time-storm repro constraint #5495 +established): + +**`upsert` is `create()`'s old shape exactly.** Single row, so a stale counter +costs it one burned number per call: `last_value` walked 1 → 2 → 3 across two +refused upserts. Its `ON CONFLICT (mergeKeys) DO UPDATE` absorbs a conflict on +the merge key only; the tenanted autonumber lives under a *different* unique +index, so that violation is still raised and still reaches the caller. + +**`bulkCreate` is worse.** Each row reserves its number in its own committed +transaction and the batch then goes in as ONE insert, so a single colliding row +burns *every* number the batch reserved and fails the whole request: + +| 3-row `bulkCreate`, counter at 10, rows 11–39 already present | before | after | +|:---|:---|:---| +| caller-visible failures | both calls threw | **0** | +| rows written | **0** | 3 | +| `last_value` | 10 → 13, then 13 → 16 | 10 → 42, by one re-seed | + +And it is the worst path to leave without recovery: framework#2678 made +`bulkCreate` the common case for seed/import, and seed/import is exactly what +*creates* the staleness — an `isSystem` replay or a `preserveAudit` import keeps +its explicit numbers and never enters `fillAutoNumberFields` (#5495/#5503). + +Both paths now reuse #5495's machinery unchanged — `collidingAutoNumberReservations` +for the three-state routing, `autoNumberValueExists` for the data-based +discriminator (the conflicting column is never determinable for a tenanted +autonumber), and the forward-only `resyncSequenceToDataMax`. A collision that is +not provably this counter's is still rethrown untouched, so a duplicate on a +value the caller supplied still reaches them as its own error. + +**Batch semantics are unchanged, and that is a measurement rather than a +choice.** `insert(rows[])` is a single statement, so the batch was already +all-or-nothing — the failed batch above left the table exactly as it found it. +Re-issuing and retrying the whole batch therefore preserves the existing +contract: no partial success is introduced, no transaction is opened, and no +"does a failed row roll back its siblings" question arises, because siblings +already fail together. Per-row retry inside the batch was rejected for the +opposite reason — it would have had to split the one statement into N and invent +partial success where none existed. + +One thing the batch may not borrow from `create()`: `create()` keeps a +reservation that did not collide, to avoid burning a second number. A batch +cannot. One that straddles the seeded range has its low rows collide and its +high rows not, and re-issuing only the collided ones would hand them numbers +*above* the kept ones — an intra-batch duplicate the driver would have +manufactured itself. Re-issue is therefore per counter: every row drawn from a +counter that went stale is re-issued, and counters that did not go stale keep +their values, so a co-tenant's rows in the same batch are undisturbed. + +As with #5495, retrying is confined to the no-caller-transaction case. Inside a +caller's transaction the sequence `UPDATE` rolls back with the refused `INSERT`, +so nothing is burned and there is nothing to repair (measured on both paths), and +on Postgres a constraint failure aborts the transaction outright. The caller owns +that retry. + +`TursoDriver` (local/replica) and `SqliteWasmDriver` inherit both fixes, each +pinned by its own test rather than assumed from the base class — Turso +*overrides* `bulkCreate`/`upsert` to route remote traffic away, so inheritance +there is a routing fact, not a class fact. Turso's remote transport builds its +own INSERT and generates no autonumber at all, so it neither has this defect nor +receives this fix (that gap is #6944). diff --git a/packages/drivers/driver-sql/src/sql-driver-autonumber-batch-resync.test.ts b/packages/drivers/driver-sql/src/sql-driver-autonumber-batch-resync.test.ts new file mode 100644 index 0000000000..67aade25d5 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-autonumber-batch-resync.test.ts @@ -0,0 +1,328 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6943] `bulkCreate` and `upsert` re-seed a stale autonumber counter, the way + * `create()` began to at #5495. + * + * # Two different shapes, measured rather than assumed + * + * The card said these two "burn a number per collision, like `create()`". Only + * one of them does. + * + * **`upsert` does** — it is single-row, so a stale counter costs it exactly one + * burned number per call, which is `create()`'s pre-#5495 shape exactly. Its + * `ON CONFLICT (mergeKeys) DO UPDATE` absorbs a conflict on the merge key only; + * the tenanted autonumber lives under a *different* unique index, so its + * violation is still raised. Measured on `main` @ `c8ff269`: `last_value` + * 1 → 2 → 3 across two refused upserts. + * + * **`bulkCreate` does not — it is worse.** Every row reserves its own number in + * its own committed transaction, then the whole batch goes in as ONE insert. So + * one colliding row burns *every* number the batch reserved and fails the whole + * request. Measured on the same commit, counter at 10 with rows 11–39 landed by + * a bypass path: + * + * | 3-row `bulkCreate` | before | after | + * |:---|:---|:---| + * | caller-visible failures | **2 of 2 calls threw** | **0** | + * | rows written | **0** | 3 | + * | `last_value` | 10 → 13, then 13 → 16 | 10 → 42, by one re-seed | + * + * And this is the worst path to have no recovery on: framework#2678 made + * `bulkCreate` the common case for seed/import, and seed/import is precisely + * what *creates* the staleness — an `isSystem` replay or a `preserveAudit` + * import keeps its explicit numbers and never enters `fillAutoNumberFields` + * (#5495/#5503). + * + * # Batch semantics are unchanged, and that is a measurement, not a taste + * + * The card expected a decision about partial success and transaction + * boundaries. There is none to make: `insert(rows[])` is a SINGLE statement, so + * the batch is already all-or-nothing — the failed batch above left the table + * exactly as it found it (31 rows before, 31 after). Re-issuing and retrying + * the whole batch therefore preserves the existing contract exactly. The + * alternatives that would have changed it — per-row retry, a driver-opened + * transaction — are rejected in `bulkCreate`'s own comment with the reasons. + * + * # The one thing the batch may NOT borrow from `create()` + * + * `create()` keeps a reservation that did not collide, to avoid burning a + * second number. A batch cannot: one that straddles the seeded range has its + * low rows collide and its high rows not, and regenerating only the low ones + * hands them numbers *above* the kept ones — an intra-batch duplicate the + * driver would have manufactured itself. So re-issue is per *counter*: every + * row drawn from a counter that went stale is re-issued. Pinned below. + * + * # On the assertion style + * + * Same as #5495's file, and for the same reason: ADR-0112's `code`/`status` + * envelope is a *wire* contract, and what a refused INSERT throws here is the + * driver's raw error, which carries neither. These assert something stronger + * than a bare `toThrow()`: the specific constraint text, `isUniqueViolationError` + * agreeing, and — for the cases that must NOT be retried — that the error names + * the CALLER's field and that no row landed. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { isUniqueViolationError } 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('[#6943] batch and upsert re-seed a stale autonumber counter', () => { + let driver: SqlDriver; + + const knex = () => (driver as any).knex; + + 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); + }; + + const rowCount = async () => Number((await knex()('crm_case').count({ c: '*' }).first()).c); + + /** Every `case_number` in the table, for the duplicate check. */ + const allNumbers = async (): Promise => + (await knex()('crm_case').select('case_number')).map((r: any) => r.case_number); + + beforeEach(async () => { + driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + // ---------------------------------------------------------------- bulkCreate + + it('serves a batch that used to fail outright, and burns the batch only once', async () => { + await driver.initObjects([CRM_CASE]); + + await bypassInsert('orgA', 9, 9); + await driver.create('crm_case', { organization_id: 'orgA', title: 'warm' }); // counter -> 10 + await bypassInsert('orgA', 11, 39); // seeds land ABOVE the counter; nothing tells the sequence + + const created = await driver.bulkCreate('crm_case', [ + { organization_id: 'orgA', title: 'b1' }, + { organization_id: 'orgA', title: 'b2' }, + { organization_id: 'orgA', title: 'b3' }, + ]); + + // Before this change the same call threw and wrote nothing. + expect(created.map((r: any) => r.case_number)).toEqual(['CASE-00040', 'CASE-00041', 'CASE-00042']); + expect(await rowCount()).toBe(34); + + // And the counter is simply +1 from here — re-seeding is not a mode the + // driver stays in. + const next = await driver.bulkCreate('crm_case', [{ organization_id: 'orgA', title: 'b4' }]); + expect(next[0].case_number).toBe('CASE-00043'); + }); + + it('re-issues the WHOLE batch drawn from a stale counter, so it cannot duplicate itself', async () => { + // The straddle case: the batch is longer than the stale gap, so its low + // rows collide and its high rows do not. Re-issuing only the collided ones + // would hand them numbers above the kept ones. + await driver.initObjects([CRM_CASE]); + + await bypassInsert('orgA', 9, 9); + await driver.create('crm_case', { organization_id: 'orgA', title: 'warm' }); // counter -> 10 + await bypassInsert('orgA', 11, 39); + + const batch = Array.from({ length: 60 }, (_, i) => ({ organization_id: 'orgA', title: `straddle ${i}` })); + const created = await driver.bulkCreate('crm_case', batch); + + expect(created).toHaveLength(60); + const numbers = await allNumbers(); + expect(new Set(numbers).size).toBe(numbers.length); // no duplicate anywhere in the table + // Every row of the batch sits above the seeded range it straddled. + for (const r of created) expect(Number(r.case_number.slice('CASE-'.length))).toBeGreaterThan(39); + }); + + it('re-seeds only the counter that went stale, leaving a co-tenant in the same batch alone', async () => { + await driver.initObjects([CRM_CASE]); + + await driver.create('crm_case', { organization_id: 'orgA', title: 'a1' }); // A -> 1 + await driver.create('crm_case', { organization_id: 'orgB', title: 'b1' }); // B -> 1 + await bypassInsert('orgA', 2, 20); // only A goes stale + + const created = await driver.bulkCreate('crm_case', [ + { organization_id: 'orgA', title: 'a2' }, + { organization_id: 'orgA', title: 'a3' }, + { organization_id: 'orgB', title: 'b2' }, + ]); + + expect(created.map((r: any) => r.case_number)).toEqual(['CASE-00021', 'CASE-00022', 'CASE-00002']); + // B never collided, so its reservation was kept rather than burned and + // re-issued: one number consumed, not two. + expect(await lastValue('orgB')).toBe(2); + }); + + it('does not swallow a duplicate the CALLER supplied, and still writes nothing', 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 before = await rowCount(); + + const caught = await driver + .bulkCreate('crm_case', [ + { organization_id: 'orgA', email: 'fresh@example.com', title: 'ok' }, + { organization_id: 'orgA', email: 'dup@example.com', title: 'the caller’s 409' }, + ]) + .then(() => undefined, (e: unknown) => e); + + expect(caught).toBeInstanceOf(Error); + expect((caught as Error).message).toMatch(/UNIQUE constraint failed|duplicate key value/); + expect(isUniqueViolationError(caught)).toBe(true); + // The 409 is the client's: not retried away, and the batch is still + // all-or-nothing — the sibling row did not sneak in. + expect(await rowCount()).toBe(before); + }); + + 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.bulkCreate( + 'crm_case', + [{ organization_id: 'orgA', title: 't1' }, { organization_id: 'orgA', title: 't2' }], + { transaction: trx } as any, + ); + }) + .then(() => undefined, (e: unknown) => e); + + // Still refused, and nothing burned: the sequence UPDATE shares the + // caller's transaction and rolls back with the refused INSERT. On Postgres + // a constraint failure aborts the transaction outright, so a retry issued + // on it could not succeed — the caller owns that retry. + expect(caught).toBeInstanceOf(Error); + expect((caught as Error).message).toMatch(/UNIQUE constraint failed|duplicate key value/); + expect(await lastValue('orgA')).toBe(before); + }); + + it('leaves a batch with no autonumber field completely untouched', async () => { + await driver.initObjects([ + { name: 'note', fields: { organization_id: { type: 'string' }, body: { type: 'string' } } } as any, + ]); + const created = await driver.bulkCreate('note', [ + { organization_id: 'orgA', body: 'one' }, + { organization_id: 'orgA', body: 'two' }, + ]); + expect(created).toHaveLength(2); + }); + + // -------------------------------------------------------------------- upsert + + it('upsert: serves the insert that used to be refused by a stale counter', async () => { + await driver.initObjects([CRM_CASE]); + + await driver.create('crm_case', { organization_id: 'orgA', title: 'warm' }); // counter -> 1 + await bypassInsert('orgA', 2, 20); + + const upserted = await driver.upsert('crm_case', { organization_id: 'orgA', title: 'u1' }); + expect(upserted.case_number).toBe('CASE-00021'); + + // +1 from here, like every other path. + expect((await driver.upsert('crm_case', { organization_id: 'orgA', title: 'u2' })).case_number).toBe('CASE-00022'); + }); + + it('upsert: a duplicate on a CALLER-supplied unique field still reaches the caller', 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 + .upsert('crm_case', { organization_id: 'orgA', email: 'dup@example.com', title: 'two' }) + .then(() => undefined, (e: unknown) => e); + + expect(caught).toBeInstanceOf(Error); + expect(isUniqueViolationError(caught)).toBe(true); + expect((caught as Error).message).toMatch(/email/); + }); + + it('upsert: an explicit autonumber value is preserved, never regenerated', async () => { + await driver.initObjects([CRM_CASE]); + await driver.create('crm_case', { organization_id: 'orgA', title: 'first' }); + + const caught = await driver + .upsert('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); + // A caller-supplied value is not a reservation, so no probe and no retry + // can apply to it — and no number was consumed. + expect(await lastValue('orgA')).toBe(1); + }); + + it('upsert: 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.upsert('crm_case', { organization_id: 'orgA', title: 'in-trx' }, undefined, { transaction: trx } as any); + }) + .then(() => undefined, (e: unknown) => e); + + expect(caught).toBeInstanceOf(Error); + expect((caught as Error).message).toMatch(/UNIQUE constraint failed|duplicate key value/); + expect(await lastValue('orgA')).toBe(before); + }); + + it('upsert: still merges onto an existing row rather than inserting a second', async () => { + // The merge branch is untouched by this change — kept here so a future + // edit to the retry loop cannot quietly turn an update into an insert. + await driver.initObjects([CRM_CASE]); + const first = await driver.create('crm_case', { organization_id: 'orgA', title: 'original' }); + + const merged = await driver.upsert('crm_case', { id: first.id, organization_id: 'orgA', title: 'edited' }); + expect(merged.id).toBe(first.id); + expect(merged.title).toBe('edited'); + expect(await rowCount()).toBe(1); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 5613898b71..279e580e8a 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -3898,6 +3898,24 @@ export class SqlDriver implements IDataDriver { }); } + /** + * Identity of the counter a reservation was drawn from — the same + * `(table, tenant, field, scope)` hash {@link getNextSequenceValue} keys the + * sequences row by (#6943). + * + * `create()` never needed this: one row draws at most one reservation per + * field, so "which counter" and "which reservation" are the same question. + * A batch breaks that — many rows draw from ONE counter — and both of the + * batch's collision decisions are per *counter*, not per reservation: re-seed + * each stale counter once rather than once per row, and re-issue every row + * that drew from it. See {@link bulkCreate}. + */ + protected autoNumberCounterKey(reservation: AutoNumberReservation): string { + const resolvedTenantId = + reservation.tenantField && reservation.tenantId ? String(reservation.tenantId) : GLOBAL_TENANT; + return this.sequenceKeyHash(reservation.tableName, resolvedTenantId, reservation.field, reservation.scope); + } + /** * Stamp the builtin audit timestamps to one canonical ISO-8601-with-`Z` * instant on the SQLite write paths (`create`/`bulkCreate`/`upsert`), so @@ -3977,23 +3995,56 @@ export class SqlDriver implements IDataDriver { this.auditMissingTenant(object, 'upsert', options); this.injectTenantOnInsert(object, toUpsert, options); - await this.fillAutoNumberFields(object, toUpsert, options); - const formatted = this.applyWriteColumnMap(object, this.formatInput(object, toUpsert)); - this.stampInsertTimestamps(object, formatted); const mergeKeys = conflictKeys && conflictKeys.length > 0 ? conflictKeys : ['id']; - // Rotation: conflict-merge is scoped to the CURRENT shard (telemetry is - // effectively append-only; a cross-shard upsert would need a probe-first - // strategy nothing on the platform requires today). - const builder = this.getBuilder(this.rotationWriteTarget(object) ?? object, options); - // `created_at` is insert-only — never overwrite it when an existing row is - // merged on conflict (the stamped/seeded value belongs to the original - // insert). Everything else (incl. `updated_at`) merges as before, so an - // upsert that updates a row still advances `updated_at`. - const mergeColumns = Object.keys(formatted).filter((c) => c !== 'created_at'); - const insertion = builder.insert(formatted).onConflict(mergeKeys); - await (mergeColumns.length > 0 ? insertion.merge(mergeColumns) : insertion.merge()); + // #6943. Measured: `upsert` does NOT share `bulkCreate`'s shape. It is + // single-row, so a stale counter costs it exactly one burned number per + // call — the same shape `create()` had before #5495, for the same reason + // (no re-seed, no retry). `ON CONFLICT (mergeKeys) DO UPDATE` absorbs a + // conflict on the merge key only; the tenanted autonumber lives under a + // DIFFERENT unique index, so its violation is still raised and still + // reaches here. The remedy is therefore `create()`'s, verbatim: re-seed and + // re-issue, but only when the collision is provably this counter's + // ({@link collidingAutoNumberReservations}), and only outside a caller + // transaction (inside one the sequence UPDATE rolls back with the refused + // INSERT, so nothing is burned and there is nothing to repair — measured). + const mayRetry = options?.transaction === undefined; + for (let attempt = 0; ; attempt++) { + const reservations = await this.fillAutoNumberFields(object, toUpsert, options); + + const formatted = this.applyWriteColumnMap(object, this.formatInput(object, toUpsert)); + this.stampInsertTimestamps(object, formatted); + + // Rotation: conflict-merge is scoped to the CURRENT shard (telemetry is + // effectively append-only; a cross-shard upsert would need a probe-first + // strategy nothing on the platform requires today). + const builder = this.getBuilder(this.rotationWriteTarget(object) ?? object, options); + // `created_at` is insert-only — never overwrite it when an existing row is + // merged on conflict (the stamped/seeded value belongs to the original + // insert). Everything else (incl. `updated_at`) merges as before, so an + // upsert that updates a row still advances `updated_at`. + const mergeColumns = Object.keys(formatted).filter((c) => c !== 'created_at'); + const insertion = builder.insert(formatted).onConflict(mergeKeys); + + try { + await (mergeColumns.length > 0 ? insertion.merge(mergeColumns) : insertion.merge()); + break; + } 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. (Safe here in a + // way it is NOT for a batch: one row cannot collide with itself. + // See {@link bulkCreate}.) + delete toUpsert[reservation.field]; + } + } + } const readback = this.getBuilder(object, options).where('id', toUpsert.id); this.applyTenantScope(readback, object, options); @@ -4043,33 +4094,118 @@ export class SqlDriver implements IDataDriver { return toInsert; }); for (const row of rows) { - if (row && typeof row === 'object') { - this.injectTenantOnInsert(object, row, options); - // Reserve a persistent sequence value for each row's autonumber - // field(s) — the engine no longer pre-fills these (see #1603). - await this.fillAutoNumberFields(object, row, options); + if (row && typeof row === 'object') this.injectTenantOnInsert(object, row, options); + } + + // #6943. A stale counter costs this path more than it costs `create()`. + // Each row reserves its own number in its own committed transaction, and + // then the whole batch goes in as ONE insert — so a single colliding row + // does not burn one number, it burns every number the batch reserved and + // fails the whole request. Measured on `main` @ `c8ff269` (counter at 10, + // rows 11–39 landed by a bypass path): a 3-row `bulkCreate` threw, wrote + // ZERO rows and moved `last_value` 10 → 13; the next call moved it 13 → 16 + // and threw again. And the exposure is the worst available, because + // framework#2678 made this the common path for seed/import — which is + // precisely what *creates* the staleness (`isSystem` replay and + // `preserveAudit` import keep their explicit numbers and never enter + // `fillAutoNumberFields`, #5495/#5503). + // + // ## Batch semantics are deliberately NOT changed + // + // The card anticipated a decision about partial success and transaction + // boundaries. Measurement dissolves it: `insert(rows[])` is a SINGLE + // statement (`… select … union all select …` on SQLite, multi-row VALUES + // elsewhere), so the batch is ALREADY all-or-nothing — the failed batch + // above left the table exactly as it found it. Re-issuing and retrying the + // WHOLE batch therefore preserves the existing contract byte for byte: no + // partial success is introduced, no transaction is opened, no sibling- + // rollback question arises because siblings already fail together. + // Rejected instead: + // - **Per-row retry inside the batch** (the shape the card sketched). + // It requires splitting the one statement into N to learn which row + // failed, which INVENTS partial success where none exists, costs N + // round-trips on the hot seed/import path, and is what the engine's + // own #6806 note warns about ("re-writing the batch could DUPLICATE + // the rows that did land"). Nothing lands here, so nothing can. + // - **Wrapping the batch in a driver-opened transaction.** It would move + // the boundary the card was worried about, for no gain (the statement + // is already atomic), and it defeats the reservation model: each + // `getNextSequenceValue` commits on purpose, which is what makes a + // forward-only re-seed meaningful. On SQLite (pool max 1) it is also + // the deadlock `ensureSequencesTable` documents. + // - **Re-seed but do not re-issue** (what the engine does at #6806). + // Correct there, because the engine cannot know whether an arbitrary + // driver applied the batch partially. Inside THIS driver that is + // measurably false, so the caller can be handed a successful batch + // instead of an error it has to retry itself. + // - **Blanket retry on any unique violation.** Rejected for #5495's + // reason: it silently eats the caller's own 409. + const mayRetry = options?.transaction === undefined; + for (let attempt = 0; ; attempt++) { + // Reserve a persistent sequence value for each row's autonumber + // field(s) — the engine no longer pre-fills these (see #1603). + const reservationsPerRow: AutoNumberReservation[][] = []; + for (const row of rows) { + reservationsPerRow.push( + row && typeof row === 'object' ? await this.fillAutoNumberFields(object, row, options) : [], + ); + } + + // Same write-side marshaling as create() (#2735): JSON-typed and + // object-valued fields must be serialized per row before they reach the + // knex binder — the raw batch used to hand `{lat, lng}` objects straight + // to SQLite ("Wrong API use: tried to bind a value of an unknown type"), + // silently failing the whole seed batch. Timestamp stamping runs on the + // FORMATTED copy, mirroring create(). + const formattedRows = rows.map((row) => { + if (!row || typeof row !== 'object') return row; + const formatted = this.applyWriteColumnMap(object, this.formatInput(object, row)); + this.stampInsertTimestamps(object, formatted); + return formatted; + }); + const builder = this.getBuilder(this.rotationWriteTarget(object) ?? object, options); + + try { + const result = await builder.insert(formattedRows).returning('*'); + // Read-back parity with create(): JSON columns come back as their stored + // strings from `returning('*')` — decode them so batch callers see the + // same shapes single-insert callers do. + return Array.isArray(result) + ? result.map((r) => this.formatOutput(object, r)) + : result; + } catch (error) { + if (!mayRetry || attempt >= AUTONUMBER_COLLISION_RETRIES) throw error; + const colliding = await this.collidingAutoNumberReservations(error, reservationsPerRow.flat(), options); + if (colliding.length === 0) throw error; + + // Re-seed each stale counter ONCE, not once per row that drew from it: + // the scan is identical for every row sharing a counter and the update + // is forward-only, so the repeats are pure cost. + const stale = new Set(); + for (const reservation of colliding) { + const key = this.autoNumberCounterKey(reservation); + if (stale.has(key)) continue; + stale.add(key); + await this.resyncSequenceToDataMax(reservation); + } + + // Re-issue EVERY row drawn from a stale counter — including rows whose + // own value did not collide. This is where the batch genuinely differs + // from `create()`, which keeps a non-colliding value to avoid burning a + // second number. Keeping them here is unsound: a batch that straddles + // the seeded range (say the counter is at 10, rows 11–39 exist, and the + // batch reserved 11–70) has its low rows collide and its high rows not, + // and regenerating only the low ones hands them numbers ABOVE the kept + // ones — an intra-batch duplicate this driver would have created + // itself. Counters that did NOT go stale keep their values, so another + // tenant's rows in the same batch are undisturbed. + for (let i = 0; i < rows.length; i++) { + for (const reservation of reservationsPerRow[i]) { + if (stale.has(this.autoNumberCounterKey(reservation))) delete rows[i][reservation.field]; + } + } } } - // Same write-side marshaling as create() (#2735): JSON-typed and - // object-valued fields must be serialized per row before they reach the - // knex binder — the raw batch used to hand `{lat, lng}` objects straight - // to SQLite ("Wrong API use: tried to bind a value of an unknown type"), - // silently failing the whole seed batch. Timestamp stamping runs on the - // FORMATTED copy, mirroring create(). - const formattedRows = rows.map((row) => { - if (!row || typeof row !== 'object') return row; - const formatted = this.applyWriteColumnMap(object, this.formatInput(object, row)); - this.stampInsertTimestamps(object, formatted); - return formatted; - }); - const builder = this.getBuilder(this.rotationWriteTarget(object) ?? object, options); - const result = await builder.insert(formattedRows).returning('*'); - // Read-back parity with create(): JSON columns come back as their stored - // strings from `returning('*')` — decode them so batch callers see the - // same shapes single-insert callers do. - return Array.isArray(result) - ? result.map((r) => this.formatOutput(object, r)) - : result; } /** diff --git a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-batch-resync.test.ts b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-batch-resync.test.ts new file mode 100644 index 0000000000..4788b0a32e --- /dev/null +++ b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-batch-resync.test.ts @@ -0,0 +1,79 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6943] The wasm face re-seeds on the batch and upsert paths too. + * + * `SqliteWasmDriver extends SqlDriver` and overrides neither `bulkCreate` nor + * `upsert`, 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 three things surviving that swap — + * the refused INSERT still throwing an error `isUniqueViolationError` + * recognises, the probe read (`autoNumberValueExists`) seeing the row that + * caused the refusal, and — new on the batch path — a multi-row + * `insert(rows[])` still failing as ONE statement, which is what makes + * re-issuing the whole batch safe rather than duplicating rows. + * + * "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('[#6943] driver-sqlite-wasm inherits the batch/upsert autonumber re-seed', () => { + let driver: SqliteWasmDriver; + + const knex = () => (driver as any).knex; + + const bypassInsert = async (from: number, to: number) => { + const rows = []; + for (let n = from; n <= to; 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); + }; + + 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('bulkCreate serves a batch that a stale counter used to fail outright', async () => { + await driver.create('crm_case', { organization_id: 'orgA', title: 'warm' }); + await bypassInsert(2, 30); + + const created = await driver.bulkCreate('crm_case', [ + { organization_id: 'orgA', title: 'b1' }, + { organization_id: 'orgA', title: 'b2' }, + ]); + + expect(created.map((r: any) => r.case_number)).toEqual(['CASE-00031', 'CASE-00032']); + // Nothing was left behind by the failed first pass — the batch is one + // statement on this transport too. + const all = (await knex()('crm_case').select('case_number')).map((r: any) => r.case_number); + expect(new Set(all).size).toBe(all.length); + }); + + it('upsert serves the insert a stale counter used to refuse', async () => { + await driver.create('crm_case', { organization_id: 'orgA', title: 'warm' }); + await bypassInsert(2, 30); + + const upserted = await driver.upsert('crm_case', { organization_id: 'orgA', title: 'u1' }); + expect(upserted.case_number).toBe('CASE-00031'); + }); +}); diff --git a/packages/drivers/driver-turso/src/turso-autonumber-batch-resync.test.ts b/packages/drivers/driver-turso/src/turso-autonumber-batch-resync.test.ts new file mode 100644 index 0000000000..38d5dab945 --- /dev/null +++ b/packages/drivers/driver-turso/src/turso-autonumber-batch-resync.test.ts @@ -0,0 +1,106 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6943] The Turso faces, held against each other on the batch/upsert 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 — and `bulkCreate` needs saying out loud even + * more than `create()` did, because Turso **overrides** it (`create` and + * `upsert` too) rather than merely inheriting: each override routes remote to + * `RemoteTransport` and everything else to `super`, so "the base class was + * fixed" is not on its own an answer about this face. + * + * - **LOCAL (and replica, same local engine)** falls through to + * `super.bulkCreate` / `super.upsert` and with them the re-seed. Asserted + * below on rows. + * - **REMOTE** hands the batch to `RemoteTransport.bulkCreate`, which builds + * its own INSERT and never enters `fillAutoNumberFields` — so it has neither + * the defect nor the fix. On that face `auto_number` is only a column-type + * mapping and no sequence machinery exists to be stale; the absence is + * pinned as an ASSERTION rather than a comment, so wiring autonumber into + * the remote transport later cannot silently inherit this file's green. + * (That gap is #6944's, not this card's.) + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { TursoDriver } from './index.js'; + +describe('[#6943] TursoDriver batch/upsert autonumber re-seed', () => { + let driver: TursoDriver; + + const knex = () => (driver as any).knex; + + 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 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: bulkCreate serves a batch that a stale counter used to fail outright', async () => { + await driver.create('crm_case', { organization_id: 'orgA', title: 'warm' }, { bypassTenantAudit: true }); + await bypassInsert('orgA', 2, 30); + + const created = await driver.bulkCreate( + 'crm_case', + [ + { organization_id: 'orgA', title: 'b1' }, + { organization_id: 'orgA', title: 'b2' }, + ], + { bypassTenantAudit: true } as any, + ); + + expect(created.map((r: any) => r.case_number)).toEqual(['CASE-00031', 'CASE-00032']); + }); + + it('LOCAL: upsert serves the insert a stale counter used to refuse', async () => { + await driver.create('crm_case', { organization_id: 'orgA', title: 'warm' }, { bypassTenantAudit: true }); + await bypassInsert('orgA', 2, 30); + + const upserted = await driver.upsert( + 'crm_case', + { organization_id: 'orgA', title: 'u1' }, + undefined, + { bypassTenantAudit: true } as any, + ); + + expect(upserted.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, on the + // batch path any more than the single-row one. When one is added (#6944), + // this assertion is what has to be revisited. + const transportSurface = Object.getOwnPropertyNames( + Object.getPrototypeOf((remote as any).remoteTransport), + ); + expect(transportSurface).toContain('bulkCreate'); + expect(transportSurface.some((m) => /autonumber|sequence/i.test(m))).toBe(false); + }); +}); From 5461f7878313a22b7c9c29f0a4769f84ac0d81a7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 08:21:19 +0000 Subject: [PATCH 2/2] fix(driver-sql): keep a batch's kept reservations described across retry passes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A retry regenerates only the fields it cleared, so `fillAutoNumberFields` reports only those. A value kept from the previous pass (drawn from a counter that did NOT go stale) is still a live reservation, and dropping it from `reservationsPerRow` meant a second collision on that counter would find nothing to route and rethrow blind. Carry the per-row reservation list across attempts, replacing only what was re-issued. Safe direction either way — the old shape rethrew the driver's own error, which is today's behaviour — but the invariant is now what the loop claims it is. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DZrSGPUVrFqYCKc3ELYRqR --- packages/drivers/driver-sql/src/sql-driver.ts | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 279e580e8a..0f6551c614 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -4141,14 +4141,22 @@ export class SqlDriver implements IDataDriver { // - **Blanket retry on any unique violation.** Rejected for #5495's // reason: it silently eats the caller's own 409. const mayRetry = options?.transaction === undefined; + // What each row currently holds, ACROSS attempts. A retry regenerates only + // the fields it cleared, so `fillAutoNumberFields` reports only those; a + // value kept from the previous pass is still a live reservation and has to + // stay described here, or a second collision on a counter that did not go + // stale the first time would find nothing to route and rethrow blind. + const reservationsPerRow: AutoNumberReservation[][] = rows.map(() => []); for (let attempt = 0; ; attempt++) { // Reserve a persistent sequence value for each row's autonumber // field(s) — the engine no longer pre-fills these (see #1603). - const reservationsPerRow: AutoNumberReservation[][] = []; - for (const row of rows) { - reservationsPerRow.push( - row && typeof row === 'object' ? await this.fillAutoNumberFields(object, row, options) : [], - ); + for (let i = 0; i < rows.length; i++) { + const row = rows[i]; + if (!row || typeof row !== 'object') continue; + const reissued = await this.fillAutoNumberFields(object, row, options); + if (reissued.length === 0) continue; + const replaced = new Set(reissued.map((r) => r.field)); + reservationsPerRow[i] = [...reservationsPerRow[i].filter((r) => !replaced.has(r.field)), ...reissued]; } // Same write-side marshaling as create() (#2735): JSON-typed and